Refactoring tryPromote to fix non-determinism in Issue #12824 #12989

EnzeXing · 2021-06-30T23:16:37Z

This PR first tests if the non-determinism will occur in [test_windows_full], and then refactors tryPromote and try to fix it

liufengyun

Thank you @EnzeXing . LGTM, I left some comments for refactoring.

liufengyun · 2021-07-02T22:09:46Z

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

-    def flatten: Errors = this match {
-      case unsafe: UnsafePromotion => unsafe.errors.flatMap(_.flatten)
-      case _ => this :: Nil
-    }


We may still need this, in case the nested error is another UnsafePromotion. But that's OK as well, so let's remove it for now.

liufengyun · 2021-07-02T22:10:43Z

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

@@ -89,20 +82,16 @@ object Errors {
  }

  /** Promote a value under initialization to fully-initialized */
-  case class UnsafePromotion(msg: String, source: Tree, trace: Seq[Tree], errors: Errors) extends Error {
-    assert(errors.nonEmpty)
+  case class UnsafePromotion(msg: String, source: Tree, trace: Seq[Tree], error: Error) extends Error {

    override def issue(using Context): Unit =
      report.warning(show, source.srcPos)

    def show(using Context): String = {
      var index = 0
      "Promoting the value to fully-initialized is unsafe. " + msg + "\n" + stacktrace +


Let's change the sentence to: Cannot prove that the value is fully-initialized.

liufengyun · 2021-07-02T22:14:02Z

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

@@ -89,20 +82,16 @@ object Errors {
  }

  /** Promote a value under initialization to fully-initialized */
-  case class UnsafePromotion(msg: String, source: Tree, trace: Seq[Tree], errors: Errors) extends Error {
-    assert(errors.nonEmpty)
+  case class UnsafePromotion(msg: String, source: Tree, trace: Seq[Tree], error: Error) extends Error {


I see the call site we have many errors -- so maybe keep the original errors: Errors is OK, but only show the first one.

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

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

+              locally {
+                given Trace = trace2
+                buffer ++= res.ensureHot(msg, source).errors
+              }
          }


Minor: remove the braces after then and align if and else vertically

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

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

-            buffer ++= res.ensureHot(msg, source).errors
+      warm.klass.baseClasses.exists { klass =>
+        klass.hasSource && klass.info.decls.exists { member =>
+          if member.isOneOf(Flags.Method | Flags.Lazy | Flags.Deferred) then {


Remove | Flags.Deferred: it means the member is only declared but not defined.

I'd write:

if !member.isType && !member.isConstructor && member.hasSource && !member.is(Flags.Deferred) then if member.isOneOf(Flags.Method | Flags.Lazy) then ... else ... buffer.nonEmpty

Why did we not have the !member.isType condition for methods before? Is that just because member.isType is always false with Flags.Method?

Yes, that's true.

EnzeXing · 2021-07-12T21:08:08Z

@liufengyun I have resolved the failed test. Shall you review it again and see if we can get it merged?

liufengyun · 2021-07-13T06:05:46Z

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

    /** Flatten UnsafePromotion errors
-     */
+      */


The trailing spaces need to be removed.

liufengyun · 2021-07-13T06:06:21Z

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

@@ -90,19 +90,15 @@ object Errors {

  /** Promote a value under initialization to fully-initialized */
  case class UnsafePromotion(msg: String, source: Tree, trace: Seq[Tree], errors: Errors) extends Error {
-    assert(errors.nonEmpty)


It's better to keep this line.

liufengyun · 2021-07-13T06:07:25Z

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

+      warm.klass.baseClasses.exists { klass =>
+        klass.hasSource && klass.info.decls.exists { member =>
+          if !member.isType && !member.isConstructor && member.hasSource  && !member.is(Flags.Deferred) then
+            if member.isOneOf(Flags.Method | Flags.Lazy) then


Can we try the following change?

Suggested change

if member.isOneOf(Flags.Method | Flags.Lazy) then

if member.is(Flags.Method) then

liufengyun · 2021-07-13T06:14:45Z

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

+              locally {
+                given Trace = trace2
+                val args = member.info.paramInfoss.flatten.map(_ => ArgInfo(Hot, EmptyTree))
+                val res = warm.call(member, args, superType = NoType, source = source)


Should we put source in trace2 and use member.defTree here? Or simply remove trace2 if it does not make much difference.

Do you mean evaluating the rhs of defTree directly here? I checked that warm.call only does one more step: resolve(addr.klass, meth). Is that necessary?

Sorry I was not clear. I meant only the trace: trace2 and source = source.

You're right. I'll just keep the trace here so that the calling trace is more clear.

liufengyun · 2021-07-13T06:17:15Z

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

+              locally {
+                given Trace = trace2
+                buffer ++= res.ensureHot(msg, source).errors
+              }


It seems we can safely remove trace2 in this case.

liufengyun · 2021-07-13T06:17:40Z

tests/init/neg/closureLeak.check

@@ -1,8 +1,7 @@
 -- Error: tests/init/neg/closureLeak.scala:11:14 -----------------------------------------------------------------------
 11 |    l.foreach(a => a.addX(this)) // error
   |              ^^^^^^^^^^^^^^^^^
-   |    Promoting the value to fully-initialized is unsafe. May only use initialized value as arguments
+   |       Cannot prove that the value is fully-initialized. May only use initialized value as arguments


Please add a period after the line.

liufengyun · 2021-07-13T06:18:37Z

tests/init/neg/early-promote5.scala

@@ -1,5 +1,5 @@
 abstract class A {
-  bar(this)
+  bar(this) // error


Why we don't have an error before?

Previously, the fields in tryPromote doesn't include c in class M(val o: Outer, c: Container) extends A with o.B, so promoting val m = new M(o, this) doesn't trigger the error

liufengyun · 2021-07-13T06:19:32Z

tests/init/neg/secondary-ctor2.scala

@@ -6,7 +6,7 @@ class A(b: B, x: Int) {
    }
    Inner().foo()

-    val f = () => new A(b, 3)
+    val f = () => new A(b, 3) // error: Cannot promote


Why it's fine before?

This is similar to early-promote5.scala. Previously fields doesn't include the parameters in the constructor of A.

liufengyun

LGTM, there are two minor issues to be addressed:

remove the trailing space
clean up the history (e.g. the two commits related to PromotionError can be squashed into one commit)

liufengyun · 2021-07-14T08:03:51Z

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

@@ -48,7 +48,7 @@ object Errors {
      }
      sb.toString
    }
-
+    


The trailing space needs to be removed.

Updating the error message of UnsafePromotion

Updating secondary-ctr2.scala

Remove space

EnzeXing · 2021-07-14T21:32:56Z

@liufengyun I've finished.

EnzeXing added 2 commits June 30, 2021 19:11

Adding checkfiles for enum

870977a

Refactoring tryPromote

ffa0a48

liufengyun reviewed Jul 2, 2021

View reviewed changes

anatoliykmetyuk assigned EnzeXing Jul 6, 2021

liufengyun reviewed Jul 13, 2021

View reviewed changes

liufengyun reviewed Jul 14, 2021

View reviewed changes

EnzeXing added 3 commits July 14, 2021 17:28

Changing error message in UnsafePromotion

1fc771a

Updating the error message of UnsafePromotion

Updating tests

5e46041

Updating secondary-ctr2.scala

Updating calling trace.

1f26b8d

Remove space

EnzeXing force-pushed the refactor-tryPromote branch from f56fba8 to 1f26b8d Compare July 14, 2021 21:31

liufengyun approved these changes Jul 15, 2021

View reviewed changes

liufengyun merged commit 27e221b into scala:master Jul 15, 2021

liufengyun mentioned this pull request Jul 15, 2021

non determinism for Ysafe-init external method trace #12824

Closed

Kordyjan added this to the 3.1.0 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring tryPromote to fix non-determinism in Issue #12824 #12989

Refactoring tryPromote to fix non-determinism in Issue #12824 #12989

EnzeXing commented Jun 30, 2021

liufengyun left a comment

liufengyun Jul 2, 2021

liufengyun Jul 2, 2021

liufengyun Jul 2, 2021

liufengyun Jul 2, 2021

liufengyun Jul 2, 2021

olhotak Jul 5, 2021

liufengyun Jul 5, 2021

EnzeXing commented Jul 12, 2021

liufengyun Jul 13, 2021

liufengyun Jul 13, 2021

liufengyun Jul 13, 2021

liufengyun Jul 13, 2021

EnzeXing Jul 13, 2021

liufengyun Jul 13, 2021

EnzeXing Jul 13, 2021

liufengyun Jul 13, 2021

liufengyun Jul 13, 2021

liufengyun Jul 13, 2021

EnzeXing Jul 13, 2021

liufengyun Jul 13, 2021

EnzeXing Jul 13, 2021

liufengyun left a comment

liufengyun Jul 14, 2021

EnzeXing commented Jul 14, 2021

	if member.isOneOf(Flags.Method \| Flags.Lazy) then
	if member.is(Flags.Method) then

Refactoring tryPromote to fix non-determinism in Issue #12824 #12989

Refactoring tryPromote to fix non-determinism in Issue #12824 #12989

Conversation

EnzeXing commented Jun 30, 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

EnzeXing commented Jul 12, 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

liufengyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnzeXing commented Jul 14, 2021