-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Make overloads and witnesses permit @Sendable variance #42194
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -518,6 +518,7 @@ static bool noteFixableMismatchedTypes(ValueDecl *decl, const ValueDecl *base) { | |
namespace { | ||
enum class OverrideCheckingAttempt { | ||
PerfectMatch, | ||
MismatchedSendability, | ||
MismatchedOptional, | ||
MismatchedTypes, | ||
BaseName, | ||
|
@@ -547,6 +548,12 @@ static void diagnoseGeneralOverrideFailure(ValueDecl *decl, | |
diags.diagnose(decl, diag::override_multiple_decls_base, | ||
decl->getName()); | ||
break; | ||
case OverrideCheckingAttempt::MismatchedSendability: | ||
// FIXME: suppress if any matches brought in via @preconcurrency import? | ||
diags.diagnose(decl, diag::override_sendability_mismatch, | ||
decl->getName()) | ||
.warnUntilSwiftVersion(6); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where we would do the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
||
break; | ||
case OverrideCheckingAttempt::BaseName: | ||
diags.diagnose(decl, diag::override_multiple_decls_arg_mismatch, | ||
decl->getName()); | ||
|
@@ -886,6 +893,7 @@ SmallVector<OverrideMatch, 2> OverrideMatcher::match( | |
DeclName name; | ||
switch (attempt) { | ||
case OverrideCheckingAttempt::PerfectMatch: | ||
case OverrideCheckingAttempt::MismatchedSendability: | ||
case OverrideCheckingAttempt::MismatchedOptional: | ||
case OverrideCheckingAttempt::MismatchedTypes: | ||
name = decl->getName(); | ||
|
@@ -976,6 +984,8 @@ SmallVector<OverrideMatch, 2> OverrideMatcher::match( | |
matchMode |= TypeMatchFlags::AllowNonOptionalForIUOParam; | ||
matchMode |= TypeMatchFlags::IgnoreNonEscapingForOptionalFunctionParam; | ||
} | ||
if (attempt == OverrideCheckingAttempt::MismatchedSendability) | ||
matchMode |= TypeMatchFlags::IgnoreFunctionSendability; | ||
|
||
auto declFnTy = getDeclComparisonType()->getAs<AnyFunctionType>(); | ||
auto parentDeclTy = getMemberTypeForComparison(parentDecl, decl); | ||
|
@@ -1277,8 +1287,15 @@ bool OverrideMatcher::checkOverride(ValueDecl *baseDecl, | |
if (emittedMatchError) | ||
return true; | ||
|
||
if (attempt == OverrideCheckingAttempt::MismatchedSendability) { | ||
// FIXME: suppress if any matches brought in via @preconcurrency import? | ||
diags.diagnose(decl, diag::override_sendability_mismatch, | ||
decl->getName()) | ||
.warnUntilSwiftVersion(6); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... and here, for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And yes. |
||
diags.diagnose(baseDecl, diag::overridden_here); | ||
} | ||
// Catch-all to make sure we don't silently accept something we shouldn't. | ||
if (attempt != OverrideCheckingAttempt::PerfectMatch) { | ||
else if (attempt != OverrideCheckingAttempt::PerfectMatch) { | ||
OverrideMatch match{decl, /*isExact=*/false}; | ||
diagnoseGeneralOverrideFailure(decl, match, attempt); | ||
} | ||
|
@@ -1374,11 +1391,12 @@ bool swift::checkOverrides(ValueDecl *decl) { | |
switch (attempt) { | ||
case OverrideCheckingAttempt::PerfectMatch: | ||
break; | ||
case OverrideCheckingAttempt::MismatchedOptional: | ||
case OverrideCheckingAttempt::MismatchedSendability: | ||
// Don't keep looking if the user didn't indicate it's an override. | ||
if (!decl->getAttrs().hasAttribute<OverrideAttr>()) | ||
return false; | ||
break; | ||
case OverrideCheckingAttempt::MismatchedOptional: | ||
case OverrideCheckingAttempt::MismatchedTypes: | ||
break; | ||
case OverrideCheckingAttempt::BaseName: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,13 +155,25 @@ getTypesToCompare(ValueDecl *reqt, Type reqtType, bool reqtTypeIsIUO, | |
// function types that aren't in a parameter can be Sendable or not. | ||
// FIXME: Should we check for a Sendable bound on the requirement type? | ||
bool inRequirement = (adjustment != TypeAdjustment::NoescapeToEscaping); | ||
(void)adjustInferredAssociatedType(adjustment, reqtType, inRequirement); | ||
Type adjustedReqtType = | ||
adjustInferredAssociatedType(adjustment, reqtType, inRequirement); | ||
|
||
bool inWitness = false; | ||
Type adjustedWitnessType = | ||
adjustInferredAssociatedType(adjustment, witnessType, inWitness); | ||
if (inWitness && !inRequirement) | ||
witnessType = adjustedWitnessType; | ||
|
||
switch (variance) { | ||
case VarianceKind::None: | ||
break; | ||
case VarianceKind::Covariant: | ||
if (inRequirement && !inWitness) | ||
reqtType = adjustedReqtType; | ||
break; | ||
case VarianceKind::Contravariant: | ||
if (inWitness && !inRequirement) | ||
witnessType = adjustedWitnessType; | ||
break; | ||
} | ||
}; | ||
|
||
applyAdjustment(TypeAdjustment::NoescapeToEscaping); | ||
|
@@ -1150,15 +1162,26 @@ swift::matchWitness(WitnessChecker::RequirementEnvironmentCache &reqEnvCache, | |
return *result; | ||
} | ||
} | ||
if (!solution || solution->Fixes.size()) | ||
return RequirementMatch(witness, MatchKind::TypeConflict, | ||
witnessType); | ||
bool requiresNonSendable = false; | ||
if (!solution || solution->Fixes.size()) { | ||
/// If the *only* problems are that `@Sendable` attributes are missing, | ||
/// allow the match in some circumstances. | ||
requiresNonSendable = solution | ||
&& llvm::all_of(solution->Fixes, [](constraints::ConstraintFix *fix) { | ||
return fix->getKind() == constraints::FixKind::AddSendableAttribute; | ||
}); | ||
Comment on lines
+1167
to
+1172
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This bit seems a little questionable—I'd like reviewers to let me know if they think it'll work as intended. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this will work. I do wonder if we need to do a similar thing for global-actor-qualified function parameters (although I suspect they'll be much more rare). |
||
if (!requiresNonSendable) | ||
return RequirementMatch(witness, MatchKind::TypeConflict, | ||
witnessType); | ||
} | ||
|
||
MatchKind matchKind = MatchKind::ExactMatch; | ||
if (hasAnyError(optionalAdjustments)) | ||
matchKind = MatchKind::OptionalityConflict; | ||
else if (anyRenaming) | ||
matchKind = MatchKind::RenamedMatch; | ||
else if (requiresNonSendable) | ||
matchKind = MatchKind::RequiresNonSendable; | ||
else if (getEffects(witness).containsOnly(getEffects(req))) | ||
matchKind = MatchKind::FewerEffects; | ||
|
||
|
@@ -2518,6 +2541,12 @@ diagnoseMatch(ModuleDecl *module, NormalProtocolConformance *conformance, | |
withAssocTypes); | ||
break; | ||
|
||
case MatchKind::RequiresNonSendable: | ||
diags.diagnose(match.Witness, diag::protocol_witness_non_sendable, | ||
withAssocTypes, | ||
module->getASTContext().isSwiftVersionAtLeast(6)); | ||
break; | ||
|
||
case MatchKind::RenamedMatch: { | ||
auto diag = diags.diagnose(match.Witness, diag::protocol_witness_renamed, | ||
req->getName(), withAssocTypes); | ||
|
@@ -4259,6 +4288,19 @@ ConformanceChecker::resolveWitnessViaLookup(ValueDecl *requirement) { | |
requirement->getName()); | ||
}); | ||
} | ||
if (best.Kind == MatchKind::RequiresNonSendable) | ||
diagnoseOrDefer(requirement, getASTContext().isSwiftVersionAtLeast(6), | ||
[this, requirement, witness] | ||
(NormalProtocolConformance *conformance) { | ||
auto &diags = DC->getASTContext().Diags; | ||
|
||
diags.diagnose(getLocForDiagnosingWitness(conformance, witness), | ||
diag::witness_not_as_sendable, | ||
witness->getDescriptiveKind(), witness->getName(), | ||
conformance->getProtocol()->getName()) | ||
.warnUntilSwiftVersion(6); | ||
diags.diagnose(requirement, diag::less_sendable_reqt_here); | ||
}); | ||
|
||
auto check = checkWitness(requirement, best); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch here.