Skip to content

[Completions] Fix duplicated methods from implicit conversions #14036

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

Closed

Conversation

dos65
Copy link
Contributor

@dos65 dos65 commented Dec 3, 2021

Fixes the following case:

Array(1,2,3).fil@@

Before this fix there were several filter and filterNot methods in
completions they were taken from all eligible implicit candidates.

Now there is a kind of ranking per denotation.

Fixes the following case:
```scala
Array(1,2,3).fil@@
```

Before this fix there were several `filter` and `filterNot` methods in
completions they were taken from all eligible implicit candidates.

Now there is a kind of ranking per denotation.
@dos65 dos65 force-pushed the completions_fix_duplicates_from_conversions branch from 39a65d0 to 02497d4 Compare December 6, 2021 09:01
@dos65 dos65 requested a review from prolativ December 6, 2021 11:53
@prolativ
Copy link
Contributor

prolativ commented Dec 7, 2021

In general this looks like going in the right direction but the solution seems quite incomplete by handling only methods from implicit conversions but not new style extension methods. To verify the results I would recommend adding the tests cases like this

  @Test def methodFromImplicitConversionWithBestResultType : Unit =
    code"""|implicit class OptionWrap(i: Int):
           |  def wrap: Option[Int] = Some(i)
           |
           |implicit class SeqWrap(i: Int):
           |  def wrap: Seq[Int] = Seq(i)
           |
           |val w: Option[Int] = 1.wr${m1}
           |""".withSource
      .completion(m1, Set(("wrap", Method, "=> Option[Int]")))
      
  @Test def extensionMethodWithBestResultType : Unit =
    code"""|given optionWrap: {} with
           |  extension (i: Int) def wrap: Option[Int] = Some(i)
           |
           |given seqWrap: {} with
           |  extension (i: Int) def wrap: Seq[Int] = Seq(i)
           |
           |val w: Option[Int] = 1.wr${m1}
           |""".withSource
      .completion(m1, Set(("wrap", Method, "=> Option[Int]")))

@dos65
Copy link
Contributor Author

dos65 commented Dec 7, 2021

@prolativ Thx! I thought to check other cases too but wasn't sure if solution is valid. Will cover the suggested samples.

@prolativ
Copy link
Contributor

prolativ commented Dec 7, 2021

Other cases that we should consider and test:

  • One method coming from an extension and one from an implicit conversion
  • If there is an ambiguity in implicits (e.g. when you remove the explicit type annotation from val w in my examples) - we should probably include both alternatives in the completions - here also one method might come from an extension and the other from an implicit conversion.
    In case of any doubts how some corner cases should be handled just try to compile some code and check which signatures are chosen by the compiler

@tgodzik
Copy link
Contributor

tgodzik commented Feb 18, 2025

Closing and leaving and open issue

@tgodzik tgodzik closed this Feb 18, 2025
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.

3 participants