The Wayback Machine - https://web.archive.org/web/20200908052416/https://github.com/scala-js/scala-js/issues/3918
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

Mixed-in field in class inside lazy val rhs is erroneously immutable in the IR -> IR checking error #3918

Open
sjrd opened this issue Jan 8, 2020 · 10 comments
Labels

Comments

@sjrd
Copy link
Member

@sjrd sjrd commented Jan 8, 2020

Reproduction

Reproducible in 0.6.x and 1.x, in 2.12 and 2.13. In 2.11, things work correctly. Full repro:

package helloworld

class Apply {
  def testTraitMixinInLocalLazyVal(): Unit = {
    trait TraitMixedInLocalLazyVal {
      val foo = "foobar"
    }

    lazy val localLazyVal = {
      class ClassExtendsTraitInLocalLazyVal extends TraitMixedInLocalLazyVal
      val obj = new ClassExtendsTraitInLocalLazyVal
      assert(obj.foo == "foobar")
    }
    localLazyVal
  }
}

object HelloWorld {
  def main(args: Array[String]): Unit = {
    new Apply().testTraitMixinInLocalLazyVal()
  }
}

In 2.12 and 2.13, trying to link the above program will cause the following errors:

[info] Fast optimizing .../helloworld-fastopt.js
[error] .../HelloWorld.scala(10:13:Assign): Assignment to immutable field foo.
[error] There were 1 IR checking errors.
[error] (helloworld2_12 / Compile / fastOptJS) There were 1 IR checking errors.

Workaround

A workaround is given down below at #3918 (comment).

Analysis

The IR of the inner class is:

class helloworld.Apply$ClassExtendsTraitInLocalLazyVal$1 extends java.lang.Object implements helloworld.Apply$TraitMixedInLocalLazyVal$1 {
  val foo: java.lang.String
  val $outer: helloworld.Apply
  def foo;Ljava.lang.String(): java.lang.String = {
    this.helloworld.Apply$ClassExtendsTraitInLocalLazyVal$1::foo
  }
  def helloworld$Apply$TraitMixedInLocalLazyVal$_setter_$foo_$eq;Ljava.lang.String;V{helloworld$Apply$TraitMixedInLocalLazyVal$_setter_$foo_=}(x$1: java.lang.String) {
    this.helloworld.Apply$ClassExtendsTraitInLocalLazyVal$1::foo = x$1
  }
  def helloworld$Apply$TraitMixedInLocalLazyVal$$$outer;Lhelloworld.Apply{$outer}(): helloworld.Apply = {
    this.helloworld.Apply$ClassExtendsTraitInLocalLazyVal$1::$outer
  }
  constructor def <init>;Lhelloworld.Apply;V(outer{$outer}: helloworld.Apply) {
    if ((outer === null)) {
      throw mod:scala.scalajs.runtime.package$.unwrapJavaScriptException;Ljava.lang.Throwable;Ljava.lang.Object(null)
    } else {
      this.helloworld.Apply$ClassExtendsTraitInLocalLazyVal$1::$outer = outer
    };
    this.java.lang.Object::<init>;V();
    this.helloworld.Apply$TraitMixedInLocalLazyVal$1::$init$;V()
  }
}

in which the field foo is immutable. This is incorrect because it is assigned to in the trait setter def helloworld$Apply$TraitMixedInLocalLazyVal$_setter_$foo_$eq;Ljava.lang.String;V. One problem is that, for scalac, that field is not marked as MUTABLE. This has always been an issue for val fields in traits, so we have logic that patches up fields that are "unexpectedly" mutated. They are recorded as such in case Assign(...) => at

if (!ctorAssignment && !suspectFieldMutable(sym))
unexpectedMutatedFields += sym

and patched up in genClassFields at
val mutable = {
static || // static fields must always be mutable
suspectFieldMutable(f) || unexpectedMutatedFields.contains(f)
}

