This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
It would be useful to have the quickfix figure out that the modifier should be removed for the following incorrect code. public class AnotherClass { public void testMethod() { setRunnable(new Runnable() { public void run() { throw new UnsupportedOperationException("Not supported yet."); } private class MemberClass { } // quick fix for this line - remove modifier } ); } private void setRunnable(Runnable runnable) { throw new UnsupportedOperationException("Not yet implemented"); } }
Reassigning.
Created attachment 132808 [details] Proposed patch @NB-Devs: I like to propose a patch for this issue. Please review, discuss and commit. The patch contains a new fixable error hint, which removes the modifier.
Created attachment 132809 [details] Shows the patch in action
1. Please use "hg diff --git" when producing patches for review - the attached patch required manual intervention to apply (see bug #227700). 2. Consider test case like this: public class NewClass { public class A { public static class B {} } } There are actually two mutually related mistakes in the current implementation: a) due to the implementation of hashCode&equals, only one fix is provided (because the equals is not checking the modifiers that are to be removed). The question is whether there should even be multiple fixes - should not there be only one that would remove all the wrong modifiers? No strong opinion, just an idea. b) it proposes to remove "public", but there is no problem with the class being public, the problem is with the class being static. I don't think we should propose removal of public here. Unfortunately, not that trivial to fix. In general two options: -add the logic to detect the wrong modifiers into the hint. More coding and maintainence, but less administrative work. -the implementation of the Diagnostic seems to hold the extra flags - there is currently no API to read that information, but with that API, not additional fragile work in java.hints would be needed to get the invalid modifiers. Two possibilities: --add a method, that would get this specific information from this specific error (e.g. SourceUtils.getInvalidFlags(Diagnostic)). More stable, but can lead to a lot of similar methods added in the future. --add a method, that would allow to get any parameter from the Diagnostic - less stable, as the number and types of the error parameters are not specified. 3. Might be reasonable to use FixFactory.removeModifiersFix instead of a custom fix (IIRC, I was considering it also for the RemoveFinal fix, but then I decided not to suggest it, for reasons I don't remember precisely).
(In reply to comment #4) > 1. Please use "hg diff --git" when producing patches for review - the attached > patch required manual intervention to apply (see bug #227700). > Next time i will do it this way. Thanks for creating the bug. By the way: it is already fixed. > 2. Consider test case like this: > a) due to the implementation of hashCode&equals, only one fix is provided > (because the equals is not checking the modifiers that are to be removed). The > question is whether there should even be multiple fixes - should not there be > only one that would remove all the wrong modifiers? No strong opinion, just an > idea. My fault. Hashcode and equals have to take account of its fields. > b) it proposes to remove "public", but there is no problem with the class being > public, the problem is with the class being static. I don't think we should > propose removal of public here. Unfortunately, not that trivial to fix. In > general two options: > -add the logic to detect the wrong modifiers into the hint. More coding and > maintainence, but less administrative work. Yes, i could parse the string "modifier static not allowed here". No nice, but possible. > -the implementation of the Diagnostic seems to hold the extra flags - there is > currently no API to read that information, but with that API, not additional > fragile work in java.hints would be needed to get the invalid modifiers. Two > possibilities: > --add a method, that would get this specific information from this specific > error (e.g. SourceUtils.getInvalidFlags(Diagnostic)). More stable, but can lead > to a lot of similar methods added in the future. > --add a method, that would allow to get any parameter from the Diagnostic - > less stable, as the number and types of the error parameters are not specified. > While debugging i found out that "JCDiagnostics.getArgs()" would provide the invalid modifier, but as you said, the API isn't open. I cannot access the JCDiagnostics delegate from the CompilationInfoImpl$RichDiagnostic which i have access to. @Jan: What would you prefer? a) The first regex-matching-approach or b) wait for an API-change or c) use reflection to get access to JCDiagnostics? I like to go the (for me) easiest way a). The corresponding errorcode/-value hasn't changed since 2007, so I think the format of this specific code is pretty stable. See http://hg.netbeans.org/main/nb-javac/annotate/9a66ca7c79fa/src/share/classes/com/sun/tools/javac/resources/compiler.properties#l291 and compare it with the tip. But i don't know if the code is still the same pattern when using other JDKs. What do you think? > 3. Might be reasonable to use FixFactory.removeModifiersFix instead of a custom > fix (IIRC, I was considering it also for the RemoveFinal fix, but then I > decided not to suggest it, for reasons I don't remember precisely). Probably you wanted to keep them separated in case you want to throw away or disable faulty hints/fixes.
(In reply to comment #5) > @Jan: What would you prefer? > a) The first regex-matching-approach or After thinking about it, I suggest to: -take the Diagnostic's message using explicit US locale (i.e. something like: new Locale("en_US")) -look inside it for things that look like a modifier, ignore everything else I hope this could be stable in the long-term, even though obviously a hack. > b) wait for an API-change or I am somewhat afraid of explosion of single-purpose API methods, each bringing its own maintenance cost. > c) use reflection to get access to JCDiagnostics? Even though java.hints can directly access JCDiagnostics and other javac's internal classes now, it is for legacy reasons only. This actually does not seem to be required anymore, so I am going to remove the dependency between java.hints and libs.javacimpl, after which java.hints won't be able to access javac's internal classes. > > 3. Might be reasonable to use FixFactory.removeModifiersFix instead of a custom > > fix (IIRC, I was considering it also for the RemoveFinal fix, but then I > > decided not to suggest it, for reasons I don't remember precisely). > > Probably you wanted to keep them separated in case you want to throw away or > disable faulty hints/fixes. Not really - the ErrorRule would still be there, it would just not implements its own Fix, but would reuse the FixFactory.removeModifiersFix.
Created attachment 133251 [details] Proposed patch v2 - improved @Jan: Here the new version of the patch. * The error message from Diagnostics is parsed * FixFactory.removeModifiersFix is used (I wasn|t aware of this helpful class) * Your code example is also included as a test - see testRemoveStatic * Some minor changes (diamond operators) were made to apply to JDK7 pattern (FYI: Sorry couldn't use "hg diff -g". For unknown reason there was missing one file. I hope you can apply the patch from within the IDE.)
Comments: 1. is there a particular reason to allow the fix only on classes? Consider: public interface I { private native void ttt() {} } 2. the same example - there are two wrong modifiers, and I guess the current code won't be able to handle cases like this - or will it handle it? (I would still prefer to drop the regexp completely, and rather look for words like "public", but I am not going to push on that.) 3. doing this, it might be worth to also fix the error span in the editor for this kind of error - not as easy as creating the fix provider, though.
(In reply to comment #8) > Comments: > 1. + 2. I will fix it in the next iteration of th patch. > 3. doing this, it might be worth to also fix the error span in the editor for > this kind of error - not as easy as creating the fix provider, though. Sorry, i don't understand what you mean. Please describe it with other words or examples.
@Jan: If you invoke FixFactory.removeModifiersFix on the modifiers of an interface (==ClassTree) then the interface is rewritten to a class. Strange. Example: public native interface FooInterface {} //remove 'native' using removeModifiersFix //then the rewritten result is public class FooInterface {} This may be a bug in FixFactory.removeModifiersFix or a misunderstanding of me.
Created attachment 133287 [details] Patch v3 - support interfaces and methods too
(In reply to comment #11) > Created attachment 133287 [details] > Patch v3 - support interfaces and methods too Changes: * multiple invalid modifiers (like in your example) are now supported + testcases FYI the issue of https://netbeans.org/bugzilla/show_bug.cgi?id=180007#c10 will lead to a failing test Please have a look.
Thanks for the patch - integrated: http://hg.netbeans.org/jet-main/rev/6ee53699d491 The problem with interface to class rewrite is fixed by: http://hg.netbeans.org/jet-main/rev/fcb545a7990a