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

Add missing span to extension method select #18557

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

kasiaMarek
Copy link
Contributor

For:

class MyIntOut(val value: Int)
object MyIntOut:
  extension (i: MyIntOut) def uneven = i.value % 2 == 1

val a = MyIntOut(1).uneven

uneven call in the typed tree has span <126..126>, where it should be <138..144>.

connected to: scalameta/metals#5630

I'm open to suggestions on how to do it nicer.

@nicolasstucki nicolasstucki self-assigned this Sep 18, 2023
@@ -860,7 +860,7 @@ trait Implicits:
/** Find an implicit conversion to apply to given tree `from` so that the
* result is compatible with type `to`.
*/
def inferView(from: Tree, to: Type)(using Context): SearchResult = {
def inferView(from: Tree, to: Type, maybeNameSpan: Option[Span] = None)(using Context): SearchResult = {
Copy link
Member

Choose a reason for hiding this comment

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

We already a NoSpan "null" object we can use, instead of having to wrap it with Option. Could you switch to that, please? You can then call .exists on the span (which just compares to NoSpan) instead of calling foldLeft (which also creates another lambda).

@nicolasstucki nicolasstucki removed their request for review September 19, 2023 07:51
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Hi, sorry for not mention this earlier. I thinking now we should try to carry the name span together with the name, in the SelectionProto. If need be, I can try that out myself when I'm back home on Monday.

@tanishiking
Copy link
Member

tanishiking commented Sep 19, 2023

@kasiaMarek Awesome, could you add a test case for SemanticDB that confirms this change fixes #18570 ?

diff --git a/tests/semanticdb/expect/Extension.scala b/tests/semanticdb/expect/Extension.scala
index c204b1ff7f..86d6ec0586 100644
--- a/tests/semanticdb/expect/Extension.scala
+++ b/tests/semanticdb/expect/Extension.scala
@@ -16,3 +16,12 @@ extension (s: String)
 
 trait Functor[F[_]]:
   extension [T](t: F[T]) def map[U](f: T => U): F[U]
+
+opaque type Deck = Long
+object Deck:
+  extension (data: Deck)
+    def fooSize: Int = ???
+
+object DeckUsage:
+  val deck: Deck = ???
+  deck.fooSize
\ No newline at end of file

and run sbt:scala3> scala3-compiler-bootstrapped/test:runMain dotty.tools.dotc.semanticdb.updateExpect.

Now fooSize in DeckUsage has a Symbol Occurrence 👍

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Very nicely done, thank you. 1 diff question left

@tgodzik tgodzik added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Sep 19, 2023
@kasiaMarek
Copy link
Contributor Author

Awesome, could you add a test case for SemanticDB that confirms this change fixes #18570 ?

done

@bishabosha bishabosha merged commit 2486191 into scala:main Sep 20, 2023
17 checks passed
@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Oct 10, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
Backports #18557 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
Backports #18557 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.

7 participants