Skip to content

Fix #8152: Rearchitect some parts of opaque types handling #8170

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

Merged
merged 12 commits into from
Feb 5, 2020

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Feb 2, 2020

Opaque types work by splitting the info in an opaque type between the type itself and the enclosing class. for instance,

class C:
  opaque type A = T

is transformed to

class C:
  this: C { type A = T } =>
  opaque type A

That way, opaque types are abstract on the outside of C but aliases on the inside where the refined type info type A = T is visible.

The question is when this transform happens. Previously we rewrote the type A and the self type of the enclosing class at the time when A was completed. This coupling of rewriting the info of C when A was completed caused a "window of vulnerablity" where there could be outdated cached info. We compensated for that by handling a special case in asSeenFrom. While this seemed to work, it was not clear there were no gaps remaining.

The new scheme implemented in this PR is more principled in that it sets the full info of C and A at the time each is completed, but it needs an extra amount of laziness to do so. The program above is transformed to

class C:
  this: C { type A = LazyRef(T) } =>
  opaque type A

Omitting the LazyRef above would cause CyclicReference errors. When the LazyRef in the self type of C is forced, we complete the opaque alias A and update the reference with the typechecked RHS T.
If the completion of A happens by another path before forcing the lazy ref, the completion will update the LazyRef, thereby cancelling the pending completion.

This scheme avoids the window of vulnerability and allows us to drop the compensation code in asSeenFrom.

A separate fix addressed the problem of #8152, that given opaque bounds were not properly abstracted
with type parameters.

@odersky odersky force-pushed the fix-#8152 branch 3 times, most recently from 6acfbad to 9b61dd3 Compare February 4, 2020 15:55
@odersky
Copy link
Contributor Author

odersky commented Feb 4, 2020

test performance please

@dottybot
Copy link
Member

dottybot commented Feb 4, 2020

performance test scheduled: 7 job(s) in queue, 1 running.

@odersky odersky marked this pull request as ready for review February 4, 2020 17:02
@odersky odersky changed the title Some refactorings involvng TempInfos Fix #8152: Rearchitect some parts of opaque types handling Feb 4, 2020
@dottybot
Copy link
Member

dottybot commented Feb 4, 2020

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/8170/ to see the changes.

Benchmarks is based on merging with master (46aec62)

@@ -1049,6 +1050,10 @@ object Types {
case _ => this
}

def stripLazyRef(given Context): Type = this match
case lzy: LazyRef => lzy.ref
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe recursively call stripLazyRef on the result ? This would match the behavior of other strip... methods on Type:

Suggested change
case lzy: LazyRef => lzy.ref
case lzy: LazyRef => lzy.ref.stripLazyRef

@@ -4316,6 +4325,37 @@ object Types {
if ((prefix eq this.prefix) && (classParents eq this.classParents) && (decls eq this.decls) && (selfInfo eq this.selfInfo)) this
else newLikeThis(prefix, classParents, decls, selfInfo)

/** If this class has opqque type alias members, a new class info
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** If this class has opqque type alias members, a new class info
/** If this class has opaque type alias members, a new class info

@@ -1727,10 +1727,14 @@ class TypeComparer(initctx: Context) extends ConstraintHandling[AbsentContext] w
else if tp1.isAny && !tp2.isLambdaSub || tp1.isAnyKind || tp2.isRef(NothingClass) then tp2
else if tp2.isAny && !tp1.isLambdaSub || tp2.isAnyKind || tp1.isRef(NothingClass) then tp1
else tp2 match { // normalize to disjunctive normal form if possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use tp2.stripLazyRef instead ? (same thing below and in lub)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants