Skip to content

[ConstraintSystem] Start to allow diagnosing ambiguity with fixes for solution sets with more than one fix. #28979

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 1 commit into from
Jan 3, 2020
Merged

[ConstraintSystem] Start to allow diagnosing ambiguity with fixes for solution sets with more than one fix. #28979

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
7 changes: 4 additions & 3 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ ERROR(no_overloads_match_exactly_in_assignment,none,
"no exact matches in assignment to %0",
(DeclBaseName))

NOTE(candidate_partial_match,none,
"candidate has partially matching parameter list %0",
(StringRef))

ERROR(ambiguous_subscript,none,
"ambiguous subscript with base type %0 and index type %1",
(Type, Type))
Expand Down Expand Up @@ -274,9 +278,6 @@ ERROR(cannot_call_with_params, none,
ERROR(cannot_call_non_function_value,none,
"cannot call value of non-function type %0", (Type))

ERROR(wrong_argument_labels_overload,none,
"argument labels '%0' do not match any available overloads", (StringRef))

ERROR(no_candidates_match_result_type,none,
"no '%0' candidates produce the expected contextual result type %1",
(StringRef, Type))
Expand Down
15 changes: 0 additions & 15 deletions lib/Sema/CSDiag.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2070,21 +2070,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
calleeInfo, TCC_ForceRecheck);
if (!argExpr)
return true; // already diagnosed.

// Handle argument label mismatches when we have multiple candidates.
if (calleeInfo.closeness == CC_ArgumentLabelMismatch) {
auto args = decomposeArgType(CS.getType(argExpr), argLabels);

// If we have multiple candidates that we fail to match, just say we have
// the wrong labels and list the candidates out.
diagnose(callExpr->getLoc(), diag::wrong_argument_labels_overload,
getParamListAsString(args))
.highlight(argExpr->getSourceRange());

// Did the user intend on invoking a different overload?
calleeInfo.suggestPotentialOverloads(fnExpr->getLoc());
return true;
}

auto overloadName = calleeInfo.declName;

Expand Down
132 changes: 88 additions & 44 deletions lib/Sema/ConstraintSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2734,34 +2734,52 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
// overloads of the same declaration.
ConstraintLocator *commonCalleeLocator = nullptr;
SmallPtrSet<ValueDecl *, 4> distinctChoices;
SmallVector<std::pair<const Solution *, const ConstraintFix *>, 4>
viableSolutions;
SmallVector<const Solution *, 4> viableSolutions;

enum class AmbiguityKind {
/// There is exactly one fix associated with each candidate.
CloseMatch,
/// Solution is ambiguous because all candidates had partially matching
/// parameter lists.
ParameterList,
/// General ambiguity failure.
General,
};
auto ambiguityKind = AmbiguityKind::CloseMatch;

bool diagnosable = llvm::all_of(solutions, [&](const Solution &solution) {
ArrayRef<ConstraintFix *> fixes = solution.Fixes;

// Currently only support a single fix in a solution,
// but ultimately should be able to deal with multiple.
if (fixes.size() != 1)
if (fixes.empty())
return false;

const auto *fix = fixes.front();
auto *locator = fix->getLocator();
if (fixes.size() > 1) {
ambiguityKind = (ambiguityKind == AmbiguityKind::CloseMatch ||
ambiguityKind == AmbiguityKind::ParameterList) &&
llvm::all_of(fixes, [](const ConstraintFix *fix) -> bool {
auto *locator = fix->getLocator();
return locator->findLast<LocatorPathElt::ApplyArgument>().hasValue();
}) ? AmbiguityKind::ParameterList
: AmbiguityKind::General;
}

for (const auto *fix: fixes) {
auto *locator = fix->getLocator();
// Assignment failures are all about the source expression,
// because they treat destination as a contextual type.
if (auto *anchor = locator->getAnchor()) {
if (auto *assignExpr = dyn_cast<AssignExpr>(anchor))
locator = getConstraintLocator(assignExpr->getSrc());
}

// Assignment failures are all about the source expression,
// because they treat destination as a contextual type.
if (auto *anchor = locator->getAnchor()) {
if (auto *assignExpr = dyn_cast<AssignExpr>(anchor))
locator = getConstraintLocator(assignExpr->getSrc());
auto *calleeLocator = getCalleeLocator(locator);
if (!commonCalleeLocator)
commonCalleeLocator = calleeLocator;
else if (commonCalleeLocator != calleeLocator)
return false;
}

auto *calleeLocator = getCalleeLocator(locator);
if (commonCalleeLocator && commonCalleeLocator != calleeLocator)
return false;

commonCalleeLocator = calleeLocator;

auto overload = solution.getOverloadChoiceIfAvailable(calleeLocator);
auto overload = solution.getOverloadChoiceIfAvailable(commonCalleeLocator);
if (!overload)
return false;

Expand All @@ -2773,7 +2791,7 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
// as viable, otherwise we'd produce the same diagnostic multiple
// times, which means that actual problem is elsewhere.
if (distinctChoices.insert(decl).second)
viableSolutions.push_back({&solution, fix});
viableSolutions.push_back(&solution);
return true;
});

Expand All @@ -2787,32 +2805,11 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
{
DiagnosticTransaction transaction(getASTContext().Diags);

const auto *fix = viableSolutions.front().second;
auto *commonAnchor = commonCalleeLocator->getAnchor();
auto &DE = getASTContext().Diags;
auto name = decl->getFullName();

if (fix->getKind() == FixKind::UseSubscriptOperator) {
auto *UDE = cast<UnresolvedDotExpr>(commonAnchor);
DE.diagnose(commonAnchor->getLoc(),
diag::could_not_find_subscript_member_did_you_mean,
getType(UDE->getBase()));
} else if (fix->getKind() == FixKind::TreatRValueAsLValue) {
DE.diagnose(commonAnchor->getLoc(),
diag::no_overloads_match_exactly_in_assignment,
decl->getBaseName());
} else if (llvm::all_of(
viableSolutions,
[](const std::pair<const Solution *, const ConstraintFix *>
&fix) {
auto *locator = fix.second->getLocator();
return locator
->isLastElement<LocatorPathElt::ContextualType>();
})) {
auto baseName = name.getBaseName();
DE.diagnose(commonAnchor->getLoc(), diag::no_candidates_match_result_type,
baseName.userFacingName(), getContextualType());
} else {
auto emitGeneralAmbiguityFailure = [&]() {
// Three choices here:
// 1. If this is a special name avoid printing it because
// printing kind is sufficient;
Expand All @@ -2839,14 +2836,61 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
diag::no_overloads_match_exactly_in_call_no_labels,
decl->getDescriptiveKind(), name.getBaseName());
}
};

switch (ambiguityKind) {
case AmbiguityKind::CloseMatch:
// Handled below
break;
case AmbiguityKind::ParameterList: {
emitGeneralAmbiguityFailure();

for (const auto &viable: viableSolutions) {
auto overload = viable->getOverloadChoice(commonCalleeLocator);
auto *fn = overload.openedType->getAs<AnyFunctionType>();
assert(fn);
DE.diagnose(overload.choice.getDecl()->getLoc(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question, I wonder if with this we might be able to get rid of the wrong_argument_labels_overload in CSDiag here? I'm asking because I was looking into this just yesterday :))

Copy link
Member Author

Choose a reason for hiding this comment

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

This change does cover ambiguity due to argument label mismatches so it's very possible that wrong_argument_labels_overload in CSDiag is obsolete. I'm testing 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.

Seems to be obsolete - thanks for catching this!

diag::candidate_partial_match,
fn->getParamListAsString(fn->getParams()));
}

return true;
}
case AmbiguityKind::General:
// TODO: Handle general ambiguity here.
return false;
}

auto *fix = viableSolutions.front()->Fixes.front();
if (fix->getKind() == FixKind::UseSubscriptOperator) {
auto *UDE = cast<UnresolvedDotExpr>(commonAnchor);
DE.diagnose(commonAnchor->getLoc(),
diag::could_not_find_subscript_member_did_you_mean,
getType(UDE->getBase()));
} else if (fix->getKind() == FixKind::TreatRValueAsLValue) {
DE.diagnose(commonAnchor->getLoc(),
diag::no_overloads_match_exactly_in_assignment,
decl->getBaseName());
} else if (llvm::all_of(
viableSolutions,
[](const Solution *viable) {
auto *locator = viable->Fixes.front()->getLocator();
return locator
->isLastElement<LocatorPathElt::ContextualType>();
})) {
auto baseName = name.getBaseName();
DE.diagnose(commonAnchor->getLoc(), diag::no_candidates_match_result_type,
baseName.userFacingName(), getContextualType());
} else {
emitGeneralAmbiguityFailure();
}

for (const auto &viable : viableSolutions) {
// Create scope so each applied solution is rolled back.
ConstraintSystem::SolverScope scope(*this);
applySolution(*viable.first);
applySolution(*viable);
// All of the solutions supposed to produce a "candidate" note.
diagnosed &= viable.second->diagnose(/*asNote*/ true);
diagnosed &= viable->Fixes.front()->diagnose(/*asNote*/ true);
}

// If not all of the fixes produced a note, we can't diagnose this.
Expand Down
5 changes: 3 additions & 2 deletions test/Constraints/diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,9 @@ enum Color {
static func rainbow() -> Color {}

static func overload(a : Int) -> Color {} // expected-note {{incorrect labels for candidate (have: '(_:)', expected: '(a:)')}}
// expected-note@-1 {{candidate has partially matching parameter list (a: Int)}}
static func overload(b : Int) -> Color {} // expected-note {{incorrect labels for candidate (have: '(_:)', expected: '(b:)')}}
// expected-note@-1 {{candidate has partially matching parameter list (b: Int)}}

static func frob(_ a : Int, b : inout Int) -> Color {}
static var svar: Color { return .Red }
Expand All @@ -517,8 +519,7 @@ let _ : (Int, Float) = (42.0, 12) // expected-error {{cannot convert value of t
let _ : Color = .rainbow // expected-error {{member 'rainbow()' is a function; did you mean to call it?}} {{25-25=()}}

let _: Color = .overload(a : 1.0) // expected-error {{cannot convert value of type 'Double' to expected argument type 'Int'}}
let _: Color = .overload(1.0) // expected-error {{ambiguous reference to member 'overload'}}
// expected-note @-1 {{overloads for 'overload' exist with these partially matching parameter lists: (a: Int), (b: Int)}}
let _: Color = .overload(1.0) // expected-error {{no exact matches in call to static method 'overload'}}
let _: Color = .overload(1) // expected-error {{no exact matches in call to static method 'overload'}}
let _: Color = .frob(1.0, &i) // expected-error {{missing argument label 'b:' in call}}
// expected-error@-1 {{cannot convert value of type 'Double' to expected argument type 'Int'}}
Expand Down
11 changes: 5 additions & 6 deletions test/Constraints/super_constructor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ class D : B {
}

init(g:Int) {
super.init("aoeu") // expected-error{{argument labels '(_:)' do not match any available overloads}}
// expected-note @-1 {{overloads for 'B.init' exist with these partially matching parameter lists: (a: UnicodeScalar), (b: UnicodeScalar), (x: Int), (z: Float)}}
super.init("aoeu") // expected-error{{no exact matches in call to initializer}}
}

init(h:Int) {
Expand All @@ -40,15 +39,15 @@ class B {
init() {
}

init(x:Int) {
init(x:Int) { // expected-note{{candidate has partially matching parameter list (x: Int)}}
}

init(a:UnicodeScalar) {
init(a:UnicodeScalar) { // expected-note {{candidate has partially matching parameter list (a: UnicodeScalar)}}
}
init(b:UnicodeScalar) {
init(b:UnicodeScalar) { // expected-note{{candidate has partially matching parameter list (b: UnicodeScalar)}}
}

init(z:Float) {
init(z:Float) { // expected-note{{candidate has partially matching parameter list (z: Float)}}
super.init() // expected-error{{'super' members cannot be referenced in a root class}}
}
}
Expand Down
9 changes: 4 additions & 5 deletions test/expr/postfix/dot/init_ref_delegation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,13 +323,12 @@ class TestOverloadSets {
self.init(5, 5) // expected-error{{extra argument in call}}
}

convenience init(a : Z0) {
self.init(42 as Int8) // expected-error{{argument labels '(_:)' do not match any available overloads}}
// expected-note @-1 {{overloads for 'TestOverloadSets.init' exist with these partially matching parameter lists: (a: Z0), (value: Double), (value: Int)}}
convenience init(a : Z0) { // expected-note{{candidate has partially matching parameter list (a: Z0)}}
self.init(42 as Int8) // expected-error{{no exact matches in call to initializer}}
}

init(value: Int) { /* ... */ }
init(value: Double) { /* ... */ }
init(value: Int) { /* ... */ } // expected-note{{candidate has partially matching parameter list (value: Int)}}
init(value: Double) { /* ... */ } // expected-note{{candidate has partially matching parameter list (value: Double)}}
}

class TestNestedExpr {
Expand Down