-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Diagnostics] Introduce extraneous call fix #27976
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 |
---|---|---|
|
@@ -7097,8 +7097,37 @@ ConstraintSystem::simplifyApplicableFnConstraint( | |
} | ||
|
||
// Handle applications of @dynamicCallable types. | ||
return simplifyDynamicCallableApplicableFnConstraint(type1, origType2, | ||
subflags, locator); | ||
auto result = simplifyDynamicCallableApplicableFnConstraint( | ||
type1, origType2, subflags, locator); | ||
|
||
if (shouldAttemptFixes() && result == SolutionKind::Error) { | ||
// Skip this fix if the type is not yet resolved or | ||
// it's a function type/metatype which points to argument mismatches. | ||
if (desugar2->is<TypeVariableType>() || desugar2->is<FunctionType>() || | ||
desugar2->is<AnyMetatypeType>()) | ||
return SolutionKind::Error; | ||
|
||
if (auto objectTy = desugar2->lookThroughAllOptionalTypes()) { | ||
if (objectTy->isAny() || objectTy->isAnyObject()) | ||
return SolutionKind::Error; | ||
} | ||
|
||
// If there are any type variables associated with arguments/result | ||
// they have to be marked as "holes". | ||
type1.visit([&](Type subType) { | ||
if (auto *typeVar = subType->getAs<TypeVariableType>()) | ||
recordHole(typeVar); | ||
}); | ||
|
||
auto *fix = RemoveInvalidCall::create(*this, getConstraintLocator(locator)); | ||
// Let's make this fix as high impact so if there is a function or member | ||
// overload with e.g. argument-to-parameter type mismatches it would take | ||
// a higher priority. | ||
return recordFix(fix, /*impact=*/10) ? SolutionKind::Error | ||
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 isn't specific to this change or something that needs to be addressed now, but what do you think about making 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. Oops, didn't see @hborla's comment before posting 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 was thinking about doing that as well, but some far most of the impact changes where contextual in nature. Although maybe that's a nice long-hanging fruit to tackle for clarity... 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. My hope is once we finish porting majority of diagnostics we would become more clear whether we can stick to weight or need something more comprehensive than that... |
||
: SolutionKind::Solved; | ||
} | ||
|
||
return result; | ||
} | ||
|
||
/// Looks up and returns the @dynamicCallable required methods (if they exist) | ||
|
@@ -8012,6 +8041,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint( | |
case FixKind::UseSubscriptOperator: | ||
case FixKind::ExplicitlyEscaping: | ||
case FixKind::RelabelArguments: | ||
case FixKind::RemoveCall: | ||
case FixKind::RemoveUnwrap: | ||
case FixKind::DefineMemberBasedOnUse: | ||
case FixKind::AllowMemberRefOnExistential: | ||
|
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.
Do you think we're developing the need for more a principled priority mechanism among fixes?
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.
I think that's something we need to think about for the long term, but logic involved there would be even more complicated than CSRanking, so I'm postponing that as much as possible. So far "weight" such as what
impact
really is seems to be doing the job well enough.