search

eclipse / xtext

Eclipse Xtext™ is a language development framework
http://www.eclipse.org/Xtext
Eclipse Public License 2.0
766 stars 319 forks source link

EmbeddedEditor Quickfix throws NPE #2398

Open miklossy opened 7 years ago

miklossy commented 7 years ago

Given is the following EmbeddedXtextEditor project referencing the domainmodel example project: EmbeddedXtextEditor.zip

The following NullPointerException is thrown after trying to apply a quickfix: embeddededitor_quickfix_npe

cdietrich commented 7 years ago

same problem as https://github.com/eclipse/xtext/issues/2427 ???

cdietrich commented 7 years ago

seems so

    at org.eclipse.equinox.launcher.Main.run(Main.java:1499)
    at org.eclipse.equinox.launcher.Main.main(Main.java:1472)
Caused by: java.lang.NullPointerException
    at org.xtext.example.mydsl.ui.quickfix.MyDslQuickfixProvider.lambda$0(MyDslQuickfixProvider.java:26)
    at org.eclipse.xtext.ui.editor.quickfix.IssueResolution.apply(IssueResolution.java:76)
    ... 33 more
ArneDeutsch commented 6 years ago

I can reproduce and have had a deeper look into the code. It would be possible to fix it but as far as I can see not without a quite massive API breakage.

Root cause is in "IssueModificationContext". It assumes that it can determine the needed document by opening an editor and get the document from it. This fails because there is no such editor.

"EmbeddedEditorFactory" contains code that is clearly intended to append the "IXtextDocument" for the embedded editor case. It tries so by providing its own implementation of "IssueResolutionProvider". But this attempt fails because the implemented method "getResolutions" is never called. Outside the "IssueResolutionProvider" is resolved by dependency injecten and locally only "hasResolutionFor" is called.

The issue could be fixed by implementing a setter for an "IXtextDocument" inside "IssueModificationContext" and using this document instead of the "IURIEditorOpener" when asking for the document. But this would also need a change in "IssueModificationContext.Factory" signature (needs to get the "IXtextDocument") and this change would ripple through to all "accept" methods of "IssueResolutionAcceptor" and in result to every class providing quick fixes.

There are hints in the code that there was a time when the "IXtextDocument" was actually handled through but was then replaced by the editor determination logic (e.g. "AbstractIssueResolutionProviderAdapter" contains a deprecated method that actually receives the "IXtextDocument" but does not use it). I guess the original code was the right idea and for some reason was changed in this suboptimal direction.

In result I'm not sure how to proceed here. The API breakage looks quite severe because "IXtextDocument" need to be added to a lot of method signature.

cdietrich commented 6 years ago

did you have a look at the idea in https://github.com/eclipse/xtext/issues/2427 as well?

ArneDeutsch commented 6 years ago

No, not yet. I have read the ticket but not understand before lookinng at the code more intense. I will have a look at that ticket now and investigate if it looks doable for me.