Skip to content

TreeUnpickler: fix cycle involving param accessor #12844

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 18, 2021
Merged

TreeUnpickler: fix cycle involving param accessor #12844

merged 1 commit into from
Jun 18, 2021

Conversation

smarter
Copy link
Member

@smarter smarter commented Jun 16, 2021 •
edited
Loading

When unpickling a template like A in i12834.scala, the first thing we
do is to unpickle its class parameters, here that's ref. While
unpickling ref we run avoidPrivateLeaks on it which forces its info
and requires unpickling B which refers to A.<init> which leads to a
crash because we haven't entered <init> in A yet. We can avoid this
cycle by simply not running avoidPrivateLeaks on param accessors, this
should be safe since a primary constructor parameter cannot refer to a
type member of the class.

Fixes #12834.

I've nominated this PR for backporting to 3.0.1 even though it's not a regression from 3.0.0 since having a broken publishLocal is a pretty big issue.

@smarter smarter requested a review from bishabosha June 16, 2021 10:25
@smarter smarter added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jun 16, 2021
@smarter smarter force-pushed the unpickle-param-cycle branch from 6703730 to 712d53b Compare June 16, 2021 10:26
@smarter
TreeUnpickler: fix cycle involving param accessor
712d53b 
When unpickling a template like `A` in i12834.scala, the first thing we
do is to unpickle its class parameters, here that's `ref`. While
unpickling `ref` we run `avoidPrivateLeaks` on it which forces its info
and requires unpickling `B` which refers to `A.<init>` which leads to a
crash because we haven't entered `<init>` in `A` yet. We can avoid this
cycle by simply not running `avoidPrivateLeaks` on param accessors, this
should be safe since a primary constructor parameter cannot refer to a
type member of the class.

Fixes scala#12834.
@bishabosha
Copy link
Member

bishabosha commented Jun 18, 2021 •
edited
Loading

I can now make a cyclic reference crash when compiling -from-tasty with this augmented example to i12834

