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

Deconst thoroughly #6452

Closed
wants to merge 3 commits into from
Closed

Conversation

milessabin
Copy link
Contributor

@milessabin milessabin commented 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 they 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 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. Whilst I'm confident that the previous behaviour was broken, I'm not completely sure than the new behaviour is correct ... I'd appreciate review by @xeno-by to make sure that the reify/unreify stuff is now doing what it's supposed to be doing.

def apply(tp: Type): Type = tp.deconst
}
mapOver(deconst0)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the deconsting logic be turned into a TypeMap rather than a virtual call? This allocates at least one object per deconst call. Even having a single DeconstMap with this logic could arguably be nicer. (A downside being that you'd need to click through to see what it's doing.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks a bit wrong, and looks like it has exponential complexity.

I think the intent is:

object deconstDeep extends TypeMap {
   def apply(tp: Type) = tp.deconst.mapOver(this)
}

That would leave the shallow deconst in place and introduce a new operation. You'd need to decide at each call to deconst which was appropriate.

But I don't think this is the right approach, and would prefer to prevent the constant type from getting into the args of a type ref in the first place.

This is the smallest change to fix provided test case:

2.13.x...retronym:topic/deconst-type-arg

We should review AsSeenFromMap to see if there are other cases to test (type members rather than params, maybe?), and find the suitable place for the shallow deconst.

Copy link
Contributor Author

@milessabin milessabin Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch ... yes, the exponential bit is a tad embarrassing. I was trying to preserve the existing internal method while reusing TypeMaps traversal logic. I'll move to an external TypeMap and replace .deconst calls case by case.

@retronym your fix doesn't go quite far enough. Although it correctly handles cases of the first form (including the reify/unreify case), examples of the second form such as test/files/pos/t10768.scala go back to being crashers. Also note that constant types can be smuggled in in other ways than via a TypeRef, eg. via a refinement, and that these things can be arbitrarily nested. I think that deep deconsting is necessary, at least at some of the sites where currently only shallow deconsting is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@retronym here's a case which escapes your fix,

object Test {
  abstract class Box {
    type T
    val t: T
    def foo: T = {
      println("effect")
      t
    }
  }

  object Box {
    def apply(x: String): Box { type T = x.type } = new Box {
      type T = x.type
      val t = x
    }
  }

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@retronym I missed your comment about AsSeenFromMap earlier. It strikes me that the deconst logic is pretty much orthogonal to the as seen from logic ... we're not relativising types based on point of view, we're making a decision about constant folding and inlining. I appreciate that the kind of traversal that AsSeenFromMap is doing might pick up the problematic inlining cases, but if it does that seems more or less accidental.

I also noticed that there's no handling of refinements AsSeenFromMap ... I'm guessing that that's deliberate. The new test I added shows that we do need to recurse through refinements to deconst properly though. I don't see how to (easily) reconcile that difference.

Copy link
Member

@retronym retronym Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to push a bit harder on the shallow approach. Here's my next attempt, that handles run/t10788.scala and your test above: https://github.com/scala/scala/compare/2.13.x...retronym:topic/deconst-type-arg?expand=1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tweaked that to handle poly types and nested method types.

Copy link
Member

@retronym retronym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Universally deconsting deeply doesn't smell right to me.

@milessabin
Copy link
Contributor Author

milessabin commented Mar 22, 2018

I've extracted out the recursive deconsting logic to a separate TypeMap and applied it at the least number of places to make all the tests pass. I've also added a refinement-based variant of the main test case, which evades @retronym's suggested fix. I've included his change however, because with the reduced deconsting it's now needed to ensure that the splicing logic is applied in the reify/unreify test.

I worry that unless we do this across the board we'll end up playing whack-a-mole with bogus inlining.

@milessabin
Copy link
Contributor Author

Damn you JavaUniverseForce ...

@milessabin
Copy link
Contributor Author

@retronym you convinced me 😄

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an even simpler test variation that is also fixed by this change.

class C {
  println("effect")
  final val const = 42
}

object Test {
  def main(args: Array[String]) {
    val x = new C().const
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I wonder if rather than fully deconsting we should instead convert ConstantType-s to LiteralType-s.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what deconst on a FoldableConstantType does.

}
tree.modifyType(deconstResult)
}
(tree, None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also been looking at this line of code in the context of figuring out why final val x = ~0 isn't constant folding as expected. I think there is a missing call to constfold here. It seems like stabilize, the new code here, and constfold are all testing similar properties e.g. sym.isStable. I think there is opportunity to consolidate some of this, but I'm still trying to get a decent mental map of this terrain, so I don't have a concrete suggestion yet.

@retronym
Copy link
Member

Select nodes an also appear from typedIdent (based on a selection from this or from an import). So sometimes a localized fix in typedSelect, as I've suggested here, also needs some corresponding change in typedIdent.

So we still elide the side effect with:

scala> class HasConst { final val c = 42 }; lazy val x : HasConst = ???; import x._; c
defined class HasConst
x: HasConst = <lazy>
import x._
res0: Int = 42

So maybe stabilize (called from both typedSelect and typedIdent should be in charge of this deconsting.

I think @adriaanm has a better idea of the design of these areas, we'll need his input.

@adriaanm
Copy link
Contributor

adriaanm commented May 9, 2018

As discussed, we'll review in Berlin and give ourselves until M5 to refine.

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.
Also added a refinement based variant of t10788.
@milessabin
Copy link
Contributor Author

Rebased.

@adriaanm we could do with some input on this ... it'd be good to get this in for M5 if possible.

@adriaanm
Copy link
Contributor

adriaanm commented Aug 6, 2018

I'll try, but a bit behind on reviewing overall.

@adriaanm
Copy link
Contributor

I spent most of today trying to understand more of this. Looks like there are a few issues with this. Quick brain dump of my current understanding...

Type aliases and singleton types hiding types that should be deconsted.
(Programs should type check in the same way if we replace tp.deconst with tp.dealiasWiden.deconst. We can't just do that, because you can't dealias / widen in the same spots that you need to deconst).

Intuitively, deconst is needed on boundaries where we don't know/can't assume expressions are pure. You can get a good sense by grepping for the actual call sites for deconst. The branches of an if (though I think you don't need to do that when the condition itself is a foldable constant). When passing arguments. The result type of a function. By analogy, I think, when selecting a method, after overload resolution and type inference, we should deconst the result type of the method.

There's too much going on here to realistically fix properly in the next few days, so we'll have to postpone for RC1. First, we should turn my brain dump in something that's actually fully thought through and correct.

@adriaanm adriaanm removed this from the 2.13.0-M5 milestone Aug 10, 2018
@adriaanm adriaanm added this to the 2.13.0-RC1 milestone Aug 10, 2018
@adriaanm
Copy link
Contributor

Also, it looks like deconstMap could be defined more simply these days:

object deconstMap extends TypeMap { def apply(tp: Type): Type = tp.mapOver(this).deconst }

@SethTisue
Copy link
Member

@adriaanm is this high enough priority to remain open and remain on the 2.13.0-RC1 milestone?

@milessabin
Copy link
Contributor Author

I'm happy to spend a little more time on this, but I need a pointer or two.

@adriaanm
Copy link
Contributor

adriaanm commented Jan 8, 2019

Sorry, I don't think we'll be able to get this into 2.13.0

@adriaanm
Copy link
Contributor

adriaanm commented Jan 8, 2019

Let's revisit after the RC rush

@adriaanm adriaanm closed this Jan 8, 2019
@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Jan 8, 2019
@mkeskells
Copy link
Contributor

if this doesn't make 2.13.0, can it be applied in 2.13.x as it affects the serialised form of object (as a val can become a def)
If this is part of your binary compatibility considerations then removing it will push any consideration of this change to 2.14

@adriaanm
Copy link
Contributor

adriaanm commented Jan 9, 2019

I've proposed a more narrow fix in the above PR.

a val can become a def

could you elaborate?

@mkeskells
Copy link
Contributor

@adriaanm rephrasing slightly - whether a constant is detected by the compiler can cause the underlying java class to contain a field + an accessor, or just an accessor

taking some demo code

package demo
final class Test{
  final val aConstant = 1
  val shouldBe = 1
  final val mightBy1 = 1 + 1/2
  final val mightBy2 = 1 + 1*1
  final val mightBy3 = 1 + (2.0).toInt
  final val mightBy4 = 1 + Math.abs(0)
}

You can see from the generated bytecode below that whether the value is detected to be a constant affects if it is emitted as a field, and clearly whether it is a field in the java serialised form

IMO shouldBe should also be detected as a constant, as it is effectively final (but that's a separate discussion)

Whether the various mightBes are emitted as a field depends on how clever the constant folding is (not that I am proposing we should be exhaustive and coping with all possibilities like Math functions)
In the 2.12 compiler 1 and 2 are not emitted as fields, but 3 and 4 are as you can see from the bytecode below

If the stability of the serialised form cant be changed during the 2.13.x timeline, there is clearly a line to draw on what should or should not be folded

Even with some stability about the decision as to what is folded away it is IMO an unexpected effect to see the serialised form affected without obvious coding intent. I know that Java made some surprising decisions about constant folding at compile time (e.g. across class boundaries) but I would hope that Scala can take a clearer view

Bytecode

// class version 52.0 (52)
// access flags 0x31
public final class demo/Test {

  // compiled from: Test.scala

  @Lscala/reflect/ScalaSignature;(bytes="\u0006\u0005M2AAD\u0008\u0003%!)\u0011\u0004\u0001C\u00015!9Q\u0004\u0001b\u0001\n\u000bq\u0002BB\u0011\u0001A\u00035q\u0004C\u0004#\u0001\u0009\u0007I\u0011A\u0012\u0009\r\u001d\u0002\u0001\u0015!\u0003%\u0011\u001dA\u0003A1A\u0005\u0006yAa!\u000b\u0001!\u0002\u001by\u0002b\u0002\u0016\u0001\u0005\u0004%)a\u000b\u0005\u0007]\u0001\u0001\u000bQ\u0002\u0017\u0009\u000f=\u0002!\u0019!C\u0003G!1\u0001\u0007\u0001Q\u0001\u000e\u0011Bq!\r\u0001C\u0002\u0013\u00151\u0005\u0003\u00043\u0001\u0001\u0006i\u0001\n\u0002\u0005)\u0016\u001cHOC\u0001\u0011\u0003\u0011!W-\\8\u0004\u0001M\u0011\u0001a\u0005\u0009\u0003)]i\u0011!\u0006\u0006\u0002-\u0005)1oY1mC&\u0011\u0001$\u0006\u0002\u0007\u0003:L(+\u001a4\u0002\rqJg.\u001b;?)\u0005Y\u0002C\u0001\u000f\u0001\u001b\u0005y\u0011!C1D_:\u001cH/\u00198u+\u0005yr\"\u0001\u0011\u001e\u0003\u0005\u0009!\"Y\"p]N$\u0018M\u001c;!\u0003!\u0019\u0008n\\;mI\n+W#\u0001\u0013\u0011\u0005Q)\u0013B\u0001\u0014\u0016\u0005\rIe\u000e^\u0001\ng\"|W\u000f\u001c3CK\u0002\n\u0001\"\\5hQR\u0014\u00150M\u0001\n[&<\u0007\u000e\u001e\"zc\u0001\n\u0001\"\\5hQR\u0014\u0015PM\u000b\u0002Y=\u0009Q&H\u0001\u0003\u0003%i\u0017n\u001a5u\u0005f\u0014\u0004%\u0001\u0005nS\u001eDGOQ=4\u0003%i\u0017n\u001a5u\u0005f\u001c\u0004%\u0001\u0005nS\u001eDGOQ=5\u0003%i\u0017n\u001a5u\u0005f$\u0004\u0005")

  ATTRIBUTE ScalaSig : unknown

  ATTRIBUTE ScalaInlineInfo : unknown

  // access flags 0x12
  private final I shouldBe

  // access flags 0x12
  private final I mightBy3

  // access flags 0x12
  private final I mightBy4

  // access flags 0x11
  public final aConstant()I
   L0
    ICONST_1
    IRETURN
   L1
    LOCALVARIABLE this Ldemo/Test; L0 L1 0
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x1
  public shouldBe()I
   L0
    LINENUMBER 4 L0
    ALOAD 0
    GETFIELD demo/Test.shouldBe : I
    IRETURN
   L1
    LOCALVARIABLE this Ldemo/Test; L0 L1 0
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x11
  public final mightBy1()I
   L0
    ICONST_1
    IRETURN
   L1
    LOCALVARIABLE this Ldemo/Test; L0 L1 0
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x11
  public final mightBy2()I
   L0
    ICONST_2
    IRETURN
   L1
    LOCALVARIABLE this Ldemo/Test; L0 L1 0
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x11
  public final mightBy3()I
   L0
    LINENUMBER 7 L0
    ALOAD 0
    GETFIELD demo/Test.mightBy3 : I
    IRETURN
   L1
    LOCALVARIABLE this Ldemo/Test; L0 L1 0
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x11
  public final mightBy4()I
   L0
    LINENUMBER 8 L0
    ALOAD 0
    GETFIELD demo/Test.mightBy4 : I
    IRETURN
   L1
    LOCALVARIABLE this Ldemo/Test; L0 L1 0
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x1
  public <init>()V
   L0
    LINENUMBER 9 L0
    ALOAD 0
    INVOKESPECIAL java/lang/Object.<init> ()V
   L1
    LINENUMBER 4 L1
    ALOAD 0
    ICONST_1
    PUTFIELD demo/Test.shouldBe : I
   L2
    LINENUMBER 7 L2
    ALOAD 0
    ICONST_1
    LDC 2.0
    D2I
    IADD
    PUTFIELD demo/Test.mightBy3 : I
   L3
    LINENUMBER 8 L3
    ALOAD 0
    ICONST_1
    ICONST_0
    INVOKESTATIC java/lang/Math.abs (I)I
    IADD
    PUTFIELD demo/Test.mightBy4 : I
   L4
    LINENUMBER 2 L4
    RETURN
   L5
    LOCALVARIABLE this Ldemo/Test; L0 L5 0
    MAXSTACK = 4
    MAXLOCALS = 1
}

@adriaanm
Copy link
Contributor

adriaanm commented Jan 10, 2019 via email

@mkeskells
Copy link
Contributor

ok understood - sorry for the noise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants