Skip to content

LambdaLift: reload non-sym denotations when needed #11303

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 1 commit into from
Feb 4, 2021

Conversation

smarter
Copy link
Member

@smarter smarter commented Feb 3, 2021

When we manually insert a SymDenotation with installAfter, we need to reload any non-sym SingleDenotation for the same symbol, since they won't automatically become stale. It would be nicer if this was done automatically (in NonSymSingleDenotation#current perhaps?), but that seems complicated.

@smarter smarter requested a review from odersky February 3, 2021 16:51
@smarter smarter force-pushed the lambdalift-transformSelect branch from 1a1257c to 63a2f00 Compare February 3, 2021 17:15
@smarter
Copy link
Member Author

smarter commented Feb 3, 2021

Actually, do we need non-sym denotations after erasure? According to the comment on top of Compiler, they shouldn't exist: https://github.com/lampepfl/dotty/blob/44931da237f47a7fda2eb0b3b7f417757679e686/compiler/src/dotty/tools/dotc/Compiler.scala#L26-L33

Should we special case SingleDenotation#current for NonSymSingleDenotations to revert to the SymDenotation instead of running denottransformers after erasure?

@smarter smarter force-pushed the lambdalift-transformSelect branch from 63a2f00 to 1781238 Compare February 3, 2021 18:35
@smarter smarter force-pushed the lambdalift-transformSelect branch from 1781238 to 694e713 Compare February 3, 2021 18:36
smarter added a commit to smarter/dotty that referenced this pull request Feb 4, 2021
This is an alternative fix for scala#11303, the downside is that when
time-travelling from post-erasure to pre-erasure, we need to recompute
the SingleDenotation. This commit passes all tests except
tests/pos/t2399.scala which fails compilation from tasty. This could
probably be fixed by changing the logic in computeDenot to only run
`memberDenot` for opaque types, for other infos we can just use
`asSeenFrom` instead.
@smarter
Copy link
Member Author

smarter commented Feb 4, 2021

Should we special case SingleDenotation#current for NonSymSingleDenotations to revert to the SymDenotation instead of running denottransformers after erasure?

This idea is implemented in smarter@3ab00a8 but leads to other complications as explained in the commit message.

@odersky odersky assigned smarter and unassigned odersky Feb 4, 2021
@smarter smarter merged commit 33ece58 into scala:master Feb 4, 2021
@smarter smarter deleted the lambdalift-transformSelect branch February 4, 2021 18:39
smarter added a commit to smarter/dotty that referenced this pull request Feb 5, 2021
The comment above `derived` used to say "Currently this fails
bootstrap", this was fixed by scala#11303, and this commit takes care of
updating the few places where we were assuming that something had to be
a SymDenotation when it didn't. Nevertheless, we still keep essentially
the same behavior of reusing denotations that existed before this commit
for performance reason, but this is now toggleable using
`Config.reuseSymDenotations`.

There's one difference with what we did before: if we need to create a
new denotation anyway because the info changed, then we always update
the prefix at the same time, this doesn't cost us anything and means
that we can rely on `prefix` being correctly set on non-sym denotations.
smarter added a commit to dotty-staging/dotty that referenced this pull request Feb 6, 2021
The comment above `derived` used to say "Currently this fails
bootstrap", this was fixed by scala#11303, and this commit takes care of
updating the few places where we were assuming that something had to be
a SymDenotation when it didn't. Nevertheless, we still keep essentially
the same behavior of reusing denotations that existed before this commit
for performance reason, but this is now toggleable using
`Config.reuseSymDenotations`.

There's one difference with what we did before: if we need to create a
new denotation anyway because the info changed, then we always update
the prefix at the same time, this doesn't cost us anything and means
that we can rely on `prefix` being correctly set on non-sym denotations.
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
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