Method calls get a line number

[som-snytt]

Fixes #17425

Forward ports scala/scala#10393

[som-snytt]

A progression in specs2 location

[info] set current project to specs2 (in build file:/__w/dotty/dotty/community-build/community-projects/specs2/)
[error]  13 != 16 (LocationSpec.scala:18)
[error]  
[error]  18 != 23 (LocationSpec.scala:27)
[error]  
[error]  19 != 20 (LocationSpec.scala:30)
[error]  
[error]  21 != 22 (LocationSpec.scala:33)

[sjrd]

You'll need a

// scalajs: --skip

at the top of this file.

[som-snytt]

aha!

[adpi2]

In your test example:

object Test:

  def void(): Unit = ???
  def self(): this.type = this

  def test() =
    this // this is line 8
      .self()
      .void()

I got a debug line on line 8, on the this instruction. This debug line is not added by your PR, but I wonder if you could remove it.

The full debug line table is:

    LineNumberTable:
      line 8: 0
      line 9: 1
      line 10: 4

For the same reason, we now got two debug lines on the breakpoint of the following example, one on loading list and another on invoking foreach, and the debugger stops twice. There should be only one debug line.

val list = List(1, 2, 3)
/* breakpoint */ list.foreach { x => 
  println(x)  
}

[adpi2]

Another example is this one:

val list = List(1)
for {
  x <- list
  y <- list  // line 4
  z = x + y // line 5
} yield x

If I put a breakpoint line 4 and 5:

• before your PR it stops on 4, 5, 5, 4, 5, 4, 4, 5
• after your PR it stops on 4, 5, 4, 5, 4, 5, 4, 4, 4, 5

As you can see the compiler puts a lot too much debug lines, notably in the lambda methods of the for loop. This is not a consequence of your PR, but it makes it slightly worse, adding even more debug lines. If you can remove the debug line on loading the variable, it would surely help.

[lrytz]

Already without this PR Scala 3 emits more line numbers than 2, for this example

val list = List(1, 2, 3)
list.foreach { x => 
  println(x)  
}

Difference between Scala 2 and 3 (without this PR):

@@ -25,20 +25,23 @@
    L1
     LINENUMBER 14 L1
     ALOAD 1
-    INVOKEDYNAMIC apply$mcVI$sp()Lscala/runtime/java8/JFunction1$mcVI$sp; [
+   L2
+    LINENUMBER 15 L2
+    ALOAD 0
+    INVOKEDYNAMIC apply$mcVI$sp(LT$;)Lscala/runtime/java8/JFunction1$mcVI$sp; [
       // handle kind 0x6 : INVOKESTATIC
       java/lang/invoke/LambdaMetafactory.altMetafactory(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;
       // arguments:
       (I)V,
-      // handle kind 0x6 : INVOKESTATIC
-      T$.$anonfun$u$1(I)V,
+      // handle kind 0x7 : INVOKESPECIAL
+      T$.u$$anonfun$1(I)V,
       (I)V,
       1
     ]
     INVOKEVIRTUAL scala/collection/immutable/List.foreach (Lscala/Function1;)V
     RETURN
-   L2
-    LOCALVARIABLE list Lscala/collection/immutable/List; L1 L2 1
-    LOCALVARIABLE this LT$; L0 L2 0
+   L3
+    LOCALVARIABLE list Lscala/collection/immutable/List; L1 L3 1
+    LOCALVARIABLE this LT$; L0 L3 0
     MAXSTACK = 6
     MAXLOCALS = 2

Lambdas are encoded slightly differently, in Scala 2 the body method is static, in Scala 3 it's private. So the receiver needs to be loaded on the stack in Scala 3. Not sure if the additional line number is related to that.

I got a debug line on line 8, on the this instruction. This debug line is not added by your PR, but I wonder if you could remove it.

@adpi2 why would you remove that line number? It seems useful to be able to stop the debugger on that line.

[adpi2]

@adpi2 why would you remove that line number? It seems useful to be able to stop the debugger on that line.

I spoke too fast. It is indeed useful to be able to stop on that line. The debug line that annoys me is the apply$mcVI$sp.

Suppose we have those lines:

1 list.foreach { x => 
2   println(x)  
3 }

In Scala 2 the debug sequence looks like this:

Line 1:
  load list
  invoke apply$mcVI$s
  invoke foreach

In Scala 3 before the PR:

Line 1: load list
Line 2: 
  load this
  invoke apply$mcVI$s
  invoke foreach

In Scala 3 after the PR:

Line 1: load list
Line 2:
  load this
  invoke apply$mcVI$s
Line 1: invoke foreach 

This round trip to line 1 is confusing. Putting a line number on the method call is okay, but the intermediate step on line 2 is annoying. If we can find a way to remove it that would be better.

[lrytz]

I think going back to line 1 is correct. Take this Java example:

public class Test {
  Test self() { return this; }
  void run(String arg) { return; }
  void t() {
    self().run(
      toString()
    );
  }
}

javac produces the following bytecode:

    LINENUMBER 5 L0
    ALOAD 0
    INVOKEVIRTUAL Test.self ()LTest;
    ALOAD 0
   L1
    LINENUMBER 6 L1
    INVOKEVIRTUAL java/lang/Object.toString ()Ljava/lang/String;
   L2
    LINENUMBER 5 L2
    INVOKEVIRTUAL Test.run (Ljava/lang/String;)V
   L3
    LINENUMBER 8 L3
    RETURN

If we don't go back to line 5, the stack trace will show that the run invocation is at line 6, but it is in fact at line 5.

[adpi2]

I think going back to line 1 is correct.

Yes it is correct. It is the one on line 2 that I wish we can remove. When I debug my program I don't care about the invocation of apply$mcVI$s. It makes the debugging experience of for comprehension really heavy and confusing. In a sense, I just want to go back to what we have in Scala 2.

[lrytz]

invocation of apply$mcVI$s

right; the "invocation of apply$mcVI$s" is the invokedynamic that creates the lambda, that shouldn't show up in line numbers, like in Scala 2.

[lrytz]

So as far as I can tell, the PR here is fine in itself, we can merge it in Scala 2. But for Scala 3 we should clean up spurious positions with lambdas first, as this PR makes this issue worse.

[adpi2]

So as far as I can tell, the PR here is fine in itself, we can merge it in Scala 2. But for Scala 3 we should clean up spurious positions with lambdas first, as this PR makes this issue worse.

💯

[som-snytt]

I was interested in returning to the issue but have not yet, and would not stand in the way of anyone with more compelling interests.