Skip to content

Fix #2888: Avoid caching supertypes of hk apps with provisional infos #2889

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 3 commits into from
Jul 19, 2017

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jul 19, 2017

If the type constructor of a hk application has a provisional info (because
we are starting to initialize an F-bounded definition), we cannot cache its
supertype. We get an empty bound if we do.

…isional infos

If the type constructor of a hk application has a provisional info (because
we are starting to initialize an F-bounded definition), we cannot cache its
supertype. We get an empty bound if we do.
@odersky odersky requested a review from smarter July 19, 2017 14:26
@@ -3012,12 +3012,16 @@ object Types {

override def superType(implicit ctx: Context): Type = {
if (ctx.period != validSuper) {
var canCache = true
Copy link
Member

Choose a reason for hiding this comment

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

This variable appears to be set but never read.

@smarter
Copy link
Member

smarter commented Jul 19, 2017

Also this is missing the testcase from #2888

@@ -3012,15 +3012,19 @@ object Types {

override def superType(implicit ctx: Context): Type = {
if (ctx.period != validSuper) {
var canCache = true
cachedSuper = tycon match {
Copy link
Member

Choose a reason for hiding this comment

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

Modifying cachedSuper when we cannot cache the value seems dangerous. If validSuper refers to a non-empty period, then a subsequent call to superType might be done with ctx.period equal to validSuper and would return the new, incorrect value in cachedSuper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough

@odersky odersky merged commit a33258e into scala:master Jul 19, 2017
@allanrenucci allanrenucci deleted the fix#-2888 branch December 14, 2017 16:59
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.

2 participants