Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CyclicReference exception, thrown only during incremental compilation #16673

Closed
marcinzh opened this issue Jan 12, 2023 · 4 comments · Fixed by #16688
Closed

CyclicReference exception, thrown only during incremental compilation #16673

marcinzh opened this issue Jan 12, 2023 · 4 comments · Fixed by #16688

Comments

@marcinzh
Copy link

Compiler version

3.2.1
3.2.2-RC2

Minimized code

Important: We need 2 separete files, or else the error won't happen.

Foo.scala:

final class Foo(_that: Foo) extends FooStub(_that)

FooStub.scala:

abstract class FooStub(val that: Foo):
  val bar = 1337;

Output

Clean compile succeeds.

To trigger the error, do the following:

  1. Use ~compile in sbt
  2. Make a minor change to FooStub source: add or remove 1 digit from bar
[error] -- Error: /myproject/FooStub.scala:2:27 
[error] 2 |abstract class FooStub(val that: Foo):
[error]   |                           ^
[error]   |Could not read definition of class Foo in /myproject/target/scala-3.2.2-RC2/classes/Foo.class
[error]   |An exception was encountered:
[error]   |  dotty.tools.dotc.core.CyclicReference: 
[error]   |Run with -Ydebug-unpickling to see full stack trace.

Again, clean recompile succeeds. Until another minor change of source.

After adding suggested -Ydebug-unpickling and -explain options, got this:

[error] -- [E046] Cyclic Error: /myproject/FooStub.scala:3:27 
[error] 3 |abstract class FooStub(val that: Foo):
[error]   |                           ^
[error]   |                           Cyclic reference involving method <init>
[error]   |-----------------------------------------------------------------------------
[error]   | Explanation (enabled by `-explain`)
[error]   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]   | constructor FooStub is declared as part of a cycle which makes it impossible for the
[error]   | compiler to decide upon <init>'s type.
[error]   | To avoid this error, try giving <init> an explicit type.
[error]    -----------------------------------------------------------------------------
[error] Explanation
[error] ===========
[error] constructor FooStub is declared as part of a cycle which makes it impossible for the
[error] compiler to decide upon <init>'s type.
[error] To avoid this error, try giving <init> an explicit type.

Also, this thing showed up once:

OpenJDK 64-Bit Server VM warning: CodeCache is full. Compiler has been disabled.
OpenJDK 64-Bit Server VM warning: Try increasing the code cache size using -XX:ReservedCodeCacheSize=
CodeCache: size=131072Kb used=86902Kb max_used=127348Kb free=44169Kb
 bounds [0x00007f8e00000000, 0x00007f8e08000000, 0x00007f8e08000000]
 total_blobs=21123 nmethods=20197 adapters=828
 compilation: enabled
@marcinzh marcinzh added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Jan 12, 2023
@Kordyjan Kordyjan added area:incremental-compilation and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jan 12, 2023
@Kordyjan Kordyjan added this to the Future versions milestone Jan 12, 2023
@odersky
Copy link
Contributor

odersky commented Jan 12, 2023

A way to reproduce directly:

  1. Compile Foo.scala and FooStub.scala together
  2. Compile just FooStub.scala

@odersky
Copy link
Contributor

odersky commented Jan 12, 2023

This is a fairly serious problem. It's because we read the complete parent constructors when unpickling. By contrast, when type-checking sources, we skip the parent constructor arguments in Namer as long as they are not needed to determine the type of the result of a constructor.

@odersky
Copy link
Contributor

odersky commented Jan 12, 2023

I think this might be a source of instability in other incremental compilation scenarios as well.

@odersky odersky self-assigned this Jan 12, 2023
odersky added a commit to dotty-staging/dotty that referenced this issue Jan 13, 2023
Avoid reading the arguments of parent constructors unless someone forces them.
We don't need them to determine the parent types of the class to which the
template belongs. This makes TreeUnpickler as lazy as Namer in this respect
and therefore avoids CyclicReferences during unpickling.

Fixes scala#16673
odersky added a commit to dotty-staging/dotty that referenced this issue Jan 13, 2023
Avoid reading the arguments of parent constructors unless someone forces them.
We don't need them to determine the parent types of the class to which the
template belongs. This makes TreeUnpickler as lazy as Namer in this respect
and therefore avoids CyclicReferences during unpickling.

Fixes scala#16673
@odersky
Copy link
Contributor

odersky commented Jan 14, 2023

@marcinzh Thanks a lot for minimizing the code to illustrate the bug so concisely!

odersky added a commit that referenced this issue Jan 16, 2023
Avoid reading the arguments of parent constructors unless someone forces
them. We don't need them to determine the parent types of the class to
which the template belongs. This makes TreeUnpickler as lazy as Namer in
this respect and therefore avoids CyclicReferences during unpickling.

Fixes #16673
little-inferno pushed a commit to little-inferno/dotty that referenced this issue Jan 25, 2023
Avoid reading the arguments of parent constructors unless someone forces them.
We don't need them to determine the parent types of the class to which the
template belongs. This makes TreeUnpickler as lazy as Namer in this respect
and therefore avoids CyclicReferences during unpickling.

Fixes scala#16673
@Kordyjan Kordyjan modified the milestones: 3.3.0 backports, 3.3.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants