[Concurrency] Allow default arguments to require actor isolation. #68794

hborla · 2023-09-27T06:11:42Z

Type checking a default argument expression will compute the required actor isolation for evaluating that argument value synchronously. Actor isolation checking is deferred to the caller; it is an error to use a default argument from across isolation domains.

This change enables code like the following:

@MainActor
func requiresMainActor() -> Int { 0 }

@MainActor
func useMainActorDefault(value: Int = requiresMainActor()) { ... }

@MainActor
func mainActorCaller() {
  useMainActorDefault() // okay
}

func nonisolatedCaller() {
  useMainActorDefault() // error
}

Currently gated behind -enable-experimental-feature IsolatedDefaultArguments.

Resolves #58177

ktoso

Nice! Had a question inline and maybe one additional case to cover with a test?

ktoso · 2023-09-29T06:17:40Z

lib/Sema/TypeCheckConcurrency.cpp

@@ -2123,6 +2129,94 @@ namespace {
      }
    }

+    bool refineRequiredIsolation(ActorIsolation refinedIsolation) {
+      if (requiredIsolationLoc.isInvalid())
+        return false;


For my education, because I've been bitten by checking Loc validity for logic before... Why are we returning early if that happens here? I think this can happen across modules, but the isolation is expected to be always in the same module here so that's why? Wanted to understand that bit to avoid missing handling similar cases 🤔

We'll never compute the required isolation of a default argument or initializer expression across modules because we don't serialize expressions; we compute the required isolation in the original module and we need to serialize the required isolation to be checked at call-sites in other modules, so we should always have a source location here. However, I have not implemented nor tested the cross-module case yet 🙂

The reason I'm returning early here is because I'm using the validity of this source location to turn on the "required isolation" computation, so there's nothing to do here if the location isn't valid.

I see, so really detecting cross module here kindof... Thanks for explaining

ktoso · 2023-09-29T06:19:54Z

test/Concurrency/isolated_default_arguments.swift

+  // expected-error@+1 {{default argument cannot be both main actor-isolated and global actor 'SomeGlobalActor'-isolated}}
+  closure: () -> Void = {
+    _ = requiresMainActor()
+    _ = requiresSomeGlobalActor()


that's a pretty nice diagnostic for these, how it falls out of the inference here... nice

ktoso · 2023-09-29T06:22:18Z

test/Concurrency/isolated_default_arguments.swift

+    // expected-error@+1 {{call to main actor-isolated global function 'requiresMainActor()' in a synchronous actor-isolated context}}
+    _ = requiresMainActor()
+  }
+) {}


I guess it is covered implicitly, but maybe worth adding test for

extension A { func closureWithIsolatedParam( closure: () -> Void = { _ in _ = requiresMainActor() } ) {} }

that's somewhat interesting... that would fail when called inside actor, but across could actually work...? Hop to main for param, then hop to invoke method...? I'm not sure what would happen today tbh, so may be worth the test-case

Default arguments are always evaluated in the caller's isolation domain, so using a MainActor-isolated default argument to an actor-isolated method is conceptually "fine" - it only has implications on where you can use the default argument from. However, I'm considering an (artificial) restriction that the required isolation has to be either nonisolated or the same isolation as the function itself because I'm not sure why you would want a default argument that has any other isolation.

Gotcha that makes sense; I'm not sure about the artificial restriction, I guess things fall into place and can work without it 🤔

hborla · 2023-09-29T06:24:49Z

@swift-ci please smoke test

hborla · 2023-09-30T04:25:31Z

@swift-ci please smoke test

hborla · 2023-10-01T02:11:04Z

@swift-ci please smoke test

hborla · 2023-10-01T02:11:20Z

@swift-ci please build toolchain

lib/Sema/TypeCheckDeclPrimary.cpp

lib/Sema/TypeCheckStorage.cpp

test/Concurrency/isolated_default_property_inits.swift

hborla · 2023-10-04T02:53:33Z

