Allowing local variables to be initialized with non-hot values in initialization checker #12867

EnzeXing · 2021-06-17T22:17:58Z

This PR will allow local variables to be initialized with non-hot values in the initialization checker, instead of ensuring local variables to be hot.

EnzeXing · 2021-06-17T22:19:10Z

@liufengyun @olhotak I'm looking forward to your comments

liufengyun · 2021-06-18T05:41:14Z

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

+        }
+      }
+    }
+  }


Thanks @EnzeXing . If we do it in the place where we give semantics to local variables, then we can reduce the total changes to just 2 or 3 lines.

This would be case ValDef in def cases.

Sorry, I was not clear, it's in the use site of variables, i.e. case TermRef(NoPrefix, _) =>, not the definition site.

You mean move the updating env part to the use site of variables?

We don't need to update env. The idea is to evaluate its initializer when the variable is accessed -- and it's automatically cached.

@liufengyun Sorry but I'm still a bit confused. My idea is evaluate the local variables when they are initialized and stored it, so that we can just look up when accessing it. How can we evaluate its initializer when it's accessed?

@liufengyun Sorry but I'm still a bit confused. My idea is evaluate the local variables when they are initialized and stored it, so that we can just look up when accessing it. How can we evaluate its initializer when it's accessed?

When the variable is accessed, rather than look up its value in env, we evaluate the rhs of the definition (ValDef) of the variable right there, when it's accessed.

Suppose we have val v = new A in some local method, and the method accesses and returns v in the end. When we access v in the end, all we know about v is that it's a TermRef, but how can we know its rhs definition at this point? I thought we analyze val v = new A first and then accessing v at last, but how do we go the other way?

From the TermRef, we should be able to get its Symbol, and from the Symbol, call defTree to find the ValDef where that variable was defined.

We need to make sure that we're doing this only for TermRefs that represent local variables. In particular, they should have NoPrefix, but there are probably additional conditions to check to ensure that it's a local variable.

From the TermRef, we should be able to get its Symbol, and from the Symbol, call defTree to find the ValDef where that variable was defined.

We need to make sure that we're doing this only for TermRefs that represent local variables. In particular, they should have NoPrefix, but there are probably additional conditions to check to ensure that it's a local variable.

Yes, that's exactly what I have in mind. Note that in case ValDef, the rhs is already evaluated and thus cached (need to add cache = true). The eval operation is actually a retrieval from the cache.

We need to set up the correct thisV for evaluating rhs, here is an example for local methods:

https://github.com/lampepfl/dotty/blob/4aa7f909f006a012cb78662b938ceda2059e67be/compiler/src/dotty/tools/dotc/transform/init/Semantic.scala#L757-L760

Two corner cases to think about:

invent an example where thisV = Cold. Just report a warning in this case.

invent an example where env != Nil.

There is a bug in handling local methods directly inside constructors called from local classes inside constructors: its env is not set up correctly. We can discuss a fix in next meeting.

liufengyun · 2021-06-18T05:41:54Z

tests/init/neg/local-warm4.scala

+
+
+
+


Minor: useless empty lines.

I would suggest inventing more tests that demonstrate the capture of local non-hot definitions in functions and local classes.

liufengyun · 2021-06-18T05:44:08Z

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

@@ -405,6 +406,7 @@ class Semantic {
              given Trace = trace1
              val cls = target.owner.enclosingClass.asClass
              val ddef = target.defTree.asInstanceOf[DefDef]
+              // try early promotion here; if returns error, returns cold


Irrelevant change?

olhotak · 2021-06-18T15:48:47Z

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

+        }
+      }
+    }
+  }


This would be case ValDef in def cases.

olhotak · 2021-06-18T15:51:57Z

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


      case If(cond, thenp, elsep) =>
-        val ress = eval(cond :: thenp :: elsep :: Nil, thisV, klass)
+        val (ress, env2) = eval(cond :: thenp :: elsep :: Nil, thisV, klass)


In places where env is unused, use _ instead (val (ress, _) = ???).

EnzeXing · 2021-06-22T21:22:22Z

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

+                  val res = eval(vdef.rhs, addr, klass)
+                  if res.value.promote("Try promote", source).isEmpty then Result(Hot, Errors.empty) else res
+                }
+                case _ => ???


How do we deal with other possible cases for thisV, like Fun or RefSet? Are they possible in this case?

The situation is similar to what we do in Value.call and Value.select. There is some code duplication. To avoid that, we actually handle local calls inside Value.call, which is more like a hack and is not ideal.

For the moment, we can create a helper method like def accessLocal(sym: Symbol, thisV: Value): Contextual[Result] near the place where we define Value.call.

EnzeXing · 2021-06-22T21:26:35Z

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

@@ -855,7 +857,8 @@ class Semantic {
      case vdef : ValDef =>
        // local val definition
        // TODO: support explicit @cold annotation for local definitions
-        eval(vdef.rhs, thisV, klass).ensureHot("Local definitions may only hold initialized values", vdef)
+        eval(vdef.rhs, thisV, klass, true)


I've added cacheResult = true here to cache the result for rhs, but I've found that in several test cases, we evaluate rhs again when accessing local variable after. Is this the right way to cache the result?

Use cache = true instead of true for better readability.

EnzeXing · 2021-06-22T21:29:38Z

@liufengyun @olhotak I have re-implemented the code based on your thoughts, but I found that there is still a failed test: tests/pos/i5766.scala, and I think the reason is we somehow didn't cache the result successfully when evaluating the rhs of local val definition. I'm looking forward to your thoughts about how to fix that. Thanks!

olhotak · 2021-06-23T13:50:34Z

Do you know which local val in i5766 is causing problems? The test has no val at all and lots of inline and implicit. Removing all the inline makes the test pass -- could it be related to bad defTrees? Also removing only the very last inline makes the test pass:

  inline implicit def baz(implicit loop: => Foo): Baz = new Baz { def next = loop }

olhotak · 2021-06-23T13:58:25Z

The error is:

-- Error: tests/pos/i5766.scala:28:83 ------------------------------------------
28 |  inline implicit def baz(implicit loop: => Foo): Baz = new Baz { def next = loop }
   |                                                                                   ^
   |Promote the value under initialization to fully-initialized. outer not yet initialized, target = class $_lazy_implicit_$8, klass = class $anon. Calling trace:
   | -> inline implicit def bar(implicit loop: Baz): Bar = new Bar { def next = loop }	[ i5766.scala:27 ]
   |  -> summon	[ i5766.scala:31 ]

The expanded code is

    Test3.summon(
      {
        final class $_lazy_implicit_$8() extends Object(), Serializable {
          val $_lazy_implicit_$7: Test3.Foo = 
            Test3.foo(
              {
                val loop$proxy1: Test3.Baz = 
                  {
                    final class $anon() extends Object(), Test3.Baz {
                      def next: Test3.Foo = 
                        $_lazy_implicit_$8.this.$_lazy_implicit_$7
                    }
                    new Object with Test3.Baz {...}():Test3.Baz
                  }:Test3.Baz
                {
                  final class $anon() extends Object(), Test3.Bar {
                    def next: Test3.Baz = loop$proxy1
                  }
                  new Object with Test3.Bar {...}():Test3.Bar
                }:Test3.Bar
              }
            )
        }
        val $_lazy_implicit_$9: $_lazy_implicit_$8 = new $_lazy_implicit_$8()
        $_lazy_implicit_$9.$_lazy_implicit_$7
      }
    )

EnzeXing · 2021-06-23T15:18:47Z

val loop$proxy1 is the local val that seems to went wrong

olhotak · 2021-06-23T15:32:03Z

@EnzeXing can you add tracing code to eval() to see what it's being called with and what it's caching?

EnzeXing · 2021-06-23T16:41:32Z

I do find that when we access this local val, the defTree is different than the expr tree that we cached in when we evaluate the initializer

liufengyun

The commit 49d879f and 9fec01e contain the same content? It seems the two commits need to be squashed into one.

liufengyun · 2021-07-02T22:30:38Z

compiler/src/dotty/tools/dotc/core/Phases.scala

    def runsAfter: Set[String] = Set.empty

+    /** Whether this phase require synchronization of symbol.defTree?


Suggested change

/** Whether this phase require synchronization of symbol.defTree?

/** Whether this phase requires synchronization of symbol.defTree when enabled?

liufengyun · 2021-07-02T22:31:36Z

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

@@ -692,7 +693,7 @@ class Semantic {
  }

  /** Evaluate a list of expressions */
-  def eval(exprs: List[Tree], thisV: Addr, klass: ClassSymbol): Contextual[List[Result]] =
+  def eval(exprs: List[Tree], thisV: Addr, klass: ClassSymbol): Contextual[List[Result]] = 


Suggested change

def eval(exprs: List[Tree], thisV: Addr, klass: ClassSymbol): Contextual[List[Result]] =

def eval(exprs: List[Tree], thisV: Addr, klass: ClassSymbol): Contextual[List[Result]] =

liufengyun · 2021-07-02T22:33:20Z

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

@@ -900,8 +901,27 @@ class Semantic {
            // It's always safe to approximate them with `Cold`.
            Result(Cold, Nil)
        else


We need to refactor the if/else here, as in the current else branch, we can still encounter sym.is(Flags.Param).

This is not addressed?

Also, I'd suggest move the body of case tmref: TermRef if tmref.prefix == NoPrefix to accessLocal.

Could we have sym.is(Flags.Param) but its owner is not a constructor? Then is this symbol an argument to a local method?

Yes, that's possible --- normal method arguments can also reach here.

liufengyun · 2021-07-02T22:34:42Z

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

+                }
+                case addr: Addr => {
+                  val res = eval(vdef.rhs, addr, klass)
+                  if res.value.promote("Try promote", source).isEmpty then Result(Hot, Errors.empty) else res


I don't see why we need this. eval(vdef.rhs, addr, klass) seems to be enough.

liufengyun · 2021-07-02T22:35:50Z

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

+                case Cold => {
+                  val error = AccessCold(sym, source, trace.toVector)
+                  Result(Hot, error :: Nil)
+                }


Minor: don't use braces for the body of a case.

Here, simply return Result(Cold, Nil) is enough.

liufengyun · 2021-07-02T22:51:58Z

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

+                  val res = eval(vdef.rhs, addr, klass)
+                  if res.value.promote("Try promote", source).isEmpty then Result(Hot, Errors.empty) else res
+                }
+                case _ => ???


The situation is similar to what we do in Value.call and Value.select. There is some code duplication. To avoid that, we actually handle local calls inside Value.call, which is more like a hack and is not ideal.

For the moment, we can create a helper method like def accessLocal(sym: Symbol, thisV: Value): Contextual[Result] near the place where we define Value.call.

EnzeXing · 2021-07-21T20:43:23Z

@liufengyun Could you please review it again? I have refactored the accessing local variable part and updating tests. I will fix the bug of Env soon.

liufengyun · 2021-07-22T07:39:50Z

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

@@ -495,6 +496,29 @@ class Semantic {
          Result(value2, errors)
      }
    }
