Bugzilla – Full Text Bug Listing
| Summary: | Compiler reports unused private field which is read inside an anonymous inner class within its constructor | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Andreas Haufler <aha> |
| Component: | Core | Assignee: | JDT-Core-Inbox <jdt-core-inbox> |
| Status: | CLOSED WONTFIX | QA Contact: | Sasikanth Bharadwaj <sasikanth.bharadwaj> |
| Severity: | normal | ||
| Priority: | P3 | CC: | jarthana, shankhba, srikanth_sankaran, stephan.herrmann |
| Version: | 3.8.1 | ||
| Target Milestone: | --- | ||
| Hardware: | Macintosh | ||
| OS: | Mac OS X | ||
| Whiteboard: | stalebug | ||
we fixed s.t. similar a while ago, I'll dig out that bug when I find the time. Shankha, please take a look. See why we think T is unused while it actually is. (Look for the references to IProblem.UnusedPrivateField and understand the surrounding code as a first step) Update:
Following piece of code seems to be responsible for the failure to recognize that T is used.
public final boolean isDefinedInField(FieldBinding field) {
Scope scope = this;
do {
if (scope instanceof MethodScope) {
MethodScope methodScope = (MethodScope) scope;
if (methodScope.initializedField == field) return true;
}
scope = scope.parent;
} while (scope != null);
return false;
}
The method is called through isFieldUseDeprecated.
This function marks if there is a use of the variable:
407: field.original().modifiers |= ExtraCompilerModifiers.AccLocallyUsed;
Call stack:
Daemon Thread [Compiler Processing Task] (Suspended)
MethodScope(Scope).isDefinedInField(FieldBinding) line: 3084
SingleNameReference(ASTNode).isFieldUseDeprecated(FieldBinding, Scope, int) line: 401
SingleNameReference.checkFieldAccess(BlockScope) line: 223
SingleNameReference.resolveType(BlockScope) line: 968
MessageSend.resolveType(BlockScope) line: 605
MessageSend(Expression).resolve(BlockScope) line: 970
MethodDeclaration(AbstractMethodDeclaration).resolveStatements() line: 510
MethodDeclaration.resolveStatements() line: 265
MethodDeclaration(AbstractMethodDeclaration).resolve(ClassScope) line: 469
TypeDeclaration.resolve() line: 1184
TypeDeclaration.resolve(BlockScope) line: 1269
QualifiedAllocationExpression.resolveType(BlockScope) line: 503
AllocationExpression.resolveType(BlockScope) line: 358
FieldDeclaration.resolve(MethodScope) line: 237
TypeDeclaration.resolve() line: 1120
TypeDeclaration.resolve(CompilationUnitScope) line: 1294
CompilationUnitDeclaration.resolve() line: 561
Compiler.process(CompilationUnitDeclaration, int) line: 770
ProcessTaskManager.run() line: 137
Thread.run() line: 722
(In reply to comment #3) > Update: > > Following piece of code seems to be responsible for the failure to recognize > that T is used. > > public final boolean isDefinedInField(FieldBinding field) { > Scope scope = this; > do { > if (scope instanceof MethodScope) { > MethodScope methodScope = (MethodScope) scope; > if (methodScope.initializedField == field) return true; > } > scope = scope.parent; > } while (scope != null); > return false; > } > > The method is called through isFieldUseDeprecated. > This function marks if there is a use of the variable: > > 407: field.original().modifiers |= ExtraCompilerModifiers.AccLocallyUsed; Seeing the comment // ignore cases where field is used from inside itself gives a strong hint that the behaviour is by design. BTW, it's always interesting to see, when a questionable piece of code was added, this particular check was added via commit 343b3de832b5278eaddde8a3101cc17190191f85, dated 03/03/03 :) Unfortunately, Philipe gave no comment on it, other than the inline code comments... The compiler assumes, that accessing T inside its own declaration doesn't yield any execution paths into relevantly reading T. Sounds reasonable? The example shows the point we're missing in this analysis: the side-effect of Test's constructor makes the inner instance accessible via an alias. We'd be hero, if we could detect this alias (via r), but we can't and will not be able. My gut feeling is: if we change anything here, we'll no longer be able to detect the following truly unused field: Runnable r = new Runnable() { public void run() { System.out.println(r); }; So what's better? Never reporting? Reporting one too many warnings? Here's a dogma: "Any warranty is void if constructors perform non-local side-effects!" "Neither humans nor compilers are very good in understanding such code" While this leads into the realm of discussing code styles, I'm leaning towards leaving everything as it is and simply asking the user to insert @SuppressWarnings("unused"), if he is keen on keeping this questionable design :) Or does anybody have an idea how we could reliably distinguish the examples from comment 0 and my simplified version here? This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |
The following code gets a warning in line 2 (The value of the field Test.T is not used - it is read): public class Test { private static final Test T = new Test(new Runnable() { @Override public void run() { System.out.println(T); } }); private static Runnable r; public Test(Runnable x) { r = x; } public static void main(String[] args) { new Thread(r).start(); } } javac -Xlint Test.java reports no errors or warnings at all. This example can be further reduced to: public class Test { private static final Test T = new Test(new Runnable() { @Override public void run() { System.out.println(T); } }); public Test(Runnable r) { r.run(); } public static void main(String[] args) { } } (In this case, T is always read before its initialization, but nevertheless it is not unused).