Skip to content

[Sema] Warn about 'Any' to 'any Sendable' override #73543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

angela-laar
Copy link
Contributor

Until Swift 6, warn about overriding Any property in parent class to any Sendable

@angela-laar
Copy link
Contributor Author

@swift-ci please smoke test

@angela-laar angela-laar marked this pull request as ready for review May 9, 2024 21:24
@angela-laar angela-laar force-pushed the warn-if-sendable-override-mismatch branch from 919483f to 9b22b0b Compare May 9, 2024 21:48
@angela-laar angela-laar requested a review from ktoso as a code owner May 9, 2024 21:48
bool failed = true;
if (baseDecl->preconcurrency() &&
(attempt == OverrideCheckingAttempt::PerfectMatch ||
attempt == OverrideCheckingAttempt::MismatchedSendability)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One major drawback of this approach is that it strips concurrency annotations from both sides which means that override with concurrency annotations would match against parent without annotation(s) in the same position which is not the case for witness matching today. For example:

protocol P {
    @preconcurrency func test(_: Any)
}

struct S : P {
    func test(_: any Sendable) {}
}

Produces the following error:

error: type 'S' does not conform to protocol 'P'
1 | protocol P {
2 |     @preconcurrency func test(_: Any)
  |                          `- note: protocol requires function 'test' with type '(Any) -> ()'; add a stub for conformance
3 | }
4 |
5 | struct S : P {
  |        `- error: type 'S' does not conform to protocol 'P'
6 |     func test(_: any Sendable) {}
  |          `- note: candidate has non-matching type '(any Sendable) -> ()'
7 | }

Taking a step back here, at line https://github.com/apple/swift/pull/73543/files#diff-149ea7785899898da39642522a1b7d555757f96e8a3d53f045f9ae80e04c361bR1208 the types are checked for exact match which means that it should be possible to set a new flag based on whether base is marked as preconcurrency or not before attempting propertyTy->matches(parentPropertyCanTy, options) and handle it inside matches but that would make it harder to deal with check that compares object types (that happens right under this one) and diagnose sendability mismatches as warnings, so I'd like to propose a combination:

  1. We still ran if (!propertyTy->matches(parentPropertyCanTy, options)) first;
  2. If that fails we add TypeMatchFlags::IgnoreSendability and TypeMatchFlags::IgnoreFunctionSendability and re-run matches
  3. If propertyTy->matches(parentPropertyCanTy, options) returns true we set mismatchedOnSendability = true; this would allow us diagnose sendability mismatch and avoid dealing with optionality mismatches diagnosed in https://github.com/apple/swift/pull/73543/files#diff-149ea7785899898da39642522a1b7d555757f96e8a3d53f045f9ae80e04c361bR1304
  4. Change matches to handle the case where any Sendable is matched against Any and ignore it if IgnoreSendability flag is set.
  • This means that matches has to be a bit more work by walking into tuples and collections but only if the flag is set.

}

class Child : Parent {
override var userInfo: [String : Any] { // expected-warning {{declaration 'userInfo' has a type with different sendability from any potential overrides; this is an error in the Swift 6 language mode}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment about adding more tests with different combinations of attributes and any Sendable use in the previous PR.

@@ -905,6 +905,25 @@ unsigned int TypeBase::getOptionalityDepth() {
}

Type TypeBase::stripConcurrency(bool recurse, bool dropGlobalActor) {
if (auto *arrayTy = dyn_cast<ArraySliceType>(this)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not technically tied for this PR since matching happens on canonical types, you can land this separately.

Until Swift 6, warn about overriding Any property in parent class to any Sendable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants