[CodeComplete] Mark results as not recommended if global actor context doesn't match #37711

ahoppen · 2021-05-31T19:06:53Z

Annotate cross-actor references to global actors as async and mark them as not-recommended if they must be executed on a different actor than the current context is on.

Resolves rdar://75902598

ahoppen · 2021-06-15T09:43:31Z

@swift-ci Please test

rintaro · 2021-06-15T17:15:52Z

lib/Sema/TypeCheckStmt.cpp

@@ -1949,6 +1949,7 @@ bool TypeCheckASTNodeAtLocRequest::evaluate(Evaluator &evaluator,
  if (auto CE = dyn_cast<ClosureExpr>(DC)) {
    if (CE->getBodyState() == ClosureExpr::BodyState::Parsed) {
      swift::typeCheckASTNodeAtLoc(CE->getParent(), CE->getLoc());
+      checkClosureActorIsolation(CE);


I'm concerned that ActorIsolationChecker walk into unrelated sibling closures or the children of the current closure. Since TypeCheckASTNodeAtLocRequest::evaluate is called multiple times in a function from the outer closure to inner closure, in this implementation, the walker walks into the inner closures multiple times.

How do you think about exposing ActorIsolationChecker::determineClosureIsolation() and call it directly?

ActorIsolationChecker checker(closure->getParent()); closure->setActorIsolation(checker->determineClosureIsolation(closure));

Or, how about calling checkFunctionActorIsolation() (or checkTopLevelActorIsolation()) in ide::typeCheckContextAt() right after calling typeCheckASTNodeAtLoc()?

Thanks. I didn’t realize that TypeCheckASTNodeAtLocRequest was called multiple times.

I think it’ll be good to just set the current closure’s actor isolation as you proposed in the first comment because it minimizes the amount of actor isolation checking that needs to be done. I’ll update it.

rintaro · 2021-06-15T17:30:20Z

lib/IDE/Refactoring.cpp

@@ -4571,7 +4571,8 @@ struct CallbackCondition {
  /// }
  /// ```
  CallbackCondition(const Decl *Subject, const CaseLabelItem *CaseItem) {
-    if (auto *EEP = dyn_cast<EnumElementPattern>(CaseItem->getPattern())) {
+    if (auto *EEP = dyn_cast<EnumElementPattern>(
+            CaseItem->getPattern()->getSemanticsProvidingPattern())) {


Unrelated changes?

Oh, yes. I’ll remove it.

ahoppen · 2021-06-15T21:46:09Z

@swift-ci Please test

swift-ci · 2021-06-16T02:06:52Z

Build failed
Swift Test OS X Platform
Git Sha - 9fcfbe59965475f3ffde62cf654f2158fceb4200

ahoppen · 2021-06-16T10:22:48Z

@swift-ci Please test macOS

ahoppen · 2021-06-16T10:57:47Z

@swift-ci Please test

swift-ci · 2021-06-16T15:20:02Z

Build failed
Swift Test OS X Platform
Git Sha - cfe51d27f873043d8720dddc3dcb7ad05880529c

rintaro · 2021-06-16T16:52:28Z

test/IDE/complete_global_actorisolation.swift

+actor MyActor {}
+
+@globalActor
+class MyGlobalActor {
+  static var shared = MyActor()
+}


Global actor definition has been revised in the proposal

A global actor is an actor type that has the @globalActor attribute and contains a static let property named shared that provides an instance of the actor.

The type of shared must be of the enclosing actor type

@globalActor actor MyGlobalActor { static let shared = MyGlobalActor() }

I know other test files still uses old syntax, but can we use the new syntax here?

I didn’t realize that. Thanks!

rintaro · 2021-06-16T17:18:22Z

test/IDE/complete_global_actorisolation.swift

+    }
+  }
+}
+


Could you add a nested single expression closure test? e.g.

@MyGlobalActor func globalFuncOnMyGlobalActor() {} @MyOtherGlobalActor func globalFuncOnMyOtherGlobalActor() {} func foo<T>(_: () async -> T) {} @MyGlobalActor func test() { foo { foo { <HERE> } } }

ahoppen · 2021-06-17T07:08:39Z

@swift-ci Please test

swift-ci · 2021-06-17T07:09:39Z

Build failed
Swift Test OS X Platform
Git Sha - c535eb54ca4e282e17430eb54694c77c2181eee7

rintaro · 2021-06-17T16:56:30Z

test/IDE/complete_global_actorisolation.swift

+
+@globalActor
+actor MyGlobalActor {
+  static var shared = MyActor()


I believe this must be MyGlobalActor in the new syntax.

The type of shared must be of the enclosing actor type

Sorry, I missed that part. Fixed it now.

…t doesn't match Annotate cross-actor references to global actors as `async` and mark them as not-recommended if they must be executed on a different actor than the current context is on. Resolves rdar://75902598

ahoppen · 2021-06-17T21:15:05Z

@swift-ci Please test

swift-ci · 2021-06-17T21:16:07Z

Build failed
Swift Test OS X Platform
Git Sha - 6725e2b

ahoppen · 2021-06-22T07:59:55Z

@swift-ci Please test macOS

swift-ci · 2021-06-22T08:54:05Z

Build failed
Swift Test OS X Platform
Git Sha - 6725e2b

ahoppen · 2021-06-22T21:57:53Z

@swift-ci Please test macOS

swift-ci · 2021-06-23T06:04:07Z

Build failed
Swift Test OS X Platform
Git Sha - 6725e2b

ahoppen · 2021-06-23T09:07:35Z

@swift-ci Please smoke test macOS

DougGregor · 2021-06-24T16:25:33Z

lib/Sema/TypeCheckStmt.cpp

+      // TypeCheckASTNodeAtLocRequest is called multiple times, as described
+      // above.
+      auto ActorIsolation = determineClosureActorIsolation(CE);
+      CE->setActorIsolation(ActorIsolation);


Hmm, it feels a bit dicey setting the actor isolation for a closure in more than one place. Should we request'ify determineClosureActorIsolation?

I agree that it’s not really pretty. But the problem is that the ActorIsolationChecker currently implicitly assumes that it’s walking the tree top to bottom.

IMHO the proper solution would be to completely requesting ActorIsolationChecker so that it works bottom to top, requesting the actor isolation of an outer type as needed. But I felt that was too big of a change for Swift 5.5 and that’s why I stuck with the current, more local, approach.

ahoppen requested review from rintaro and DougGregor May 31, 2021 19:06

ahoppen force-pushed the pr/global-actor-completion branch from c94a2e7 to 16785b1 Compare June 15, 2021 09:42

swiftlang deleted a comment from swift-ci Jun 15, 2021

rintaro reviewed Jun 15, 2021

View reviewed changes

ahoppen force-pushed the pr/global-actor-completion branch from 16785b1 to 9fcfbe5 Compare June 15, 2021 21:45

ahoppen force-pushed the pr/global-actor-completion branch from 9fcfbe5 to cfe51d2 Compare June 16, 2021 10:57

rintaro reviewed Jun 16, 2021

View reviewed changes

ahoppen force-pushed the pr/global-actor-completion branch from cfe51d2 to c535eb5 Compare June 17, 2021 07:08

rintaro reviewed Jun 17, 2021

View reviewed changes

ahoppen force-pushed the pr/global-actor-completion branch from c535eb5 to 6725e2b Compare June 17, 2021 21:14

rintaro approved these changes Jun 22, 2021

View reviewed changes

ahoppen merged commit 872ecb0 into swiftlang:main Jun 23, 2021

ahoppen deleted the pr/global-actor-completion branch June 23, 2021 16:49

ahoppen mentioned this pull request Jun 23, 2021

[5.5][CodeComplete] Mark results as not recommended if global actor context doesn't match #38056

Merged

DougGregor reviewed Jun 24, 2021

View reviewed changes

[CodeComplete] Mark results as not recommended if global actor context doesn't match #37711

[CodeComplete] Mark results as not recommended if global actor context doesn't match #37711

Uh oh!

Conversation

ahoppen commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Jun 15, 2021

Uh oh!

rintaro Jun 15, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jun 15, 2021

Uh oh!

swift-ci commented Jun 16, 2021

Uh oh!

ahoppen commented Jun 16, 2021

Uh oh!

ahoppen commented Jun 16, 2021

Uh oh!

swift-ci commented Jun 16, 2021

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!

ahoppen commented Jun 17, 2021

Uh oh!

swift-ci commented Jun 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jun 17, 2021

Uh oh!

swift-ci commented Jun 17, 2021

Uh oh!

ahoppen commented Jun 22, 2021

Uh oh!

swift-ci commented Jun 22, 2021

Uh oh!

ahoppen commented Jun 22, 2021

Uh oh!

swift-ci commented Jun 23, 2021

Uh oh!

ahoppen commented Jun 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented May 31, 2021 •

edited

Loading

rintaro Jun 15, 2021 •

edited

Loading