Community
Participate
Working Groups
Build Identifier: If the setting Java-> Compiler-> Errors & Warnings/Unnessesary code is changed, and "Local variable is never read" is set to Warning or Error, we get a warning if the variable is created and set to a value but never used. Which is correct. But if we increase it by one by using ++ it is not warned for anymore. Reproducible: Always Steps to Reproduce: 1.Set compiler to warn for not used variables. 2. Write: int i==; Warning is raised here which is correct. 3.Add line: i++; Now the warning is gone which is not correct. It is still not used.
Created attachment 173190 [details] Change setting
Created attachment 173191 [details] This is warning OK
Created attachment 173192 [details] The warning is missing here. This is not correct.
I don't see a bug here, this is legitimate, reasonable and expected behavior from the compiler. i's value has been read and has been incremented and written back. It is the computed result that is not used any further. i itself has been used and hence the absence of the warning is justified. The compiler cannot divine anymore than this, one can easily put together a series of "uses" that don't amount to any real use for a human reader.
(In reply to comment #4) > I don't see a bug here, this is legitimate, reasonable and expected > behavior from the compiler. i's value has been read and has been > incremented and written back. It is the computed result that is > not used any further. i itself has been used and hence the absence > of the warning is justified. I totally agree. It's not a bug ... > The compiler cannot divine anymore than this, one can easily put > together a series of "uses" that don't amount to any real use for > a human reader. ... no, the compiler can't read the programmers mind, but in this particular situation I would indeed consider a small enhancement: Given that the compound operator is actually an optimized syntax for a read & and a write, the compiler could easily just for this situation consider that the value of "i" has flown into no other location than back into "i". The same would hold for all compound assignments, so "i+=3" doesn't "get the value out off i" either. Put differently: deleting the variable i will not change the semantics of the program. I made a quick experiment disabling these lines in SimpleNameReference #analyseAssignment(..): if (isCompound) { // check the variable part is initialized if blank final ... // if (isReachable) { // localBinding.useFlag = LocalVariableBinding.USED; // } else if (localBinding.useFlag == LocalVariableBinding.UNUSED) { // localBinding.useFlag = LocalVariableBinding.FAKE_USED; // } } This effectively enables the warning for the situations discussed here. I'm currently running tests from compiler.regression and apparently none of the existing tests will be affected by this deletion. Srikanth, do you see other tests that would be relevant for this situation? My take on this would be: + JDT could make a few people happy who will detect that a local variable was indeed not used in the way they intended - JDT will confuse a few people seeing a warning in code that previously was without warning, but I hold that for all occurrences of the newly raised warning the code is indeed suspicious/wrong and deserves a closer look. I suggest that the pro outweighs the con. WAIT, just before submitting this comment I found an example that breaks with the simple deletion of 5 lines: int foo() { int i = 0; return i++; } If the compound expression itself is used as an expression the warning must not be shown. So the solution would require a second look, which I'm happy to do, if the direction is generally accepted.
Created attachment 173258 [details] proposed patch OK, the initial patch by just deleting 5 lines actually broke some tests in ForeachStatementTests. So, enough hand-waving :) Here's a patch that looks promising to me. Given this program: public class Bug318571 { int foo() { int i=1; boolean b=false; b|=true; int k = 2; --k; k+=3; return i++; } } The compiler now complains: ---------- 1. WARNING in Bug318571.java (at line 4) boolean b=false; ^ The local variable b is never read ---------- 2. WARNING in Bug318571.java (at line 6) int k = 2; ^ The local variable k is never read ---------- 2 problems (2 warnings) Note, that i is not complained about, nor is k reported twice. As I said, could be considered as an enhancement, take it or leave it as you like.
Stephen, I don't have a very good feeling about showing "not read" warnings for these cases when we know that the compiler cannot increment or affect the value of a variable without having read it in the first place. I agree with Srikanth that one can easily put together a series of "uses" that don't amount to any real use for a human reader. Had this warning been present for cases like i++, I'd surely have filed a bug! And there may be others out there like me. :)
(In reply to comment #6) > Created an attachment (id=173258) [details] > proposed patch Thanks for the patch Stephen, it looks very specific to locals and SingleNameReference though, so will not behave consistently across fields, QualifiedNameReference etc. However, before you spend more cycles on repairing it, let us also ask for opinion from Olivier on this. I personally am inclined to leave it as it is. I don't know what the return on (even a low) investment will be - I suspect it to be low. For the time being I'll close this as a duplicate of bug 185682. See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=135684 *** This bug has been marked as a duplicate of bug 185682 ***
(In reply to comment #7) > I agree with Srikanth that one can easily put together a series of > "uses" that don't amount to any real use for a human reader. Sure, if we take the human reader as the criterion this would be a never-ending story, agreed. I was more thinking of: - does removing the variable change the semantics of the program? - can the compiler analyze this without real flow-analysis? I was quickly convinced just by the fact that the language provides specialized syntax so the thing can be analyzed purly syntactically: any postIncrement or compoundAssignment used as a statement is not a "real" read - and some Java programmers may not even realize that it technically involves a read! :) (In reply to comment #8) > Thanks for the patch Stephen, it looks very specific to locals > and SingleNameReference though, so will not behave consistently > across fields, QualifiedNameReference etc. OK, to this point it only addresses the unread-local warning. If unused-private-field should be treated consistently, I'm sure this could be done in analogy. > For the time being I'll close this as a duplicate of bug 185682. Yep, it's exactly the same issue. > See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=135684 That's an interesting pointer, way more sophisticated than the issue in this bug.
(In reply to comment #8) > Thanks for the patch Stephen, it looks very specific to locals > and SingleNameReference though, so will not behave consistently > across fields, QualifiedNameReference etc. > However, before you spend more cycles on repairing it, let us also > ask for opinion from Olivier on this. We get this kind of bug reports for this "problem" at least once a year. So even if I was not inclined to get it fixed in the past, I think we might want to fix it this time. prefix/postfix increments are doing a "read" under the cover, but for most users this is not a "read". Now if we report that the variable is not read, this means it can be removed without any semantic changes to the program. I tried Stephan's patch on this code (which is stupid, I admit): public class X { public static void main(String[] args) { Integer i = null; i++; } } and I got: ---------- 1. WARNING in D:\tests_sources\X.java (at line 3) Integer i = null; ^ The local variable i is never read ---------- 2. WARNING in D:\tests_sources\X.java (at line 4) i++; ^ Null pointer access: The variable i can only be null at this location ---------- I don't expect the first warning in this case as the i++ is causing a NPE. So removing it has an effect on the semantic of the program. I am sure we can fix it in a way that would produce the expected diagnosis in all cases. So yes, I think we should "improve" the error reporting in this case and be more accurate to report only unused warnings for locals that are really not read. 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. We can continue the discussion in bug 185682.
Verified.
The file I used is called eclipse-SDK-3.6.1-win32.zip ..and I am very sorry about many equal comments... got som message about unsynced pages.. and did not realize my comments piled up at the bottom...
Sorry again.. wrong message report.. :-)
