Skip to content

[Constraint solver] Improve fix behaviors for better concurrency-related recovery and overloading #59958

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

Merged
merged 4 commits into from
Jul 8, 2022
Merged

[Constraint solver] Improve fix behaviors for better concurrency-related recovery and overloading #59958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c564698
[Constraint solver] Improve modeling of fix behavior.
DougGregor Jul 7, 2022
2e3aa67
[Constraint solver] Treat downgraded errors as "disfavored overloads".
DougGregor Jul 8, 2022
c2f88ab
[Constraint system] Always add recoverable concurrency fixes.
DougGregor Jul 8, 2022
6cab661
[Constraint solver] Don't count suppressed diagnostics against the sc…
DougGregor Jul 8, 2022
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
77 changes: 46 additions & 31 deletions include/swift/Sema/CSFix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "swift/AST/Types.h"
#include "swift/Basic/Debug.h"
#include "swift/Sema/ConstraintLocator.h"
#include "swift/Sema/FixBehavior.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/TrailingObjects.h"
Expand All @@ -45,6 +46,7 @@ class ConstraintSystem;
class ConstraintLocator;
class ConstraintLocatorBuilder;
enum class ConversionRestrictionKind;
enum ScoreKind: unsigned int;
class Solution;
struct MemberLookupResult;

Expand Down Expand Up @@ -410,13 +412,12 @@ class ConstraintFix {
ConstraintLocator *Locator;

/// The behavior limit to apply to the diagnostics emitted.
DiagnosticBehavior behaviorLimit;
FixBehavior fixBehavior;

public:
ConstraintFix(ConstraintSystem &cs, FixKind kind, ConstraintLocator *locator,
DiagnosticBehavior behaviorLimit =
DiagnosticBehavior::Unspecified)
: CS(cs), Kind(kind), Locator(locator), behaviorLimit(behaviorLimit) {}
FixBehavior fixBehavior = FixBehavior::Error)
: CS(cs), Kind(kind), Locator(locator), fixBehavior(fixBehavior) {}

virtual ~ConstraintFix();

