Skip to content
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

Fix implicit search failure reporting #20261

Merged
merged 4 commits into from
May 1, 2024
Merged

Fix implicit search failure reporting #20261

merged 4 commits into from
May 1, 2024

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Apr 24, 2024

Fixes #19414.


retyped
else
val res = untpd.Apply(tree, args).withType(firstFailure.get)
Copy link
Member Author

@mbovel mbovel Apr 25, 2024

Choose a reason for hiding this comment

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

Before we would use the first non-ambiguous error if there is one; here I simplified it to just use the first error.

@odersky commented on #19414 that:

Ambiguous implicits often arise when something else is failing (for instance a variable was not instantiated enough). That's why we try to find first other errors. That was the thinking behind the current semantic. [...]

But I think this is not relevant anymore after the changes in this PR; as we're anyway reporting all errors by recursively visiting the argument trees.

@mbovel mbovel marked this pull request as ready for review April 25, 2024 10:38
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Let's discuss how we can be the functionality of "I found" in the error messages before a more detailed code review.

| Test.listOrd[List[T]](Test.listOrd[T](/* missing */summon[Test.Ord[T]]))
|
| But no implicit values were found that match type Test.Ord[T].
| No given instance of type Test.Ord[T] was found for parameter o of method listOrd in object Test
Copy link
Contributor

Choose a reason for hiding this comment

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

The change in parameter name is good, but I think we should keep the "I found" part of the error message. Otherwise it's not clear why a listOrd is required in the first place. The "I found" part tells you that.

@odersky odersky assigned mbovel and unassigned odersky Apr 25, 2024
Instead, fix the display of nested `AmbiguousImplicits` errors.
@mbovel
Copy link
Member Author

mbovel commented Apr 25, 2024

I tried something a bit different to keep the "I found" messages.

The problematic case with the current infrastructure (apart from handling default arguments) is really ambiguous implicit that are nested. In this case, we just bubble the error up and the error message then use the wrong param names, but all the other cases are fine.

So, to solve this, I tried to just add a nested flag in AmbiguousImplicits and modified the error message to also use the ""I found"-flavored if this flag is set to true. This avoids having to recursively traverse the tree of the failing implicit to display error.

This approach leaves all existing tested message outputs unchanged, but only fixes the new test cases.

@mbovel mbovel assigned odersky and unassigned mbovel Apr 26, 2024
@odersky odersky removed their assignment Apr 27, 2024
@mbovel
Copy link
Member Author

mbovel commented May 1, 2024

@odersky: I changed the message to be "No best given [...]" for ambiguous implicits, as discussed.

@mbovel mbovel requested a review from odersky May 1, 2024 07:23
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Nice!

@odersky odersky merged commit 2e41763 into scala:main May 1, 2024
19 checks passed
@mbovel mbovel deleted the mb/19414-3 branch May 7, 2024 12:53
@mbovel
Copy link
Member Author

mbovel commented May 7, 2024

Potential regression: #20344.

odersky added a commit to dotty-staging/dotty that referenced this pull request May 8, 2024
This reverts one part of scala#20261. When we fail with both an ambiguity on one implicit
argument and another error on another we prefer the other error. I added a comment
why this is needed.

Fixes scala#20354
odersky added a commit to dotty-staging/dotty that referenced this pull request May 8, 2024
This reverts one part of scala#20261. When we fail with both an ambiguity on one implicit argument and another error on another argument we prefer the other error. I added a comment why this is needed.

Fixes scala#20344
odersky added a commit that referenced this pull request May 8, 2024
#20368)

This reverts one part of #20261. When we fail with both an ambiguity on
one implicit argument and another error on another argument we prefer
the other error. I added a comment why this is needed.

Fixes #20344
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
WojciechMazur added a commit to WojciechMazur/dotty that referenced this pull request Jun 17, 2024
…uity on one implicit argument and another error on another argument we prefer the other error. I added a comment why this is needed.
WojciechMazur added a commit that referenced this pull request Jun 17, 2024
…on one implicit argument and another error on another argument we prefer the other error. I added a comment why this is needed.
WojciechMazur pushed a commit that referenced this pull request Jul 6, 2024
This reverts one part of #20261. When we fail with both an ambiguity on one implicit argument and another error on another argument we prefer the other error. I added a comment why this is needed.

Fixes #20344

[Cherry-picked 863077c]
WojciechMazur pushed a commit that referenced this pull request Jul 6, 2024
This reverts one part of #20261. When we fail with both an ambiguity on one implicit argument and another error on another argument we prefer the other error. I added a comment why this is needed.

Fixes #20344

[Cherry-picked 863077c]
WojciechMazur added a commit that referenced this pull request Jul 6, 2024
Backports #20261 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
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.

Wrong error message for ambiguous given instances
3 participants