scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

Symbol position is different in Scala 2.13.12 #12877

Closed ddworak closed 6 months ago

ddworak commented 1 year ago

Reproduction steps

Scala version: 2.13.12 Java version: 17 Example, minimal project: https://github.com/ddworak/scala-position-change Run sbt +app/run

Problem

scala.reflect.api.Symbols.SymbolApi#pos returns different results in Scala 2.13.12 than in 2.13.11, at least for classes and methods. Given Scala file:

package com.example.app

class ExampleClass {
  def method(arg: Int): Unit = println(arg)
}

In Scala 2.13.11: weakTypeOf[ExampleClass].typeSymbol.pos.start is 25 weakTypeOf[ExampleClass].typeSymbol.pos.end is 91 pointing to the content of the whole class.

In Scala 2.13.12: weakTypeOf[ExampleClass].typeSymbol.pos.start is 31 weakTypeOf[ExampleClass].typeSymbol.pos.end is 43 pointing to just the class name.

This was originally detected in https://github.com/AVSystem/scala-commons/pull/492 on a slightly more complex macro and its test: https://github.com/AVSystem/scala-commons/blob/642f96842a180466e7652ca9ed5e358045d522c6/core/src/test/scala/com/avsystem/commons/rpc/SymbolSourceTest.scala, which also detected changes in method symbols.

som-snytt commented 1 year ago

This was a requested feature, and has a follow-up PR. The change is not behind a compiler flag.

The position of a symbol of a named definition is not the tree position (which covers everything) but just the name.

Here is the ticket https://github.com/scala/bug/issues/12735

I see that it was also to align with Scala 3, as well as be more practical for reporting.

ddworak commented 1 year ago

I see. What would be the suggested, idiomatic way to get the position of the tree now?

som-snytt commented 1 year ago

The tree's position is unchanged, but the symbol's position is tweaked.

If I understand the question, Scala 2 macros receive trees (which are local with respect to the def macro or paradise annotation).

Discovering other trees is more of an adventure (enclosing trees, sibling trees in enclosing trees).

ddworak commented 1 year ago

The question was whether there is a way to retrieve previously available symbol position after this change, as I'm trying to update the code to Scala 2.13.12 without changing the original functionality.

From my limited understanding of Scala 2 macros, discovering this specific Tree from a Symbol / WeakTypeTag is a "take the Ring to Mordor" type of adventure, but I was hoping I might be missing something.

som-snytt commented 1 year ago

I took a quick look at your repo and went what, so I thought I'd do a quick test. I expected to retrieve tree positions by looking at compilation units of the current run, which know their trees.

However, the naive macro works for some reason:

import language.experimental.macros
import scala.reflect.macros.blackbox.Context

// macro to turn a symbol into a spanning position of the definition

object denamer {
  def span[A]: Option[(Int, Int, String)] = macro spanner[A]

  def spanner[A: c.WeakTypeTag](c: Context) = {
    import c.universe._

    val sym = symbolOf[A]
    val text = String.valueOf(sym.pos.source.content, sym.pos.start, sym.pos.end - sym.pos.start)
    q"Option.apply((${sym.pos.start}, ${sym.pos.end}, $text))"
  }
}

with usage

/* some header cruft */

class C {
  def p() = println("hello, world")
}

object Test extends App {
  println(denamer.span[C])
}

and output

$ scalac -d /tmp/sandbox denamer.scala && scalac -d /tmp/sandbox -cp /tmp/sandbox denamer-test.scala && scala -cp /tmp/sandbox Test
Some((26,73,class C {
  def p() = println("hello, world")
}))

Wait let me check --

scalac -version
Scala compiler version 2.13.12 -- Copyright 2002-2023, LAMP/EPFL and Lightbend, Inc.

It's so hard to keep up with obsolete technology, so let me know what you are doing differently.

ddworak commented 1 year ago

Thank you for the example, things got way more interesting now. I updated my project to match your code, yet the results were exactly the same as before. The difference seems to come from the compilation process with scalac directly and in SBT with a separate macro module. If you clone my repository now:

