Support leak non-hot values to constructors #12711

liufengyun · 2021-06-04T14:46:04Z

The following code will be OK:

  class A(b: B) {
    val b2 = new B(this)
    val c = b2.a2
  }

  class B(a: A) {
    val a2 = new A(this)
    val c = a2.b2
  }

The following will get an error:

  class A(b: B) {
    println(b.a2)           // error
    val b2 = new B(this)
  }

  class B(a: A) {
    println(a.b2)           // error
    val a2 = new A(this)
  }

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

EnzeXing · 2021-06-04T19:42:52Z

@liufengyun Could you explain more about the bug you mentioned in the email?

liufengyun · 2021-06-04T20:05:25Z

@liufengyun Could you explain more about the bug you mentioned in the email?

I just add a test case which makes it more clear:

abstract class A {
  bar()
  private val a = 5
  def foo() = a
  def bar(): Unit
}

class M extends A {
  def bar() = promote(this)    // error
  def promote(m: M) = m.foo()
}

The checker in the master does not catch the problem, because the set of fields it gets doesn't include those not accessible from the class M. Therefore, in the call promote(this), it claims this is hot because all fields are initialized, because the set of fields is empty.

The commit d12ebde fixes the problem: we just cannot use Type.fields. Instead, we compute from classes, which is also faster (without asSeenFrom).

michelou · 2021-06-05T07:48:50Z

@liufengyun Remove line 515 (val obj = heap(addr)) in file Semantic.scala ?

liufengyun · 2021-06-05T09:31:00Z

@liufengyun Remove line 515 (val obj = heap(addr)) in file Semantic.scala ?

Good catch, thank you @michelou ! Now it's fixed.

olhotak · 2021-06-07T16:41:34Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

+   *  Despite that we have environment for evaluating expressions in secondary
+   *  constructors, we don't need to put environment as the cache key. The
+   *  reason is that constructor parameters are determined by the value of
+   *  `this` --- it suffices to make the value of `this` as part of the cache


These constructor parameters are the args field in Warm, right?

Warm only contain arguments to the constructor of the concrete class, but not arguments to its super-constructors.

