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

Scala 3: semantic info for Infix arguments cannot be looked up #1594

Closed
bjaglin opened this issue Apr 10, 2022 · 11 comments · Fixed by scalameta/scalameta#2736 or #1604
Closed

Scala 3: semantic info for Infix arguments cannot be looked up #1594

bjaglin opened this issue Apr 10, 2022 · 11 comments · Fixed by scalameta/scalameta#2736 or #1604

Comments

@bjaglin
Copy link
Collaborator

bjaglin commented Apr 10, 2022

trait SymbolTest {
def shouldBe(right: Any): Unit
def arg = 1
this shouldBe (arg)
}

L13: Term.ApplyInfix(_, Term.Name("shouldBe"), _, arg :: Nil)

Symbol lookup for arg fails against Scala 3.1.1 SemanticDB as the position there excludes surrounding parentheses while 2.x (scalac-semanticdb) and the parser include them

https://github.com/scalacenter/scalafix/pull/1528/files#r846793225

@tgodzik
Copy link
Contributor

tgodzik commented Apr 12, 2022

Good catch! Did you report it in the Scala 3 compiler?

@bjaglin
Copy link
Collaborator Author

bjaglin commented Apr 12, 2022

@tgodzik to be honest, I wasn't sure what was at fault here. Is there any spec or tests in the reference implementation I could link to if I create a ticket on dotty?

@tanishiking
Copy link
Contributor

I checked how the Scala3 compiler generates the SemanticDB for infix argument tanishiking/scala3@5781617 (though the latest Scala3 generates it in main branch, there's no change around the position of symbol occurrence these days AFAIK).
But it seems like the symbol position for arg takes the surrounding parentheses into account.

@tanishiking
Copy link
Contributor

tanishiking commented Apr 22, 2022

I got it, for the following code

trait SymbolTest {
  def shouldBe(right: Any): Unit
  def arg = 1
  this shouldBe (arg)
}

Scala2 (semanticdb-scalac) generates:

trait SymbolTest/*<=_empty_.SymbolTest#*/ { 
  ...
  this shouldBe/*=>_empty_.SymbolTest#shouldBe().*/ (arg)/*=>_empty_.SymbolTest#arg().*/ 
} 

[3:16..3:21): (arg) => _empty_/SymbolTest#arg().

and Scala3 generates:

trait SymbolTest/*<=_empty_.SymbolTest#*/ { 
  ...
  this shouldBe/*=>_empty_.SymbolTest#shouldBe().*/ (arg/*=>_empty_.SymbolTest#arg().*/)
} 

[3:17..3:20): arg -> _empty_/SymbolTest#arg().

@tanishiking
Copy link
Contributor

I personally see the Scala3 generated symbol occurrence for arg seems more accurate, and it would be better to handle on the scalafix side? 🤔

@tgodzik
Copy link
Contributor

tgodzik commented Apr 22, 2022

Hmm... having just arg seems more resonable, or are we missing something? The alternative would be to change it in the parser and in semanticdb Scala 2 plugin

@rochala
Copy link
Contributor

rochala commented Apr 22, 2022

The parenthesis inclusion is done only by scalameta parser. Both scala2 and scala3 trees at compilation exclude them and properly show position for arg ( excluding the parenthesis ). Scala3 semantic db position is thou different because its based on trees from compiler.
I'm no expert but in my opinion fixing it in scala3 would be a hack.

@tanishiking
Copy link
Contributor

I think scalameta parser should exclude The parentheses (if there're no special reasons), that should fix both scala2 and scala3?

@tgodzik
Copy link
Contributor

tgodzik commented Apr 22, 2022

I think scalameta parser should exclude The parentheses (if there're no special reasons), that should fix both scala2 and scala3?

Looks like it! We should use the Positions suite for that, are you able to take a look in scalameta @tanishiking ?

@tanishiking
Copy link
Contributor

@bjaglin
Copy link
Collaborator Author

bjaglin commented May 1, 2022

Thanks @tanishiking & @tgodzik !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants