Add `_unavailableFromAsync` attribute #40149

etcwilde · 2021-11-12T02:56:15Z

This PR introduces a new underscored attribute _unavailableFromAsync, which marks synchronous API as being unavailable from asynchronous contexts. Using an annotated API directly in an asynchronous context will result in a compiler warning message.

https://forums.swift.org/t/discussion-unavailability-from-asynchronous-contexts/53088
rdar://70256865

etcwilde · 2021-11-12T02:56:23Z

@swift-ci please test

etcwilde · 2021-11-12T04:56:22Z

Windows bot disk is full.

DougGregor · 2021-11-12T04:57:52Z

lib/Sema/TypeCheckConcurrency.cpp

+    }
+
+  public:
+    UsageWalker(AbstractFunctionDecl &afd) : baseDecl(afd), ctx(afd.getASTContext()) {}


There are a bunch of other kinds of "declaration references" that can show up in the AST that aren't DeclRefExprs, such as MemberRefExpr and SubscriptExpr, and using your own usage walker is likely to get tedious as you find all of the corner cases. I recommend hooking into an existing place where we do checking (e.g., swift::diagnoseDeclAvailability) so you can have just one check.

DougGregor · 2021-11-12T04:59:40Z

lib/Sema/TypeCheckStmt.cpp

