Fix invalid element references during completion resolution using ElementHandle which has additional text edits #9005
Conversation
…mentHandle which has additional text edits
Achal1607
added
Java
lahodaj
left a comment
lahodaj
left a comment
There was a problem hiding this comment.
Looks reasonable to me. I am looking at possibilities to increase coverage for this problem, but I think this patch can go in without it.
| wc.toPhase(JavaSource.Phase.ELEMENTS_RESOLVED); | ||
| ExecutableElement currentElem = elemHandle.resolve(wc); | ||
| if (currentElem == null) { | ||
| //cannot resolve? |
There was a problem hiding this comment.
Nit - maybe remove this comment, as other places don't have it either. (Or add logging to this and other places.)
| if (handle != null) { | ||
| builder.documentation(getDocumentation(doc, offset, handle)); | ||
|
|
||
| if (SUPPORTED_ELEMENT_KINDS.contains(elem.getKind().name())) { |
There was a problem hiding this comment.
I think the SUPPORTED_ELEMENT_KINDS check here does not serve any function anymore. I believe the point here was to not call ElementHandle.create with an unsupported ElementKind, but: a) I think all kinds that can occur here are supported; b) we have already created the ElementHandle by this point, so if that would not work, it would have failed already. I think I would suggest to simply delete the condition here.
3 participants
Fixes crashes and incorrect behavior when resolving completion items across different javac contexts.
When completion items are created, they capture Element references from the initial javac context. Later, when the LSP client calls
completionItem/resolveto fetchadditionalTextEdits, a new javac context may be used (especially if the file was modified). The old Element instances are invalid in the new context, causingAssertion Error rootPackage missing!; currModule: java.base.So, store ElementHandles instead of stale Elements when creating completion items.
Thanks @lahodaj for helping out.