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

Smart Step Into #122

Closed
adpi2 opened this issue Oct 6, 2021 · 0 comments · Fixed by #253
Closed

Smart Step Into #122

adpi2 opened this issue Oct 6, 2021 · 0 comments · Fixed by #253
Labels
core enhancement New feature or request

Comments

@adpi2
Copy link
Member

adpi2 commented Oct 6, 2021

Introduction

The scala-debug-adapter is the debugger of Scala programs in Metals.

Problem

Some Scala constructs generate methods in the bytecode that do not contain any part of the source code. For example we have the getter methods, the mixin forwarders, the bridges.

When the compiler generates those methods, it fills their source lines tables with the line of the class or field definition. That's because there is nothing else to go since there is no corresponding source code of the method.

As a consequence the JVM debugger steps into those generated methods showing the class or field definition.
This is:

  • confusing: the user does not need to know about those generated methods
  • useless: the user does not need to step into those methods since there is none of their code inside

Live example (mixin forwarder):
step-into-mixin-forwarder

Instead, the user expects those intermediate steps to be skipped.

Identified patterns

In these examples, when the user steps into 1=> the debugger goes to 2=>.

Getter

class Foo {
2=> val value = "foo"
}

object Main3 {
  def main(args: Array[String]): Unit = {
    val foo = new Foo
1=> println(foo.value) // step into
  }
}

The getter method in the class file of Foo is:

public java.lang.String value();
    descriptor: ()Ljava/lang/String;
    flags: ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: getfield      #13                 // Field value:Ljava/lang/String;
         4: areturn
      LineNumberTable:
        line 4: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       5     0  this   Lexample/Foo;

It is not flagged as ACC_SYNTHETIC . Its line number table contains line 4 where the field foo is defined.

How to detect that a method is a getter?

  • the class has a field with the same name
  • the bytecode just return the value of that field

Setter

class Foo {
2=> var value = "foo"
}

object Main3 {
  def main(args: Array[String]): Unit = {
    val foo = new Foo
1=> foo.value = "bar" // step into
  }
}

The setter method in the class file of Foo is:

  public void value_$eq(java.lang.String);
    descriptor: (Ljava/lang/String;)V
    flags: ACC_PUBLIC
    Code:
      stack=2, locals=2, args_size=2
         0: aload_0
         1: aload_1
         2: putfield      #13                 // Field value:Ljava/lang/String;
         5: return
      LineNumberTable:
        line 4: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       6     0  this   Lexample/Foo;
            0       6     1   x$1   Ljava/lang/String;
    MethodParameters:
      Name                           Flags
      x$1                            final

It is not flagged as ACC_SYNTHETIC . Its line number table contains line 4 where the field foo is defined.

How to detect that a method is a setter?

  • the name of the method is of the form <field>_$eq where <field> is a field of the class
  • the bytecode just set the value of that field

Mixin forwarder

trait A {
  def foo: String = "foo"
}

2=> class B () extends A

object Main2 {
  def main(args: Array[String]): Unit = {
    val b = new B
1=> println(b.foo)
  }
}

The mixin forwarder in the class file of B is:

  public java.lang.String foo();
    descriptor: ()Ljava/lang/String;
    flags: ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokestatic  #16                 // InterfaceMethod example/A.foo$:(Lexample/A;)Ljava/lang/String;
         4: areturn
      LineNumberTable:
        line 7: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       5     0  this   Lexample/B;

Again it is not flagged as ACC_SYNTHETIC. Its line table contains line 7 which is the definition of class B.

How to detect that a method is a mixin forwarder?

  • It is public (maybe protected?)
  • It's bytecode just calls a static method <methodname>$ in a interface x that have the same signature but takes an instance of x as first parameter.

Bridge method

class A {
  def foo: Object = "object"
}

2=> class D extends A {
  override def foo: String = "bar"
}

object Main4 {
  def main(args: Array[String]): Unit = {
    val a: A = new D
1=> println(a.foo)
  }
}

The bridge in the class file of D is:

  public java.lang.Object foo();
    descriptor: ()Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokevirtual #17                 // Method foo:()Ljava/lang/String;
         4: areturn
      LineNumberTable:
        line 7: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       5     0  this   Lexample/D;

It is flagged ACC_BRIDGE. Its line number table contains line 7 which is the definition of class D.

In this case we can skip the bridge method on the fact that is flagged ACC_BRIDGE. That should be fine because all ACC_BRIGE methods should be skipped, even the java ones.

Implementation

Solution 1: Create custom StepFilters

Currently the StepFilters in https://github.com/scalacenter/java-debug contains:

        public boolean skipSynthetics;
        public boolean skipStaticInitializers;
        public boolean skipConstructors;

and it inherits from ClassFilters

        public String[] allowClasses = new String[0];
        public String[] skipClasses = new String[0];

This is not what we need because we do not want:

  • to skip all synthetics: lambdas must not be skipped even if they are synthetics
  • to skip static initializers or constructors
  • to skip or allow base on the class name

But we can adapt this class with something more customizable:

public static class ClassFilters {
  ...
   public CustomClassFilter[] customClassFilters
}

interface CustomClassFilter {
  def shouldSkip(Location location)
}

Then create one custom filter for each identified patterns (in https://github.com/scalacenter/scala-debug-adapter).

The heuristic to identify such patterns can be taken from or inspired by the intellij-scala smartStepInto classes.

Solution 2: The compiler does not set line numbers in those methods

We change the compiler so that it does not set line numbers in those methods.
We should check, in the JVM specification, that it is valid to have a method with an empty source line table.
Also we should test that the JVM can skip those methods and step into the next method call.
And we should sync with JetBrains team to not break their debugger.

Solution 3: The compiler set some attribute in the class file.

It is easier for the compiler to know which method should be skipped and which one should not be skipped.
So if the Solution 2 is not viable, the compiler can still add some attribute in the class file that stores a list of methods that should be skipped.

Expectations

  • Even if solutions 2 and 3 are simpler, we expect solution 1 to be implemented so that we can support current versions of the Scala compiler. Solution 2 and 3 are not expected.
  • Stepping into and out should behave consistently.
  • Most of the described patterns should be implemented successfully.
  • Scala 2.12, 2.13 and 3 should be supported.
  • Each implemented patterns should be tested in the latest versions of Scala 2.12, 2.13 and 3.

Going further

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant