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.
Please add a fixable hint for the error "cannot assign a value to final variable XXX". The fixable hint should remove the "final" attribute from the variable's declaration (if possible). F.e. eclipse provides such this via a quickfix. ---- NetBeans IDE Dev (Build 201205090400) /7.2
Created attachment 129639 [details] Proposed patch (incl. test) I like to propose the following patch to solve the issue. It adds a hint to remove the final modifier from variables or parameters. The hint is an adaption of the existing org.netbeans.modules.java.hints.errors.MakeVariableFinal from Jan Lahoda. I included a testcase. You can also try out the hint manually by using the following code <code> package org.netbeans.test.java.hints; /** * Use for manual testing. */ public class TestFile { public static void main1() { final int a = 1; a = 2; // <--fixable hint } public static void main2(final int parameter) { parameter = 2;// <--fixable hint } final int classvar = 1; public void main3() { classvar = 2;// <--fixable hint } static final int staticvar = 1; public void main4() { staticvar = 2;// <--fixable hint } } <code> @NetBeans-Dev: Please review, discuss and commit.
Created attachment 129640 [details] Patch in action
Created attachment 129704 [details] Proposed patch (incl. test) Updated patch (added missed test, minimized delta)
*** Bug 218845 has been marked as a duplicate of this bug. ***
Thanks for the patch, good work. I don't think there is a real chance to include it in 7.3, so the integration will happen no sooner then after 7.3 is branched. Comments: What I would like to see changed before integration: 1. not sure why the comment in CODES - I would suggest to simply drop it. Yes, the keys are defined in compiler.properties, as do virtually all the other keys used in ErrorRules, so this is nothing that would need to be pointed out. That particular change in the webrev does not seem to be related. If there is a reason to point out a change or particular part of compiler.properties, link to a valid changeset/repository (ideally to http://hg.netbeans.org/main/nb-javac). Webrevs are generally transient and may disappear at any moment. 2. in tests, do not use '|' to mark the "caret" location, use '-1' as the position (another parameter). That will set the position passed to the ErrorRule from the corresponding error (via getSupportedErrorKeys). Strange and unintuitive, but is due to the way the error infrastructure evolved and noone so far invested into cleaning this up. 3. the fixes in MakeVariableFinal and here are basically the same, would be better to have it only once (taking parameter saying if it should add or remove FINAL). 4. make sure annotation attributes whose value is an array does not end with a comma (the @NbBundle.Messages in the patch). Older 1.6 compilers do not accept that unfortunately, and some people still build using them. 5. please do not copy the fix texts from the "production" bundle content into the tests. Either add test keys into Bundle_test.properties and set branding to "test", or lookup the current content using NbBundle.getMessage or Bundle.<key> (that last recommended when using NbBundle.Messages). 6. text.replaceAll("\n", "").replaceAll("\\s\\s+", "") seems wrong to me. It can lead to an uncompilable code by removing important spaces (place two spaces between "static" and "void" in a test and the test will break). Use something like: text.replaceAll("[\\s\n]+", " "). Things to consider (i.e. not blockers for integration): 7. merge MakeVariableFinal and RemoveFinalModifierFromVariableOrParameter (take a parameter in the constructor), create using two factory methods and register using instanceCreate (not 100% sure if that would work for ErrorRule, but I am almost sure it would). Would make 3 resolved automatically and a lot of code would disappear. 8. split @NbBundle.Messages so each method that uses a particular key would have that particular definition. Would basically solve 4. No need to use NbBundle.getMessages, BTW, use "Bundle.<key>(<parameters>)" instead.
Created attachment 129771 [details] Proposed patch (incl. test) v2 Thanks for the patch, good work. > Thanks, the main work was done by you. I only had to figure out the trigger and the Treemaker-infrastructure was already there. I don't think there is a real chance to include it in 7.3, so the integration will happen no sooner then after 7.3 is branched. > I know - code freeze, no problem. I am also thinking about creating a 'additional hints plugin' for 7.3, which will include this fix too. 1. not sure why the comment in CODES - I would suggest to simply drop it. Yes, the keys are defined in compiler.properties, as do virtually all the other keys used in ErrorRules, so this is nothing that would need to be pointed out. That particular change in the webrev does not seem to be related. If there is a reason to point out a change or particular part of compiler.properties, link to a valid changeset/repository (ideally to http://hg.netbeans.org/main/nb-javac). Webrevs are generally transient and may disappear at any moment. > I am new to this API, so i inserted the link because I wanted a reference to the used constants. Updated the link showing to NB repo. Feel free to modify the patch to your needs. 2. in tests, do not use '|' to mark the "caret" location, use '-1' as the position (another parameter). That will set the position passed to the ErrorRule from the corresponding error (via getSupportedErrorKeys). Strange and unintuitive, but is due to the way the error infrastructure evolved and noone so far invested into cleaning this up. > It did not work out. I added -1 as parameter, but then the tests failed. It is curious because the hints still worked in the IDE. Please try it for yourself and tell me what I did wrong. I do not see the mistake. So I am stuck on '|'. 3. the fixes in MakeVariableFinal and here are basically the same, would be better to have it only once (taking parameter saying if it should add or remove FINAL). > Done. There is an enum for it. 4. make sure annotation attributes whose value is an array does not end with a comma (the @NbBundle.Messages in the patch). Older 1.6 compilers do not accept that unfortunately, and some people still build using them. > Done. See 8. 5. please do not copy the fix texts from the "production" bundle content into the tests. Either add test keys into Bundle_test.properties and set branding to "test", or lookup the current content using NbBundle.getMessage or Bundle.<key> (that last recommended when using NbBundle.Messages). > Done. Now using Bundle.<key> (where possible) 6. text.replaceAll("\n", "").replaceAll("\\s\\s+", "") seems wrong to me. It can lead to an uncompilable code by removing important spaces (place two spaces between "static" and "void" in a test and the test will break). Use something like: text.replaceAll("[\\s\n]+", " "). > Done. I removed all the \n from the Strings and use text.replaceAll("[\\s]+", " ") now. Things to consider (i.e. not blockers for integration): 7. merge MakeVariableFinal and RemoveFinalModifierFromVariableOrParameter (take a parameter in the constructor), create using two factory methods and register using instanceCreate (not 100% sure if that would work for ErrorRule, but I am almost sure it would). Would make 3 resolved automatically and a lot of code would disappear. > Done. createRemoveFinalFromVariable(), createRemoveFinalFromParameter(), createAddFinalModifier() are the new factory methods. Registration in layer.xml works. Removed MakeVariableFinal and its old tests (ugly file based tests). The original testdata were transfered into the newly created AddFinalModifierTest. This way the test is much easier to grasp. 8. split @NbBundle.Messages so each method that uses a particular key would have that particular definition. Would basically solve 4. No need to use NbBundle.getMessages, BTW, use "Bundle.<key>(<parameters>)" instead. > Done. Message definitions are put next to factory methods.
(In reply to comment #6) > Created attachment 129771 [details] > Proposed patch (incl. test) v2 > > Thanks for the patch, good work. > > Thanks, the main work was done by you. I only had to figure out the trigger and the Treemaker-infrastructure was already there. Next time, please generate the diff using "hg diff --git" - I was not able to apply the patch without manual changes. > > I don't think there is a real chance to > include it in 7.3, so the integration will happen no sooner then after 7.3 is > branched. > > I know - code freeze, no problem. I am also thinking about creating a 'additional hints plugin' for 7.3, which will include this fix too. Feel free to do that - I have contrib/javahints, but that contains also some pretty experimental stuff, so is not particularly fit to be published for a released version. You might use some of the hints/fixes there in the additional hints plugin, though. > > 1. not sure why the comment in CODES - I would suggest to simply drop it. Yes, > the keys are defined in compiler.properties, as do virtually all the other keys > used in ErrorRules, so this is nothing that would need to be pointed out. That > particular change in the webrev does not seem to be related. If there is a > reason to point out a change or particular part of compiler.properties, link to > a valid changeset/repository (ideally to http://hg.netbeans.org/main/nb-javac). > Webrevs are generally transient and may disappear at any moment. > > > I am new to this API, so i inserted the link because I wanted a reference to the used constants. Updated the link showing to NB repo. Feel free to modify the patch to your needs. > > 2. in tests, do not use '|' to mark the "caret" location, use '-1' as the > position (another parameter). That will set the position passed to the > ErrorRule from the corresponding error (via getSupportedErrorKeys). Strange and > unintuitive, but is due to the way the error infrastructure evolved and noone > so far invested into cleaning this up. > > > It did not work out. I added -1 as parameter, but then the tests failed. It is curious because the hints still worked in the IDE. Please try it for yourself and tell me what I did wrong. I do not see the mistake. So I am stuck on '|'. Hm, did not work due to off-by-one inconsistency/error, fixed by: http://hg.netbeans.org/jet-main/rev/b036f8206b27 '|' should work now. Thanks for updating the test.
Integrated into 'main-golden', will be available in build *201301070001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/b036f8206b27 User: Jan Lahoda <jlahoda@netbeans.org> Log: CreatorBasedLazyFixList does TreeUtilities.pathFor(offset + 1), so need to do the same in ErrorHintsTestBase (noted in #212655#c6)
Created attachment 132719 [details] Proposed patch v4 - regenerated and removed pipe-syntax (In reply to comment #5) > 2. in tests, do not use '|' to mark the "caret" location, use '-1' as the > position (another parameter). That will set the position passed to the > ErrorRule from the corresponding error (via getSupportedErrorKeys). Strange and > unintuitive, but is due to the way the error infrastructure evolved and noone > so far invested into cleaning this up. I regenerated the patch based on current sources and I replaced the pipe-syntax with -1 within the tests. So please try to apply the new patch.
Thanks for the patch, applied: http://hg.netbeans.org/jet-main/rev/118122ec17fb
Integrated into 'main-golden', will be available in build *201303192300* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/118122ec17fb User: Benno Markiewicz <markiewb@netbeans.org> Log: #212655: fixable hint for "cannot assign a value to final variable"
Thanks Honzo for integration of Benno's patch. Thanks a lot Benno for providing the patch to Honza! :)