olhotak · 2021-06-07T16:43:50Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

  object Promoted {
+    class PromotionInfo {
+      var isCurrentObjectPromoted: Boolean = false
+      val values = mutable.Set.empty[Value]


Is isCurrentObjectPromoted for ThisRef and values for Warm values?

Yes, that's correct. However, values may also contain functions values.

olhotak · 2021-06-07T16:49:30Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

@@ -251,14 +305,24 @@ class Semantic {

      case (RefSet(refs1), RefSet(refs2))     => RefSet(refs1 ++ refs2)

+    def widen: Value =


Consider a more specific name such as widenArg. Also consider a comment to say that this conservatively approximates the Value with Hot or Cold.

Good suggestion.

olhotak · 2021-06-07T16:57:00Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala


        case addr: Addr =>
+          val isLocal = meth.owner.isClass


Isn't a local method one whose owner is a method, and thus is not a class?

Good catch, I'll add a test case to avoid such mistakes.

olhotak · 2021-06-07T17:02:33Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

              if target.isPrimaryConstructor then
+                given Env = env2


Is there a specific reason to prefer this pattern of given over the use() method?

I try to use given T = e whenever possible. However, in same places we cannot easily use given, because of scoping interference. I described the problem in more detail here.

olhotak · 2021-06-07T17:39:23Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

+
+        def default() = Result(Hot, Nil)
+
+        if sym.is(Flags.Param) then


&& would be clearer here than two ifs.

olhotak · 2021-06-07T17:40:38Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala


      case tmref: TermRef =>
        cases(tmref.prefix, thisV, klass, source).select(tmref.symbol, source)

      case tp @ ThisType(tref) =>
-        if tref.symbol.is(Flags.Package) then Result(Hot, Errors.empty)
+        val cls = tref.classSymbol.asClass


I don't understand what this case corresponds to.

cls.isStaticOwner means it's a global object, klass.isContainedIn(cls) means we have O.this outside the body of the object O. We don't check global objects, so simply return Hot here.

olhotak · 2021-06-07T17:43:16Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

    }

-    def superCall(tref: TypeRef, ctor: Symbol, source: Tree): Unit =
+    type Handler = (() => Unit) => Unit


A comment would be helpful here to explain what Handlers are used for.

olhotak · 2021-06-07T17:43:43Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

      case tree @ Block(stats, NewExpr(tref, New(tpt), ctor, argss)) =>  // can happen
        eval(stats, thisV, klass).foreach { res => errorBuffer ++= res.errors }
-        errorBuffer ++= evalArgs(argss.flatten, thisV, klass)
-        superCall(tref, ctor, tree)
+        val (erros, args) = evalArgs(argss.flatten, thisV, klass)


type errors?

olhotak · 2021-06-07T17:47:22Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala


  def typeRefOf(tp: Type)(using Context): TypeRef = tp.dealias.typeConstructor match {
    case tref: TypeRef => tref
    case hklambda: HKTypeLambda => typeRefOf(hklambda.resType)
  }

-  opaque type Arg  = Tree | ByNameArg
+  type Arg  = Tree | ByNameArg


Why is it no longer opaque? Why was it opaque before?

This is a restriction in Scala 3: we cannot define an inline method and an opaque type in the same object.

liufengyun · 2021-06-07T19:50:57Z

@EnzeXing Could you please have a look as well?

liufengyun · 2021-06-08T15:31:45Z

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

-          val res = value.call(ctor, superType = NoType, source)
-          Result(value, res.errors)
+          val value = Warm(klass, outer, ctor, args.map(_.value).widenArgs).ensureExists
+          val res = value.call(ctor, args, superType = NoType, source)


This is problematic: still possible to crash.

@olhotak @EnzeXing In 98a0977 and 7f91775, I proposed two different fixes. Which fix makes more sense?

I can squash the commits after we decide on one.

I'm not sure but I think I like 98a0977 better. The error it gives is more explicit about why the code is being rejected. Also, in tests/init/neg/secondary-ctor4.scala, 7f91775 reports more errors that are not related to inner classes in a secondary constructor.

The errors are fewer in 98a0977 because the analysis just skipped the check.

I kind of prefer 7f91775, as the code is more regular --- we only have one special case in variable resolution. As the examples are all invented, I don't expect the two have an observable difference in expressiveness nor usability.

OK, I'm OK with 7f91775 as well. Simpler implementation is an advantage.

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

The previous code does not work, because private fields are not included.

With the flag <static>, the enclosing class will be skipped. See the documentation `SymDenotation.enclosingClass`

For traits, its outers will be proxy methods of the class that extends the trait. As the prefix is stable and is a valid value before any super constructor calls. Therefore, we may think the outers for traits are immediately set following the class parameters. Also, when trying promotion of warm values, we never try warm values whose fields are not fully filled -- which corresponds to promote ThisRef with non-initialized fields, and errors will be reported when the class is checked separately.

This aligns with the design of restricting method arguments to be hot. We only allow non-hot to constructors.

This is a simple fix which should have no impact on expressiveness and usability, as local classes inside secondary constructors are extremely rare.

liufengyun · 2021-06-09T21:03:57Z

@olhotak Do you have other comments or we are ready to merge it?

olhotak · 2021-06-09T21:17:56Z

You added some comments during our meeting on Tuesday. One was fixed by 7f91775 but are the other comments that you added addressed? I can't find them anymore. Aside from that, I think it's ready to merge.

liufengyun · 2021-06-09T21:21:41Z

You added some comments during our meeting on Tuesday. One was fixed by 7f91775 but are the other comments that you added addressed? I can't find them anymore. Aside from that, I think it's ready to merge.

Sorry, I forgot to mention it: the handler issue is addressed in d9892f9, and the local classes in secondary constructors issue is addressed in ae566b4 (I squashed the two trials into one commit).

olhotak · 2021-06-09T21:33:03Z

OK, great. LGTM then. (I forgot about the handler issue.)

After scala#12711, it is no longer the case that `Warm < ThisRef`. The reason is that the class parameters for `ThisRef` are always hot, while it is not the case anymore after scala#12711. Actually even before scala#12711, it's not the case because the outers of warm objects may not be hot. The theory does not have inner classes, it thus does not suffer from the problem.

liufengyun force-pushed the leak-ctor branch from 0820aca to ff2455a Compare June 4, 2021 14:55

liufengyun commented Jun 4, 2021

View reviewed changes

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala Outdated Show resolved Hide resolved

liufengyun marked this pull request as ready for review June 4, 2021 16:58

liufengyun force-pushed the leak-ctor branch from e07bb78 to 9e9c4a5 Compare June 4, 2021 22:01

liufengyun force-pushed the leak-ctor branch from fc2ae30 to afed473 Compare June 5, 2021 09:30

liufengyun force-pushed the leak-ctor branch from a89601a to e5d7ae4 Compare June 5, 2021 09:38

liufengyun requested a review from olhotak June 7, 2021 07:46

olhotak reviewed Jun 7, 2021

View reviewed changes

liufengyun commented Jun 8, 2021

View reviewed changes

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala Outdated Show resolved Hide resolved

liufengyun force-pushed the leak-ctor branch from 7f91775 to 856e316 Compare June 9, 2021 08:57

liufengyun added 14 commits June 9, 2021 16:40

Add definition

8cdb1ed

Add doc for env

a431955

Fix compilation

2a47d89

Refactor trace

5a82985

Support Value.source for friendly error message

08f040c

Disallow non-hot arguments to non-constructors

632b7d3

Handle access of parameters in constructors

0faa404

Handle exception

0af900a

No exceptions

f00b0c9

Fix Env.isHot

746373b

Add missing checks for arguments

748edc1

Handle new expression with only cold args

bfaef00

Fix soundness test

c5475b4

Reorganize tests

1e98f5c

liufengyun added 19 commits June 9, 2021 16:40

Add test

629f57c

More logging

6edc40a

Fix bug with promotion of this

9842479

The previous code does not work, because private fields are not included.

Refactor code: introduce ArgInfo instead of Value.source

436609e

Handle O.this outside of it's definition

3ce2532

Fix owner of values with static flag

2804eec

With the flag <static>, the enclosing class will be skipped. See the documentation `SymDenotation.enclosingClass`

Add documentation about caching

a6d96ef

Add test

bc0cce3

Add another neg test for promotion of ThisRef

c033c9e

Address review (thanks @michelou)

a4b504a

Rename isFullyInitialized to isFullyFilled to avoid confusion

03f0cde

Restrict function arguments to be hot

0214051

This aligns with the design of restricting method arguments to be hot. We only allow non-hot to constructors.

Add documentation

29cc844

Address review

cea9f26

Simplify logic for checking local classes inside secondary constructors

ec1de2f

Address review: simplify logic

d9892f9

Abstract parameters of non-local constructor parameters as cold

ae566b4

This is a simple fix which should have no impact on expressiveness and usability, as local classes inside secondary constructors are extremely rare.

Update docs

7fde75e

liufengyun force-pushed the leak-ctor branch from 856e316 to 7fde75e Compare June 9, 2021 14:40

liufengyun merged commit 30da597 into scala:master Jun 9, 2021

liufengyun deleted the leak-ctor branch June 9, 2021 21:50

Kordyjan added this to the 3.0.2 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support leak non-hot values to constructors #12711

Support leak non-hot values to constructors #12711

liufengyun commented Jun 4, 2021

EnzeXing commented Jun 4, 2021

liufengyun commented Jun 4, 2021

michelou commented Jun 5, 2021

liufengyun commented Jun 5, 2021

olhotak Jun 7, 2021

liufengyun Jun 7, 2021 •

edited

Loading

olhotak Jun 7, 2021

liufengyun Jun 7, 2021

olhotak Jun 7, 2021

liufengyun Jun 7, 2021

olhotak Jun 7, 2021

liufengyun Jun 7, 2021

olhotak Jun 7, 2021

liufengyun Jun 7, 2021 •

edited

Loading

olhotak Jun 7, 2021

olhotak Jun 7, 2021

liufengyun Jun 7, 2021

olhotak Jun 7, 2021

olhotak Jun 7, 2021

olhotak Jun 7, 2021

liufengyun Jun 7, 2021

liufengyun commented Jun 7, 2021 •

edited

Loading

liufengyun Jun 8, 2021

liufengyun Jun 8, 2021 •

edited

Loading

olhotak Jun 8, 2021

liufengyun Jun 8, 2021

liufengyun Jun 8, 2021 •

edited

Loading

olhotak Jun 9, 2021

liufengyun commented Jun 9, 2021

olhotak commented Jun 9, 2021

liufengyun commented Jun 9, 2021

olhotak commented Jun 9, 2021

		@@ -251,14 +305,24 @@ class Semantic {

		case (RefSet(refs1), RefSet(refs2)) => RefSet(refs1 ++ refs2)

		def widen: Value =

Support leak non-hot values to constructors #12711

Support leak non-hot values to constructors #12711

Conversation

liufengyun commented Jun 4, 2021

EnzeXing commented Jun 4, 2021

liufengyun commented Jun 4, 2021

michelou commented Jun 5, 2021

liufengyun commented Jun 5, 2021

Choose a reason for hiding this comment

liufengyun Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufengyun Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufengyun commented Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

liufengyun Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufengyun Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufengyun commented Jun 9, 2021

olhotak commented Jun 9, 2021

liufengyun commented Jun 9, 2021

olhotak commented Jun 9, 2021

liufengyun Jun 7, 2021 •

edited

Loading

liufengyun Jun 7, 2021 •

edited

Loading

liufengyun commented Jun 7, 2021 •

edited

Loading

liufengyun Jun 8, 2021 •

edited

Loading

liufengyun Jun 8, 2021 •

edited

Loading