class C(val d: D, val ref: Option[d.Type])
class D extends C(null, None) {
  type Type
}
Exception in thread "main" java.lang.AssertionError: assertion failed: Cyclic reference while unpickling definition at address 47 in unit tests/pos/i12834.scala
	at scala.runtime.Scala3RunTime$.assertFailed(Scala3RunTime.scala:8)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedDef(TreeUnpickler.scala:770)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$Completer.complete(TreeUnpickler.scala:122)
	at dotty.tools.dotc.core.SymDenotations$SymDenotation.completeFrom(SymDenotations.scala:167)
	at dotty.tools.dotc.core.Denotations$Denotation.completeInfo$1(Denotations.scala:188)
	at dotty.tools.dotc.core.Denotations$Denotation.info(Denotations.scala:190)
	at dotty.tools.dotc.core.Denotations$SingleDenotation.signature(Denotations.scala:610)
	at dotty.tools.dotc.core.Denotations$SingleDenotation.signature(Denotations.scala:602)
	at dotty.tools.dotc.core.Denotations$SingleDenotation.atSignature(Denotations.scala:644)
	at dotty.tools.dotc.core.Denotations$SingleDenotation.atSignature(Denotations.scala:642)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readLengthTerm$1(TreeUnpickler.scala:1223)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTerm(TreeUnpickler.scala:1297)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readLengthTerm$1(TreeUnpickler.scala:1137)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTerm(TreeUnpickler.scala:1297)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.$anonfun$4(TreeUnpickler.scala:932)
	at dotty.tools.tasty.TastyReader.collectWhile(TastyReader.scala:137)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTemplate(TreeUnpickler.scala:935)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readNewDef(TreeUnpickler.scala:857)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedDef(TreeUnpickler.scala:776)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$Completer.complete(TreeUnpickler.scala:122)
	at dotty.tools.dotc.core.SymDenotations$SymDenotation.completeFrom(SymDenotations.scala:167)
	at dotty.tools.dotc.core.Denotations$Denotation.completeInfo$1(Denotations.scala:188)
	at dotty.tools.dotc.core.Denotations$Denotation.info(Denotations.scala:190)
	at dotty.tools.dotc.core.Denotations$Denotation.completeInfo$1(Denotations.scala:188)
	at dotty.tools.dotc.core.Denotations$Denotation.info(Denotations.scala:190)
	at dotty.tools.dotc.core.SymDenotations$ClassDenotation.computeMembersNamed(SymDenotations.scala:2030)
	at dotty.tools.dotc.core.SymDenotations$ClassDenotation.membersNamed(SymDenotations.scala:2000)
	at dotty.tools.dotc.core.SymDenotations$ClassDenotation.findMember(SymDenotations.scala:2051)
	at dotty.tools.dotc.core.Types$Type.go$1(Types.scala:683)
	at dotty.tools.dotc.core.Types$Type.findMember(Types.scala:870)
	at dotty.tools.dotc.core.tasty.TreeUnpickler.dotty$tools$dotc$core$tasty$TreeUnpickler$TreeReader$$_$accessibleDenot$1(TreeUnpickler.scala:1091)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.completeSelect$1(TreeUnpickler.scala:1082)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readSimpleTerm$1(TreeUnpickler.scala:1110)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTerm(TreeUnpickler.scala:1297)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTpt(TreeUnpickler.scala:1327)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.$anonfun$27(TreeUnpickler.scala:1263)
	at dotty.tools.tasty.TastyReader.until(TastyReader.scala:125)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readLengthTerm$1(TreeUnpickler.scala:1263)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTerm(TreeUnpickler.scala:1297)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTpt(TreeUnpickler.scala:1327)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readNewDef(TreeUnpickler.scala:875)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedDef(TreeUnpickler.scala:776)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedParams$$anonfun$2(TreeUnpickler.scala:1042)
	at dotty.tools.tasty.TastyReader.collectWhile(TastyReader.scala:137)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedParams(TreeUnpickler.scala:1042)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readParams(TreeUnpickler.scala:1047)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readParamss$1(TreeUnpickler.scala:794)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readNewDef(TreeUnpickler.scala:830)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedDef(TreeUnpickler.scala:776)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTemplate(TreeUnpickler.scala:948)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readNewDef(TreeUnpickler.scala:857)
	at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedDef(TreeUnpickler.scala:776)

Edit: before this PR it would create the same error as #12834 so no compatibility will be lost here

@smarter
Copy link
Member Author

smarter commented Jun 18, 2021

Since this is a more complex and less realistic example, I suggest we open a separate issue for that. We might have to simply disallow that kind of cycle in typer since I don't see an easy way to deal with it in unpickler.

@smarter
Copy link
Member Author

smarter commented Jun 18, 2021

(though it'd be interesting to understand what happens differently in Namer for this example such that it doesn't lead to a cycle)

@bishabosha bishabosha merged commit f003711 into scala:master Jun 18, 2021
@bishabosha bishabosha deleted the unpickle-param-cycle branch June 18, 2021 10:06
@bishabosha bishabosha mentioned this pull request Jun 18, 2021
Open
@smarter
Copy link
Member Author

smarter commented Jun 18, 2021

@bishabosha If you agree this should be backported, could you switch the backport:nominated label to backport:accepted?

@bishabosha bishabosha added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Jun 18, 2021
@smarter smarter mentioned this pull request Jun 18, 2021
Merged
@dwijnand dwijnand removed the backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" label Sep 28, 2021
@Kordyjan Kordyjan added this to the 3.0.2 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bishabosha bishabosha bishabosha approved these changes

Assignees

@bishabosha bishabosha

Labels
None yet
Projects
None yet
Milestone
3.0.2
Development

Successfully merging this pull request may close these issues.

tasty error when declaring subclass field type and overriding (thrown when publishing)
4 participants
@smarter @bishabosha @dwijnand @Kordyjan