[Constraint solver] Improve fix behaviors for better concurrency-related recovery and overloading #59958
Conversation
Rather than re-using `DiagnosticBehavior` to describe how a fix should act, introduce `FixBehavior` to cover the differences between (e.g.) always-as-awarning and downgrade-to-warning. While here, split the `isWarning` predicate into two different predicates: * `canApplySolution`: Whether we can still apply a solution when it contains this particular fix. * `affectsSolutionScore`: Whether These two predicates are currently tied together, because that's the existing behavior, but we don't necessarily want them to stay that way.
This allows us to still maintain them in the score kind, but not treat them as being as severe as an error requiring a fix.
This avoids us having to go down the less-efficient "salvage" path when dealing with concurrency issues. It also fixes overloading behavior when dealing with `@preconcurrency` and `@Sendable` functions, such as in swiftlang#59909.
|
@swift-ci please smoke test |
DougGregor
changed the title
|
@swift-ci please smoke test |
| /// The diagnostic behavior limit that will be applied to any emitted | ||
| /// diagnostics. | ||
| DiagnosticBehavior diagBehaviorLimit() const { return behaviorLimit; } | ||
| FixBehavior diagfixBehavior() const { return fixBehavior; } |
There was a problem hiding this comment.
I think it might make sense to turn this into a limitsDiagnosticBehaviorTo() or something like that which returns Optional<DiagnosticBehavior> so that we could just supply it for to FailureDiagnostic and avoid having to mix fixes and diagnostics.
There was a problem hiding this comment.
We need to keep around the FixBehavior for when we create a FailureDiagnostic from a ConstraintFix, which is why I factored this out.
There was a problem hiding this comment.
Please see my comment about this attached to FailureDiagnostic constructor.
| namespace constraints { | ||
|
|
||
| /// Describes the behavior of the diagnostic corresponding to a given fix. | ||
| enum class FixBehavior { |
There was a problem hiding this comment.
See my comment about limitsDiagnosticBehaviorTo, I think this could be turned into an inner enum of ConstraintFix.
| for (const auto fix : solution.Fixes) { | ||
| if (!fix->canApplySolution()) | ||
| canApplySolution = false; | ||
| if (fix->affectsSolutionScore() == SK_Fix && fix->canApplySolution()) |
There was a problem hiding this comment.
This seems odd to me because anything which sets SK_Fix should imply "fatal"...
There was a problem hiding this comment.
Good catch, this change became unnecessary with my last commit, because we no longer increase SK_Fix for anything recoverable. I can remove it now.
There was a problem hiding this comment.
Ah, but I've been thinking about adding a RecoverableError to FixBehavior, which is an error from which we can still recover. We still want it to bump SK_Fix so there's no difference from Error in the solver behavior, but we can still produce a type-checked AST, which helps downstream code. Adding @Sendable in an argument position is a case where this would work, because CSApply will just toss in another function conversion expression.
There was a problem hiding this comment.
It sounds like we need to split SK_Fix into SK_FatalFix and SK_RecoverableFIx or something like that.
| *this, func1, func2, getConstraintLocator(locator), | ||
| getConcurrencyDiagnosticBehavior(true)); | ||
| if (recordFix(fix)) | ||
| if (auto fixBehavior = getConcurrencyFixBehavior(true)) { |
There was a problem hiding this comment.
We have a pattern for this kind of a situation - a fix declares a static attempt method which returns a fix pointer. This way getConcurrencyFixBehavior could be turned into a implementation detail of AddSendableAttribute and MarkGlobalActorFunction and moved to CSFix.cpp and the code here becomes:
if (auto fix = AddSendableAttribute::attempt(*this, func1, func2, getConstraintLocator(locator))) {
if (record(fix))
return getTypeMatchFailure(locator);
} else {
return getTypeMatchFailure(locator);
}
| DiagnosticBehavior behaviorLimit = | ||
| DiagnosticBehavior::Unspecified) | ||
| : S(solution), Locator(locator), behaviorLimit(behaviorLimit) {} | ||
| FixBehavior fixBehavior = FixBehavior::Error) |
There was a problem hiding this comment.
I think we can avoid mixing fixes and diagnostics here. How about we bring back behaviorLimit but make it Optional<DiagnosticBehavior> which is going to be None by default, and it's going to be supplied by the fix that constructs a diagnostic via call to limitsDiagnosticBehaviorTo()?
There was a problem hiding this comment.
Because we want isFatal and impact to operate on FixBehavior, we need to carry FixBehavior through here and map it down to DiagnosticBehavior only at the point of diagnostic emission.
There was a problem hiding this comment.
I'm not exactly sure what you mean - FailureDiagnostic represents a diagnostic emission for a ConstraintFix point of view, so the fix should tell it whether it wants to limit the behavior or not, it shouldn't be up to diagnostic to decide what to do based on the fix behavior that's why I suggested to package that logic into ConstraintFix::limitsDiagnosticBehaviorTo() which is going to produce Optional< DiagnosticBehavior>.
|
Thank you for the review @xedin ! I agree with most of your feedback and will follow-up with a cleanup PR to address it. |
Improve the modeling of fix behaviors in the constraint solver rather than abusing
DiagnosticBehaviorfor this purpose. Clearly call out the various cases (error, always-warning, downgrade-to-warning, ignore) and separate the predicates for "prevents solution application" and "should impact the score."With that refactoring in place, tune the behavior for recoverable concurrency fixes to (1) apply even when we aren't allowing fixes (i.e., along the fast path), which improves solver performance overall when dealing with concurrency issues like a missing
@Sendable, and (2) classify recoverable errors as "disfavored overloads" so that are more desirable than outright errors, which addresses the overloading problem from #59909.