+
+    def accessLocal(sym: Symbol, default: Result, klass: ClassSymbol, source: Tree): Contextual[Result] =


It's better to remove default here, as it simplifies the protocol.

Do you mean to just hardcode it as Hot?

Oh, I see: you mean to move the whole case tmref ... from cases to accessLocal. That case is what defines default to be Hot.

liufengyun · 2021-07-22T07:40:34Z

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

+
+            case addr: Addr => 
+              val res = eval(vdef.rhs, addr, klass)
+              if res.value.promote("Try promote", source).isEmpty then Result(Hot, Errors.empty) else res


Why we call promote here?

It's tests/pos/i8892.scala. If we don't promote here, dotty will crash on this test.

Thanks for the info. The crash seems to imply there is some bug elsewhere, it's difficult to justify the promotion here.

Could you try to debug the test and see why it crashes?

OK, I'll take a closer look at it.

@liufengyun It seems that we are selecting a non-exist field 1_<outer> from Zero in val j31. I think we are meant to call Zero.++, but "++" is transparent inlined def, so it somehow got translated to 1_<outer>

@EnzeXing You can try the following patch: https://gist.github.com/liufengyun/de6198e4a8a28e090c41e62c8af65ed6

With this patch, this test is resolved, but now we would fail on tests/init/special/i12128/Test_3.scala since dotty can't identify the object MacroCompat

