[ConstraintSystem] Overhaul ambiguity diagnostics #31713

xedin · 2020-05-11T20:57:43Z

Don't attempt to rank solutions based on overloads if they have fixes
Factor out overload ambiguity diagnostics
Switch to a different algorithm:
- Aggregate all of the available fixes based on callee locator;
- For each ambiguous overload match aggregated fixes and diagnose;
- Discard all of the fixes which have been already considered
  as part of overload diagnostics;
- Diagnose remaining (uniqued based on kind + locator) fixes
  iff they appear in all of the solutions.

Resolves: SR-12689
Resolves: rdar://problem/62481592

xedin · 2020-05-11T20:57:58Z

xedin · 2020-05-11T20:58:58Z

This is still a WIP because we need to implement diagnoseForAmbiguity for some of the fixes e.g. treat r-value as l-value and do other work, but I'd like to get some initial feedback on this to make sure that everybody on the same page.

lib/Sema/ConstraintSystem.cpp

LucianoPAlmeida · 2020-05-11T22:46:45Z

lib/Sema/ConstraintSystem.cpp

+    auto aggregate = fixes->second;
+    diagnosed |= ::diagnoseAmbiguity(*this, ambiguity, aggregate, solutions);
+
+    consideredFixes.insert(aggregate.begin(), aggregate.end());


I wonder if In the cases where diagnoseAmbiguity returns false(if there's any) ... since we are adding them independently it maybe is a problem since they would be dropped from fixes undiagnosed? If there are such cases, maybe we should let them to attempt diagnose by kind?

The reason why I did that is related to the fact that it could be a bunch of different fixes all related to the same callee e.g. 1 + "" would produce one fix for first argument and one for second. Even if we didn't produce ambiguity related to overload in such case keeping them around wouldn't help to diagnose anything later because there is going to be no intersection so it's better to produce "I don't know" diagnostic and figure out case by case what went wrong...

Ah makes sense, I think I actually have seen situations like that too in SR-12689. I was just worried that it may end up discarding useful fixes in some way... but now I see the point :)

hborla

The new approach looks great!

…r fix diagnostics have failed If there are multiple solutions with fixes, let's not try to rank them based on overload choices because doing so would filter out viable candidates.

…ate fixes

Refactor `diagnoseAmbiguityWithFixes` as follows: - Aggregate all of the available fixes based on callee locator; - For each ambiguous overload match aggregated fixes and diagnose; - Discard all of the fixes which have been already considered as part of overload diagnostics; - Diagnose remaining (uniqued based on kind + locator) fixes iff they appear in all of the solutions.

…eAmbiguityWithFixes`

…y diagnosis

…uity

…irements If there is a conditional requirement failure associated with member/function reference used in a call let's increase a score of a fix for such failure because it renders member/function unreachable in current context or with a given set of arguments.

…ssion pattern use

…nose ambiguity with fixes

…t specially If aggregate fix (based on callee locator) differs only in picked overload choices, let's diagnose that via `diagnoseForAmbiguity` associated with a particular fix kind all solutions share, that would produce a tailored diagnostic instead of the most general one.

…pe before diagnosing ambiguity Instead of requiring sub-classes of `ContextualMismatch` to implement `diagnoseForAmbiguity` let's implement it directly on `ContextualMismatch` itself and check whether all of the aggregated fixes have same types on both sides and if so, diagnose as-if it was a single fix.

…e mismatch

…Ambiguity` For cases like this: ```swift struct X {} struct Y {} func overloaded<T>(_ value: T) -> T { value } func overloaded<T>(_ value: T) -> Int { 0 } func test(x: inout X, y: Y) { x = overloaded(y) } ``` Solver would record a `IgnoreAssignmentDestinationType` fix per overload, `diagnoseForAmbiguity` could be used to properly diagnose ambiguity cases like that.

…with a known foundation type Just like we already do for stdlib types, extend impact of missing conformances on _known_ foundation types because it's unlikely to be a valid suggestion to extend a foundation type with a new conformance.