@@ -2058,6 +2058,7 @@ TypeCheckFunctionBodyRequest::evaluate(Evaluator &evaluator,
  TypeChecker::computeCaptures(AFD);
  if (!AFD->getDeclContext()->isLocalContext()) {
    checkFunctionActorIsolation(AFD);
+    checkAsyncAvailability(*AFD);


Similar to my comment above, there are actually a couple of different places where we would need to add this call (see, e.g., the other check<something>ActorIsolation function callers), but if you hook into swift::diagnoseDeclAvailability you'll get all of that for free.

The challenge with moving the diagnostic to diagnoseDeclAvailability is that we need to be able to ask the declContext whether we're directly in an async context. For AbstractClosureExpr::isAsyncBody, there needs to be a type assigned to the expression. diganoseDeclAvailability is run at a time where we may not have a type or see the expression with a type associated with it.

https://github.com/apple/swift/blob/05fa143a8894bd66c6179abb2928444ae49c04cf/lib/Sema/TypeCheckConstraints.cpp#L400-L418

In TypeChecker::typeCheckExpression, we attempt to apply a solution before calling performSyntacticDiagnosticsForTarget, which eventually calls into diagnoseDeclAvailability. For the simpler test cases, that goes through and the closure expression gets updated with its type. For the more complicated closures like the ones in https://github.com/apple/swift/blob/05fa143a8894bd66c6179abb2928444ae49c04cf/test/Constraints/result_builder.swift, the updated expression doesn't have a type. The constraint system is able to produce a type, but doesn't apply it to the expression until after the children have all type checked, so we don't see the type applied to the decl-context.

https://github.com/apple/swift/blob/cd8da12e0af806642816afefbcd7fd3dbe2c81b3/lib/Sema/CSClosure.cpp#L1393-L1394

There is a type somewhere, though I'm not sure if driving the type from the constraint system, through the availability checking, and into the async checker is the right solution. For the time being, I'm skipping the check and saying that everything is fine.

DougGregor · 2021-11-12T05:03:42Z

include/swift/AST/DiagnosticsSema.def

@@ -4699,6 +4699,11 @@ ERROR(actor_isolation_superclass_mismatch,none,
      "%0 class %1 has different actor isolation from %2 superclass %3",
      (ActorIsolation, DeclName, ActorIsolation, DeclName))

+ERROR(warn_async_function_must_be_available_from_async,none,
+        "asynchronous function %0 must be available from asynchronous contexts", (DeclName))
+WARNING(warn_async_unavailable_decl,none,


Do we think this should be an error in Swift 6 mode, or will it always be a warning? Or should there be more of a distinction for the user about, e.g., "deprecated" (warning) vs. "unavailable" (error, albeit maybe only in Swift 6)?

DougGregor · 2021-11-12T05:03:57Z

include/swift/AST/DiagnosticsSema.def

@@ -4699,6 +4699,11 @@ ERROR(actor_isolation_superclass_mismatch,none,
      "%0 class %1 has different actor isolation from %2 superclass %3",
      (ActorIsolation, DeclName, ActorIsolation, DeclName))

+ERROR(warn_async_function_must_be_available_from_async,none,


Minor nit: it's an error but it's prefixed with warn_.

DougGregor · 2021-11-12T05:05:17Z

lib/Sema/TypeCheckConcurrency.cpp

+  if (decl.hasAsync() && decl.getAttrs().hasAttribute<UnavailableFromAsyncAttr>())
+    decl.getASTContext().Diags.diagnose(decl.getLoc(),
+        diag::warn_async_function_must_be_available_from_async,
+        decl.getName());


Presumably we need the same kind of check for an unavailable-from-async property, subscript, or initializer, all of which can be async. Could you move this to TypeCheckAttr.cpp, where we validate the correctness of attributes that are applied to declarations?

DougGregor · 2021-11-12T05:06:00Z

lib/Sema/TypeCheckAttr.cpp

@@ -119,6 +119,7 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
  IGNORED_ATTR(ImplicitSelfCapture)
  IGNORED_ATTR(InheritActorContext)
  IGNORED_ATTR(Isolated)
+  IGNORED_ATTR(UnavailableFromAsync)


... here's where I think we should do the "you can't put this attribute on an async declaration" check.

test/ClangImporter/objc_async.swift

etcwilde · 2021-11-19T00:03:54Z

@swift-ci please test

swift-ci · 2021-11-19T01:15:50Z

Build failed
Swift Test Linux Platform
Git Sha - 154d4bc105d727ff1cb2715bda13932f5bb3a1f7

swift-ci · 2021-11-19T02:38:51Z

Build failed
Swift Test OS X Platform
Git Sha - 154d4bc105d727ff1cb2715bda13932f5bb3a1f7

etcwilde · 2021-11-29T19:00:18Z

@swift-ci please test

swift-ci · 2021-11-29T20:33:34Z

Build failed
Swift Test Linux Platform
Git Sha - 39e71d210f5687fa5398a395864dc03bea894121

swift-ci · 2021-11-29T20:58:46Z

Build failed
Swift Test OS X Platform
Git Sha - 39e71d210f5687fa5398a395864dc03bea894121

Querying the context for whether it is an async context is the easiest way to detect if the context is async. Many decl contexts can be checked at any point, but AbstractClosureExpr contexts must have been typechecked. If this is called on an AbstractClosureExpr before the types have been assigned, it may either crash or do bad things.

This patch adds a bunch of tests to verify the behaviour of the @_unavailableFromAsync attribute. The attribute is only allowed on functions, so we verify that an error is emitted when it is applied to structs, extensions, classes, and actors. The attribute may be applied to constructors to disallow construction in an async context, but cannot be applied to destructors since a destructor must be callable from anywhere. Additionally, the attribute cannot be applied to asynchronous functions since an async function _must_ be called from an async context. Specific checks include - Errors are emitted in an async context (global function, e.g.) - Errors are emitted when the async context is nested in a sync context - Errors are not emitted from a sync context nested in an async context This patch also includes verification that the attribute is propagated across module boundaries and is be imported from ObjC functions. Lastly, this patch adds the IDE completion testing to verify that the attribute is considered.

The core part of the check runs as follows 1. Are we in an async context 2. Does the decl we're calling have the unavailableFromAsync attribute 3. Emit a diagnostic There are a couple challenges that muddy the implementation. First, single-expression closures are typechecked differently than multi-expression closures. The single-expression closure is never added to the closure stack. We have to record it separately upon entry to verify separately. This is necessary for `_ = { foo() }()` where `foo` is unavailable from async, and the surrounding context is async. The second challenge is regarding when things get typechecked. A type must be assigned to AbstractClosureExprs to determine whether they are an async context or not. Unfortunately, availability checking runs before types are fully assigned in some cases. This specifically happens when working with result builders in specific conditions. I haven't been able to figure out how to reduce the issue further.

Instead of tracking the single-expression closures in a separate structure and passing that in under the right conditions, it makes more sense to simply set the 'Where' decl context to the single-expr closure and use the correct declaration context to determine whether the context is async. The reduces the number of variables that need to get plumbed through to the actual unavailable-from-async check and simplifies the actual check from figuring out whether we're in a single-expr closure or in an async context.

etcwilde · 2021-11-30T22:32:33Z

#40341

@swift-ci please test

swift-ci · 2021-12-01T00:52:07Z

Build failed
Swift Test OS X Platform
Git Sha - c57b80a

etcwilde · 2021-12-01T00:55:36Z

Failure was due to the warning

/Users/buildnode/jenkins/workspace/swift-PR-macos/branch-main/swift/test/ClangImporter/objc_async.swift:127:3: error: unexpected warning produced: conformance of 'NonSendableClass' to 'Sendable' is unavailable
  takesSendable(nonSendableClass)     // expected-FIXME-warning{{something about missing conformance}}
  ^

CI apparently doesn't pick up branches made to the same repo, so we will have to wait for #40341 to merge before this will pass.

etcwilde · 2021-12-01T05:05:22Z

@swift-ci please test macOS

etcwilde · 2021-12-01T19:05:57Z

@swift-ci please test

etcwilde · 2021-12-01T19:48:18Z

unrelated windows failure.

@swift-ci please test windows

etcwilde · 2021-12-02T16:58:22Z

@swift-ci please test windows

etcwilde · 2021-12-02T17:59:51Z

Requires #40368 to pass windows test.

etcwilde · 2021-12-02T21:08:29Z

@swift-ci please test windows

DougGregor

This is looking great!

include/swift/AST/Attr.def

DougGregor · 2021-12-03T22:23:43Z

lib/Sema/TypeCheckAvailability.cpp

+static bool
+diagnoseDeclUnavailableFromAsync(const ValueDecl *D, SourceRange R,
+                                 const Expr *call, const ExportContext &Where) {
+  // FIXME: I don't think this is right, but I don't understand the issue well


This is a bit of a depressing comment, but... okay. There's certainly a bug somewhere if we're getting this code before constraint application.

Yeah, I'm probably going to need some help with someone more familiar with the constraint solver and result builders to get this fully debugged.

test/IDE/complete_decl_attribute.swift

Just making this user inaccessible until I go through the full proposal pipeline.

etcwilde · 2021-12-03T22:55:11Z

@swift-ci please test

swift-ci · 2021-12-04T00:18:53Z

Build failed
Swift Test Linux Platform
Git Sha - a84650b

etcwilde · 2021-12-04T00:20:05Z

@swift-ci please clean test Linux

swift-ci · 2021-12-04T02:59:04Z

Build failed
Swift Test Linux Platform
Git Sha - a84650b

etcwilde · 2021-12-06T16:42:55Z

@swift-ci please test Linux

etcwilde requested a review from DougGregor November 12, 2021 02:56

DougGregor requested changes Nov 12, 2021

View reviewed changes

etcwilde force-pushed the ewilde/concurrency/underscored-unavailablefromasync branch from 154d4bc to 39e71d2 Compare November 29, 2021 18:59

etcwilde force-pushed the ewilde/concurrency/underscored-unavailablefromasync branch from 39e71d2 to 49e225f Compare November 30, 2021 21:49

etcwilde added 3 commits November 30, 2021 14:20

etcwilde force-pushed the ewilde/concurrency/underscored-unavailablefromasync branch from 49e225f to c57b80a Compare November 30, 2021 22:31

etcwilde requested a review from DougGregor November 30, 2021 23:17

Update ModuleFormat to reflect unavailablefromasync

ed81cf2

etcwilde mentioned this pull request Dec 2, 2021

Adding optional message to _unavailableFromAsync #40378

Merged

DougGregor approved these changes Dec 3, 2021

View reviewed changes

Make _unavailableFromAsync user inaccessible

a84650b

Just making this user inaccessible until I go through the full proposal pipeline.

etcwilde merged commit cce3e8a into swiftlang:main Dec 6, 2021

Add _unavailableFromAsync attribute #40149

Add _unavailableFromAsync attribute #40149

Uh oh!

Conversation

etcwilde commented Nov 12, 2021

Uh oh!

etcwilde commented Nov 12, 2021

Uh oh!

etcwilde commented Nov 12, 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!

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!

etcwilde commented Nov 19, 2021

Uh oh!

swift-ci commented Nov 19, 2021

Uh oh!

swift-ci commented Nov 19, 2021

Uh oh!

etcwilde commented Nov 29, 2021

Uh oh!

swift-ci commented Nov 29, 2021

Uh oh!

swift-ci commented Nov 29, 2021

Uh oh!

etcwilde commented Nov 30, 2021

Uh oh!

swift-ci commented Dec 1, 2021

Uh oh!

etcwilde commented Dec 1, 2021

Uh oh!

etcwilde commented Dec 1, 2021

Uh oh!

etcwilde commented Dec 1, 2021

Uh oh!

etcwilde commented Dec 1, 2021

Uh oh!

etcwilde commented Dec 2, 2021

Uh oh!

etcwilde commented Dec 2, 2021

Uh oh!

etcwilde commented Dec 2, 2021

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

etcwilde commented Dec 3, 2021

Uh oh!

swift-ci commented Dec 4, 2021

Uh oh!

etcwilde commented Dec 4, 2021

Uh oh!

swift-ci commented Dec 4, 2021

Uh oh!

etcwilde commented Dec 6, 2021

Uh oh!

Uh oh!

Add `_unavailableFromAsync` attribute #40149

Add `_unavailableFromAsync` attribute #40149