Skip to content

Fix #5333: Invalid entries in the baseType cache #5343

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 2 commits into from
Oct 31, 2018

Conversation

smarter
Copy link
Member

@smarter smarter commented Oct 30, 2018

Provisional types should not have their baseType cached, it may change
as type variables get refined and instantiated.

The seqtype-cycle test had to be adjusted to avoid some missing
reference errors, probably due to the completion order changing.

Provisional types should not have their baseType cached, it may change
as type variables get refined and instantiated.

The seqtype-cycle test had to be adjusted to avoid some missing
reference errors, probably due to the completion order changing.
@dottybot
Copy link
Member

Performance test finished successfully:

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

Benchmarks is based on merging with master (0fcbfdb)

@scala scala deleted a comment from dottybot Oct 30, 2018
@scala scala deleted a comment from dottybot Oct 30, 2018
@scala scala deleted a comment from dottybot Oct 30, 2018
@smarter
Copy link
Member Author

smarter commented Oct 30, 2018

@odersky This can be reviewed now, but there's one thing I haven't fully figured out, in:
https://github.com/lampepfl/dotty/blob/7ada2642d53b745b6efbe50aa37bae13d18ccbce/compiler/src/dotty/tools/dotc/core/SymDenotations.scala#L1688-L1689

I assume that the "typeref cannot be a GADT" comment is based on the fact that we check tp.symbol.maybeOwner.isType and gadts on class parameters are not supported currently. However that check alone seems to be insufficient: if caching the base type of a GADT is bad, then caching any type which refers to a GADT could also be bad.
On the other hand, as far as I can tell, the state of the GADT constraints does not influence the result of baseType, therefore it seems that we're safe anyway (and we could potentially cache typerefs even if they come from methods, although that may bloat the cache).

@odersky
Copy link
Contributor

odersky commented Oct 31, 2018

On the other hand, as far as I can tell, the state of the GADT constraints does not influence the result of baseType, therefore it seems that we're safe anyway (and we could potentially cache typerefs even if they come from methods, although that may bloat the cache).

I think that's correct. So we should probably remove the comment since it is irrelevant.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Looks good. Very good analysis, also.

@smarter
Copy link
Member Author

smarter commented Oct 31, 2018

test performance please

@dottybot
Copy link
Member

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

@dottybot
Copy link
Member

Performance test finished successfully:

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

Benchmarks is based on merging with master (0fcbfdb)

@odersky odersky merged commit 6ccdec3 into scala:master Oct 31, 2018
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