8000 Refactor transformStats impls for consistency by retronym · Pull Request #3 · lrytz/scala · GitHub
[go: up one dir, main page]

Skip to content

Refactor transformStats impls for consistency #3

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

Merged
merged 1 commit into from
Feb 3, 2015

Conversation

retronym
Copy link
@retronym retronym commented Feb 3, 2015

Uses the same idiom in Flatten and LambdaLift as is established
in Extender and (recently) Delambdafy. This avoids any possibility
of adding a member to a package twice, as used to happen in SI-9097.

Uses the same idiom in `Flatten` and `LambdaLift` as is established
in `Extender` and (recently) `Delambdafy`. This avoids any possibility
of adding a member to a package twice, as used to happen in SI-9097.
@lrytz lrytz merged commit 486f92c into lrytz:t9097 Feb 3, 2015
lrytz pushed a commit that referenced this pull request Apr 16, 2015
Under `-Ydelambdafy:method`, a public, static accessor method is
created to expose the private method containing the body of the
lambda.

Currently this accessor method has its parameters in the same order
structure as those of the lambda body method.

What is this order? There are three categories of parameters:

  1. lambda parameters
  2. captured parameters (added by lambdalift)
  3. self parameters (added to lambda bodies that end up in trait
     impl classes by mixin, and added unconditionally to the static
     accessor method.)

These are currently emitted in order #3, #1, #2.

Here are examples of the current behaviour:

BEFORE (trait):

```
% cat sandbox/test.scala && scalac-hash v2.11.5 -Ydelambdafy:method sandbox/test.scala && javap -private -classpath . 'Test$class'
trait Member; class Capture; trait LambdaParam
trait Test {
  def member: Member
  def foo {
    val local = new Capture
    (arg: LambdaParam) => "" + arg + member + local
  }
}
Compiled from "test.scala"
public abstract class Test$class {
  public static void foo(Test);
  private static final java.lang.String $anonfun$1(Test, LambdaParam, Capture);
  public static void $init$(Test);
  public static final java.lang.String accessor$1(Test, LambdaParam, Capture);
}
```

BEFORE (class):

```
% cat sandbox/test.scala && scalac-hash v2.11.5 -Ydelambdafy:method sandbox/test.scala && javap -private -classpath . Test
trait Member; class Capture; trait LambdaParam
abstract class Test {
  def member: Member
  def foo {
    val local = new Capture
    (arg: LambdaParam) => "" + arg + member + local
  }
}
Compiled from "test.scala"
public abstract class Test {
  public abstract Member member();
  public void foo();
  private final java.lang.String $anonfun$1(LambdaParam, Capture);
  public Test();
  public static final java.lang.String accessor$1(Test, LambdaParam, Capture);
}
```

Contrasting the class case with Java:

```
% cat sandbox/Test.java && javac -d . sandbox/Test.java && javap -private -classpath . Test
public abstract class Test {
  public static class Member {};
  public static class Capture {};
  public static class LambaParam {};
  public static interface I {
    public abstract Object c(LambaParam arg);
  }
  public abstract Member member();
  public void test() {
    Capture local = new Capture();
    I i1 = (LambaParam arg) -> "" + member() + local;
  }
}

Compiled from "Test.java"
public abstract class Test {
  public Test();
  public abstract Test$Member member();
  public void test();
  private java.lang.Object lambda$test$0(Test$Capture, Test$LambaParam);
}
```

We can see that in Java 8 lambda parameters come after captures. If we
want to use Java's LambdaMetafactory to spin up our anoymous FunctionN
subclasses on the fly, our ordering must change.

I can see three options for change:

  1. Adjust `LambdaLift` to always prepend captured parameters,
     rather than appending them. I think we could leave `Mixin` as
     it is, it already prepends the self parameter. This would result
     a parameter ordering, in terms of the list above: #3, #2, #1.
  2. More conservatively, do this just for methods known to hold
     lambda bodies. This might avoid needlessly breaking code that
     has come to depend on our binary encoding.
  3. Adjust the parameters of the accessor method only. The body
     of this method can permute params before calling the lambda
     body method.

This commit implements option #2.

In also prototyped #1, and found it worked so long as I limited it to
non-constructors, to sidestep the need to make corresponding
changes elsewhere in the compiler to avoid the crasher shown
in the enclosed test case, which was minimized from a bootstrap
failure from an earlier a version of this patch.

We would need to defer option #1 to 2.12 in any case, as some of
these lifted methods are publicied by the optimizer, and we must
leave the signatures alone to comply with MiMa.

I've included a test that shows this in all in action. However, that
is currently disabled, as we don't have a partest category for tests
that require Java 8.
lrytz pushed a commit that referenced this pull request Aug 24, 2015
The log messages intented to chronicle implicit search were
always being filtered out by virtue of the fact that the the tree
passed to `printTyping` was already typed, (e.g. with an implicit
MethodType.)

This commit enabled printing in this case, although it still
filters out trees that are deemed unfit for typer tracing,
such as `()`. In the context of implicit search, this happens
to filter out the noise of:

```
|    |    |    [search #2] start `()`, searching for adaptation to pt=Unit => Foo[Int,Int] (silent: value <local Test> in Test) implicits disabled
|    |    |    [search #3] start `()`, searching for adaptation to pt=(=> Unit) => Foo[Int,Int] (silent: value <local Test> in Test) implicits disabled
|    |    |    \-> <error>
```

... which I think is desirable.

The motivation for this fix was to better display the interaction
between implicit search and type inference. For instance:

```
class Foo[A, B]
class Test {
  implicit val f: Foo[Int, String] = ???
  def t[A, B](a: A)(implicit f: Foo[A, B]) = ???
  t(1)
}
```

