[PlaygroundTransform] Disable logging `~Copyable` types as they can’t be passed to the generic logging function #71293

dbukowski · 2024-01-31T23:34:23Z

Summary

Xcode Playgrounds cannot execute code with ~Copyable types, as the Playground Transform tries to pass them to the generic logging function, which is not supported yet. These changes disable logging them altogether for now, which can be reverted once generics start supporting move-only types. Another approach would be to introduce a placeholder log, which could be recognized by the clients in order to inform the user about the reason for the missing logs.

Changes

Add a new function in Playground Transform shouldLogType(Type) to check the isNoncopyable flag and use it in all paths that can lead to creating a playground log.
Add unit tests verifying that the move-only types are omitted by the Playground Transform in various scenarios.

rdar://115035973

dbukowski · 2024-01-31T23:39:24Z

@swift-ci please test windows platform

dbukowski · 2024-01-31T23:39:39Z

@swift-ci please smoke test

…layground Transform, as such types are not supported in generics yet.

…Playground Transform.

dbukowski · 2024-02-01T21:41:23Z

@swift-ci please test windows platform

dbukowski · 2024-02-01T21:41:38Z

@swift-ci please smoke test

dbukowski · 2024-02-02T01:39:22Z

@swift-ci please test windows platform

xedin · 2024-02-15T19:03:27Z

lib/Sema/PlaygroundTransform.cpp

@@ -786,6 +799,14 @@ class Instrumenter : InstrumenterBase {
    return std::make_pair(PBD, VD);
  }

+  bool shouldLogType(Type Ty) {


Maybe generalize this over ASTNode instead i.e. shouldLog(ASTNode) so that you can cast to ValueDecl * internally to get its interface type?

bool shouldLog(ASTNode node) { if (auto *VD = node.dyn_cast<ValueDecl *>()) return VD->hasInterfaceType() ? !VD->getInterfaceType()->isNoncopyable() : true; if (auto *E = node.dyn_cast<Expr *>()) return !E->getType()->isNoncopyable(); return true; }

I tried this change and the CI is failing with:

/Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/ADT/STLExtras.h:172:42: error: implicit instantiation of undefined template 'llvm::FirstIndexOfType<swift::ValueDecl *>' : std::integral_constant<size_t, 1 + FirstIndexOfType<T, Us...>::value> {}; ^ /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/ADT/STLExtras.h:172:42: note: in instantiation of template class 'llvm::FirstIndexOfType<swift::ValueDecl *, swift::CaseLabelItem *>' requested here /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/ADT/STLExtras.h:172:42: note: in instantiation of template class 'llvm::FirstIndexOfType<swift::ValueDecl *, swift::StmtConditionElement *, swift::CaseLabelItem *>' requested here /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/ADT/STLExtras.h:172:42: note: in instantiation of template class 'llvm::FirstIndexOfType<swift::ValueDecl *, swift::TypeRepr *, swift::StmtConditionElement *, swift::CaseLabelItem *>' requested here /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/ADT/STLExtras.h:172:42: note: in instantiation of template class 'llvm::FirstIndexOfType<swift::ValueDecl *, swift::Pattern *, swift::TypeRepr *, swift::StmtConditionElement *, swift::CaseLabelItem *>' requested here /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/ADT/STLExtras.h:172:42: note: in instantiation of template class 'llvm::FirstIndexOfType<swift::ValueDecl *, swift::Decl *, swift::Pattern *, swift::TypeRepr *, swift::StmtConditionElement *, swift::CaseLabelItem *>' requested here /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/ADT/STLExtras.h:172:42: note: (skipping 1 context in backtrace; use -ftemplate-backtrace-limit=0 to see all) /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/ADT/PointerUnion.h:230:30: note: in instantiation of template class 'llvm::FirstIndexOfType<swift::ValueDecl *, swift::Expr *, swift::Stmt *, swift::Decl *, swift::Pattern *, swift::TypeRepr *, swift::StmtConditionElement *, swift::CaseLabelItem *>' requested here return F.Val.getInt() == FirstIndexOfType<To, PTs...>::value; ^ /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/ADT/PointerUnion.h:248:27: note: in instantiation of function template specialization 'llvm::CastInfoPointerUnionImpl<swift::Expr *, swift::Stmt *, swift::Decl *, swift::Pattern *, swift::TypeRepr *, swift::StmtConditionElement *, swift::CaseLabelItem *>::isPossible<swift::ValueDecl *>' requested here return Impl::template isPossible<To>(f); ^ /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/Support/Casting.h:311:19: note: in instantiation of member function 'llvm::CastInfo<swift::ValueDecl *, llvm::PointerUnion<swift::Expr *, swift::Stmt *, swift::Decl *, swift::Pattern *, swift::TypeRepr *, swift::StmtConditionElement *, swift::CaseLabelItem *>>::isPossible' requested here if (!Derived::isPossible(f)) ^ /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/llvm-project/llvm/include/llvm/Support/Casting.h:406:23: note: in instantiation of member function 'llvm::DefaultDoCastIfPossible<swift::ValueDecl *, llvm::PointerUnion<swift::Expr *, swift::Stmt *, swift::Decl *, swift::Pattern *, swift::TypeRepr *, swift::StmtConditionElement *, swift::CaseLabelItem *>, llvm::CastInfo<swift::ValueDecl *, llvm::PointerUnion<swift::Expr *, swift::Stmt *, swift::Decl *, swift::Pattern *, swift::TypeRepr *, swift::StmtConditionElement *, swift::CaseLabelItem *>>>::doCastIfPossible' requested here return ForwardTo::doCastIfPossible(const_cast<NonConstFrom>(f)); ^ /Users/ec2-user/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/lib/Sema/PlaygroundTransform.cpp:803:25: note: in instantiation of function template specialization 'llvm::PointerUnion<swift::Expr *, swift::Stmt *, swift::Decl *, swift::Pattern *, swift::TypeRepr *, swift::StmtConditionElement *, swift::CaseLabelItem *>::dyn_cast<swift::ValueDecl *>' requested here if (auto *VD = node.dyn_cast<ValueDecl *>())

ah, it's union with a Decl * so you need to do dyn_cast_or_null<ValueDecl>(node.dyn_cast<Decl *>())

Thanks Pavel, CI is green with this change now. Does the rest of the code look reasonable to you?

dbukowski · 2024-02-21T22:07:37Z

@swift-ci please smoke test

dbukowski · 2024-02-22T02:01:16Z

@swift-ci please smoke test

dbukowski · 2024-02-22T17:06:19Z

@swift-ci please test windows platform

dbukowski · 2024-02-22T19:52:52Z

@swift-ci please test windows platform

xedin

Looks good with one small note I left inline. Do you want to do the same for ~Escapable?

xedin · 2024-02-22T23:09:19Z

lib/Sema/PlaygroundTransform.cpp

@@ -686,6 +694,11 @@ class Instrumenter : InstrumenterBase {
    } else if (auto *MRE = dyn_cast<MemberRefExpr>(*RE)) {
      Expr *B = MRE->getBase();
      ConcreteDeclRef M = MRE->getMember();
+      VarDecl *VD = cast<VarDecl>(M.getDecl());


I don't think you need to do this, can pass M.getDecl() to shouldLog directly.

Good point, thanks! Removed.

dbukowski · 2024-02-22T23:26:29Z

@swift-ci please smoke test

dbukowski · 2024-02-22T23:30:30Z

Do you want to do the same for ~Escapable?

Oh, we should definitely do that. I can address that in a separate PR.

xedin · 2024-02-22T23:31:29Z

Sounds good to me!

dbukowski · 2024-02-23T00:23:53Z

@swift-ci please smoke test

xedin · 2024-02-23T17:19:29Z

@swift-ci please smoke test

abertelrud

LGTM! Thanks @dbukowski!

slavapestov

Please test with -enable-experimental-feature NoncopyableGenerics

slavapestov · 2024-02-23T21:38:09Z

lib/Sema/PlaygroundTransform.cpp

@@ -786,6 +799,14 @@ class Instrumenter : InstrumenterBase {
    return std::make_pair(PBD, VD);
  }

+  bool shouldLogType(Type Ty) {
+    // Don't try to log ~Copyable types, as we can't pass them to the generic logging functions yet.
+    if (Ty->isNoncopyable()) {


You can’t pass interface types to isNoncopyable(), it will assert once the noncopyable generics flag is on.

slavapestov · 2024-02-23T21:38:51Z

lib/Sema/PlaygroundTransform.cpp

@@ -686,6 +694,11 @@ class Instrumenter : InstrumenterBase {
    } else if (auto *MRE = dyn_cast<MemberRefExpr>(*RE)) {
      Expr *B = MRE->getBase();
      ConcreteDeclRef M = MRE->getMember();
+      VarDecl *VD = cast<VarDecl>(M.getDecl());
+
+      if (!shouldLogType(VD->getInterfaceType())) {


Use VD->getTypeInContext()

slavapestov · 2024-02-23T21:39:17Z

lib/Sema/PlaygroundTransform.cpp

@@ -658,6 +658,10 @@ class Instrumenter : InstrumenterBase {
  // after or instead of the expression they're looking at.  Only call this
  // if the variable has an initializer.
  Added<Stmt *> logVarDecl(VarDecl *VD) {
+    if (!shouldLogType(VD->getInterfaceType())) {


VD-getTypeInContext()

slavapestov · 2024-02-23T21:39:35Z

lib/Sema/PlaygroundTransform.cpp

@@ -674,6 +678,10 @@ class Instrumenter : InstrumenterBase {
    if (auto *DRE = dyn_cast<DeclRefExpr>(*RE)) {
      VarDecl *VD = cast<VarDecl>(DRE->getDecl());

+      if (!shouldLogType(VD->getInterfaceType())) {


VD->getTypeInContext()

slavapestov · 2024-02-23T21:41:43Z

lib/Sema/PlaygroundTransform.cpp

+  bool shouldLog(ASTNode node) {
+    // Don't try to log ~Copyable types, as we can't pass them to the generic logging functions yet.
+    if (auto *VD = dyn_cast_or_null<ValueDecl>(node.dyn_cast<Decl *>()))
+      return VD->hasInterfaceType() ? !VD->getInterfaceType()->isNoncopyable() : true;


Don’t call hasInterfaceType(), it doesn’t do what you think it does (it means “have I computed the type yet” not “this has a type”)

Thanks Slava! Based on your comments, it sounds like shouldLog should look something like this:

bool shouldLog(ASTNode node) { // Don't try to log ~Copyable types, as we can't pass them to the generic logging functions yet. if (auto *VD = dyn_cast_or_null<ValueDecl>(node.dyn_cast<Decl *>())) return !VD->getTypeInContext()->isNoncopyable(); if (auto *E = node.dyn_cast<Expr *>()) return !E->getType()->isNoncopyable(); return true; }

Does that look reasonable? Unfortunately, I have already merged this PR, so I would open a new one with the necessary changes (after testing with -enable-experimental-feature NoncopyableGenerics as you suggested).

Yes that looks good, thanks!

dbukowski requested review from jckarter and cwakamo January 31, 2024 23:34

dbukowski self-assigned this Jan 31, 2024

dbukowski requested review from hborla, slavapestov and xedin as code owners January 31, 2024 23:34

dbukowski added 2 commits February 1, 2024 13:39

Disable passing move-only types to our generic logging functions in P…

d459ec8

…layground Transform, as such types are not supported in generics yet.

Add unit tests verifying that the move only types are omitted by the …

af5f647

…Playground Transform.

dbukowski force-pushed the eng/dbukowski/disable-logging-moveonly-types-in-playground-transform branch from ad61364 to af5f647 Compare February 1, 2024 21:40

dbukowski requested a review from abertelrud February 8, 2024 21:04

xedin reviewed Feb 15, 2024

View reviewed changes

dbukowski force-pushed the eng/dbukowski/disable-logging-moveonly-types-in-playground-transform branch from 03e769e to 4d6284d Compare February 22, 2024 02:00

xedin approved these changes Feb 22, 2024

View reviewed changes

Generalize shouldLog over ASTNode as suggested in Code Review.

e0cbc62

dbukowski force-pushed the eng/dbukowski/disable-logging-moveonly-types-in-playground-transform branch from 4d6284d to e0cbc62 Compare February 22, 2024 23:25

abertelrud approved these changes Feb 23, 2024

View reviewed changes

dbukowski merged commit a16372e into main Feb 23, 2024

dbukowski deleted the eng/dbukowski/disable-logging-moveonly-types-in-playground-transform branch February 23, 2024 21:27

slavapestov reviewed Feb 23, 2024

View reviewed changes

[PlaygroundTransform] Disable logging ~Copyable types as they can’t be passed to the generic logging function #71293

[PlaygroundTransform] Disable logging ~Copyable types as they can’t be passed to the generic logging function #71293

Uh oh!

Conversation

dbukowski commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Uh oh!

dbukowski commented Jan 31, 2024

Uh oh!

dbukowski commented Jan 31, 2024

Uh oh!

dbukowski commented Feb 1, 2024

Uh oh!

dbukowski commented Feb 1, 2024

Uh oh!

dbukowski commented Feb 2, 2024

Uh oh!

xedin Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbukowski commented Feb 21, 2024

Uh oh!

dbukowski commented Feb 22, 2024

Uh oh!

dbukowski commented Feb 22, 2024

Uh oh!

dbukowski commented Feb 22, 2024

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbukowski commented Feb 22, 2024

Uh oh!

dbukowski commented Feb 22, 2024

Uh oh!

xedin commented Feb 22, 2024

Uh oh!

dbukowski commented Feb 23, 2024

Uh oh!

xedin commented Feb 23, 2024

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[PlaygroundTransform] Disable logging `~Copyable` types as they can’t be passed to the generic logging function #71293

[PlaygroundTransform] Disable logging `~Copyable` types as they can’t be passed to the generic logging function #71293

dbukowski commented Jan 31, 2024 •

edited

Loading

xedin Feb 15, 2024 •

edited

Loading