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 export statements Go To Definition/Hover is broken #4325

Open
keynmol opened this issue Sep 1, 2022 · 11 comments
Open

Scala 3 export statements Go To Definition/Hover is broken #4325

keynmol opened this issue Sep 1, 2022 · 11 comments
Labels
bug Something that is making a piece of functionality unusable semanticdb Related to semanticdb usage in Metals

Comments

@keynmol
Copy link
Contributor

keynmol commented Sep 1, 2022

Describe the bug

Say you have a Scala CLI file like this (BSP doesn't matter, I originally spotted it in SBT project way back):

//> using scala "3.2.0"

object enumerations:
  trait SymbolKind
  trait CymbalKind

object all:
  export enumerations.*

@main def hello = 
  import all.SymbolKind
  import enumerations.CymbalKind

  val x = new SymbolKind {}
  val y = new CymbalKind {}

When you GTD on SymbolKind, it takes you to export statement - not to definition of the trait.
When you hover on SymbolKind it shows type SymbolKind, not trait.

2022-09-01 08 06 52

Expected behavior

If you try either of the two things above with Cymbal, it takes you to the definition and hover shows "trait".

I'd expect the same work with SymbolKind, i.e. bypass the all object and its export statement completely and take you to the definition.

Operating system

macOS

Editor/Extension

Nvim (nvim-metals)

Version of Metals

0.11.8+55-e48b5ad4-SNAPSHOT

Extra context or search terms

scala 3, export, hover, go to definition

@tgodzik tgodzik added bug Something that is making a piece of functionality unusable semanticdb Related to semanticdb usage in Metals labels Sep 1, 2022
@tgodzik
Copy link
Contributor

tgodzik commented Sep 1, 2022

Thanks for the reproduction! I see great use of ScalCLI 🚀

@tanishiking
Copy link
Member

Indeed Scala3 generates all.SymbolKind# instead of enumerations.SymbolKind.

package `export-stmt`

object enumerations/*<-`export-stmt`::enumerations.*/:
  trait SymbolKind/*<-`export-stmt`::enumerations.SymbolKind#*/
  trait CymbalKind/*<-`export-stmt`::enumerations.CymbalKind#*/

object all/*<-`export-stmt`::all.*/:
  export enumerations/*->`export-stmt`::enumerations.*/.*

@main/*->scala::main#*/ def hello/*<-`export-stmt`::`exports-stmt$package`.hello().*/ =
  import all/*->`export-stmt`::all.*/.SymbolKind
  import enumerations/*->`export-stmt`::enumerations.*/.CymbalKind

  val x/*<-local2*/ = new SymbolKind/*->`export-stmt`::all.SymbolKind#*/ {}
  val y/*<-local5*/ = new CymbalKind/*->`export-stmt`::enumerations.CymbalKind#*/ {}

@keynmol
Copy link
Contributor Author

keynmol commented Sep 5, 2022

Ohh that's a nice view of the code!
Pardon the stupid question - but how did you get that?

@tanishiking
Copy link
Member

Pardon the stupid question - but how did you get that?

It's not a stupid question!

In lampepfl/dotty repository, it provides a way to regression test for semanticdb.
We can put (basically) a self-contained Scala file in the expect directory (e.g. Vals.scala), and we can generate Vals.expect.scala (something like the scala code I pasted above) by running sbt:scala3> scala3-compiler-bootstrapped/test:runMain dotty.tools.dotc.semanticdb.updateExpect 👍
`

@tanishiking
Copy link
Member

For more details: https://github.com/lampepfl/dotty/blob/fba41063b7cae5b9235ec2c5e2d92b58de776c9b/docs/_docs/contributing/testing.md#semanticdb-tests

@tanishiking
Copy link
Member

I'm wondering if we should see this behavior as "broken" 🤔

  • SemanticDB points to the right place
    • In the above example, the SemanticDB symbol for new SymbolKind points to all.enumeration.*, which is semantically correct
    • Because the export clause seems like a syntax sugar that expands to export aliases https://dotty.epfl.ch/docs/reference/other-new-features/export.html
    • Therefore, if we change the go-to-definition behavior over export, we should fix it in Metals, I think.
  • Should we jump to the original SymbolKind instead of the export clause?
    • For example, in the code example of the official doc, jumping directly to the original method may not be helpful.
    • because it exports printUnit.{status as _, *} where printUnit is the instance of Printer with type PrinterType = InkJet.
    • Developers may miss the information about type PrinterType = InkJet if Metals jump the directory to methods in Printer.
class Copier:
  private val printUnit = new Printer { type PrinterType = InkJet }
  private val scanUnit = new Scanner

  export scanUnit.scan
  export printUnit.{status as _, *}
  • Hover signature is the same, I think we should keep the current behavior for the type signature. Because it actually has a final modifier.
    • However, it would be really helpful if Metals could show the original scaladoc for export clause 👍

@pavelfatin
Copy link

@tgodzik
Copy link
Contributor

tgodzik commented Sep 8, 2022

Should we jump to the original SymbolKind instead of the export clause?

I think it should be fine to jump to the export clause if it's more complex, but it would be cool to jump to the original if the export is for example a wildcard 🤔

However, it would be really helpful if Metals could show the original scaladoc for export clause

100% agree, this could be a separate issue.

@Swoorup
Copy link

Swoorup commented Sep 8, 2022

I think it should be fine to jump to the export clause if it's more complex, but it would be cool to jump to the original if the export is for example a wildcard 🤔

I normally have a single file called syntax.scala where multiple symbols from various file for that project are exported. Would be cool if we could jump right to the source in case of * and givens

@tgodzik
Copy link
Contributor

tgodzik commented Jan 15, 2024

Raised an upstream issue since it might be hard to figure out without the compiler support scala/scala3#19444

@ckipp01
Copy link
Member

ckipp01 commented Jan 27, 2024

I just hit on another maybe related example of this, but the behavior is slightly different. I'll include it in here to double-check when this is fixed to see if it then also behaves correctly. The tricky part is that I can't get a minimal reproduction to behave the same, so something else might be at play.

The minimal reproduction that I made was this:

//> using scala 3.3.1

object Thing:

  export Inner.example

  private object Inner:
    val example = ???

object Test:
  Thing.example

This behaves the same as what @keynmol describes. Where I hit on this in the wild where the behavior was slightly different was when triggering a goto definition on command here. It brings me to the object definition here instead of the export statement here like the minimal example does. Not sure what is happening here. It's probably @keynmol 's fault though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that is making a piece of functionality unusable semanticdb Related to semanticdb usage in Metals
Projects
None yet
Development

No branches or pull requests

6 participants