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

No way to find the original symbol for exported symbols #19444

Closed
tgodzik opened this issue Jan 15, 2024 · 10 comments · Fixed by #19494
Closed

No way to find the original symbol for exported symbols #19444

tgodzik opened this issue Jan 15, 2024 · 10 comments · Fixed by #19494
Labels
area:export area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools itype:enhancement
Milestone

Comments

@tgodzik
Copy link
Contributor

tgodzik commented Jan 15, 2024

Compiler version

3.4.0-RC1

Minimized code

We can get the example from the docs:

class BitMap
class InkJet

class Printer:
  type PrinterType
  def print(bits: BitMap): Unit = ???
  def status: List[String] = ???

class Scanner:
  def scan(): BitMap = ???
  def status: List[String] = ???

class Copier:
  private val printUnit = new Printer { type PrinterType = InkJet }
  private val scanUnit = new Scanner

  export scanUnit.scan
  export printUnit.{status as _, *}

  def status: List[String] = printUnit.status ++ scanUnit.status

Output

Currently when using the exported symbols there will be no link to the original symbol, which means both semanticdb and the presentation compiler will not work properly. This most likely affects Intellij also.

Expectation

There is a .sourceSymbol filed, which could maybe link to the original symbol?

@tgodzik tgodzik added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Jan 15, 2024
@jchyb jchyb added area:export area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jan 15, 2024
@odersky
Copy link
Contributor

odersky commented Jan 15, 2024

Adding a field to a symbol is a huge undertaking which will make to codebase messier. We were very careful to have only few fields that can easily be copied and transformed. For instance, we had a demand recently that symbols should include the compiler version under which they were compiled. Sounds reasonable, but I suggested we refactor code elsewhere so that we don't need a new field.

Generally, the design of the compiler follows the "no duplicated information" principle. If you add a field that you have to set manually but that could be recomputed in some way, you have created a possible loophole of information mismatches. That's why we generally don't do that, and rely on caches instead.

So, can we compute this info somehow? First, can you find to the export statement that introduced an exported symbol? If yes, you can take the type of its qualifier tree (let's call it prefix, and do

prefix.member(sym.name).suchThat(_.matches(sym))

@dwijnand
Copy link
Member

What's the reproduction for this? I tried to build one in CustomCompletionTests and that returns the original Scanner.status method symbol.

@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 15, 2024

What's the reproduction for this? I tried to build one in CustomCompletionTests and that returns the original Scanner.status method symbol.

I think completions would work as that is a correct symbol, you would need to check definition suite for example.

@odersky I wasn't talking about adding a field, I thought sourceSymbol which already exist was created for that exact purpose to be used by interactive features. If it's not possible to work with that, we will need to find the source symbol and then work on that. That's less than ideal, because we don't really have features that work on multiple files currently within the presentation compiler and I don't think ExtractSemanticdb phase also works on multiple trees at the same time. What would be the way to get that exact tree in that case from a totally different file?

@odersky
Copy link
Contributor

odersky commented Jan 16, 2024

@tgodzik Ah I see. You wrote "sourceSymbol filed" and I read that as "field". Yes, if we should absolutely try to make the sourceSymbol method work for that use case. Would the technique I suggested work?

@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 16, 2024

@tgodzik Ah I see. You wrote "sourceSymbol filed" and I read that as "field". Yes, if we should absolutely try to make the sourceSymbol method work for that use case. Would the technique I suggested work?

The technique would work for sure, though I did try that for a bit and hit a wall, since if we have exported symbols from another file, is it possible to get the tree for that? Or is there a way via tasty to do that?

@odersky
Copy link
Contributor

odersky commented Jan 16, 2024

Exports are reified in Tasty. So there should be an Export node in the tree of the file where the symbol comes from. It's just that we need to be able to find it.

@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 16, 2024

Do we have any similar logic somewhere that we could base the implementation on?

@dwijnand
Copy link
Member

I think completions would work as that is a correct symbol, you would need to check definition suite for example.

I've tried a case in HoverDefnSuite, one in PcDefinitionSuite and one in TypeDefinitionSuite, after the one in CustomCompletionTests, and they all work. Could you provide a reproduction and clarify the expectation?

@tgodzik
Copy link
Contributor Author

tgodzik commented Jan 16, 2024

  @Test def `export` =
    check(
      """|
        |object enumerations:
        |  trait <<SymbolKind>>
        |  trait CymbalKind
        |
        |object all:
        |  export enumerations.*
        |
        |@main def hello =
        |  import all.SymbolKind
        |  import enumerations.CymbalKind
        |
        |  val x = new Symbo@@lKind {}
        |  val y = new CymbalKind {}
        |""".stripMargin
    )

I added this case in PcDefinitionSuite and it does fail. In what cases did it work for you?

@dwijnand
Copy link
Member

  @Test def exports =
    val code =
      """class BitMap
        |class InkJet
        |
        |class Printer:
        |  type PrinterType
        |  def print(bits: BitMap): Unit = ???
        |  def status: List[String] = ???
        |
        |class Scanner:
        |  def scan(): BitMap = ???
        |  def <<status>>: List[String] = ???
        |
        |class Copier:
        |  private val printUnit = new Printer { type PrinterType = InkJet }
        |  private val scanUnit = new Scanner
        |
        |  export scanUnit.scan
        |  export printUnit.{status as _, *}
        |
        |  def status: List[String] =
        |    printUnit.status ++ scanUnit.sta@@tus""".stripMargin
    check(code)

@dwijnand dwijnand linked a pull request Jan 19, 2024 that will close this issue
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:export area:presentation-compiler Related to the presentation compiler module used by Metals and possibly other tools itype:enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants