Refine error messages for early promotion #15263

liufengyun · 2022-05-22T23:18:57Z

Stop early promotion on first error
Refine error messages for early promotion

liufengyun · 2022-05-23T00:11:09Z

The failure comes from the tree checker, due to a limitation in subtyping checker:

dotty.tools.dotc.transform.init.Semantic.Hot.type 
| (dotty.tools.dotc.transform.init.Semantic.Hot.type 
|  dotty.tools.dotc.transform.init.Semantic.Cold.type )  

<:  

dotty.tools.dotc.transform.init.Semantic.Hot.type 
| dotty.tools.dotc.transform.init.Semantic.Cold.type 

= false

The failure comes from the tree checker, due to a limitation in subtyping checker: ``` dotty.tools.dotc.transform.init.Semantic.Hot.type | (dotty.tools.dotc.transform.init.Semantic.Hot.type | dotty.tools.dotc.transform.init.Semantic.Cold.type ) <: dotty.tools.dotc.transform.init.Semantic.Hot.type | dotty.tools.dotc.transform.init.Semantic.Cold.type = false ```

olhotak · 2022-05-23T11:04:17Z

tests/init/neg/closureLeak.check

-   |
-   |             The unsafe promotion may cause the following problem:
-   |             Cannot prove that the value is fully initialized. Only initialized values may be used as arguments.
+   |              Cannot prove the argument is fully initialized.


I still find these messages difficult to understand. Here's how I would explain this particular failure to someone. (We can think about making the error messages closer to such an explanation.)

The arguments of a method call in a constructor must be fully initialized. I could not prove that the underlined argument is fully initialized.

For a closure to be considered fully initialized, its body must access only fully initialized variables. I could not prove that the underlined access is fully initialized.

One general distinction I see is between "X must be fully initialized because that is the rule (e.g. argument to a method call in a constructor)" vs. "X must be fully initialized in order to prove that Y is fully initialized (e.g. expression in body of closure)".

olhotak · 2022-05-23T11:16:38Z

tests/init/neg/inherit-non-hot.check

-  |                           ^
-  |                        -> val bAgain = toB.getBAgain	[ inherit-non-hot.scala:16 ]
-  |                                        ^^^
+  |                                  May only assign fully initialized value. Calling trace:


The RHS of an assignment to a field that may be called from a constructor must be fully initialized.

This assignment may be called from a constructor because:
2a. The constructor of object Foo calls new C (line 19)
(It seems line 15 is not really relevant to the explanation.)
2b. The constructor of C calls A.toB (line 16)
2c. A.toB contains the assignment to the field.

I could not prove that the assigned value is fully initialized for the following reason.
3a. For the result of a constructor call to be fully initialized, all arguments of the call must be fully initialized. I could not prove that the argument this for the parameter a is fully initialized.
3b. I could not prove that this is fully initialized because the initializer of its field c has not yet run.

olhotak · 2022-05-23T11:24:18Z

tests/init/neg/local-warm4.check

-   |          ^^^^^^^^^^^
-   |       -> updateA()	[ local-warm4.scala:21 ]
-   |          ^^^^^^^^^
+   |                    May only assign fully initialized value. Calling trace:


Again, I think there are two issues here:

Why is line 18 called from a constructor? (Maybe say from the constructor of which class, in this case localWarm, through a stack of several other calls.)

Why is newA not considered fully initialized? (In this case, because it comes from new A(y), and y is not fully initialized. Why is not fully initialized? Because the tree new A(y) executes before the initializer of A.y runs. Why before? Because the constructor of B, which is a subclass of class A that contains the initializer of y, calls increment (line 23), which calls updateA (line 21), which contains the tree new A(y) (line 17). )

olhotak · 2022-05-23T11:28:44Z

tests/init/neg/promotion-loop.check

-   |
-   |          The unsafe promotion may cause the following problem:
-   |          Cannot prove that the value is fully initialized. Only initialized values may be used as arguments.
+   |          Cannot prove the argument is fully initialized.


The arguments of a method call in a constructor must be fully initialized. I could not prove that the underlined argument is fully initialized.

The initializer of b instantiates class B.

The constructor of B references test, the self parameter of class Test.

The this of class Test is not yet initialized because the initializer of its field n has not yet run.

olhotak · 2022-05-24T07:51:07Z

While interpreting other error messages from the initialization checker, it occurred to me that it would be useful for each error message to start with a statement of which class (constructor) is being checker. The error message starts with the location where the initialization checker failed to prove that a class is safely initialized, but often this location is far removed from the class that is not safely initialized.

liufengyun · 2022-05-24T23:01:24Z

While interpreting other error messages from the initialization checker, it occurred to me that it would be useful for each error message to start with a statement of which class (constructor) is being checker. The error message starts with the location where the initialization checker failed to prove that a class is safely initialized, but often this location is far removed from the class that is not safely initialized.

This is a good improvement. Now we make sure that the top of the trace is always the class under check. The tracing and error reporting mechanism is overhauled and the low-hanging fruits are dealt with.