$ scalac -d /tmp/sandbox macros/src/main/scala/com/example/macros/denamer.scala && scalac -d /tmp/sandbox -cp /tmp/sandbox app/src/main/scala/com/example/app/Main.scala && scala -cp /tmp/sandbox com.example.app.Main

Some((60,107,class C {
  def p() = println("hello, world")
}))

but:

$ sbt app/run

[info] welcome to sbt 1.9.6 (Private Build Java 17.0.8.1)
[info] loading settings for project global-plugins from sbt-updates.sbt ...
[info] loading global plugins from /home/nuk/.sbt/1.0/plugins
[info] loading project definition from /privcode/scala-position-change/scala-position-change/project
[info] loading settings for project root from build.sbt ...
[info] set current project to root (in build file:/privcode/scala-position-change/scala-position-change/)
[info] compiling 1 Scala source to /privcode/scala-position-change/scala-position-change/macros/target/scala-2.13/classes ...
[info] compiling 1 Scala source to /privcode/scala-position-change/scala-position-change/app/target/scala-2.13/classes ...
[info] running com.example.app.Main 
Some((66,67,C))

Running a JAR packaged by sbt in scala directly does not change the output:

$ scala -cp app/target/scala-2.13/app_2.13-0.1.0-SNAPSHOT.jar com.example.app.Main

Some((66,67,C))

However, when I have added a ScalaTest in macros module:

$ sbt macros/test
[info] welcome to sbt 1.9.6 (Private Build Java 17.0.8.1)
[info] loading settings for project global-plugins from sbt-updates.sbt ...
[info] loading global plugins from /home/nuk/.sbt/1.0/plugins
[info] loading project definition from /privcode/scala-position-change/scala-position-change/project
[info] loading settings for project root from build.sbt ...
[info] set current project to root (in build file:/privcode/scala-position-change/scala-position-change/)
Some((71,118,class A {
  def p() = println("hello, world")
}))

So the position behaves differently when the macro is in another module / JAR file in Scala 2.13.12?

som-snytt commented 1 year ago

Thanks for the insight. I don't know offhand what the difference is with a resident sbt compiler, as to how a symbol is presented to a macro. I assume any difference would be a bug.

I saw that Scala 3 has options for retaining trees, which probably would not be a viable approach.

But maybe for 2.13.13, there can be API for the "definitionPos" or similar. That did not come up on the ticket for namePos (where the requirement was for positions in diagnostics). I remember there was a comment, Why keep that pos if every client (IDE) discards it.

I would guess that the new API would be sym.pos.treePos. Dotty has outer that seems to be for inlined things.

ddworak commented 1 year ago

@som-snytt if 2.13.13 could have that API, it would solve the issue for the projects I maintain. Is this something you can easily add based on previous logic for pos or would a more significant effort be required?

som-snytt commented 1 year ago

I'm guessing that adding API is OK for internal/experimental. Nothing is easy. The qualifier might be performance, but I suspect just adding a definitionPos field is not terrible.

To clarify terms, focus means a position that is the point of this position. (namePos as a subrange is not a focus in this sense.)

Previously, we didn't notice

➜  scalac -d /tmp/sandbox denamer.scala && scalac -d /tmp/sandbox -cp /tmp/sandbox denamer-test.scala && scala -cp /tmp/sandbox Test
Some((32,33,C))
➜  scalac -d /tmp/sandbox denamer.scala && scalac -d /tmp/sandbox -cp /tmp/sandbox denamer-test.scala && scala -cp /tmp/sandbox Test
Some((26,73,class C {
  def p() = println("hello, world")
}))
som-snytt commented 8 months ago

The PR offers -Ylegacy-symbol-pos to restore the previous behavior.

An alternative was to use the "internal API" to retrieve a special attachment, but a compiler option is more straightforward.

SethTisue commented 8 months ago

note that the discussion of how to proceed is now split between this ticket and the PR, https://github.com/scala/scala/pull/10678

som-snytt commented 8 months ago

As noted, this will need another swing, to solve the discrepancy shown above, and possibly also provide the previous function via internal.

ddworak commented 7 months ago

Thank you for trying @som-snytt! If it proves too much of an issue or is a very isolated case, then feel free to drop this altogether. Our use case was not critical for production usage and we've migrated to 2.13.12 without the feature in question.