Expand All @@ -427,14 +428,27 @@ class ConstraintFix {

FixKind getKind() const { return Kind; }

bool isWarning() const {
return behaviorLimit == DiagnosticBehavior::Warning ||
behaviorLimit == DiagnosticBehavior::Ignore;
/// Whether it is still possible to "apply" a solution containing this kind
/// of fix to get a usable AST.
bool canApplySolution() const {
switch (fixBehavior) {
case FixBehavior::AlwaysWarning:
case FixBehavior::DowngradeToWarning:
case FixBehavior::Suppress:
return true;

case FixBehavior::Error:
return false;
}
}

/// Whether this kind of fix affects the solution score, and which score
/// it affects.
Optional<ScoreKind> affectsSolutionScore() const;

/// The diagnostic behavior limit that will be applied to any emitted
/// diagnostics.
DiagnosticBehavior diagBehaviorLimit() const { return behaviorLimit; }
FixBehavior diagfixBehavior() const { return fixBehavior; }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to keep around the FixBehavior for when we create a FailureDiagnostic from a ConstraintFix, which is why I factored this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please see my comment about this attached to FailureDiagnostic constructor.


virtual std::string getName() const = 0;

Expand Down Expand Up @@ -680,16 +694,15 @@ class ContextualMismatch : public ConstraintFix {

ContextualMismatch(ConstraintSystem &cs, Type lhs, Type rhs,
ConstraintLocator *locator,
DiagnosticBehavior behaviorLimit)
: ConstraintFix(cs, FixKind::ContextualMismatch, locator, behaviorLimit),
FixBehavior fixBehavior)
: ConstraintFix(cs, FixKind::ContextualMismatch, locator, fixBehavior),
LHS(lhs), RHS(rhs) {}

protected:
ContextualMismatch(ConstraintSystem &cs, FixKind kind, Type lhs, Type rhs,
ConstraintLocator *locator,
DiagnosticBehavior behaviorLimit =
DiagnosticBehavior::Unspecified)
: ConstraintFix(cs, kind, locator, behaviorLimit), LHS(lhs), RHS(rhs) {}
FixBehavior fixBehavior = FixBehavior::Error)
: ConstraintFix(cs, kind, locator, fixBehavior), LHS(lhs), RHS(rhs) {}

public:
std::string getName() const override { return "fix contextual mismatch"; }
Expand Down Expand Up @@ -777,9 +790,9 @@ class MarkExplicitlyEscaping final : public ContextualMismatch {
class MarkGlobalActorFunction final : public ContextualMismatch {
MarkGlobalActorFunction(ConstraintSystem &cs, Type lhs, Type rhs,
ConstraintLocator *locator,
DiagnosticBehavior behaviorLimit)
FixBehavior fixBehavior)
: ContextualMismatch(cs, FixKind::MarkGlobalActorFunction, lhs, rhs,
locator, behaviorLimit) {
locator, fixBehavior) {
}

public:
Expand All @@ -789,7 +802,7 @@ class MarkGlobalActorFunction final : public ContextualMismatch {

static MarkGlobalActorFunction *create(ConstraintSystem &cs, Type lhs,
Type rhs, ConstraintLocator *locator,
DiagnosticBehavior behaviorLimit);
FixBehavior fixBehavior);

static bool classof(ConstraintFix *fix) {
return fix->getKind() == FixKind::MarkGlobalActorFunction;
Expand Down Expand Up @@ -825,9 +838,9 @@ class ForceOptional final : public ContextualMismatch {
class AddSendableAttribute final : public ContextualMismatch {
AddSendableAttribute(ConstraintSystem &cs, FunctionType *fromType,
FunctionType *toType, ConstraintLocator *locator,
DiagnosticBehavior behaviorLimit)
FixBehavior fixBehavior)
: ContextualMismatch(cs, FixKind::AddSendableAttribute, fromType, toType,
locator, behaviorLimit) {
locator, fixBehavior) {
assert(fromType->isSendable() != toType->isSendable());
}

Expand All @@ -840,7 +853,7 @@ class AddSendableAttribute final : public ContextualMismatch {
FunctionType *fromType,
FunctionType *toType,
ConstraintLocator *locator,
DiagnosticBehavior behaviorLimit);
FixBehavior fixBehavior);

static bool classof(ConstraintFix *fix) {
return fix->getKind() == FixKind::AddSendableAttribute;
Expand Down Expand Up @@ -1403,11 +1416,14 @@ class AllowTypeOrInstanceMember final : public AllowInvalidMemberRef {
};

class AllowInvalidPartialApplication final : public ConstraintFix {
bool isWarning;

AllowInvalidPartialApplication(bool isWarning, ConstraintSystem &cs,
ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::AllowInvalidPartialApplication, locator,
isWarning ? DiagnosticBehavior::Warning
: DiagnosticBehavior::Unspecified) {}
isWarning ? FixBehavior::AlwaysWarning
: FixBehavior::Error),
isWarning(isWarning) {}

public:
std::string getName() const override {
Expand Down Expand Up @@ -2141,10 +2157,9 @@ class AllowArgumentMismatch : public ContextualMismatch {

AllowArgumentMismatch(ConstraintSystem &cs, FixKind kind, Type argType,
Type paramType, ConstraintLocator *locator,
DiagnosticBehavior behaviorLimit =
DiagnosticBehavior::Unspecified)
FixBehavior fixBehavior = FixBehavior::Error)
: ContextualMismatch(
cs, kind, argType, paramType, locator, behaviorLimit) {}
cs, kind, argType, paramType, locator, fixBehavior) {}

public:
std::string getName() const override {
Expand Down Expand Up @@ -2292,9 +2307,9 @@ class TreatEphemeralAsNonEphemeral final : public AllowArgumentMismatch {
TreatEphemeralAsNonEphemeral(ConstraintSystem &cs, ConstraintLocator *locator,
Type srcType, Type dstType,
ConversionRestrictionKind conversionKind,
DiagnosticBehavior behaviorLimit)
FixBehavior fixBehavior)
: AllowArgumentMismatch(cs, FixKind::TreatEphemeralAsNonEphemeral,
srcType, dstType, locator, behaviorLimit),
srcType, dstType, locator, fixBehavior),
ConversionKind(conversionKind) {}

public:
Expand Down Expand Up @@ -2458,7 +2473,7 @@ class AllowCoercionToForceCast final : public ContextualMismatch {
AllowCoercionToForceCast(ConstraintSystem &cs, Type fromType, Type toType,
ConstraintLocator *locator)
: ContextualMismatch(cs, FixKind::AllowCoercionToForceCast, fromType,
toType, locator, DiagnosticBehavior::Warning) {}
toType, locator, FixBehavior::AlwaysWarning) {}

public:
std::string getName() const override {
Expand Down Expand Up @@ -2572,7 +2587,7 @@ class SpecifyLabelToAssociateTrailingClosure final : public ConstraintFix {
SpecifyLabelToAssociateTrailingClosure(ConstraintSystem &cs,
ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::SpecifyLabelToAssociateTrailingClosure,
locator, DiagnosticBehavior::Warning) {}
locator, FixBehavior::AlwaysWarning) {}

public:
std::string getName() const override {
Expand Down Expand Up @@ -2742,7 +2757,7 @@ class SpecifyBaseTypeForOptionalUnresolvedMember final : public ConstraintFix {
DeclNameRef memberName,
ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::SpecifyBaseTypeForOptionalUnresolvedMember,
locator, DiagnosticBehavior::Warning),
locator, FixBehavior::AlwaysWarning),
MemberName(memberName) {}
DeclNameRef MemberName;

Expand Down Expand Up @@ -2773,7 +2788,7 @@ class CheckedCastContextualMismatchWarning : public ContextualMismatch {
CheckedCastKind kind,
ConstraintLocator *locator)
: ContextualMismatch(cs, fixKind, fromType, toType, locator,
DiagnosticBehavior::Warning),
FixBehavior::AlwaysWarning),
CastKind(kind) {}
CheckedCastKind CastKind;
};
Expand Down Expand Up @@ -2932,7 +2947,7 @@ class AllowTupleLabelMismatch final : public ContextualMismatch {
AllowTupleLabelMismatch(ConstraintSystem &cs, Type fromType, Type toType,
ConstraintLocator *locator)
: ContextualMismatch(cs, FixKind::AllowTupleLabelMismatch, fromType,
toType, locator, DiagnosticBehavior::Warning) {}
toType, locator, FixBehavior::AlwaysWarning) {}

public:
std::string getName() const override { return "allow tuple label mismatch"; }
Expand Down
2 changes: 1 addition & 1 deletion include/swift/Sema/ConstraintSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,7 +819,7 @@ class FunctionArgApplyInfo {

/// Describes an aspect of a solution that affects its overall score, i.e., a
/// user-defined conversions.
enum ScoreKind {
enum ScoreKind: unsigned int {
// These values are used as indices into a Score value.

/// A fix needs to be applied to the source.
Expand Down
41 changes: 41 additions & 0 deletions include/swift/Sema/FixBehavior.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
//===--- FixBehavior.h - Constraint Fix Behavior --------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2018 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//
//
// This file provides information about how a constraint fix should behavior.
//
//===----------------------------------------------------------------------===//

#ifndef SWIFT_SEMA_FIXBEHAVIOR_H
#define SWIFT_SEMA_FIXBEHAVIOR_H

namespace swift {
namespace constraints {

/// Describes the behavior of the diagnostic corresponding to a given fix.
enum class FixBehavior {
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment about limitsDiagnosticBehaviorTo, I think this could be turned into an inner enum of ConstraintFix.

/// The diagnostic is an error, and should prevent constraint application.
Error,
/// The diagnostic is always a warning, which should not prevent constraint
/// application.
AlwaysWarning,
/// The diagnostic should be downgraded to a warning, and not prevent
/// constraint application.
DowngradeToWarning,
/// The diagnostic should be suppressed, and not prevent constraint
/// application.
Suppress
};

}
}

#endif // SWIFT_SEMA_FIXBEHAVIOR_H
17 changes: 12 additions & 5 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8480,7 +8480,7 @@ bool ConstraintSystem::applySolutionFixes(const Solution &solution) {

auto diagnosed =
primaryFix->coalesceAndDiagnose(solution, secondaryFixes);
if (primaryFix->isWarning()) {
if (primaryFix->canApplySolution()) {
assert(diagnosed && "warnings should always be diagnosed");
(void)diagnosed;
} else {
Expand Down Expand Up @@ -9132,16 +9132,23 @@ Optional<SolutionApplicationTarget> ConstraintSystem::applySolution(
Solution &solution, SolutionApplicationTarget target) {
// If any fixes needed to be applied to arrive at this solution, resolve
// them to specific expressions.
unsigned numResolvableFixes = 0;
if (!solution.Fixes.empty()) {
if (shouldSuppressDiagnostics())
return None;

bool diagnosedErrorsViaFixes = applySolutionFixes(solution);
bool canApplySolution = true;
for (const auto fix : solution.Fixes) {
if (!fix->canApplySolution())
canApplySolution = false;
if (fix->affectsSolutionScore() == SK_Fix && fix->canApplySolution())
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems odd to me because anything which sets SK_Fix should imply "fatal"...

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like we need to split SK_Fix into SK_FatalFix and SK_RecoverableFIx or something like that.

++numResolvableFixes;
}

// If all of the available fixes would result in a warning,
// we can go ahead and apply this solution to AST.
if (!llvm::all_of(solution.Fixes, [](const ConstraintFix *fix) {
return fix->isWarning();
})) {
if (!canApplySolution) {
// If we already diagnosed any errors via fixes, that's it.
if (diagnosedErrorsViaFixes)
return None;
Expand All @@ -9158,7 +9165,7 @@ Optional<SolutionApplicationTarget> ConstraintSystem::applySolution(
// produce a fallback diagnostic to highlight the problem.
{
const auto &score = solution.getFixedScore();
if (score.Data[SK_Fix] > 0 || score.Data[SK_Hole] > 0) {
if (score.Data[SK_Fix] > numResolvableFixes || score.Data[SK_Hole] > 0) {
maybeProduceFallbackDiagnostic(target);
return None;
}
Expand Down
16 changes: 16 additions & 0 deletions lib/Sema/CSDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,22 @@ template <typename... ArgTypes>
InFlightDiagnostic
FailureDiagnostic::emitDiagnosticAt(ArgTypes &&... Args) const {
auto &DE = getASTContext().Diags;
DiagnosticBehavior behaviorLimit;
switch (fixBehavior) {
case FixBehavior::Error:
case FixBehavior::AlwaysWarning:
behaviorLimit = DiagnosticBehavior::Unspecified;
break;

case FixBehavior::DowngradeToWarning:
behaviorLimit = DiagnosticBehavior::Warning;
break;

case FixBehavior::Suppress:
behaviorLimit = DiagnosticBehavior::Ignore;
break;
}

return std::move(DE.diagnose(std::forward<ArgTypes>(Args)...)
.limitBehavior(behaviorLimit));
}
Expand Down
Loading