````
% scalac -Ytyper-debug sandbox/instantiate.scala
...
|    |-- t(1) BYVALmode-EXPRmode (site: value <local Test> in Test)
|    |    |-- t BYVALmode-EXPRmode-FUNmode-POLYmode (silent: value <local Test> in Test)
|    |    |    [adapt] [A, B](a: A)(implicit f: Foo[A,B])Nothing adapted to [A, B](a: A)(implicit f: Foo[A,B])Nothing
|    |    |    \-> (a: A)(implicit f: Foo[A,B])Nothing
|    |    |-- 1 BYVALmode-EXPRmode-POLYmode (site: value <local Test> in Test)
|    |    |    \-> Int(1)
|    |    solving for (A: ?A, B: ?B)
|    |    solving for (B: ?B)
|    |    [search #1] start `[A, B](a: A)(implicit f: Foo[A,B])Nothing` inferring type B, searching for adaptation to pt=Foo[Int,B] (silent: value <local Test> in Test) implicits disabled
|    |    [search #1] considering f
|    |    [adapt] f adapted to => Foo[Int,String] based on pt Foo[Int,B]
|    |    [search #1] solve tvars=?B, tvars.constr= >: String <: String
|    |    solving for (B: ?B)
|    |    [search #1] success inferred value of type Foo[Int,=?String] is SearchResult(Test.this.f, TreeTypeSubstituter(List(type B),List(String)))
|    |    |-- [A, B](a: A)(implicit f: Foo[A,B])Nothing BYVALmode-EXPRmode (site: value <local Test> in Test)
|    |    |    \-> Nothing
|    |    [adapt] [A, B](a: A)(implicit f: Foo[A,B])Nothing adapted to [A, B](a: A)(implicit f: Foo[A,B])Nothing
|    |    \-> Nothing
```
lrytz pushed a commit that referenced this pull request Oct 20, 2015
must replace old trait accessor symbols by mixed in symbols
in the infos of the mixed in symbols

```
trait T { val a: String ; val b: a.type }
class C extends T {
  // a, b synthesized, but the a in b's type, a.type, refers to the original symbol, not the clone in C
}
```

symbols occurring in types of synthesized members
do not get rebound to other synthesized symbols

package <empty>scala#4 {
  abstract <defaultparam/trait> trait N#7352 extends scala#22.AnyRef#2378 {
    <method> <deferred> <mutable> <accessor> <triedcooking> <sub_synth> def N$_setter_$self_$eq#15011(x$1#15012: <empty>#3.this.N#7352): scala#23.this.Unit#2340;
    <method> <deferred> <mutable> <accessor> <triedcooking> <sub_synth> def N$_setter_$n_$eq#15013(x$1#15014: N#7352.this.self#7442.type): scala#23.this.Unit#2340;
    <method> def /*N*/$init$scala#7441(): scala#23.this.Unit#2340 = {
      ()
    };
    <method> <deferred> <stable> <accessor> <triedcooking> <sub_synth> def self#7442: <empty>#3.this.N#7352;
    N#7352.this.N$_setter_$self_$eq#15011(scala#22.Predef#1729.$qmark$qmark$qmark#6917);
    <method> <deferred> <stable> <accessor> <sub_synth> def n#7443: N#7352.this.self#7442.type;
    N#7352.this.N$_setter_$n_$eq#15013(N#7352.this.self#7442)
  };
  abstract class M#7353 extends scala#22.AnyRef#2378 {
    <method> <triedcooking> def <init>#13465(): <empty>#3.this.M#7353 = {
      M#7353.super.<init>scala#2719();
      ()
    };
    <method> <deferred> <stable> <accessor> <triedcooking> def self#13466: <empty>#3.this.N#7352;
    <method> <deferred> <stable> <accessor> def n#13467: M#7353.this.self#13466.type
  };
  class C#7354 extends M#7353 with <empty>#3.this.N#7352 {
    <method> <stable> <accessor> <triedcooking> def self#15016: <empty>#3.this.N#7352 = C#7354.this.self #15015;
    <triedcooking> private[this] val self #15015: <empty>#3.this.N#7352 = _;
    <method> <stable> <accessor> def n#15018: C#7354.this.self#7442.type = C#7354.this.n #15017;
    <triedcooking> private[this] val n #15017: C#7354.this.self#7442.type = _;
    <method> <mutable> <accessor> <triedcooking> def N$_setter_$self_$eq#15019(x$1#15021: <empty>#3.this.N#7352): scala#23.this.Unit#2340 = C#7354.this.self #15015 = x$1#15021;
    <method> <mutable> <accessor> <triedcooking> def N$_setter_$n_$eq#15022(x$1#15025: C#7354.this.self#7442.type): scala#23.this.Unit#2340 = C#7354.this.n #15017 = x$1#15025;
    <method> def <init>#14997(): <empty>#3.this.C#7354 = {
      C#7354.super.<init>#13465();
      ()
    }
  }
}

[running phase pickler on dependent_rebind.scala]
[running phase refchecks on dependent_rebind.scala]
test/files/trait-defaults/dependent_rebind.scala:16: error: overriding field n#15049 in trait N#7352 of type C#7354.this.self#15016.type;
 value n #15017 has incompatible type;
 found   : => C#7354.this.self#7442.type (with underlying type => C#7354.this.self#7442.type)
 required: => N#7352.this.self#7442.type
class C extends M with N
      ^
lrytz pushed a commit that referenced this pull request Feb 18, 2016
the info-transformed of constructors:
  - traits receive trait-setters for vals
  - classes receive fields & accessors for mixed in traits

Constructors tree transformer
  - makes trees for decls added during above info transform
  - adds mixin super calls to the primary constructor

```
trait OneConcreteVal[T] {
var x = 1 // : T = ???
def foo = x
}

trait OneOtherConcreteVal[T] {
var y: T = ???
}

class C extends OneConcreteVal[Int] with OneOtherConcreteVal[String]
```

we don't have a field -- only a getter -- so,
where will we keep non-getter but-field annotations?

mixin only deals with lazy accessors/vals

do not used referenced to correlate getter/setter
it messes with paramaccessor's usage, and we don't really need it

make sure to clone info's, so we don't share symbols for method args
this manifests itself as an exception in lambdalift, finding proxies

Use NEEDS_TREES for all comms between InfoTransform and tree transform

yep, need SYNTHESIZE_IMPL_IN_SUBCLASS

distinguish accessors that should be mixed into subclass,
and those that simply need to be implemented in tree transform,
after info transform added the decl

commit 4b4932e
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   6 days ago

    do assignment to trait fields in AddInterfaces

    regular class vals get assignments during constructors, as before

impl classes get accessors + assignments through trait setters in addinterfaces
so that constructors acts on them there,
and produced the init method in the required spot (the impl class)

bootstrapped compiler needs new partest

commit baf568d
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   3 weeks ago

    produce identical bytecode for constant trait val getters

    I couldn't bring myself to emit the unused fields that we
    used to emit for constant vals, even though the getters
    immediately return the constant, and thus the field goes unused.

    In the next version, there's no need to synthesize impls
    for these in subclasses -- the getter can be implemented
    in the interface.

commit b9052da
Author: Lukas Rytz <lukas.rytz@gmail.com>
Date:   3 weeks ago

    Fix enclosing method attribute for classes nested in trait fields

    Trait fields are now created as MethodSymbol (no longer TermSymbol).
    This symbol shows up in the `originalOwner` chain of a class declared
    within the field initializer. This promoted the field getter to
    being the enclosing method of the nested class, which it is not
    (the EnclosingMethod attribute is a source-level property).

commit cf845ab
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   3 weeks ago

    don't suppress field for unit-typed vals

    it affects the memory model -- even a write of unit to a field is relevant...

commit 337a9dd
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   4 weeks ago

    unit-typed lazy vals should never receive a field

    this need was unmasked by test/files/run/t7843-jsr223-service.scala,
    which no longer printed the output expected from the `0 to 10 foreach`

    Currently failing tests:

     - test/files/pos/t6780.scala
     - test/files/neg/anytrait.scala
     - test/files/neg/delayed-init-ref.scala
     - test/files/neg/t562.scala
     - test/files/neg/t6276.scala
     - test/files/run/delambdafy_uncurry_byname_inline.scala
     - test/files/run/delambdafy_uncurry_byname_method.scala
     - test/files/run/delambdafy_uncurry_inline.scala
     - test/files/run/delambdafy_uncurry_method.scala
     - test/files/run/inner-obj-auto.scala
     - test/files/run/lazy-traits.scala
     - test/files/run/reify_lazyunit.scala
     - test/files/run/showraw_mods.scala
     - test/files/run/t3670.scala
     - test/files/run/t3980.scala
     - test/files/run/t4047.scala
     - test/files/run/t6622.scala
     - test/files/run/t7406.scala
     - test/files/run/t7843-jsr223-service.scala
     - test/files/jvm/innerClassAttribute
     - test/files/specialized/SI-7343.scala
     - test/files/specialized/constant_lambda.scala
     - test/files/specialized/spec-early.scala
     - test/files/specialized/spec-init.scala
     - test/files/specialized/spec-matrix-new.scala
     - test/files/specialized/spec-matrix-old.scala
     - test/files/presentation/scope-completion-3

commit b1b4e5c
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   4 weeks ago

    wip: lambdalift fix

    test/files/trait-defaults/lambdalift.scala works,
    but still some related tests failing

    (note that we can't use a bootstrapped compiler yet
    due to binary incompatibility in partest)

commit eae7dac
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   4 weeks ago

    update check now that trait vals can be concrete, use names not letters

    note the progression in a concrete trait val now being recognized as such
    ```
    -trait T => true
    -method $init$ => false
    -value z1 => true
    -value z2 => true // z2 is actually concrete!
    ```

commit 6555c74
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   4 weeks ago

    bootstraps again by running info transform once per class...

    not sure how this ever worked, as separate compilation would transform a trait's info
    multiple times, resulting in double defs...

commit 273cb20
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   6 weeks ago

    skip presuper vals in new encoding

commit 728e71e
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   6 weeks ago

    incoherent cyclic references between synthesized members

    must replace old trait accessor symbols by mixed in symbols
    in the infos of the mixed in symbols

    ```
    trait T { val a: String ; val b: a.type }
    class C extends T {
      // a, b synthesized, but the a in b's type, a.type, refers to the original symbol, not the clone in C
    }
    ```

    symbols occurring in types of synthesized members
    do not get rebound to other synthesized symbols

    package <empty>scala#4 {
      abstract <defaultparam/trait> trait N#7352 extends scala#22.AnyRef#2378 {
        <method> <deferred> <mutable> <accessor> <triedcooking> <sub_synth> def N$_setter_$self_$eq#15011(x$1#15012: <empty>#3.this.N#7352): scala#23.this.Unit#2340;
        <method> <deferred> <mutable> <accessor> <triedcooking> <sub_synth> def N$_setter_$n_$eq#15013(x$1#15014: N#7352.this.self#7442.type): scala#23.this.Unit#2340;
        <method> def /*N*/$init$scala#7441(): scala#23.this.Unit#2340 = {
          ()
        };
        <method> <deferred> <stable> <accessor> <triedcooking> <sub_synth> def self#7442: <empty>#3.this.N#7352;
        N#7352.this.N$_setter_$self_$eq#15011(scala#22.Predef#1729.$qmark$qmark$qmark#6917);
        <method> <deferred> <stable> <accessor> <sub_synth> def n#7443: N#7352.this.self#7442.type;
        N#7352.this.N$_setter_$n_$eq#15013(N#7352.this.self#7442)
      };
      abstract class M#7353 extends scala#22.AnyRef#2378 {
        <method> <triedcooking> def <init>#13465(): <empty>#3.this.M#7353 = {
          M#7353.super.<init>scala#2719();
          ()
        };
        <method> <deferred> <stable> <accessor> <triedcooking> def self#13466: <empty>#3.this.N#7352;
        <method> <deferred> <stable> <accessor> def n#13467: M#7353.this.self#13466.type
      };
      class C#7354 extends M#7353 with <empty>#3.this.N#7352 {
        <method> <stable> <accessor> <triedcooking> def self#15016: <empty>#3.this.N#7352 = C#7354.this.self #15015;
        <triedcooking> private[this] val self #15015: <empty>#3.this.N#7352 = _;
        <method> <stable> <accessor> def n#15018: C#7354.this.self#7442.type = C#7354.this.n #15017;
        <triedcooking> private[this] val n #15017: C#7354.this.self#7442.type = _;
        <method> <mutable> <accessor> <triedcooking> def N$_setter_$self_$eq#15019(x$1#15021: <empty>#3.this.N#7352): scala#23.this.Unit#2340 = C#7354.this.self #15015 = x$1#15021;
        <method> <mutable> <accessor> <triedcooking> def N$_setter_$n_$eq#15022(x$1#15025: C#7354.this.self#7442.type): scala#23.this.Unit#2340 = C#7354.this.n #15017 = x$1#15025;
        <method> def <init>#14997(): <empty>#3.this.C#7354 = {
          C#7354.super.<init>#13465();
          ()
        }
      }
    }

    [running phase pickler on dependent_rebind.scala]
    [running phase refchecks on dependent_rebind.scala]
    test/files/trait-defaults/dependent_rebind.scala:16: error: overriding field n#15049 in trait N#7352 of type C#7354.this.self#15016.type;
     value n #15017 has incompatible type;
     found   : => C#7354.this.self#7442.type (with underlying type => C#7354.this.self#7442.type)
     required: => N#7352.this.self#7442.type
    class C extends M with N
          ^

    pos/t9111-inliner-workaround revealed need for:
-  override def transformInfo(sym: Symbol, tp: Type): Type = synthFieldsAndAccessors(tp)
+  override def transformInfo(sym: Symbol, tp: Type): Type = if (!sym.isJavaDefined) synthFieldsAndAccessors(tp) else tp

commit b56ca2f
Author: Adriaan Moors <adriaan.moors@typesafe.com>
Date:   6 weeks ago

    static forwarders & private[this] val in trait

    object Properties extends PropertiesTrait

    trait PropertiesTrait {
      // the protected member is not emitted as a static member of  -- it works when dropping the access modifier
      // somehow, the module class member is considered deferred in BForwardersGen
      protected val propFilename: String = /
    }

    // [log jvm] No forwarder for 'getter propFilename' from Properties to 'module class Properties': false || m.isDeferred == true || false || false

    // the following method is missing compared to scalac
    // public final class Properties {
    //     public static String propFilename() {
    //         return Properties$.MODULE$.propFilename();
    //     }

    trait Chars {
      private[this] val char2uescapeArray = Array[Char]('\', 'u', 0, 0, 0, 0)
    }

    object Chars extends Chars

    // +++ w/reflect/scala/reflect/internal/Chars$.class
    // -  private final [C scala83014char2uescapeArray

    constant fold in forwarder for backwards compat
    constant-typed final val in trait should yield impl method

    bean{setter,getter} delegates to setter/getter (commit 1655d1b)
lrytz pushed a commit that referenced this pull request Oct 21, 2016
Manually tested with:

```
% cat sandbox/test.scala
package p {
  object X { def f(i: Int) = ??? ; def f(s: String) = ??? }
  object Main {
    val res = X.f(3.14)
  }
}

% qscalac  -Ytyper-debug sandbox/test.scala
|-- p EXPRmode-POLYmode-QUALmode (site: package <root>)
|    \-> p.type
|-- object X BYVALmode-EXPRmode (site: package p)
|    |-- super EXPRmode-POLYmode-QUALmode (silent: <init> in X)
|    |    |-- this EXPRmode (silent: <init> in X)
|    |    |    \-> p.X.type
|    |    \-> p.X.type
|    |-- def f BYVALmode-EXPRmode (site: object X)
|    |    |-- $qmark$qmark$qmark EXPRmode (site: method f in X)
|    |    |    \-> Nothing
|    |    |-- Int TYPEmode (site: value i in X)
|    |    |    \-> Int
|    |    |-- Int TYPEmode (site: value i in X)
|    |    |    \-> Int
|    |    \-> [def f] (i: Int)Nothing
|    |-- def f BYVALmode-EXPRmode (site: object X)
|    |    |-- $qmark$qmark$qmark EXPRmode (site: method f in X)
|    |    |    \-> Nothing
|    |    |-- String TYPEmode (site: value s in X)
|    |    |    [adapt] String is now a TypeTree(String)
|    |    |    \-> String
|    |    |-- String TYPEmode (site: value s in X)
|    |    |    [adapt] String is now a TypeTree(String)
|    |    |    \-> String
|    |    \-> [def f] (s: String)Nothing
|    \-> [object X] p.X.type
|-- object Main BYVALmode-EXPRmode (site: package p)
|    |-- X.f(3.14) EXPRmode (site: value res  in Main)
|    |    |-- X.f BYVALmode-EXPRmode-FUNmode-POLYmode (silent: value res  in Main)
|    |    |    |-- X EXPRmode-POLYmode-QUALmode (silent: value res  in Main)
|    |    |    |    \-> p.X.type
|    |    |    \-> (s: String)Nothing <and> (i: Int)Nothing
|    |    |-- 3.14 BYVALmode-EXPRmode (silent: value res  in Main)
|    |    |    \-> Double(3.14)
|    |    [search #1] start `<?>`, searching for adaptation to pt=Double => String (silent: value res  in Main) implicits disabled
|    |    [search #2] start `<?>`, searching for adaptation to pt=(=> Double) => String (silent: value res  in Main) implicits disabled
|    |    [search #3] start `<?>`, searching for adaptation to pt=Double => Int (silent: value res  in Main) implicits disabled
|    |    1 implicits in companion scope
|    |    [search scala#4] start `<?>`, searching for adaptation to pt=(=> Double) => Int (silent: value res  in Main) implicits disabled
|    |    1 implicits in companion scope
|    |    second try: <error> and 3.14
|    |    [search scala#5] start `p.X.type`, searching for adaptation to pt=p.X.type => ?{def f(x$1: ? >: Double(3.14)): ?} (silent: value res  in Main) implicits disabled
|    |    [search scala#6] start `p.X.type`, searching for adaptation to pt=(=> p.X.type) => ?{def f(x$1: ? >: Double(3.14)): ?} (silent: value res  in Main) implicits disabled
sandbox/test.scala:4: error: overloaded method value f with alternatives:
  (s: String)Nothing <and>
  (i: Int)Nothing
 cannot be applied to (Double)
    val res = X.f(3.14)
                ^
```
lrytz pushed a commit that referenced this pull request Mar 28, 2017
The following commit message is a squash of several commit messages.

- This is the 1st commit message:

Add position to stub error messages

Stub errors happen when we've started the initialization of a symbol but
key information of this symbol is missing (the information cannot be
found in any entry of the classpath not sources).

When this error happens, we better have a good error message with a
position to the place where the stub error came from. This commit goes
into this direction by adding a `pos` value to `StubSymbol` and filling
it in in all the use sites (especifically `UnPickler`).

This commit also changes some tests that test stub errors-related
issues. Concretely, `t6440` is using special Partest infrastructure and
doens't pretty print the position, while `t5148` which uses the
conventional infrastructure does. Hence the difference in the changes
for both tests.

- This is the commit message #2:

Add partest infrastructure to test stub errors

`StubErrorMessageTest` is the friend I introduce in this commit to help
state stub errors. The strategy to test them is easy and builds upon
previous concepts: we reuse `StoreReporterDirectTest` and add some
methods that will compile the code and simulate a missing classpath
entry by removing the class files from the class directory (the folder
where Scalac compiles to).

This first iteration allow us to programmatically check that stub errors
are emitted under certain conditions.

- This is the commit message #3:

Improve contents of stub error message

This commit does three things:

* Keep track of completing symbol while unpickling

  First, it removes the previous `symbolOnCompletion` definition to be
  more restrictive/clear and use only positions, since only positions are
  used to report the error (the rest of the information comes from the
  context of the `UnPickler`).

  Second, it adds a new variable called `lazyCompletingSymbol` that is
  responsible for keeping a reference to the symbol that produces the stub
  error. This symbol will usually (always?) come from the classpath
  entries and therefore we don't have its position (that's why we keep
  track of `symbolOnCompletion` as well). This is the one that we have to
  explicitly use in the stub error message, the culprit so to speak.

  Aside from these two changes, this commit modifies the existing tests
  that are affected by the change in the error message, which is more
  precise now, and adds new tests for stub errors that happen in complex
  inner cases and in return type of `MethodType`.

* Check that order of initialization is correct

  With the changes introduced previously to keep track of position of
  symbols coming from source files, we may ask ourselves: is this going to
  work always? What happens if two symbols the initialization of two
  symbols is intermingled and the stub error message gets the wrong
  position?

  This commit adds a test case and modifications to the test
  infrastructure to double check empirically that this does not happen.
  Usually, this interaction in symbol initialization won't happen because
  the `UnPickler` will lazily load all the buckets necessary for a symbol
  to be truly initialized, with the pertinent addresses from which this
  information has to be deserialized. This ensures that this operation is
  atomic and no other symbol initialization can happen in the meantime.

  Even though the previous paragraph is the feeling I got from reading the
  sources, this commit creates a test to double-check it. My attempt to be
  better safe than sorry.

* Improve contents of the stub error message

  This commit modifies the format of the previous stub error message by
  being more precise in its formulation. It follows the structured format:

  ```
  s"""|Symbol '${name.nameKind} ${owner.fullName}.$name' is missing from the classpath.
      |This symbol is required by '${lazyCompletingSymbol.kindString} ${lazyCompletingSymbol.fullName}'.
  ```

  This format has the advantage that is more readable and explicit on
  what's happening. First, we report what is missing. Then, why it was
  required. Hopefully, people working on direct dependencies will find the
  new message friendlier.

Having a good test suite to check the previously added code is
important. This commit checks that stub errors happen in presence of
well-known and widely used Scala features. These include:

* Higher kinded types.
* Type definitions.
* Inheritance and subclasses.
* Typeclasses and implicits.

- This is the commit message scala#4:

Use `lastTreeToTyper` to get better positions

The previous strategy to get the last user-defined position for knowing
what was the root cause (the trigger) of stub errors relied on
instrumenting `def info`.

This instrumentation, while easy to implement, is inefficient since we
register the positions for symbols that are already completed.

However, we cannot do it only for uncompleted symbols (!hasCompleteInfo)
because the positions won't be correct anymore -- definitions using stub
symbols (val b = new B) are for the compiler completed, but their use
throws stub errors. This means that if we initialize symbols between a
definition and its use, we'll use their positions instead of the
position of `b`.

To work around this we use `lastTreeToTyper`. We assume that stub errors
will be thrown by Typer at soonest.

The benefit of this approach is better error messages. The positions
used in them are now as concrete as possible since they point to the
exact tree that **uses** a symbol, instead of the one that **defines**
it. Have a look at `StubErrorComplexInnerClass` for an example.

This commit removes the previous infrastructure and replaces it by the
new one. It also removes the fields positions from the subclasses of
`StubSymbol`s.

- This is the commit message scala#5:

Keep track of completing symbols

Make sure that cycles don't happen by keeping track of all the
symbols that are being completed by `completeInternal`. Stub errors only
need the last completing symbols, but the whole stack of symbols may
be useful to reporting other error like cyclic initialization issues.

I've added this per Jason's suggestion. I've implemented with a list
because `remove` in an array buffer is linear. Array was not an option
because I would need to resize it myself. I think that even though list
is not as efficient memory-wise, it probably doesn't matter since the
stack will usually be small.

- This is the commit message scala#6:

Remove `isPackage` from `newStubSymbol`

Remove `isPackage` since in 2.12.x its value is not used.
lrytz pushed a commit that referenced this pull request Apr 11, 2017
The following commit message is a squash of several commit messages.

- This is the 1st commit message:

Add position to stub error messages

Stub errors happen when we've started the initialization of a symbol but
key information of this symbol is missing (the information cannot be
found in any entry of the classpath not sources).

When this error happens, we better have a good error message with a
position to the place where the stub error came from. This commit goes
into this direction by adding a `pos` value to `StubSymbol` and filling
it in in all the use sites (especifically `UnPickler`).

This commit also changes some tests that test stub errors-related
issues. Concretely, `t6440` is using special Partest infrastructure and
doens't pretty print the position, while `t5148` which uses the
conventional infrastructure does. Hence the difference in the changes
for both tests.

- This is the commit message #2:

Add partest infrastructure to test stub errors

`StubErrorMessageTest` is the friend I introduce in this commit to help
state stub errors. The strategy to test them is easy and builds upon
previous concepts: we reuse `StoreReporterDirectTest` and add some
methods that will compile the code and simulate a missing classpath
entry by removing the class files from the class directory (the folder
where Scalac compiles to).

This first iteration allow us to programmatically check that stub errors
are emitted under certain conditions.

- This is the commit message #3:

Improve contents of stub error message

This commit does three things:

* Keep track of completing symbol while unpickling

  First, it removes the previous `symbolOnCompletion` definition to be
  more restrictive/clear and use only positions, since only positions are
  used to report the error (the rest of the information comes from the
  context of the `UnPickler`).

  Second, it adds a new variable called `lazyCompletingSymbol` that is
  responsible for keeping a reference to the symbol that produces the stub
  error. This symbol will usually (always?) come from the classpath
  entries and therefore we don't have its position (that's why we keep
  track of `symbolOnCompletion` as well). This is the one that we have to
  explicitly use in the stub error message, the culprit so to speak.

  Aside from these two changes, this commit modifies the existing tests
  that are affected by the change in the error message, which is more
  precise now, and adds new tests for stub errors that happen in complex
  inner cases and in return type of `MethodType`.

* Check that order of initialization is correct

  With the changes introduced previously to keep track of position of
  symbols coming from source files, we may ask ourselves: is this going to
  work always? What happens if two symbols the initialization of two
  symbols is intermingled and the stub error message gets the wrong
  position?

  This commit adds a test case and modifications to the test
  infrastructure to double check empirically that this does not happen.
  Usually, this interaction in symbol initialization won't happen because
  the `UnPickler` will lazily load all the buckets necessary for a symbol
  to be truly initialized, with the pertinent addresses from which this
  information has to be deserialized. This ensures that this operation is
  atomic and no other symbol initialization can happen in the meantime.

  Even though the previous paragraph is the feeling I got from reading the
  sources, this commit creates a test to double-check it. My attempt to be
  better safe than sorry.

* Improve contents of the stub error message

  This commit modifies the format of the previous stub error message by
  being more precise in its formulation. It follows the structured format:

  ```
  s"""|Symbol '${name.nameKind} ${owner.fullName}.$name' is missing from the classpath.
      |This symbol is required by '${lazyCompletingSymbol.kindString} ${lazyCompletingSymbol.fullName}'.
  ```

  This format has the advantage that is more readable and explicit on
  what's happening. First, we report what is missing. Then, why it was
  required. Hopefully, people working on direct dependencies will find the
  new message friendlier.

Having a good test suite to check the previously added code is
important. This commit checks that stub errors happen in presence of
well-known and widely used Scala features. These include:

* Higher kinded types.
* Type definitions.
* Inheritance and subclasses.
* Typeclasses and implicits.

- This is the commit message scala#4:

Use `lastTreeToTyper` to get better positions

The previous strategy to get the last user-defined position for knowing
what was the root cause (the trigger) of stub errors relied on
instrumenting `def info`.

This instrumentation, while easy to implement, is inefficient since we
register the positions for symbols that are already completed.

However, we cannot do it only for uncompleted symbols (!hasCompleteInfo)
because the positions won't be correct anymore -- definitions using stub
symbols (val b = new B) are for the compiler completed, but their use
throws stub errors. This means that if we initialize symbols between a
definition and its use, we'll use their positions instead of the
position of `b`.

To work around this we use `lastTreeToTyper`. We assume that stub errors
will be thrown by Typer at soonest.

The benefit of this approach is better error messages. The positions
used in them are now as concrete as possible since they point to the
exact tree that **uses** a symbol, instead of the one that **defines**
it. Have a look at `StubErrorComplexInnerClass` for an example.

This commit removes the previous infrastructure and replaces it by the
new one. It also removes the fields positions from the subclasses of
`StubSymbol`s.

- This is the commit message scala#5:

Keep track of completing symbols

Make sure that cycles don't happen by keeping track of all the
symbols that are being completed by `completeInternal`. Stub errors only
need the last completing symbols, but the whole stack of symbols may
be useful to reporting other error like cyclic initialization issues.

I've added this per Jason's suggestion. I've implemented with a list
because `remove` in an array buffer is linear. Array was not an option
because I would need to resize it myself. I think that even though list
is not as efficient memory-wise, it probably doesn't matter since the
stack will usually be small.

- This is the commit message scala#6:

Remove `isPackage` from `newStubSymbol`

Remove `isPackage` since in 2.12.x its value is not used.
lrytz pushed a commit that referenced this pull request Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0