Community
Participate
Working Groups
3.3M7 I20070503-1400 public void method() { int a = 5; a++; } In the above code it is not reported that 'a is never read locally' (applies with prefix/postfix increment/decrement operations). Although technically it is read in order to increment the variable, the spirit of the warning is to highlight code that is unnecessary or variables which should be used elsewhere but for some reason aren't. For that reason I think a should be marked as unread.
Note that in following case, variable would still be read. public void method() { int a = 5; System.out.println(a++); } public void method() { int a = 5; System.out.println(++a); }
This would mean we tag as read if valueRequired is true ?
Yep
I suppose to an extent this also applies to the += style of operator. int a = 5; a += 1; // a not used int b = 5; System.out.println(b *= 2); // b is used
*** Bug 318571 has been marked as a duplicate of this bug. ***
bug 318571 comment 6 has a preliminary patch.
I think the main issue is the variabel that got assigned (on the left side). That variable will not have been used even if it is read on the right side: int a = 5; int b = 3; a = b * 365 / a; // b is used here, not a since it is the one getting the result.
Sorry for repeating my comment .. :-) But I have one more: <So if boxing/unboxing is involved, I don't think the compiler can report what <Lars is expecting. < <Lars, what do you expect in this case ? < <public class X { < public static void main(String[] args) { < Integer i = foo(); < i++; < } < < private static Integer foo() { < return 2; < } < <} < <With current flow analysis, there is no way we can "guess" that i is not null <and therefore we cannot claim that i is not read, because if foo() returns <null this is changing the runtime semantic. Given my previous comment(s), I can just not see that i would be anything else but "not used" here.. no matter if it is null or not. Since no other variable is reading it, only itself.
(In reply to comment #9) > I think the main issue is the variabel that got assigned (on the left side). > That variable will not have been used even if it is read on the right side: > > > int a = 5; > int b = 3; > > a = b * 365 / a; // b is used here, not a since it is the one getting the > result. In this expression 'a = b * 365 / a;', 'a' is read. The previous value of 'a' (5) is used to compute the new one. So I don't want to see 'a' reported as unused in this case.
(In reply to comment #10) > Given my previous comment(s), I can just not see that i would be anything else > but "not used" here.. no matter if it is null or not. Since no other variable > is reading it, only itself. This is where I disagree. If we report a variable is not used, the "quickfix" would be to get rid of it. This should have no side-effect on the semantic of the program. In this case remove the variable would mean that it doesn't throw a NPE anymore. So the runtime behavior is different.
I, too, think that the warning should be conservative such that removing the "unused" local will never change the semantics. When I mentioned this before I was mainly thinking of things like: int i = 0; i += expr; which the matching quickfix should generally reduce to expr; unless we syntactically know that expr cannot produce a side effect. I wouldn't go into deeper analysis on expr, nor would I analyse things like: a = b * 365 / a; We clearly see a read access of a and we would open a whole can of worms if we'd promise that compiler could analyse all that. So my suggestions remains to stick to postIncrement and compoundAssignment where special syntax kind of hides the factual but perhaps useless read access. Also considering NPE on unboxing is a great catch. However, while playing with some examples, I found bug 319201 which should probably be fixed before we go on with this issue.
(In reply to comment #13) > I, too, think that the warning should be conservative such that > removing the "unused" local will never change the semantics. I'll look at this for 3.7 - I plan to address only the simple scenarios, that are conservative and don't need any fancy analysis.
(In reply to comment #14) > I'll look at this for 3.7 - I plan to address only the simple > scenarios, that are conservative and don't need any fancy analysis. +1. I think if the value is not required and no unboxing is involved we can report it as an unread local.
Created attachment 174421 [details] proposed patch Here is an improved version of the patch from bug 318571 comment 6. With this patch every access involving unboxing is considered as relevant, regardless of null-analysis. The patch defers the actual reporting till code generation (that's when we have valueRequired available). AFAICS this is consistent with comment 14 and comment 15.
(In reply to comment #16) > Created an attachment (id=174421) [details] > proposed patch Thanks a lot for the patch Stephen, I'll take a look at this during the 3.7 M2 time frame.
Discussed this Ayush, as I haven't had time to look into this, he said he can take it forward to closure. Thanks!
Created attachment 178716 [details] improved fix + regression tests I found the above fix, although correct, was not complete. It had the following shortcomings: 1) It had no check for the local declaration being null or not (which can be the case for hidden or secret locals in say, method local classes). 2) It had no check for whether the local declaration is actually reachable or not (org.eclipse.jdt.core.tests.compiler.regression.ProgrammingProblemsTest#test0050 shows one case) 3) It didn't distinguish between local variables and method arguments. So for the case: void method(int param) { param++; } It marked param as "local variable never read". It should rather be marked as "parameter never read" 3a) The parameter never read warning itself has 2 suboptions (Prefs>Java>Compiler>Errors/warnings) - warning for overriding and implementing methods, and warning when @param tag refers to the parameter in question The new patch patch puts the checks in place to make sure the warning is not shown when the first suboption is disabled. The second suboption's case is automatically covered since the USED flag for the parameter gets set when it is referred by @param The new patch takes care of all these, adds more tests and the copyrights. Stephen, can you please do a sanity check for this patch? Thanks!
Created attachment 178806 [details] cosmetic updates (In reply to comment #19) > Created an attachment (id=178716) [details] > improved fix + regression tests > [...] > The new patch takes care of all these, adds more tests and the copyrights. > Stephen, can you please do a sanity check for this patch? Thanks! I agree with all your changes. I only made some cosmetic changes: + extract duplicate code block into a new method + changed complex logic expression into more imperative style The second item is probably very much a matter of taste. I'll leave the decision to you which version you consider better maintainable.
(In reply to comment #20) > Created an attachment (id=178806) [details] > cosmetic updates >[..] > I only made some cosmetic changes: > + extract duplicate code block into a new method > + changed complex logic expression into more imperative style > The second item is probably very much a matter of taste. > I'll leave the decision to you which version you consider better maintainable. I'm ok with this patch since its easier to understand. I'll release it soon.
Released in HEAD for 3.7M3
Reversing the patch because of a few points that Dani made: 1. it only works for special constructs but not for e.g. a= a+1, which is almost the same for the user 2. it's not consistent with private fields 3. it is inconsistent to the tooling, e.g. search reports such constructs as read access The fix itself is ok, but due to the above points it does introduce some independencies. We should either address all 3 points above (which in Dani's opinion is overkill, and I agree), or we should introduce a new suboption to the error/warnings setting "local variable never read", and then think of raising the unread warning. Olivier, whats your opinion on this?
Reopening
(In reply to comment #23) [..] > The fix itself is ok, but due to the above points it does introduce some > independencies. [..] I meant inconsistencies.
(In reply to comment #23) > Reversing the patch because of a few points that Dani made: > 1. it only works for special constructs but not for e.g. a= a+1, which is > almost the same for the user I don't quite agree with this one. The conditional expression is almost like an if statement, but it has its own semantic. Same is valid for the compound assignment and a normal assignment. > 2. it's not consistent with private fields Don't see what you mean with this. > 3. it is inconsistent to the tooling, e.g. search reports such constructs as > read access ok, this is indeed an inconsistency. > The fix itself is ok, but due to the above points it does introduce some > independencies. We should either address all 3 points above (which in Dani's > opinion is overkill, and I agree), or we should introduce a new suboption to > the error/warnings setting "local variable never read", and then think of > raising the unread warning. +1, for a suboption of unused local.
> (In reply to comment #23) > > Reversing the patch because of a few points that Dani made: > > 1. it only works for special constructs but not for e.g. a= a+1, which is > > almost the same for the user > I don't quite agree with this one. The conditional expression is almost like an > if statement, but it has its own semantic. Same is valid for the compound > assignment and a normal assignment. For the user it's adding (or subtracting) a number from a variable. He doesn't care about internals. In "a= a+1" the variable is also unused but we don't catch that case. > > 2. it's not consistent with private fields > Don't see what you mean with this. The private field is not reported as unused while the local variable is. >+1, for a suboption of unused local. -1 from here. That's overkill and doesn't solve anything. People are used to get a++ reported as read access and that's nothing I want to change now. There is no good reason why fields and local variables should differ here.
Then we revert and close as WONTFIX.
Closing as WONTFIX
Simultaneous with Ayushman closing this bug I was writing: -1 for extending to "a= a+1", because here the read is explicitly visible, which would open the door to requiring all kinds of analysis whether the value is read in a *useful* way. +1 I'd support extending it to private fields (iff we'd keep this at all) As to the inconsistency with search: my gut feeling says that's not a real problem. I would have to do some introspection to find out why I feel that way :) I did see some benefit raising more warnings for a limited set of situations. However, if a majority agrees that we're not finding a simple, consistent set of rules, I'm OK with reverting.
(In reply to comment #30) > I did see some benefit raising more warnings for a limited set of situations. > However, if a majority agrees that we're not finding a simple, consistent > set of rules, I'm OK with reverting. For me this was a useful warning to spot potential problem in nested for loops. for example: int[][] tab = ...; for (int i = 0, max = tab.length; i < max; i++) { for (int j = 0, max2 = tab[i].length; i < max2; j++) { System.out.print(tab[i][i]); } } With the warning we would get j flagged as unread. So it is clearly a problem. My 2 cents.
(In reply to comment #31) > (In reply to comment #30) > > I did see some benefit raising more warnings for a limited set of situations. > > However, if a majority agrees that we're not finding a simple, consistent > > set of rules, I'm OK with reverting. > For me this was a useful warning to spot potential problem in nested for loops. > for example: > int[][] tab = ...; > for (int i = 0, max = tab.length; i < max; i++) { > for (int j = 0, max2 = tab[i].length; i < max2; j++) { > System.out.print(tab[i][i]); > } > } > With the warning we would get j flagged as unread. So it is clearly a problem. > My 2 cents. Right, but the same should be warned then if I use "i=i+1". Either we detect it correctly or we drop it. If it only works in some cases then it lowers the trust in the tooling.
(In reply to comment #32) > (In reply to comment #31) > > (In reply to comment #30) > > > I did see some benefit raising more warnings for a limited set of situations. > > > However, if a majority agrees that we're not finding a simple, consistent > > > set of rules, I'm OK with reverting. > > For me this was a useful warning to spot potential problem in nested for loops. > > for example: > > int[][] tab = ...; > > for (int i = 0, max = tab.length; i < max; i++) { > > for (int j = 0, max2 = tab[i].length; i < max2; j++) { > > System.out.print(tab[i][i]); > > } > > } > > With the warning we would get j flagged as unread. So it is clearly a problem. > > My 2 cents. I'd support that. > Right, but the same should be warned then if I use "i=i+1". Either we detect it > correctly or we drop it. If it only works in some cases then it lowers the > trust in the tooling. I don't think that's a fair argument: the compiler never makes a promise to detect all silly programs. Thus you should NEVER trust that the compiler finds all your stupid mistakes. Only if it signals a mistake, there must be real reason. Compare to null-analysis, which is inherently conservative, i.e., it will definitely miss things that *could* be a problem. But if it reports something, the report must be justified. Let's not drop useful warnings simply because we didn't solve the halting problem first.
(In reply to comment #33) >[...] Thus you should NEVER trust that the compiler > finds all your stupid mistakes. Sorry, should have said: "finds all MY stupid mistakes" :)
Well, I could probably live with the fact that we only go for ++/--/... but what I don't want to see are the inconsistencies with other parts of the tooling. We could replace the term "read" in the problem message with e.g. "unused" to remove the inconsistency and be closer to reality and add the support for fields but that means work to update the string everywhere (Core, UI, doc) and also implement it for fields.
(In reply to comment #35) > Well, I could probably live with the fact that we only go for ++/--/... but > what I don't want to see are the inconsistencies with other parts of the > tooling. > > We could replace the term "read" in the problem message with e.g. "unused" to > remove the inconsistency and be closer to reality and add the support for > fields but that means work to update the string everywhere (Core, UI, doc) and > also implement it for fields. My intension for introducing a suboption was two-fold: 1) to give it a proper name telling the user that it only warns when the variable is not read anywhere else than its own assignment. (Hence there wont be a confusion about a++ being considered read even if the java search or anything else says its a read access - Yes, its a read access, but since it only assigns to itself, its just useless code, and thats why we're warning.) 2) since its a suboption for "local variable never read", it'll be clear that it doesnt warn for fields. Then maybe later we can also consider introducing a similar option for 'private' fields. I'm still a bit inclined for a new suboption, since many users have requested for this warning.
An additional option makes no sense. No one would enable the detection but disable that sub-option.
(In reply to comment #27) > > (In reply to comment #23) [...] > > > 2. it's not consistent with private fields > > Don't see what you mean with this. > The private field is not reported as unused while the local variable is. This was called out in bug 318571 comment 8, guess it fell through the cracks. The initial reactions at bug 318571 are to close it as won't fix, but this issue supposedly repeatedly gets requested.
(In reply to comment #35) > We could replace the term "read" in the problem message with e.g. "unused" to > remove the inconsistency and be closer to reality and add the support for > fields but that means work to update the string everywhere (Core, UI, doc) and > also implement it for fields. How about leaving the option as "Local variable is never read" but making compiler messages a tiny bit smarter: if a useless read is involved say: "The local variable {0} is never significantly read" viz. "The field {0}.{1} is never significantly read locally" (hm, is that good English grammar? is "locally" needed?) Thus, when search finds a read access to that field, the user may infer that this read access must be insignificant. I'm proposing this as a compromise between being explicit and not adding long winded explanations "you're kind of reading from that variable, but that read doesn't really count here because .." Extending the analysis to fields shouldn't be difficult, but I was waiting for a general "go" before diving into that.
> "The field {0}.{1} is never significantly read locally" > (hm, is that good English grammar? is "locally" needed?) This sounds too technical. I would go for "unused" since we already use that term in the preferences UI (Unused local or private member). Also, I would replace "locally" with "private" ==> The private field {0} is never used ==> The local variable {0} is never used ==> The parameter {0} is never used This would align with the other "unused" message like "The import {0} is never used". If Olivier agrees with that and Stephan signs up for the JDT Core work, then I'm fine with it and would also be willing to align the UI side.
I reopen and assign to Stephan. (In reply to comment #39) > Extending the analysis to fields shouldn't be difficult, but I was waiting > for a general "go" before diving into that. Stephan, feel free to investigate. If we end up with a consistent story for locals and fields, then this will be released.
Ayush, can you please try the latest version against SDK source? I wonder how many false positives we get with this (e.g. loops that just loop n-times without accessing the variable otherwise).
Stephan, do you think you can take care of this for M3? If not, please adjust the target milestone. Thanks.
(In reply to comment #43) > Stephan, do you think you can take care of this for M3? I can do it for M3, no problem, scheduling to work on it in week 42. FYI, I'm still in state "Waiting for webmaster to provision.". (In reply to comment #42) > Ayush, can you please try the latest version against SDK source? I wonder how > many false positives we get with this (e.g. loops that just loop n-times > without accessing the variable otherwise). Has anybody seen an actual false positive? I wouldn't know how that could be constructed, even this loop would go without a warning: for (int i=0; i<n; i++); ("i" is used in "i<n")
(In reply to comment #44) > Has anybody seen an actual false positive? Nope, but it's always good to be prepared. We had cases before where we released it and then it caused many unwanted warnings.
Created attachment 181232 [details] updated patch - covering also private fields This patch implements Dani's concerns (on top of the previous patch): + Extend the analysis to private fields. + Change wording in warning message. I briefly considered changing all 'unused' messages to avoid the "locally" thing, but then I realized that those messages also cover public members in private types, so saying "The private field A.foo is never used" is not always correct. For that reason I introduced IProblem.UnusedScopedField (for non-privates in a private type) so we can give a more precise warning message for really-private fields. A similar distinction *could* be introduced for other kinds of members, too. While contemplating the "used locally" phrase, I came to prefer something like "used within its scope", but again this might be too technical? The point is that "locally" doesn't really tell where usages are searched and why. Tests are included for field access using SingleNameReference, QualifiedNameReference and FieldReference, plus one test for non-private fields in a private type. I ran and updated all tests in compiler.regression - other suites pending.
Created attachment 181235 [details] update - more wording adjusted I forgot to change the wording in warnings re locals and parameters. Done. Otherwise this is the same patch as described in comment 46.
(In reply to comment #47) > Created an attachment (id=181235) [details] [diff] > update - more wording adjusted > > I forgot to change the wording in warnings re locals and parameters. Done. > > Otherwise this is the same patch as described in comment 46. Thanks for the patch. Stephen, can you please raise at the top right of this page, and put in my email id in the field next to it? This is the usual way of asking for review from a particular person. :)
(In reply to comment #48) > [..] > Thanks for the patch. Stephen, can you please raise at the top right of this > page, and put in my email id in the field next to it? This is the usual way of > asking for review from a particular person. :) Oops! I meant "please raise the review flag at the top right of this page".
Created attachment 181303 [details] Updated patch Same patch with updated copyrights
(In reply to comment #50) > Created an attachment (id=181303) [details] [diff] > Updated patch > > Same patch with updated copyrights The patch has some shortcomings. If a field is only used in post increment, or compound assignment statements, the IsCompoundAssignedFlag on it will be set. So even a statement like return field++; will cause the flag to be set, even though it actually uses this field. Now when we start the code gen stage, if there was a plain compound assignment statement before the return statement, it will cause the warning to be raised because of the IsCompundAssignedFlag being set. This leads to a false positive. Ex: public class PField { private int a; // shouldn't warn here, but does. public int prints() { a++; return a++; // using a } } This problem doesnt occur if there's any expression in the code which uses the field in a non-postincrement or non-compoundAssignment way. hence this code works fine: public class PField { private int a; // shouldn't warn here, but does. public int prints() { a++; return a; } } (Same is the case with public field of private type) Stephen, I think that we should perhaps use a flag other than IsCompoundAssigned? Also, I'm not comfortable with org.eclipse.jdt.internal.compiler.ast.SingleNameReference.reportOnlyUselesslyReadPrivateField(BlockScope, Reference, FieldBinding) being a static method in SingleNameReference. It looks misplaced in that class. Wouldn't it be better to have it in FieldBinding, and then instead of passing the binding as parameter, call the method on the binding? We can move this to M4 if required.
Two comments: Should not the method org.eclipse.jdt.internal.compiler.ast.SingleNameReference.reportOnlyUselesslyReadLocal(BlockScope, LocalVariableBinding) be static ? If yes, I would move both methods org.eclipse.jdt.internal.compiler.ast.SingleNameReference.reportOnlyUselesslyReadLocal(BlockScope, LocalVariableBinding) org.eclipse.jdt.internal.compiler.ast.SingleNameReference.reportOnlyUselesslyReadPrivateField(BlockScope, Reference, FieldBinding) in the class org.eclipse.jdt.internal.compiler.ast.Reference. I found a potential problem line 883 in org.eclipse.jdt.internal.compiler.ast.SingleNameReference.reportOnlyUselesslyReadLocal(BlockScope, LocalVariableBinding). The cast should be AbstractMethodDeclaration and not simply MethodDeclaration. If the referenceContext is a constructor, we would get a CCE.
(In reply to comment #51) > We can move this to M4 if required. These are not big changes, so I think we should still target M3 unless something really bad shows up. To be part of M3, the fix needs to be ready for the I-build on Monday night.
(In reply to comment #53) > (In reply to comment #51) > > We can move this to M4 if required. > These are not big changes, so I think we should still target M3 unless > something really bad shows up. > To be part of M3, the fix needs to be ready for the I-build on Monday night. Did you already measure how many additional warnings this causes in the SDK?
(In reply to comment #54) > Did you already measure how many additional warnings this causes in the SDK? No. I plan to do this when the patch is fully ready.
(In reply to comment #55) > (In reply to comment #54) > > Did you already measure how many additional warnings this causes in the SDK? > No. I plan to do this when the patch is fully ready. OK. Just note that the Monday I-build is too late as we also have to do the UI. If the fix is not in by Monday morning Europe time (CEST), we have to shift it to M4.
(In reply to comment #51) > The patch has some shortcomings. If a field is only used in post increment, or > compound assignment statements, the IsCompoundAssignedFlag on it will be set. > So even a statement like return field++; will cause the flag to be set, even > though it actually uses this field. Now when we start the code gen stage, if > there was a plain compound assignment statement before the return statement, it > will cause the warning to be raised because of the IsCompundAssignedFlag being > set. This leads to a false positive. You're right. The same also holds for the case of local variables due to similarly constructed logic. > Stephen, I think that we should perhaps use a flag other than > IsCompoundAssigned? What do you mean? A different flag value? Or a separate boolean rather than squeezing into tagBits? Why? > Also, I'm not comfortable with > org.eclipse.jdt.internal.compiler.ast.SingleNameReference.reportOnlyUselesslyReadPrivateField(BlockScope, > Reference, FieldBinding) being a static method in SingleNameReference. It looks > misplaced in that class. Wouldn't it be better to have it in FieldBinding, and > then instead of passing the binding as parameter, call the method on the > binding? I liked it being close to its sibling reportOnlyUselesslyReadLocal. For that reason I actually prefer Olivier's proposal to put both methods into Reference. > We can move this to M4 if required. I'll see whether I can come up with a quick solution.
(In reply to comment #56) > OK. Just note that the Monday I-build is too late as we also have to do the UI. > If the fix is not in by Monday morning Europe time (CEST), we have to shift it > to M4. A fix for the UI can be prepared before this is completely finalized. All comments concern internal implementation details. What changes are actually needed in the UI besides rephrasing some compiler options ?
Created attachment 181409 [details] Updated patch Updated patch with the fix for wrong cast line 883
(In reply to comment #58) > (In reply to comment #56) > > OK. Just note that the Monday I-build is too late as we also have to do the UI. > > If the fix is not in by Monday morning Europe time (CEST), we have to shift it > > to M4. > A fix for the UI can be prepared before this is completely finalized. Yep, but we (UI) still have other stuff on the plate for M3 and I want to avoid spending time for a feature that might not make it. If it's in on Monday morning CEST we'll take it. > What changes are actually needed in the UI besides rephrasing some compiler > options ? We might want to adjust the wording.
(In reply to comment #52) [This comment collided with Olivier's updates and clicking "submit only my comments" made bugzilla crash, oh my ;-) ] > I found a potential problem line 883 in > org.eclipse.jdt.internal.compiler.ast.SingleNameReference.reportOnlyUselesslyReadLocal(BlockScope, > LocalVariableBinding). The cast should be AbstractMethodDeclaration and not > simply MethodDeclaration. > > If the referenceContext is a constructor, we would get a CCE. Yes, and I think this if (currentScope instanceof MethodScope) { should be replace with MethodScope methodScope = currentScope.methodScope(); if (methodScope != null) { Olivier: as I'm in the middle of preparing an updated patch: does your update in comment 59 contain any other changes than fixing the CCE?
(In reply to comment #61) > Olivier: as I'm in the middle of preparing an updated patch: does your > update in comment 59 contain any other changes than fixing the CCE? Besides the copyright changes, I don't think so.
Created attachment 181419 [details] patch v7 This patch fixes the issues identified in comment 50 et al: Since we are using the 'valueRequired' flag from code gen phase, we need to know when all compound usages have been seen. For that reason I replaced the modifier bit AccCompoundUsed with a field "public int compoundUseFlag" in FieldBinding, for counting up during resolve and counting back down during code gen. Only when none of the compound usages require a value, the warning is issued. For local variables I encoded the same information into LocalVariableBinding.useFlag: values < 0 now indicate the number of compound usages. Also contained: - moved two methods from SingleNameReference to Reference. - fixed CCE - fixed issue of nested scope within MethodScope We still need more tests for the latest issues.
(In reply to comment #63) > We still need more tests for the latest issues. I'll add one for the CCE case I found.
(In reply to comment #54) > Did you already measure how many additional warnings this causes in the SDK? 70 new warnings on the whole SDK code. I checked the first ones and they are all valid detections. No false positives. I don't have time to check all the 70 warnings.
Created attachment 181439 [details] patch v8 This is my last version for today: + code cosmetics: put more logic into the new reportOnlyUselesslyReadLocal(..) less inside methods of SingleNameReference + added a few variations in existing tests: + multiple postIncr. of the same variable (comment 51) + issue of usage inside nested block (comment 61)
(In reply to comment #65) > (In reply to comment #54) > > Did you already measure how many additional warnings this causes in the SDK? > 70 new warnings on the whole SDK code. > I checked the first ones and they are all valid detections. No false positives. > I don't have time to check all the 70 warnings. That's more than I expected. Maybe the warning will be useful after all :)
(In reply to comment #67) > That's more than I expected. Maybe the warning will be useful after all :) Definitely. All the examples I can check can be cleaned and this makes the code easier to read. Thanks for the patch.
One more consideration regarding the wording: The message for private fields was changed in bug 34896 from "never used" to "never read". For locals it was changed even earlier (commit on 2002/01/22 - no bug ref available). It could be (and bug 102819 could be an indication) that some people are confused about a variable not being "used" although it *is* written to. Maybe the message was then changed to "never read" to appease those people? When changing (back) to "never used" people might start again to blame the compiler. I still feel that throwing in "significantly" might lead some people to think ("are my uses of the variable insignificant??") before blaming the compiler. I admit that my proposal "The field {0}.{1} is never significantly read locally" was horrible, but how about: "The field {0}.{1} is never significantly used" ? I believe when fixing Bug 328281 (next on my agenda) we can actually drop the suffix "locally" with no loss of information or precision. Ideally we would add "significantly" only when a write are compound access exists, ie. say "never used" only if there's no access at all. I can certainly live with the current patch "never used (locally)", but why not try to avoid bogus bug reports while we still can :)
Created attachment 181481 [details] patch v8 + one more test I added the test for the CCE case as test0051 in ProgrammingProblemsTest. Patch looks good. The above comment about the message wording is a valid one. Dani, can you please advice on the best possible approach here? If you think the wording is correct as given in the patch, Stephen can release the patch.
> The above comment about the message wording is a valid one. Dani, can you > please advice on the best possible approach here? If you think the wording is > correct as given in the patch, Stephen can release the patch. I'll look at it this afternoon, including the UI.
I've tested (but not reviewed) the patch. It revealed some interesting bad code pieces. Good work! I had a lengthy discussion with Markus about the wording. We should try to have a unique wording for the user in both the preference UI and the problem message. Markus made a good point that it's all about whether the *value* of the variable is used or not. This covers the old/existing and the additional cases. "The value of the field {0}.{1} is not used" "The value of the local variable {0} is not used" "The value of the parameter {0} is not used" Stephan, please adjust the messages and commit the code to HEAD. I've changed the UI options in HEAD accordingly: Value of local variable is not used: Value of parameter is not used: BTW: Do the changes affect org.eclipse.jdt.core.JavaCore.COMPILER_CODEGEN_UNUSED_LOCAL? I've filed bug 328481 to adjust our quick fixes.
(In reply to comment #72) > BTW: Do the changes affect > org.eclipse.jdt.core.JavaCore.COMPILER_CODEGEN_UNUSED_LOCAL? In fact the unused local is no longer optimized out. I am investigating. I have one last comment: In org.eclipse.jdt.internal.compiler.ast.Reference line 134: if (localBinding.useFlag != 0) I would replace '0' with LocalVariableBinding.UNUSED. I am checking the optimization issue. This should be trivial to fix.
(In reply to comment #73) > In fact the unused local is no longer optimized out. I am investigating. The problem is related to the fact that the new locals tagged as unused with this new diagnostic was never meant to be optimized out in the past. I have a few changes in the generateCode to optimize out the local if its resolvedPosition is -1. Stephan, do you want a new patch or you take care of this yourself ?
(In reply to comment #74) > (In reply to comment #73) > > In fact the unused local is no longer optimized out. I am investigating. > The problem is related to the fact that the new locals tagged as unused with > this new diagnostic was never meant to be optimized out in the past. > I have a few changes in the generateCode to optimize out the local if its > resolvedPosition is -1. > > Stephan, do you want a new patch or you take care of this yourself ? I can easily take care of wording and " != 0", just regarding the generateCode optimization I don't exactly see what you mean, so if you could provide what you have by updating the patch I can add the other issues afterwards and I will then release later today, OK?
(In reply to comment #72) > I had a lengthy discussion with Markus about the wording. We should try to have > a unique wording for the user in both the preference UI and the problem > message. Markus made a good point that it's all about whether the *value* of > the variable is used or not. Very good point, indeed. > This covers the old/existing and the additional cases. > > "The value of the field {0}.{1} is not used" > "The value of the local variable {0} is not used" > "The value of the parameter {0} is not used" > > Stephan, please adjust the messages and commit the code to HEAD. I'll happily do that. Just to double check: since the above message does not say "private field" I assume the same message shall be used for private fields and non-private fields in private classes, right? If so, I can remove the additional IProblem (UnusedScopedField) again.
> Just to double check: since the above message does not say > "private field" I assume the same message shall be used for private > fields and non-private fields in private classes, right? Yep.
(In reply to comment #75) > I can easily take care of wording and " != 0", just regarding the generateCode > optimization I don't exactly see what you mean, so if you could provide what > you have by updating the patch I can add the other issues afterwards and I will > then release later today, OK? This is what I mean: import java.util.List; import java.util.ArrayList; import java.util.Iterator; public class X { public static void main(String[] args) { List l = new ArrayList(); int i = 0; for (Iterator iter = l.iterator(); iter.hasNext(); i++) { System.out.println(iter.next()); } } } If you compile this with the batch option without -preserveAllLocals, I would expect the 'i' local from not being generated at all. Since I am not sure anymore, I have the latest patch with the right changes in it, I would prefer that you take care of the changes on your side. Let me know if you have a problem with fixing this issue. We can also let this as is and fix this optimization issue later on as a separate patch for clarity.
(In reply to comment #78) > This is what I mean: > import java.util.List; > import java.util.ArrayList; > import java.util.Iterator; > > public class X { > public static void main(String[] args) { > List l = new ArrayList(); > int i = 0; > for (Iterator iter = l.iterator(); iter.hasNext(); i++) { > System.out.println(iter.next()); > } > } > } > > If you compile this with the batch option without -preserveAllLocals, I would > expect the 'i' local from not being generated at all. > Since I am not sure anymore, I have the latest patch with the right changes in > it, I would prefer that you take care of the changes on your side. > Let me know if you have a problem with fixing this issue. We can also let this > as is and fix this optimization issue later on as a separate patch for clarity. I got your patch and started to analyze what it does to our existing test cases. However, we seem to be stuck here, because we only know *after* generateStatements which locals are actually used. Not generating purely "compound used" variables is asking for trouble, if it turns out that the value of "i++" is actually used. Right now I don't see an easy way out, so I will leave at the previous state, i.e., locals with useFlag < 0 will be generated. Rambling about possible solutions I could think of: - restarting generateCode if unused state of a local was detected late, but I don't think the infrastructure for such restart is in place - change signatures of all analyzeCode method adding a flag valueRequired just as we have in generateCode -> need to change all AST classes below Expression (Statement?). Unless I'm missing something we should really not worry about those uselessly generated locals. We issue a warning and the user can do that optimization manually from there on :)
> Rambling about possible solutions I could think of: > - restarting generateCode if unused state of a local was detected late, > but I don't think the infrastructure for such restart is in place > - change signatures of all analyzeCode method adding a flag valueRequired > just as we have in generateCode -> need to change all AST classes below > Expression (Statement?). Let's keep it as is for now. Just make the error message changes and we can take a look at this later. As it stands, the warning is useful and I don't want to postpone it just to solve this small issue. Please open a bug report to keep track of it as soon as the fix for this bug is released. Thanks for your contribution on this, Stephan!
Created attachment 181557 [details] patch v11 based on Ayush's last version this patch - changes the wording according to comment 72 - reverts some changes around IProblem.UnusedScopedField - replaces 0 by constant according to comment 73 Tests in package compiler.regression passed, still awaiting results from full run of tests.compiler plugin.
(In reply to comment #80) > Please open a bug report to keep track of it as soon as the fix for this bug is > released. Done, it's bug 328519.
(In reply to comment #83) > Done, it's bug 328519. Thanks, I'll take a look.
Verified for 3.7M3 using build I20101025-0901.
Hello, I've discovered accidentally this bug happening again even on ecj 4.4.1 I did: int i = 0; for (....) { System.out.println("I is now: " + i++ + "."); }
(In reply to Rumen Nikiforov from comment #86) > Hello, > > I've discovered accidentally this bug happening again even on ecj 4.4.1 > I did: > int i = 0; > for (....) { > System.out.println("I is now: " + i++ + "."); > } This example is intentionally not covered by this bug: the value if i *is* used for the println. There's no way you could remove i without changing the observable behaviour of that snippet.
(In reply to Stephan Herrmann from comment #87) > (In reply to Rumen Nikiforov from comment #86) > > Hello, > > > > I've discovered accidentally this bug happening again even on ecj 4.4.1 > > I did: > > int i = 0; > > for (....) { > > System.out.println("I is now: " + i++ + "."); > > } > > This example is intentionally not covered by this bug: the value if i *is* > used for the println. There's no way you could remove i without changing the > observable behaviour of that snippet. The exception was thrown from the the part of the code that there was link to this bug report. SingleNameReference#generatePostIncrement ------------------------------ case Binding.LOCAL : // assigning to a local variable LocalVariableBinding localBinding = (LocalVariableBinding) this.binding; // https://bugs.eclipse.org/bugs/show_bug.cgi?id=185682 // check if postIncrement is the only usage of this local Reference.reportOnlyUselesslyReadLocal(currentScope, localBinding, valueRequired); if (localBinding.resolvedPosition == -1) { if (valueRequired) { // restart code gen localBinding.useFlag = LocalVariableBinding.USED; throw new AbortMethod(CodeStream.RESTART_CODE_GEN_FOR_UNUSED_LOCALS_MODE, null); } return; } ------------------------------
(In reply to Rumen Nikiforov from comment #88) > (In reply to Stephan Herrmann from comment #87) > > (In reply to Rumen Nikiforov from comment #86) > > > Hello, > > > > > > I've discovered accidentally this bug happening again even on ecj 4.4.1 > > > I did: > > > int i = 0; > > > for (....) { > > > System.out.println("I is now: " + i++ + "."); > > > } > > > > This example is intentionally not covered by this bug: the value if i *is* > > used for the println. There's no way you could remove i without changing the > > observable behaviour of that snippet. > > The exception was thrown from the the part of the code that there was link > to this bug report. I'm confused now. What problem are you seeing? Lack of a warning, where you expected a warning? An exception thrown by the compiler? Note that throwing AbortMethod does not indicate a compiler bug. It is a means for aborting a part of compilation and restarting it in a different mode.
