Community
Participate
Working Groups
This error message, that one can see by forgetting a ; actually tells what to do. So it should also be a quick-fix, adding the ";"
New Gerrit change created: https://git.eclipse.org/r/56405
Tentatively put 4.6.M3 as target, since patch is already available, we can hope it can be reviewed/polished/merged for 4.6.M3.
Comment from JDT/Core perspective: error messages for syntax errors are heuristically generated by a clever algorithm, and may in some situations be quite off the mark. I'm not sure how best to evaluate the hit-ratio. Our syntax tests provide some hints already, but plain monkey testing may perhaps provide deeper insights. Of course we are doing a best effort so that "good" suggestions are generated. But if the quick fix leads to more user complaints about "wrong" suggestions, we'll be in a difficult positions, since fine tuning those messages is an extremely tricky task. Just saying.
@Markus: I see this change makes a test fail. The test is checking that most Parser errors don't have a quickfix. Is it fine if I rewrite it to allow quickfix on parser errors?
(In reply to Stephan Herrmann from comment #3) > Of course we are doing a best effort so that "good" suggestions are > generated. But if the quick fix leads to more user complaints about "wrong" > suggestions, we'll be in a difficult positions, since fine tuning those > messages is an extremely tricky task. The suggested implementation is currently restricted to very simple cases, and seems to work fine on them. It introduces quite a lot of value compared to the current status, especially for people coming from other programming languages. In case we see a case which is very poorly handled and that introduce negative value in the quick-fix we can tweak the quick-fix to skip it. IMHO, the ration between value and risk of false suggestion makes it worth including it.
@Stephan: Do you have examples of tricky cases where this suggestion would lead user to "worse" results ?
Dup of bug 437927. (In reply to Mickael Istria from comment #4) > @Markus: I see this change makes a test fail. The test is checking that most > Parser errors don't have a quickfix. Is it fine if I rewrite it to allow > quickfix on parser errors? Hmm, do you really think I remember each and every test we have, so that I can easily find the one you're talking about? If it's org.eclipse.jdt.ui.tests.quickfix.LocalCorrectionsQuickFixTest#testNoFixFor_ParsingErrorInsertToComplete(), then just see the reference on top and Team > Show Annotations, which all point to bug 434188. In short: We should only add these Quick Fixes when we're reasonably sure that they do something useful in most cases. I cannot asses that off-hands. Someone would need to add the Quick Fixes locally and then work with them for a while and always apply the Quick Fixes during normal development.
(In reply to Markus Keller from comment #7) > In short: We should only add these Quick Fixes when we're reasonably sure > that they do something useful in most cases. I cannot asses that off-hands. > Someone would need to add the Quick Fixes locally and then work with them > for a while and always apply the Quick Fixes during normal development. Who do you mean by someone? I did try the submitted patch while developing Java code, and the quick-fixes it suggest are useful and worth it IMO. FYI, other popular Java IDEs have this kind of quick-fix that JDT doesn't provide yet. It's mainly targetting new Java developers who're not very comfortable with JDT grammar. Just doing Ctrl+1 would help them to fix syntax error automatically.
It would be great if this feature could be considered for Neon M4. Despite the test failure (which is expected as there are now some new quickfixes that some test weren't expected), the patch submitted on Gerrit can already be evaluated to discuss the usability of this feature and the reliability of the implementation.
Bug 481016 (thanks Markus) is a perfect example of what I meant in comment 3.
I'm doing the real-life tests and will report back. As explained in bug 437927 and other referenced bugs, we need more infrastructure to avoid showing the light bulb in situations where we don't actually have a fix to offer. Maybe also needs better support from the compiler (separate error id for cases where the proposed token(s) can be inserted because it's a literal string and not a reference to a nonterminal of the grammar). In the UI, we don't use terms like "token(s)". Use separate strings for singular and plural.
Search for bugs about "recovery" and "syntax" errors to see more examples where the compiler's recovery needs work.
This seems to be a DUP of bug 39813, but since more discussion and a patch are provided here, is it OK to mark bug 39813 as a DUP of this one?
Since the syntax error reported in bug 97822 is actually a good one, and since the proposed patch suggests the right fix for the syntax error, I'm removing the dep on that bug. The suggested patch + the one for 481016 seem to be everything needed to make a step forward in better quick fix of parsing error (with limitations explained in comment 5 that prevent from doing wrong suggestions). Are you aware of any other 1-character parsing error for which parser give a bad hint?
I would also agree that including such a quick-fix would be helpful to Java newcomers, let me know if there's anything I could do to make this happen.
(In reply to Robert Roth from comment #15) > I would also agree that including such a quick-fix would be helpful to Java > newcomers, let me know if there's anything I could do to make this happen. Thanks Robert, I think you can give a try to the suggested patch and compare the behavior for JDT quickfix with/without it, and report how useful it was to you (or some Java rookies) while developing, and whether you identified cases of "false-positive". If it was helpful at least once and without perceived false-positive, then it means that this patch is worth being included.
What is holding on this patch?
(In reply to Alexander Kurtakov from comment #17) > What is holding on this patch? The review assignee no longer working for Eclipse, plus the rest of the team being heavily overbooked by Java 9 work and minimal triage of severe bugs. Hope this explains :)
(In reply to Stephan Herrmann from comment #18) > (In reply to Alexander Kurtakov from comment #17) > > What is holding on this patch? > > The review assignee no longer working for Eclipse, plus the rest of the team > being heavily overbooked by Java 9 work and minimal triage of severe bugs. > > Hope this explains :) I changed the review assignee to Noopur.
(In reply to Stephan Herrmann from comment #3) > Comment from JDT/Core perspective: error messages for syntax errors are > heuristically generated by a clever algorithm, and may in some situations be > quite off the mark. I'm not sure how best to evaluate the hit-ratio. Our > syntax tests provide some hints already, but plain monkey testing may > perhaps provide deeper insights. > Of course we are doing a best effort so that "good" suggestions are > generated. But if the quick fix leads to more user complaints about "wrong" > suggestions, we'll be in a difficult positions, since fine tuning those > messages is an extremely tricky task. > Just saying. Do you think it would be worth adding a note on the quick-fix label for quick-fixes derived from parsing error? So it would be something like " Insert ';' (potential fix)"