It works without any problem for me (including the test above). Could you please debug?

nvm, this has been resolved.

liufengyun · 2021-07-22T07:41:17Z

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

@@ -855,7 +857,8 @@ class Semantic {
      case vdef : ValDef =>
        // local val definition
        // TODO: support explicit @cold annotation for local definitions
-        eval(vdef.rhs, thisV, klass).ensureHot("Local definitions may only hold initialized values", vdef)
+        eval(vdef.rhs, thisV, klass, true)


Use cache = true instead of true for better readability.

liufengyun · 2021-07-22T07:41:28Z

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

@@ -855,7 +879,8 @@ class Semantic {
      case vdef : ValDef =>
        // local val definition
        // TODO: support explicit @cold annotation for local definitions
-        eval(vdef.rhs, thisV, klass).ensureHot("Local definitions may only hold initialized values", vdef)
+        eval(vdef.rhs, thisV, klass, true)
+        // .ensureHot("Local definitions may only hold initialized values", vdef)


Remove this line.

liufengyun · 2021-07-22T07:43:37Z

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

@@ -900,8 +901,27 @@ class Semantic {
            // It's always safe to approximate them with `Cold`.
            Result(Cold, Nil)
        else


This is not addressed?

Also, I'd suggest move the body of case tmref: TermRef if tmref.prefix == NoPrefix to accessLocal.

EnzeXing · 2021-07-22T20:50:01Z

@liufengyun I have encountered a failed test on Github: secondary-ctor2.scala, but it didn't fail on my local computer. Do you know what may cause it to fail?

liufengyun

@liufengyun I have encountered a failed test on Github: secondary-ctor2.scala, but it didn't fail on my local computer. Do you know what may cause it to fail?

I think we need to remove the error annotation at line 9: that line should not report an error.

liufengyun · 2021-07-23T05:41:06Z

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

@@ -900,8 +901,27 @@ class Semantic {
            // It's always safe to approximate them with `Cold`.
            Result(Cold, Nil)
        else


Yes, that's possible --- normal method arguments can also reach here.

liufengyun · 2021-07-23T05:50:38Z

tests/init/neg/local-warm4.check

+-- Error: tests/init/neg/local-warm4.scala:18:20 -----------------------------------------------------------------------
+18 |                a = newA // error
+   |                    ^^^^
+   |Promote the value under initialization to fully-initialized. May only assign fully initialized value. Calling trace:


Let's change the first part of the message to: Cannot prove that the value is fully initialized. May ...

liufengyun · 2021-07-23T06:01:12Z

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

+              val res = eval(vdef.rhs, addr, klass)
+              if res.value.promote("Try promote", source).isEmpty then Result(Hot, Errors.empty) else res
+
+            case _ => ???


Let's print some debug information here: report.error(...). We can define a helper method:

def reportImpossibleError(message: String)(using Trace, Context): Unit = ...

For end-users, this is better than a compiler crash and useful debug information can be reported.

This can be done in another PR.

EnzeXing · 2021-07-23T20:51:09Z

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

+          // It's always safe to approximate them with `Cold`.
+          Result(Cold, Nil)
+      else if sym.is(Flags.Param) then
+        default()


For parameters for normal method calls, we just return Result(Hot, Nil) since we expect that those arguments have been promoted in Call, right?

Yes, that's correct.

liufengyun · 2021-07-28T19:18:20Z

@EnzeXing There are few minor issues:

Remove the last merge commit (use rebase instead)
It would be nice to keep the outerSelect fix as a different commit from access local
It would be nice to keep the soundness patch as a separate commit
Remove trailing spaces in the file Semantic.scala (You can change your IDE setting to automatically remove trailing spaces on save)

Fixing inherit-non-hot.check

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.

EnzeXing · 2021-07-30T19:04:48Z

@liufengyun Could you review it again? I've fixed those issue.

liufengyun

LGTM, thank you @EnzeXing 🎉

liufengyun reviewed Jun 18, 2021

View reviewed changes

liufengyun changed the title ~~Allowing local variables to be assigned with non-hot values in initialization checker~~ Allowing local variables to be initialized with non-hot values in initialization checker Jun 18, 2021

romanowski assigned liufengyun Jun 18, 2021

olhotak reviewed Jun 18, 2021

View reviewed changes

EnzeXing commented Jun 22, 2021

View reviewed changes

liufengyun reviewed Jul 2, 2021

View reviewed changes

EnzeXing force-pushed the non-hot-local-val branch from 1a11195 to 90f2277 Compare July 21, 2021 20:41

liufengyun reviewed Jul 22, 2021

View reviewed changes

liufengyun reviewed Jul 23, 2021

View reviewed changes

EnzeXing force-pushed the non-hot-local-val branch 2 times, most recently from 6a4d226 to 25069bb Compare July 23, 2021 20:47

EnzeXing commented Jul 23, 2021

View reviewed changes

EnzeXing force-pushed the non-hot-local-val branch 4 times, most recently from ea89248 to bfb10bb Compare July 28, 2021 14:23

EnzeXing added 3 commits July 30, 2021 14:17

Allowing non-hot assignment to local val and updating tests

c886d5a

Try early promotion before updating env

2560f15

Evaluate local variable during access

0f1586d

EnzeXing and others added 6 commits July 30, 2021 14:17

Resolving cases for def-tree

fef4ddf

Synchronize Symbol.defTree before analysis

ccab4eb

Refactoring accessLocal

ba5f4f0

Update tests

6cbfd9a

Fixing inherit-non-hot.check

Handle OuterSelect in init checker

a59e6a3

EnzeXing force-pushed the non-hot-local-val branch from 76eaaff to e901a36 Compare July 30, 2021 18:42

liufengyun approved these changes Jul 30, 2021

View reviewed changes

liufengyun merged commit 9e8d5c5 into scala:master Jul 30, 2021

EnzeXing deleted the non-hot-local-val branch August 9, 2021 18:52

Kordyjan added this to the 3.1.0 milestone Aug 2, 2023

		def runsAfter: Set[String] = Set.empty

		/** Whether this phase require synchronization of symbol.defTree?

	/** Whether this phase require synchronization of symbol.defTree?
	/** Whether this phase requires synchronization of symbol.defTree when enabled?

	def eval(exprs: List[Tree], thisV: Addr, klass: ClassSymbol): Contextual[List[Result]] =
	def eval(exprs: List[Tree], thisV: Addr, klass: ClassSymbol): Contextual[List[Result]] =

Allowing local variables to be initialized with non-hot values in initialization checker #12867

Allowing local variables to be initialized with non-hot values in initialization checker #12867

Conversation

EnzeXing commented Jun 17, 2021 • edited by liufengyun Loading

EnzeXing commented Jun 17, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnzeXing commented Jun 22, 2021

olhotak commented Jun 23, 2021

olhotak commented Jun 23, 2021

EnzeXing commented Jun 23, 2021

olhotak commented Jun 23, 2021

EnzeXing commented Jun 23, 2021

liufengyun left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnzeXing commented Jul 21, 2021

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

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

EnzeXing commented Jul 22, 2021

liufengyun left a 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 Jul 28, 2021

EnzeXing commented Jul 30, 2021

liufengyun left a comment

Choose a reason for hiding this comment

EnzeXing commented Jun 17, 2021 •

edited by liufengyun

Loading