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

Inferred ConstantType type arguments are not properly deconsted resulting in bogus constant inlining #10788

Closed
milessabin opened this issue Mar 20, 2018 · 4 comments · Fixed by scala/scala#7620
Assignees
Milestone

Comments

@milessabin
Copy link

milessabin commented Mar 20, 2018

This appears to go way back ... it's present in 2.13.x, 2.12, 2.11 and 2.10 at least. Whilst literal types in 2.13.x provide more opportunities for manifesting it, the underlying problem is orthogonal to literal types. Related to #8564 (the fix reported there as being in the literal types PR clearly wasn't thorough enough) and possibly also to #10340.

No output is produced when the following is run,

object Test {
  class Box[T](t: T) {
    def foo: T = {
      println("effect")
      t
    }
  }

  object Box {
    def apply(x: String): Box[x.type] = new Box[x.type](x)
  }

  def main(args: Array[String]): Unit = {
    val bar = Box("foo").foo
  }
}

If we look at the trees following typer we can see why,

object Test extends scala.AnyRef {
  class Box[T] extends scala.AnyRef {
    <paramaccessor> private[this] val t: T = _;
    def foo: T = {
      scala.Predef.println("effect");
      Box.this.t
    }
  };
  object Box extends scala.AnyRef {
    def apply(x: String): Test.Box[x.type] = new Test.Box[x.type](x)
  };
  def main(args: Array[String]): Unit = {
    val bar: String = "foo";
    ()
  }
}

The definition of bar,

val bar = Box("foo").foo

has been transformed to,

val bar: String = "foo";

The cause of the problem is that the Box value is created with its type parameter T instantiated as x.type which is computed as the (internal) ConstantType(Literal("foo")). This results in the subsequent application of foo also being seen as having a constant type and so being eligible to be inlined as the constant value, eliding the object creation, method call and effect.

@milessabin milessabin added this to the 2.13.0-M4 milestone Mar 20, 2018
@milessabin milessabin self-assigned this Mar 20, 2018
@dwijnand

This comment has been minimized.

@milessabin
Copy link
Author

@dwijnand Yup ... see above in the description ;-)

@milessabin
Copy link
Author

PR on its way, BTW ...

milessabin added a commit to milessabin/scala that referenced this issue Mar 21, 2018
Prior to this commit deconst only converted a top level
FoldableConstantType to a LiteralType. This left FoldableConstantTypes
possibly embedded somewhere in a TypeRef or suchlike where it might
reemerge unexpectedly at a later point. This could result in result
bogus inlining and the elimination of expected side-effects. For
instance, no output is produced when the following is run,

  class Box[T](t: T) {
    def foo: T = {
      println("effect")
      t
    }
  }

  object Box {
    def apply(x: String): Box[x.type] = new Box[x.type](x)
  }

  val bar = Box("foo").foo

The cause of the problem is that the Box value is created with its type
parameter T instantiated as x.type which is computed as the
ConstantType(Literal("foo")). This results in the subsequent application
of foo also being seen as having a constant type and so being eligible
to be inlined as the constant value, eliding the object creation, method
call and effect. So the definition of bar,

  val bar = Box("foo").foo

was transformed to,

  val bar: String = "foo";

Note that although the earlier mention of LiteralType suggests that this
is related to the literal types extension, this problem is present in
compiler versions going back to 2.10.x at least.

The fix is for deconst to recurse through the type eliminating all
FoldableConstantType components. This fixes scala/bug#10788 and almost
incidentally fixes scala/bug#10768. In,

  object Test {
    type Id[T] = T
    def foo(x: Int): Id[x.type] = x

    lazy val result = foo(1)
  }

the inferred FoldableConstantType for T is hidden in the Id type
constructor and so is not deconsted. It then reemerges during Uncurry
where Id[Int(1)] is dealiased to Int(1). Subsequently during Fields this
constant type interferes with synthesis of the various fields and
accessors associated with the lazy val.

This fix also changes the output of run/macro-reify-unreify. I've been
able to convince myself that the previous output was incorrect due to
the typer seeing Expr[String("world")](...).splice as having the
constant type String("world") and hence inlining the literal String
without invoking any of the splicing machinery, ie. it's an actual
instance of the issue this commit fixes.
@milessabin
Copy link
Author

Fixed in scala/scala#6452.

@lrytz lrytz added the has PR label Apr 19, 2018
@SethTisue SethTisue modified the milestones: 2.13.0-M4, 2.13.0-M5 May 14, 2018
milessabin added a commit to milessabin/scala that referenced this issue Aug 6, 2018
Prior to this commit deconst only converted a top level
FoldableConstantType to a LiteralType. This left FoldableConstantTypes
possibly embedded somewhere in a TypeRef or suchlike where it might
reemerge unexpectedly at a later point. This could result in result
bogus inlining and the elimination of expected side-effects. For
instance, no output is produced when the following is run,

  class Box[T](t: T) {
    def foo: T = {
      println("effect")
      t
    }
  }

  object Box {
    def apply(x: String): Box[x.type] = new Box[x.type](x)
  }

  val bar = Box("foo").foo

The cause of the problem is that the Box value is created with its type
parameter T instantiated as x.type which is computed as the
ConstantType(Literal("foo")). This results in the subsequent application
of foo also being seen as having a constant type and so being eligible
to be inlined as the constant value, eliding the object creation, method
call and effect. So the definition of bar,

  val bar = Box("foo").foo

was transformed to,

  val bar: String = "foo";

Note that although the earlier mention of LiteralType suggests that this
is related to the literal types extension, this problem is present in
compiler versions going back to 2.10.x at least.

The fix is for deconst to recurse through the type eliminating all
FoldableConstantType components. This fixes scala/bug#10788 and almost
incidentally fixes scala/bug#10768. In,

  object Test {
    type Id[T] = T
    def foo(x: Int): Id[x.type] = x

    lazy val result = foo(1)
  }

the inferred FoldableConstantType for T is hidden in the Id type
constructor and so is not deconsted. It then reemerges during Uncurry
where Id[Int(1)] is dealiased to Int(1). Subsequently during Fields this
constant type interferes with synthesis of the various fields and
accessors associated with the lazy val.

This fix also changes the output of run/macro-reify-unreify. I've been
able to convince myself that the previous output was incorrect due to
the typer seeing Expr[String("world")](...).splice as having the
constant type String("world") and hence inlining the literal String
without invoking any of the splicing machinery, ie. it's an actual
instance of the issue this commit fixes.
@SethTisue SethTisue modified the milestones: 2.13.0-M5, 2.13.0-RC1 Aug 12, 2018
adriaanm added a commit to adriaanm/scala that referenced this issue Jan 9, 2019
When a constant value is returned from a method --
even one without argument list -- we must deconst,
as we know nothing about the method's purity, and
thus cannot constant fold through that application.

Fix scala/bug#10788
adriaanm added a commit to adriaanm/scala that referenced this issue Jan 9, 2019
When a constant value is returned from a method --
even one without argument list -- we must deconst,
as we know nothing about the method's purity, and
thus cannot constant fold through that application.

Fix scala/bug#10788
@adriaanm adriaanm self-assigned this Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants