Fix #12128: Add test #12592

liufengyun · 2021-05-25T10:54:00Z

Fix #12128: Add test

When we are reading a new definition from tasty and setting info of
a symbol in readNewDef:

      sym.info = methodType(paramss, resType)

The line above will reach TreeChecker.transformSym:

      assert(symd.signature == initial.signature

At the time, the symd is not yet completed. Getting signature of the
denotation will force the symbol and trigger completion of the info,
which reaches the PositionTree in readIndexedDef.

compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala

compiler/src/dotty/tools/dotc/transform/TreeChecker.scala

griggt · 2021-05-31T06:06:27Z

tests/init/crash/i12128/Reflect_1.scala

@@ -0,0 +1,30 @@
+trait Context: // Dummy scala.reflect.macros.Context


This regression test using the dummy scala.reflect.macros.Context for some reason does not trigger the issue reported in #12128. It passes when compiled with 3.0.0 (aside from the cyclic reference while unpickling if -Ycheck is specified).

So, I have manually verified that the issue is fixed, but the test passes even without the new init checker from #12495 in place. 😕

It's the end of my day here, but I can probably find time to investigate this tomorrow if needed.

Edit: I notice you had the same observation: #12128 (comment)

For 3.0.0-RC2, the dependence on scala-reflect seems to be essential. If that's replaced with a dummy definition, things work fine

This regression test using the dummy scala.reflect.macros.Context for some reason does not trigger the issue reported in #12128.
So, I have manually verified that the issue is fixed

Thanks for checking that. I intended to add the original example as a test, but as it depends on scala-reflect, which makes it difficult to do so --- creating a dummy Context seems to be the closest thing that can be done.

I have some (mostly complete) changes that I believe will make the test case work properly without the dependency on scala-reflect. I'll finish and push them in the morning.

Considering the regression test consists of three components:

(a) Reflect_1 (as a stand-in for scala-reflect)
(b) Macro_2 (as a stand-in for munit)
(c) Test_3 (user code)

the key to reproduce the issue in #12128 is to compile these separately, with

the output from (a) on the classpath while compiling (b)

the output from (b) on the classpath while compiling (c)

the output from (a) not on the classpath while compiling (c)

In the real-world case of #12128, this situation occurs because MUnit has a Provided dependency on scala-reflect:
https://github.com/scalameta/munit/blob/d592d71f8360f1eaae2f19c1d582b75fe5d44714/build.sbt#L178-L182

I have rebased (to resolve conflicts) and pushed a commit which modifies the testing to meet the above requirements.

Incidentally, #12128 seems to have been fixed both by #12484 (for the old init checker) and #12495 (the new init checker).

griggt · 2021-06-01T05:44:54Z

compiler/src/dotty/tools/dotc/transform/TreeChecker.scala

@@ -91,7 +91,7 @@ class TreeChecker extends Phase with SymTransformer {
    // until erasure, see the comment above `Compiler#phases`.
    if (ctx.phaseId <= erasurePhase.id) {
      val initial = symd.initial
-      assert(symd.signature == initial.signature,
+      assert(symd == initial || symd.signature == initial.signature,


Suggested change

assert(symd == initial || symd.signature == initial.signature,

assert((symd eq initial) || symd.signature == initial.signature,

Nitpick: is reference equality more appropriate here?

Yes, using eq is a good idea, semantically they are the same for Dotty. BTW, we usually use backquote for non-symbolic infix operators.

griggt · 2021-06-01T06:03:34Z

tests/init/crash/i12128/Reflect_1.scala

@@ -0,0 +1,30 @@
+trait Context: // Dummy scala.reflect.macros.Context


I have some (mostly complete) changes that I believe will make the test case work properly without the dependency on scala-reflect. I'll finish and push them in the morning.

When we are reading a new definition from tasty and setting `info` of a symbol in `readNewDef`: sym.info = methodType(paramss, resType) The line above will reach `TreeChecker.transformSym`: assert(symd.signature == initial.signature At the time, the `symd` is not yet completed. Getting signature of the denotation will force the symbol and trigger completion of the info, which reaches the `PositionTree` in `readIndexedDef.

To properly reproduce scala#12128, the regression test has some atypical classpath requirements in addition to separate compilation.

liufengyun

Thank you @griggt , LGTM.

I'm wondering if we should keep a copy of the test with normal compilation, as it tests a different stack trace. Or, it has the same stack trace as if compiled normally?

liufengyun · 2021-06-02T06:28:20Z

I'm wondering if we should keep a copy of the test with normal compilation, as it tests a different stack trace. Or, it has the same stack trace as if compiled normally?

Just checked, the modified test still has the same stack trace, so we are good.

griggt · 2021-06-02T06:28:58Z

I'm wondering if we should keep a copy of the test with normal compilation, as it tests a different stack trace. Or, it has the same stack trace as if compiled normally?

The revised test I pushed (with separate classpaths) gives the following stack trace on 3.0.0:

java.lang.AssertionError: assertion failed: Cyclic reference while unpickling definition at address 188 in unit tests/init/special/i12128/Test_3.scala while compiling tests/init/special/i12128/Test_3.scala
Fatal compiler crash when compiling: tests/init/special/i12128/Test_3.scala:
assertion failed: Cyclic reference while unpickling definition at address 188 in unit tests/init/special/i12128/Test_3.scala
scala.runtime.Scala3RunTime$.assertFailed(Scala3RunTime.scala:8)
dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedDef(TreeUnpickler.scala:771)
dotty.tools.dotc.core.tasty.TreeUnpickler$Completer.complete(TreeUnpickler.scala:122)
dotty.tools.dotc.core.SymDenotations$SymDenotation.completeFrom(SymDenotations.scala:167)
dotty.tools.dotc.core.Denotations$Denotation.completeInfo$1(Denotations.scala:188)
dotty.tools.dotc.core.Denotations$Denotation.info(Denotations.scala:190)
dotty.tools.dotc.core.Denotations$SingleDenotation.signature(Denotations.scala:610)
dotty.tools.dotc.core.Denotations$SingleDenotation.signature(Denotations.scala:602)
dotty.tools.dotc.transform.TreeChecker.transformSym(TreeChecker.scala:94)
    ....

and this stack trace on 3.0.0 with the TreeChecker fix from this PR:

cannot resolve reference to type c.type.Tree
the classfile defining the type might be missing from the classpath while compiling tests/init/special/i12128/Test_3.scala
Fatal compiler crash when compiling: tests/init/special/i12128/Test_3.scala:
dotty.tools.dotc.core.TypeErasure.dotty$tools$dotc$core$TypeErasure$$sigName(TypeErasure.scala:776)
dotty.tools.dotc.core.TypeErasure$.sigName(TypeErasure.scala:204)
dotty.tools.dotc.core.Signature$.apply(Signature.scala:169)
dotty.tools.dotc.core.Types$MethodOrPoly.computeSignature$1(Types.scala:3394)
dotty.tools.dotc.core.Types$MethodOrPoly.signature(Types.scala:3415)
dotty.tools.dotc.core.Denotations$SingleDenotation.signature(Denotations.scala:612)
dotty.tools.dotc.core.Denotations$SingleDenotation.signature(Denotations.scala:602)
  ....

So I believe we are ok.

liufengyun marked this pull request as ready for review May 25, 2021 12:58

liufengyun requested a review from griggt May 26, 2021 08:11

griggt suggested changes May 27, 2021

View reviewed changes

compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala Outdated Show resolved Hide resolved

liufengyun force-pushed the fix-12128 branch from 9048f0f to c259f4d Compare May 27, 2021 21:34

griggt reviewed May 31, 2021

View reviewed changes

liufengyun force-pushed the fix-12128 branch from c259f4d to 83d1883 Compare May 31, 2021 08:08

griggt reviewed Jun 1, 2021

View reviewed changes

liufengyun and others added 3 commits June 1, 2021 15:13

Fix scala#12128: Add test

fc037bf

Fix regression test for scala#12128

7a0ca56

To properly reproduce scala#12128, the regression test has some atypical classpath requirements in addition to separate compilation.

griggt force-pushed the fix-12128 branch from ba65823 to 7a0ca56 Compare June 1, 2021 22:14

liufengyun commented Jun 2, 2021

View reviewed changes

griggt approved these changes Jun 2, 2021

View reviewed changes

liufengyun merged commit 833f5b9 into scala:master Jun 2, 2021

liufengyun deleted the fix-12128 branch June 2, 2021 06:31

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

Fix #12128: Add test #12592

Fix #12128: Add test #12592

liufengyun commented May 25, 2021 •

edited

Loading

griggt May 31, 2021 •

edited

Loading

liufengyun May 31, 2021

griggt Jun 1, 2021

griggt Jun 1, 2021

griggt Jun 1, 2021

liufengyun Jun 1, 2021

griggt Jun 1, 2021

liufengyun left a comment

liufengyun commented Jun 2, 2021

griggt commented Jun 2, 2021

		@@ -0,0 +1,30 @@
		trait Context: // Dummy scala.reflect.macros.Context

	assert(symd == initial \|\| symd.signature == initial.signature,
	assert((symd eq initial) \|\| symd.signature == initial.signature,

Fix #12128: Add test #12592

Fix #12128: Add test #12592

Conversation

liufengyun commented May 25, 2021 • edited Loading

griggt May 31, 2021 • edited Loading

Choose a reason for hiding this comment

liufengyun May 31, 2021

Choose a reason for hiding this comment

griggt Jun 1, 2021

Choose a reason for hiding this comment

griggt Jun 1, 2021

Choose a reason for hiding this comment

griggt Jun 1, 2021

Choose a reason for hiding this comment

liufengyun Jun 1, 2021

Choose a reason for hiding this comment

griggt Jun 1, 2021

Choose a reason for hiding this comment

liufengyun left a comment

Choose a reason for hiding this comment

liufengyun commented Jun 2, 2021

griggt commented Jun 2, 2021

liufengyun commented May 25, 2021 •

edited

Loading

griggt May 31, 2021 •

edited

Loading