@swift-ci please smoke test

hborla · 2023-10-04T02:55:14Z

@swift-ci please smoke test

Type checking a default argument expression will compute the required actor isolation for evaluating that argument value synchronously. Actor isolation checking is deferred to the caller; it is an error to use a default argument from across isolation domains. Currently gated behind -enable-experimental-feature IsolatedDefaultArguments.

…d isolation for a default argument, and improve diagnostics for conflicting isolation.

properties to require actor isolation. Member initializer expressions are only used in a constructor with matching actor isolation. If the isolation prohibits the member initializer from being evaluated synchronously (or propagating required isolation through closure bodies), then the default value cannot be used and the member must be explicitly initialized in the constructor. Member initializer expressions are also used as default arguments for the memberwise initializer, and the same rules for default argument isolation apply.

…allers to go through the DefaultInitializerIsolation request.

hborla · 2023-10-04T22:17:42Z

@swift-ci please smoke test

hborla mentioned this pull request Sep 29, 2023

[SR-15916] @MainActor-isolated default arguments for @MainActor-isolated functions #58177

Closed

hborla force-pushed the isolated-initializer-expressions branch 3 times, most recently from 36b2a9e to b98cc28 Compare September 29, 2023 06:07

ktoso approved these changes Sep 29, 2023

View reviewed changes

ktoso added the concurrency Feature: umbrella label for concurrency language features label Sep 29, 2023

hborla force-pushed the isolated-initializer-expressions branch 3 times, most recently from debc7d5 to 8ad3e04 Compare September 30, 2023 04:19

hborla force-pushed the isolated-initializer-expressions branch 3 times, most recently from c44147d to 43ade00 Compare October 1, 2023 02:10

hborla marked this pull request as ready for review October 1, 2023 02:11

hborla requested review from kavon, slavapestov, xedin and tshortli as code owners October 1, 2023 02:11

xedin approved these changes Oct 3, 2023

View reviewed changes

lib/Sema/TypeCheckDeclPrimary.cpp Outdated Show resolved Hide resolved

lib/Sema/TypeCheckStorage.cpp Show resolved Hide resolved

test/Concurrency/isolated_default_property_inits.swift Show resolved Hide resolved

hborla force-pushed the isolated-initializer-expressions branch from a398602 to 7d39b93 Compare October 4, 2023 02:54

hborla added 5 commits October 4, 2023 13:12

[Concurrency] Handle nested decl contexts while computing the require…

88b2332

…d isolation for a default argument, and improve diagnostics for conflicting isolation.

[Concurrency] Serialize default argument isolation.

3cb9bb8

[Concurrency] Remove swift::computeRequiredIsolation and change all c…

67e3326

…allers to go through the DefaultInitializerIsolation request.

hborla force-pushed the isolated-initializer-expressions branch from 7d39b93 to 67e3326 Compare October 4, 2023 22:17

hborla merged commit f7a8bc2 into swiftlang:main Oct 5, 2023

hborla deleted the isolated-initializer-expressions branch October 5, 2023 03:10

hborla mentioned this pull request Oct 12, 2023

[5.10][Concurrency] Allow default arguments to require actor isolation. #69149

Merged

[Concurrency] Allow default arguments to require actor isolation. #68794

[Concurrency] Allow default arguments to require actor isolation. #68794

Uh oh!

Conversation

hborla commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

ktoso Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

hborla Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

ktoso Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

ktoso Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

ktoso Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

hborla Sep 30, 2023

Choose a reason for hiding this comment

Uh oh!

ktoso Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

hborla commented Sep 29, 2023

Uh oh!

hborla commented Sep 30, 2023

Uh oh!

hborla commented Oct 1, 2023

Uh oh!

hborla commented Oct 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hborla commented Oct 4, 2023

Uh oh!

hborla commented Oct 4, 2023

Uh oh!

hborla commented Oct 4, 2023

Uh oh!

Uh oh!

hborla commented Sep 27, 2023 •

edited

Loading