Fix a REPL bad symbolic reference #19786
Conversation
- Be more specific when we go into the special case of not updating checkedPeriod - Always update lastDenotation. - Keep computeDenot small, move work to recomputeDenot
|
If we always update the lastDenotation then we lose the denotation in the truncated, tab-completion run (we replace an outdated denotation with NoDenotation). Here's what I'm trying: another |
Yes, but I am not sure where it would make a difference. Presumably, we get at the denotation only in the case where we do import suggestions, so it's an error case and we won't get in the same run to the later phase where the denotation matters. |
Correct. And then in the next run we do get to a later phase, and the denotation is gone, with all the information that we had from classfile parsing. |
|
Right. But then I wonder why that was not caught in the test case? |
…ario This brings back an element of the original solution.
odersky
force-pushed
the
rebase/repl-bad-symbolic-ref
branch
from
0a5da73
to
5631d76
Compare
That patch failed for me locally. |
|
Ah, I only ran testCompilation and that does not include the repl tests. How does it work for you with the latest patch? (I force pushed with a fix not to set lastDenot to NoDenotation). |
|
Yeah it does. I think that approach has the downside that it disables any caching of these denotations. Perhaps that's fine, but do you want to consider my new subclass approach? |
|
I was worried about the caching before since we never cached |
dwijnand
added
the
backport:nominated
While bringing forward the denotation to a new run, we now check if the symbol was moved from its owner to a companion object. If so, we return NoDenotation, as that denotation seems to be a leftover from pre-MoveStatics phases in a previous run. In the issue reproduction, we had a symbol created in the `LazyVals` phase, which was then later moved to a companion class in `MoveStatics` in the first run. In the second run, this caused the leftover denotation for pre-MoveStatics phases of the first run to be tried to brought forward (since the phaseID became valid at that point), failing to do so (because that symbol should no longer exist as a member of the initial companion object at that point). It looks like before #19786, since this denotation was valid at a later phase, it would be visited somewhere before the MegaPhase with `LazyVals` and replaced with a NoDenotation (pretty much by accident), which back then ended up being cached for the latter phases as well. So in the fix here we check for that specific `MoveStatics` -caused case and, if found, update the symbol with a NoDenotation (just like before, but this time, on purpose).
WojciechMazur
removed
the
backport:nominated
Fixes #15562
Copy of #19775 with a final tweak commit at the end.
I created a new PR since the original PRs were not on staging.