For other suggestions, we need to discuss how to tackle them. As this PR mainly focuses on overhauling the infrastructure of error reporting, can we address the remaining concerns in other PRs?

olhotak

My comments here are minor. After you consider them, this can be merged.

olhotak · 2022-05-25T08:23:43Z

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

+              if allArgsPromote then
+                Hot: Value
+              else if errors.nonEmpty then
+                for error <- errors do reporter.report(error)


Could be factored into a method (reportAll) in Reporter.

olhotak · 2022-05-25T08:23:56Z

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

+                Hot: Value
+              else if errors.nonEmpty then
+                for error <- errors do reporter.report(error)
+                Hot: Value


Why is the ascription needed?

This is due to a TreeChecker failure:

#15263 (comment)

I have no idea why the subtyping failed --- it shouldn't be.

olhotak · 2022-05-25T08:24:18Z

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

              val cls = target.owner.enclosingClass.asClass
              val ddef = target.defTree.asInstanceOf[DefDef]
-              val argErrors = checkArgs
+              val argErrors = Reporter.errorsIn { args.foreach(_.promote) }


Why not Reporter.errorsIn { checkArgs } ?

olhotak · 2022-05-25T08:26:44Z

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

-    def call(meth: Symbol, args: List[ArgInfo], receiver: Type, superType: Type, source: Tree, needResolve: Boolean = true): Contextual[Result] = log("call " + meth.show + ", args = " + args, printer, (_: Result).show) {
-      def checkArgs = args.flatMap(_.promote)
+    def call(meth: Symbol, args: List[ArgInfo], receiver: Type, superType: Type, needResolve: Boolean = true): Contextual[Value] = log("call " + meth.show + ", args = " + args, printer, (_: Value).show) {
+      def checkArgs = args.foreach(_.promote)


Consider renaming to promoteArgs. Also consider parentheses promoteArgs() to indicate side effect and Unit return type. But many other methods here have side effects, so maybe not.

olhotak · 2022-05-25T08:41:24Z

While interpreting other error messages from the initialization checker, it occurred to me that it would be useful for each error message to start with a statement of which class (constructor) is being checker. The error message starts with the location where the initialization checker failed to prove that a class is safely initialized, but often this location is far removed from the class that is not safely initialized.

This is a good improvement. Now we make sure that the top of the trace is always the class under check. The tracing and error reporting mechanism is overhauled and the low-hanging fruits are dealt with.

For other suggestions, we need to discuss how to tackle them. As this PR mainly focuses on overhauling the infrastructure of error reporting, can we address the remaining concerns in other PRs?

I agree that this PR is already a good improvement - the call traces are more informative. The class being checked at the top of the trace really helps.

I also agree with a separate discussion and separate PR for further improvements.

liufengyun added 3 commits May 23, 2022 02:16

Don't chain promotion

4a7e420

Refactor error reporting to stop on first error

9a9db23

Refine marker positions

c1b83f6

liufengyun force-pushed the restrict-early-promote branch from b9d3145 to cbdda5f Compare May 23, 2022 00:20

liufengyun force-pushed the restrict-early-promote branch from cbdda5f to efb9776 Compare May 23, 2022 05:50

Clean up

a98076f

liufengyun marked this pull request as ready for review May 23, 2022 07:14

liufengyun requested a review from olhotak May 23, 2022 07:15

olhotak reviewed May 23, 2022

View reviewed changes

nicolasstucki assigned olhotak May 23, 2022

liufengyun added 3 commits May 24, 2022 11:37

Organize code

c1595c5

Improve error message

43553ea

Always show current class at the top of trace

a4c78a4

liufengyun added 2 commits May 25, 2022 08:36

Fix missing stacktrace item

73e64a7

Tweak message: make clear it is reassignment

7c12df0

olhotak approved these changes May 25, 2022

View reviewed changes

Address review comments

ce3c803

liufengyun enabled auto-merge May 25, 2022 20:34

liufengyun merged commit 4518be0 into main May 25, 2022

liufengyun deleted the restrict-early-promote branch May 25, 2022 22:14

Kordyjan added this to the 3.2.0 milestone Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine error messages for early promotion #15263

Refine error messages for early promotion #15263

liufengyun commented May 22, 2022

liufengyun commented May 23, 2022

olhotak May 23, 2022

olhotak May 23, 2022

olhotak May 23, 2022

olhotak May 23, 2022

olhotak commented May 24, 2022

liufengyun commented May 24, 2022

olhotak left a comment

olhotak May 25, 2022

olhotak May 25, 2022

liufengyun May 25, 2022

olhotak May 25, 2022

olhotak May 25, 2022

olhotak commented May 25, 2022

Refine error messages for early promotion #15263

Refine error messages for early promotion #15263

Conversation

liufengyun commented May 22, 2022

liufengyun commented May 23, 2022

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

olhotak commented May 24, 2022

liufengyun commented May 24, 2022

olhotak 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

olhotak commented May 25, 2022