However, when all the following conditions are met:

  • Scala 2.12+
  • The class that does the mixin is in the rhs of a lazy val
  • That lazy val is inside a def (so it's a local lazy val, not a field)

then scalac ends up creating 2 different Symbols for that field. One is used in the Assign node, and the other is listed a member of the class, and hence used by genClassFields.

This causes our patching up to fail to process that field, since it is not the same symbol (but they have the same name/owner/etc.).

@sjrd sjrd added bug upstream labels Jan 8, 2020
@sjrd
Copy link
Member Author

@sjrd sjrd commented Jan 8, 2020

I discovered this while trying to upgrade https://github.com/lihaoyi/sourcecode to Scala.js 1.0.0-RC2. There are two tests (Apply.scala and Implicits.scala) that exhibit this pattern and hence fail. In 0.6.x, this did not show up because we do not enable the IR checker by default.

I do not think this is a blocker for 1.0.0.

  • It does not reveal any issue with the structure of the IR; only with the IR produced by the compiler for some patterns of code.
  • It does not endanger the ecosystem, because problematic code will fail to pass unit tests and hence should not end up being published on Maven.
@sjrd
Copy link
Member Author

@sjrd sjrd commented Jan 8, 2020

An ideal fix would be to fix scalac upstream not to create two different symbols.

A workaround in Scala.js would be to use the Names of the symbols in unexpectedMutatedFields instead of the symbols themselves.

@sjrd
Copy link
Member Author

@sjrd sjrd commented Jan 22, 2020

Workaround

As a workaround, put the rhs of the lazy val in a def just before it, then call that def as the new rhs of the lazy val. This can always be done.

For example, for the reproduction above, replace with:

class Apply {
  def testTraitMixinInLocalLazyVal(): Unit = {
    trait TraitMixedInLocalLazyVal {
      val foo = "foobar"
    }

    def localLazyValWorkaroundScalaJSIssue3918 = {
      class ClassExtendsTraitInLocalLazyVal extends TraitMixedInLocalLazyVal
      val obj = new ClassExtendsTraitInLocalLazyVal
      assert(obj.foo == "foobar")
    }
    lazy val localLazyVal = localLazyValWorkaroundScalaJSIssue3918

    localLazyVal
  }
}
@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Feb 12, 2020

Is this reported as a bug upstream already?

@sjrd
Copy link
Member Author

@sjrd sjrd commented Feb 12, 2020

No, not yet.

@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Feb 26, 2020

/cc @lrytz

@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Feb 26, 2020

This is -Xprint:mixin -uniqid with 2.12.10.

[[syntax trees at end of                     mixin]] // HelloWorld.scala
package helloworld#14 {
  class Apply#5004 extends Object#419 {
    def testTraitMixinInLocalLazyVal#5011(): Unit#2093 = {
      lazy <artifact> val localLazyVal$lzy#9299: scala#7.runtime#2160.LazyUnit#2763 = new scala#7.runtime#2160.LazyUnit#2763();
      Apply#5004.this.localLazyVal$1#6485(localLazyVal$lzy#9299)
    };
    final <artifact> private[this] def localLazyVal$lzycompute$1#9306(localLazyVal$lzy$1#12994: scala#7.runtime#2160.LazyUnit#2763): Unit#2093 = localLazyVal$lzy$1#12994.synchronized#4047[Unit#2093](if (localLazyVal$lzy$1#12994.initialized#9302())
      ()
    else
      {
        {
          val obj#6490: helloworld#15.Apply$ClassExtendsTraitInLocalLazyVal$1#6489 = new helloworld#15.Apply$ClassExtendsTraitInLocalLazyVal$1#6489(Apply#5004.this);
          scala#6.Predef#1566.assert#4631(obj#6490.foo#6487().==#4042("foobar"))
        };
        localLazyVal$lzy$1#12994.initialize#9303()
      });
    final lazy private[this] def localLazyVal$1#6485(localLazyVal$lzy$1#12995: scala#7.runtime#2160.LazyUnit#2763): Unit#2093 = if (localLazyVal$lzy$1#12995.initialized#9302())
      ()
    else
      Apply#5004.this.localLazyVal$lzycompute$1#9306(localLazyVal$lzy$1#12995);
    def <init>#5010(): helloworld#15.Apply#5004 = {
      Apply#5004.super.<init>#892();
      ()
    }
  };
  object HelloWorld#5006 extends Object#419 {
    def main#6527(args#6529: Array#1076[String#698]): Unit#2093 = new helloworld#15.Apply#5004().testTraitMixinInLocalLazyVal#5011();
    def <init>#6526(): helloworld#15.HelloWorld#5006.type = {
      HelloWorld#5006.super.<init>#892();
      ()
    }
  };
  abstract trait Apply$TraitMixedInLocalLazyVal$1#6484 extends Object#419 {
    <accessor> <sub_synth> protected[this] def helloworld$Apply$TraitMixedInLocalLazyVal$_setter_$foo_=#9292(x$1#9293: String#698): Unit#2093;
    <stable> <accessor> <sub_synth> def foo#6487(): String#698;
    <synthetic> <stable> <artifact> def $outer#12955(): helloworld#15.Apply#5004;
    def /*Apply$TraitMixedInLocalLazyVal$1*/$init$#6486(): Unit#2093 = {
      Apply$TraitMixedInLocalLazyVal$1#6484.this.helloworld$Apply$TraitMixedInLocalLazyVal$_setter_$foo_=#9292(("foobar": String#698));
      ()
    }
  };
  class Apply$ClassExtendsTraitInLocalLazyVal$1#6489 extends Object#419 with helloworld#15.Apply$TraitMixedInLocalLazyVal$1#6484 {
    override <stable> <accessor> def foo#9295(): String#698 = (Apply$ClassExtendsTraitInLocalLazyVal$1#6489.this.foo#9294: String#698);
    private[this] val foo#9294: String#698 = _;
    override <accessor> protected[this] def helloworld$Apply$TraitMixedInLocalLazyVal$_setter_$foo_=#9296(x$1#9298: String#698): Unit#2093 = Apply$ClassExtendsTraitInLocalLazyVal$1#6489.this.foo#9294 = (x$1#9298: String#698);
    <synthetic> <paramaccessor> <artifact> private[this] val $outer#12957: helloworld#15.Apply#5004 = _;
    <synthetic> <stable> <artifact> def $outer#12958(): helloworld#15.Apply#5004 = Apply$ClassExtendsTraitInLocalLazyVal$1#6489.this.$outer#12957;
    def <init>#6491($outer#12959: helloworld#15.Apply#5004): helloworld#15.Apply$ClassExtendsTraitInLocalLazyVal$1#6489 = {
      if ($outer#12959.eq#4038(null))
        throw null
      else
        Apply$ClassExtendsTraitInLocalLazyVal$1#6489.this.$outer#12957 = $outer#12959;
      Apply$ClassExtendsTraitInLocalLazyVal$1#6489.super.<init>#892();
      Apply$ClassExtendsTraitInLocalLazyVal$1#6489.super./*Apply$TraitMixedInLocalLazyVal$1*/$init$#6486();
      ()
    }
  }
}
@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Feb 26, 2020

The relevant parts are (field def and field setter).

private[this] val foo#9294: String#698 = _;
override <accessor> protected[this] def helloworld$Apply$TraitMixedInLocalLazyVal$_setter_$foo_=#9296(x$1#9298: String#698): Unit#2093 =
    Apply$ClassExtendsTraitInLocalLazyVal$1#6489.this.foo#9294 = (x$1#9298: String#698);
@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Feb 26, 2020

printing symbol and ID when we assign / define yields this:

assign: value foo #9294
assign: value $outer #12957
define: value foo #9320
define: value $outer #12957

We conclude that the symbol in classSym.info.decl is not the same than the one in the trees (which all use a consistent symbol).

@lrytz
Copy link
Contributor

@lrytz lrytz commented Feb 27, 2020

So it seems the tree looks correct (as posted above by @gzm0). But looking at the symbol of class K$ClassExtendsTraitInLocalLazyVal$1 at the cleanup phase, sym.info.decls has

constructor K$ClassExtendsTraitInLocalLazyVal$1 - (arg$outer: K): K$ClassExtendsTraitInLocalLazyVal$1
value foo - (): String
value foo - String
variable foo - (x$1: String): Unit
value $outer - K
method $outer - (): K

So there's a variable foo (looks good), a getter foo (looks also good), and onther value foo which i don't know what it is or where it comes from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.
Morty Proxy This is a proxified and sanitized view of the page, visit original site.