getDocument(ITextEditor) incorrectly assumes an implementation of AbstractTextEditor

[GeraldMit]

lsp4e/org.eclipse.lsp4e/src/org/eclipse/lsp4e/LSPEclipseUtils.java

Line 608 in e0d4c0d

Method getSourceViewerMethod= AbstractTextEditor.class.getDeclaredMethod("getSourceViewer"); //$NON-NLS-1$

(Note that this is for older version of code but issue still remains)
java.lang.IllegalArgumentException: java.lang.ClassCastException@1466e463
at jdk.internal.reflect.GeneratedMethodAccessor130.invoke(Unknown Source)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.eclipse.lsp4e.LSPEclipseUtils.getDocument(LSPEclipseUtils.java:604)
at org.eclipse.lsp4e.operations.rename.LSPRenameHandler.setEnabled(LSPRenameHandler.java:110)
at org.eclipse.ui.internal.handlers.HandlerProxy.setEnabled(HandlerProxy.java:229)
at org.eclipse.ui.internal.handlers.E4HandlerProxy.setEnabled(E4HandlerProxy.java:133)
at jdk.internal.reflect.GeneratedMethodAccessor12.invoke(Unknown Source)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.eclipse.e4.core.internal.di.MethodRequestor.execute(MethodRequestor.java:58)
at org.eclipse.e4.core.internal.di.InjectorImpl.invokeUsingClass(InjectorImpl.java:317)
at org.eclipse.e4.core.internal.di.InjectorImpl.invoke(InjectorImpl.java:251)
at org.eclipse.e4.core.contexts.ContextInjectionFactory.invoke(ContextInjectionFactory.java:173)
at org.eclipse.e4.core.commands.internal.HandlerServiceHandler.setEnabled(HandlerServiceHandler.java:84)
at org.eclipse.core.commands.Command.setEnabled(Command.java:856)
at org.eclipse.ui.menus.CommandContributionItem.isEnabled(CommandContributionItem.java:916)
at org.eclipse.ui.menus.CommandContributionItem.updateMenuItem(CommandContributionItem.java:531)
at org.eclipse.ui.menus.CommandContributionItem.update(CommandContributionItem.java:484)
at org.eclipse.jface.action.MenuManager.update(MenuManager.java:858)
at org.eclipse.jface.action.MenuManager.update(MenuManager.java:858)
at org.eclipse.ui.internal.Workbench.updateActiveWorkbenchWindowMenuManager(Workbench.java:3152)
at org.eclipse.ui.internal.Workbench.lambda$0(Workbench.java:3134)
at .bindingManagerChanged(Unknown Source)
at org.eclipse.jface.bindings.BindingManager.fireBindingManagerChanged(BindingManager.java:901)
at org.eclipse.jface.bindings.BindingManager.setActiveBindings(BindingManager.java:2181)
at org.eclipse.jface.bindings.BindingManager.recomputeBindings(BindingManager.java:1742)
at org.eclipse.jface.bindings.BindingManager.contextManagerChanged(BindingManager.java:691)
at org.eclipse.core.commands.contexts.ContextManager.fireContextManagerChanged(ContextManager.java:164)
at org.eclipse.core.commands.contexts.ContextManager.setEventCaching(ContextManager.java:323)
at org.eclipse.core.commands.contexts.ContextManager.deferUpdates(ContextManager.java:85)
at org.eclipse.e4.ui.internal.workbench.PartServiceImpl.activate(PartServiceImpl.java:778)
at org.eclipse.e4.ui.internal.workbench.PartServiceImpl.activate(PartServiceImpl.java:680)
at org.eclipse.e4.ui.internal.workbench.PartServiceImpl.activate(PartServiceImpl.java:675)
at org.eclipse.ui.internal.WorkbenchPage.busyOpenEditor(WorkbenchPage.java:3216)
at org.eclipse.ui.internal.WorkbenchPage.lambda$9(WorkbenchPage.java:3112)
at .run(Unknown Source)
at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:74)
at org.eclipse.ui.internal.WorkbenchPage.openEditor(WorkbenchPage.java:3110)
at org.eclipse.ui.internal.WorkbenchPage.openEditor(WorkbenchPage.java:3080)
at org.eclipse.ui.internal.WorkbenchPage.openEditor(WorkbenchPage.java:3063)
at org.eclipse.ui.ide.IDE.openEditor(IDE.java:636)
...

The current getDocument(ITextEditor) assumes that the ITextEditor implementation is only default installed Eclipse Editor.

However LSPRenameHandler setEnabled() path can be triggered from any Editor, including ones not using AbstractTextEditor to build an implementation, resulting in an error in the log.

I am unsure as to the technical reason for using the reflection to access the document, other than to assure only an existing ITextViewer is used.

I do note that openFileLocationInEditor uses ITextEditor via getDocumentProvider().getDocument(IEditorInput) and getEditorInput() to get the document, or that getTextViewer(IEditorPart) also seems that it could be used by getDocument, as ITextEditor extends IEditorPart.

I have a proposed change set cc762b4 to keep the section for which I do not have a greater understanding of the technical reasoning and add a class type check to assure AbstractTextEditor, and then adds the other technique to get the IDocument as backup, which is very much over-compensation. Likely a better long-term solution is to find the most performant and go with that.

[mickaelistria]

A PR that removes reflection and replaces it with something smarter (eg getDocumentProvider().getDocument(getEditorInput()) would be welcome.

[mickaelistria]

We'd like to release LSP4E soon, any chance you can submit a PR promptly with the suggested fix?

[GeraldMit]

@mickaelistria I was working on testing just using the getDocumentProvider() as it needed test against the new TM4E and some extended projects with ITextEditors.
Update - I see my original PR was pulled in. I will investigate simplification for next release...
Update2 - I see my original PR was NOT pulled in.

[GeraldMit]

@mickaelistria I see my original change set cc762b4 was not pulled in, and instead the PR uses getDocument(IEditorInput) first.

I see that there was a comment to submit the PR, but I logged in to do so to find the Issue was already changed and closed?

Can you give feedback on the changes and why getDocument(IEditorInput) was used instead of the ITextEditor API getDocumentProvider().getDocument(getEditorInput()) ?

[mickaelistria]

I merged a quick patch, but it may be incomplete. Please create a PR to improve it. I'm reopening the issue in the meantime.

[GeraldMit]

OK I will work on and test and submit a PR or close the issue in the next 4 hours. Primarily testing external possible implementations of ITextEditor and performance. Will make a Junit if it makes sense.

[GeraldMit]

From what I can tell, the only open usage is the one in the issue, where because LSP4E is registered as a handler and needs the LSP checks to check for providers for the document. Current change seems to be fine accept that getExistingDocument and get Document(Resource) don't check the bufferManager, and so can spew irrelevant NPEs on shutdown ( can see/cause using debugger but not easily in real usage)

I will add a PR to add the bufferManager checks ( to return null as it is shutdown so as to not throw NPEs) but not change the getDocument usage until I can get more testing in, because of impact and trying to get out the release.

My only concern is that that the getDocument(Resource) will attempt to retrieve the resource if not already in the file buffer... in theory with a large file this may decrease performance, but for the action ( menumanager invoke of listeners) so far the files are already loaded in buffer before reaching the menu invoke (because triggered in an editor that has already loaded) - testing that right now.

[GeraldMit]

LSPFormatHandler and LSPRenameHandler cause 2 invokes to getDocument on a menu action, seeing the file is already in the buffer before either. Not able to replicate issue with the quick patch.

[mickaelistria]

This should be fixed now in 0.20.2 release. Thanks.