Skip to content

[Diagnostics] Refactor diagnoseAmbiguityWithFixes. #29746

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

Merged
merged 13 commits into from
Feb 13, 2020

Conversation

hborla
Copy link
Member

@hborla hborla commented Feb 10, 2020

This change refactors diagnoseAmbiguityWithFixes to have a generalized strategy based on the differences between solutions in order to find the ambiguous overload, or other source of ambiguity. The new strategy will:

  • Aggregate fixes from all of the solutions, grouping "common" ones based on the FixKind and locator.
  • Identify the ambiguous overload based on the callee anchor of the overload choice and the anchor of the aggregated fixes.
  • If there is no ambiguous overload, it will diagnose common fixes that appear in all of the solutions.

This change also introduces a new method on CSFix called diagnoseForAmbiguity. This is called when there is no ambiguous overload, but the fix appears in each of the solutions.

Resolves: rdar://problem/55605863

@hborla hborla requested a review from xedin February 10, 2020 21:04
@hborla
Copy link
Member Author

hborla commented Feb 10, 2020

This will conflict with #29734 once merged

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

Looks good so far!

// TODO(diagnostics): The problem here is that `&` is interpreted as a binary operator, we need to re-think
// how "missing member" fix is implemented because currently it finds N solutions with multiple fixes.
takesP1AndP2([Swift.DoesNotExist & P1 & P2]()) // expected-error {{cannot invoke '' with no arguments}}
takesP1AndP2([Swift.DoesNotExist & P1 & P2]()) // expected-error {{module 'Swift' has no member named 'DoesNotExist'}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@@ -186,7 +186,7 @@ func testMap() {
}

// <rdar://problem/22414757> "UnresolvedDot" "in wrong phase" assertion from verifier
[].reduce { $0 + $1 } // expected-error {{cannot invoke 'reduce' with an argument list of type '(_)'}}
[].reduce { $0 + $1 } // expected-error {{missing argument for parameter #1 in call}}
Copy link
Contributor

Choose a reason for hiding this comment

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

An argument list of type... what?!

Nice improvement!

hborla and others added 9 commits February 11, 2020 14:52
…ces between

solutions in order to figure out the source of ambiguity.
the ambiguity diagnostic by combining a few special errors into one.
…all"

if the expression is not a function application.
…c parameters

It's done by first retrieving all generic parameters from each solution,
filtering boundings into distrinct set and diagnosing any differences.

For example:

```swift
func foo<T>(_: T, _: T) {}
func bar(x: Int, y: Float) {
  foo(x, y)
}
```
@xedin xedin force-pushed the ambiguity-with-fixes branch from 2278bdc to 4d5c676 Compare February 13, 2020 02:10
@xedin xedin marked this pull request as ready for review February 13, 2020 02:11
@xedin
Copy link
Contributor

xedin commented Feb 13, 2020

@hborla I have left all of your commits intact (expect for changes related to contextual result vs. operator ref), added diagnoseConflictingGenericArguments which handles situations like min(Int(0), Float(1)). Please take a look.

@xedin
Copy link
Contributor

xedin commented Feb 13, 2020

@swift-ci please smoke test

takeTuples((x, y), (x, y)) // expected-error{{cannot convert value of type '(Int, Float)' to expected argument type '(Float, Int)'}}

takeTuples((x, y), (x, y))
// expected-error@-1 {{conflicting arguments to generic parameter 'U' ('Float' vs. 'Int')}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Member Author

@hborla hborla left a comment

Choose a reason for hiding this comment

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

@xedin I left 2 small comments, but otherwise your changes look great - especially the new "conflicting generic args" error message. Thank you!

@xedin
Copy link
Contributor

xedin commented Feb 13, 2020

@swift-ci please smoke test

@xedin xedin merged commit 355148d into swiftlang:master Feb 13, 2020
@hborla hborla deleted the ambiguity-with-fixes branch February 13, 2020 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants