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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions lib/AST/Type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,6 +898,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.

auto newBaseTy =
arrayTy->getBaseType()->stripConcurrency(recurse, dropGlobalActor);
return newBaseTy->isEqual(arrayTy->getBaseType())
? Type(this)
: ArraySliceType::get(newBaseTy);
}

if (auto *dictTy = dyn_cast<DictionaryType>(this)) {
auto keyTy = dictTy->getKeyType();
auto strippedKeyTy = keyTy->stripConcurrency(recurse, dropGlobalActor);
auto valueTy = dictTy->getValueType();
auto strippedValueTy = valueTy->stripConcurrency(recurse, dropGlobalActor);

return keyTy->isEqual(strippedKeyTy) && valueTy->isEqual(strippedValueTy)
? Type(this)
: DictionaryType::get(strippedKeyTy, strippedValueTy);
}

// Look through optionals.
if (Type optionalObject = getOptionalObjectType()) {
Type newOptionalObject =
Expand Down Expand Up @@ -1083,25 +1102,6 @@ Type TypeBase::stripConcurrency(bool recurse, bool dropGlobalActor) {
return anyChanged ? TupleType::get(elts, getASTContext()) : Type(this);
}

if (auto *arrayTy = dyn_cast<ArraySliceType>(this)) {
auto newBaseTy =
arrayTy->getBaseType()->stripConcurrency(recurse, dropGlobalActor);
return newBaseTy->isEqual(arrayTy->getBaseType())
? Type(this)
: ArraySliceType::get(newBaseTy);
}

if (auto *dictTy = dyn_cast<DictionaryType>(this)) {
auto keyTy = dictTy->getKeyType();
auto strippedKeyTy = keyTy->stripConcurrency(recurse, dropGlobalActor);
auto valueTy = dictTy->getValueType();
auto strippedValueTy = valueTy->stripConcurrency(recurse, dropGlobalActor);

return keyTy->isEqual(strippedKeyTy) && valueTy->isEqual(strippedValueTy)
? Type(this)
: DictionaryType::get(strippedKeyTy, strippedValueTy);
}

return Type(this);
}

Expand Down
45 changes: 36 additions & 9 deletions lib/Sema/TypeCheckDeclOverride.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,6 +1202,8 @@ bool OverrideMatcher::checkOverride(ValueDecl *baseDecl,
// If this is an exact type match, we're successful!
Type declTy = getDeclComparisonType();
Type owningTy = dc->getDeclaredInterfaceType();
bool mismatchedOnSendability =
attempt == OverrideCheckingAttempt::MismatchedSendability;
auto isClassContext = classDecl != nullptr;
if (declIUOAttr == matchDeclIUOAttr && declTy->isEqual(baseTy)) {
// Nothing to do.
Expand Down Expand Up @@ -1263,17 +1265,42 @@ bool OverrideMatcher::checkOverride(ValueDecl *baseDecl,
CanType parentPropertyCanTy =
parentPropertyTy->getReducedType(
decl->getInnermostDeclContext()->getGenericSignatureOfContext());
if (!propertyTy->matches(parentPropertyCanTy,
TypeMatchFlags::AllowOverride)) {
diags.diagnose(property, diag::override_property_type_mismatch,
property->getName(), propertyTy, parentPropertyTy);
noteFixableMismatchedTypes(decl, baseDecl);
diags.diagnose(baseDecl, diag::property_override_here);
return true;

auto options = TypeMatchFlags::AllowOverride;
if (!propertyTy->matches(parentPropertyCanTy, options)) {
// The perfect match failed
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.

propertyTy = propertyTy->stripConcurrency(/*recursive=*/true,
/*dropGlobalActor=*/true);
parentPropertyCanTy =
Type(parentPropertyCanTy)->stripConcurrency(/*recursive=*/true,
/*dropGlobalActor=*/true)->getCanonicalType();

parentPropertyTy =
parentPropertyTy->stripConcurrency(/*recursive=*/true,
/*dropGlobalActor=*/true);


if (propertyTy->matches(parentPropertyCanTy, options)) {
mismatchedOnSendability = true;
failed = false;
}
}

if (failed) {
diags.diagnose(property, diag::override_property_type_mismatch,
property->getName(), propertyTy, parentPropertyTy);
noteFixableMismatchedTypes(decl, baseDecl);
diags.diagnose(baseDecl, diag::property_override_here);
return true;
}
}

// Differing only in Optional vs. ImplicitlyUnwrappedOptional is fine.
bool IsSilentDifference = false;
bool IsSilentDifference = mismatchedOnSendability;
if (auto propertyTyNoOptional = propertyTy->getOptionalObjectType())
if (auto parentPropertyTyNoOptional =
parentPropertyTy->getOptionalObjectType())
Expand All @@ -1293,7 +1320,7 @@ bool OverrideMatcher::checkOverride(ValueDecl *baseDecl,
if (emittedMatchError)
return true;

if (attempt == OverrideCheckingAttempt::MismatchedSendability) {
if (mismatchedOnSendability) {
SendableCheckContext fromContext(decl->getDeclContext(),
SendableCheck::Explicit);
auto baseDeclClass = baseDecl->getDeclContext()->getSelfClassDecl();
Expand Down
18 changes: 18 additions & 0 deletions test/Concurrency/predates_concurrency.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,21 @@ extension MainActorPreconcurrency: NotIsolated {
}
}
}


open class Parent {
init(userInfo: [String : Any]) {
self.userInfo = userInfo
}

@preconcurrency open var userInfo : [String : any Sendable] // expected-note {{overridden declaration is here}}
}

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.

get {
[:]
}
set {}
}
}