…uity diagnosis

…alid loc) or AST node

…ment positions If parameter has a function type and no argument overload choices match let's diagnose that specifically. Resolves: [SR-12689](https://bugs.swift.org/browse/SR-12689) Resolves: rdar://problem/62481592

xedin · 2020-06-12T22:12:10Z

This PR is finally ready for review. @LucianoPAlmeida this PR incorporates a fix for SR-12689 which means that it supersedes #31359

LucianoPAlmeida · 2020-06-12T22:20:55Z

This PR is finally ready for review. @LucianoPAlmeida this PR incorporates a fix for SR-12689 which means that it supersedes #31359

Awesome @xedin \o/
Not sure if you've seen, but I left a comment on that PR few days ago with another SR that I believe may be fixed by these changes too, so maybe you can take a look if thats the case and add the regression test as well :)

xedin · 2020-06-12T23:29:46Z

@LucianoPAlmeida I've seen it, thanks! I think it's a different problem those, we'd need to split LabelingFailure into one-per-mismatch, that way we could diagnose cases like that better...

LucianoPAlmeida · 2020-06-13T01:40:36Z

@LucianoPAlmeida I've seen it, thanks! I think it's a different problem those, we'd need to split LabelingFailure into one-per-mismatch, that way we could diagnose cases like that better...

Ahh I see, but that's fine ... thanks for verifying :))

lib/Sema/CSFix.cpp

lib/Sema/ConstraintSystem.cpp

LucianoPAlmeida · 2020-06-13T02:32:27Z

test/expr/postfix/dot/init_ref_delegation.swift

@@ -323,12 +323,12 @@ class TestOverloadSets {
    self.init(5, 5) // expected-error{{extra argument in call}}
  }

-  convenience init(a : Z0) { // expected-note{{candidate has partially matching parameter list (a: Z0)}}
+  convenience init(a : Z0) { // expected-note{{candidate expects value of type 'Z0' for parameter #1}}


From what I remember from the other PR, the calls that trigger those have also a relabel fix alongside the argument mismatch. I think this is fine, but do you think to mention that in some way could be valid in those situations?

I don't really know, this is something I was thinking about as well. I don't really like partially matching parameter list note because it's as un-actionable as it gets but I don't know how to also mention a label here. It would be very unfortunate to produce two diagnostics/notes - one for labeling and one for type because that would be confusing...

Yeah, I agree that show two notes wouldn't be ideal. But in the other PR I also thought that partially matching parameter list was not ideal, but I did choose to show the relabel diagnostic, but to be honest there was no particular reason other than is better than partially matching parameter list. But in fact neither correct the type or relabel the call would fix the problem by itself because both type and label are wrong in this situation.
I've been thinking about it and also not sure how to mention the label and also don't know how much we are loosing not showing one of them ... probably not much since as you mention both are actionable

I think we could re-visit this once it's possible to produce a fix per label mismatch, that way we could probably coalesce type and label mismatch for each argument and produce a much better note...

…is pattern match

xedin · 2020-06-15T22:40:28Z

@swift-ci please smoke test

xedin · 2020-06-16T00:55:42Z

@swift-ci please smoke test Linux platform

xedin requested a review from hborla May 11, 2020 20:57

LucianoPAlmeida reviewed May 11, 2020

View reviewed changes

lib/Sema/ConstraintSystem.cpp Show resolved Hide resolved

LucianoPAlmeida reviewed May 11, 2020

View reviewed changes

hborla reviewed May 12, 2020

View reviewed changes

xedin mentioned this pull request May 18, 2020

[ConstraintSystem] Add a new locator element - argument attribute #31867

Merged

xedin added 18 commits June 12, 2020 11:46

[ConstraintSystem] Rank solutions based on overload choices only afte…

6b52016

…r fix diagnostics have failed If there are multiple solutions with fixes, let's not try to rank them based on overload choices because doing so would filter out viable candidates.

[ConstraintSystem] Encapsulate a way to diagnose ambiguity for aggreg…

a888518

…ate fixes

[ConstraintSystem] NFC: Leave notes on the algorithm used by `diagnos…

cf3f52c

…eAmbiguityWithFixes`

[TypeChecker] NFC: Adjust tests improved by new approach for ambiguit…

d9594c7

…y diagnosis

[CSFix] Make it possible to diagnose r-value -> l-value fix for ambig…

7641d9c

…uity

[ConstraintSystem] Specialize diagnostic for ~= to talk about expre…

9067ed2

…ssion pattern use

[ConstraintSystem] NFC/Debug: Log solutions before attempting to diag…

b9aa70b

…nose ambiguity with fixes

[Diagnostics] Diagnose ambiguity related to contextual type specifically

13b4fcc

[ConstraintSystem] Turn 'omit ' into a contextual mismatch it is

e2e57ad

[ConstraintSystem] Avoid recording duplicate fixes for contextual typ…

b407f7c

…e mismatch

[Diagnostics] Allow property wrapper fixes to be diagnosed as ambiguity

0af9a28

[Diagnostics] NFC: Adjust diagnostic test-cases improved by new ambig…

82fcee7

…uity diagnosis

xedin force-pushed the no-ranking-with-ambiguous-with-fixes branch from a01984e to 82fcee7 Compare June 12, 2020 20:13

xedin added 3 commits June 12, 2020 14:59

[ConstraintSystem] Attach candidate notes either to decl (if it has v…

f854cc6

…alid loc) or AST node

[TypeChecker] NFC: Restore test-cases removed by incorrect merge

c0840fd

xedin marked this pull request as ready for review June 12, 2020 22:10

LucianoPAlmeida mentioned this pull request Jun 13, 2020

[SR-12689] Diagnose arg mismatch with exact same fixes across multiple solutions #31359

Closed

LucianoPAlmeida reviewed Jun 13, 2020

View reviewed changes

lib/Sema/CSFix.cpp Outdated Show resolved Hide resolved

theblixguy reviewed Jun 13, 2020

View reviewed changes

lib/Sema/CSFix.cpp Show resolved Hide resolved

lib/Sema/ConstraintSystem.cpp Outdated Show resolved Hide resolved

lib/Sema/ConstraintSystem.cpp Show resolved Hide resolved

LucianoPAlmeida reviewed Jun 13, 2020

View reviewed changes

[Diagnostics] Use Id_MatchOperator to check whether given operator …

73667cd

…is pattern match

hborla approved these changes Jun 16, 2020

View reviewed changes

xedin merged commit 16862e5 into swiftlang:master Jun 16, 2020

LucianoPAlmeida mentioned this pull request Jun 18, 2020

[SR-12689] Undiagnosed argument mismatch involving pointer types #55133

Closed

[ConstraintSystem] Overhaul ambiguity diagnostics #31713

[ConstraintSystem] Overhaul ambiguity diagnostics #31713

Uh oh!

Conversation

xedin commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented May 11, 2020

Uh oh!

xedin commented May 11, 2020

Uh oh!

Uh oh!

LucianoPAlmeida May 11, 2020

Choose a reason for hiding this comment

Uh oh!

xedin May 11, 2020

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida May 11, 2020

Choose a reason for hiding this comment

Uh oh!

hborla left a comment

Choose a reason for hiding this comment

Uh oh!

xedin commented Jun 12, 2020

Uh oh!

LucianoPAlmeida commented Jun 12, 2020

Uh oh!

xedin commented Jun 12, 2020

Uh oh!

LucianoPAlmeida commented Jun 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LucianoPAlmeida Jun 13, 2020

Choose a reason for hiding this comment

Uh oh!

xedin Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

xedin Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

xedin commented Jun 15, 2020

Uh oh!

xedin commented Jun 16, 2020

Uh oh!

Uh oh!

xedin commented May 11, 2020 •

edited

Loading