Bugzilla – Full Text Bug Listing
| Summary: | "Method can be static" warning on method that accesses instance field in inner class | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Nick Radov <nradov> | ||||||
| Component: | Core | Assignee: | Ayushman Jain <amj87.iitr> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | minor | ||||||||
| Priority: | P3 | CC: | amj87.iitr, deepakazad, satyam.kandula, srikanth_sankaran, stephan.herrmann | ||||||
| Version: | 3.8 | Flags: | srikanth_sankaran:
review+
stephan.herrmann: review+ |
||||||
| Target Milestone: | 3.8 RC1 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows 7 | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
Thanks for the test case. I will take a look. Created attachment 215226 [details]
proposed fix v1.0 + regression tests
For the current bug, the problem was in BlockScope.resetEnclosingMethodStaticFlag() i.e. we expected a method declaration to be associated with a method scope but in this case there is a type declaration instead, since this is an intiliazation scope. So the resetting of the CanBeStatic flag did not happen for go().
However, digging deeper I found that the current way of resetting the flag of all enclosing methods is over-eager and even if a local class method cannot be static, there can be cases where the enclosing method can be static. To really reset the CanBeStatic only upto the enclosingMethod which actually cannot be static, I added BlockScope.resetDeclaringClassMethodStaticFlag(TypeBinding). Take for example, the case below:
class X{
int i1= 1;
void foo() {
class Local {
int i2 = 1;
void localMethod() {
i2 = 1;
}
}
}
}
The localMethod cannot be static because of an unqualified access to an instance field of its declaring class, however foo() can be still be static. Hence just finding an unqalified field reference is not a necessary condition for all enclosing methods to not be static. We need to find where the field is actually declared and limit the resetting upto that scope.
Srikanth, can you review for RC1? This fix should make this diagnostic more usable. (In reply to comment #3) > Srikanth, can you review for RC1? This fix should make this diagnostic more > usable. Sure thing. Marking Stephan as an additional reviewer. Stephan, Thanks in advance. Patch looks alright to me - One thing that looked strange is though is the difference between the code blocks in FieldReference and MessageSend. What justifies this change ? Created attachment 215387 [details] proposed fix v1.1 + regression tests (In reply to comment #5) > Patch looks alright to me - One thing that looked strange is though > is the difference between the code blocks in FieldReference and MessageSend. > What justifies this change ? They should both be the same. Only the enclosing method's flag should be reset, since using a 'this' reference, we cannot call an outer class method anyway. I corrected it in the new patch. Released in master via commit 5903b5532387cc7ed73791cbff2d52e4ceee6d52. Stephan, you can probably take this up for verification and do the review alongside. Thanks! Found a minor problem with this fix. Filed bug 379784 to take care of this. Verified for 4.2RC1 using build I20120516-1900. How about this case:
public class X<T> {
public void foo() {
java.util.List<T> k;
}
}
It reports "The method foo() from the type X<T> can potentially be declared as static" but adding "static" yields of course:
"Cannot make a static reference to the non-static type T".
Is that the meaning of "can potentially" (i.e., a known limitation of the analysis), or should I file a new bug for that?
I checked that it has been this way before (3.7 and all 3.8 milestones).
BTW: I've added other cases where the analysis is shyer than necessary to bug 379784.
(In reply to comment #9) > How about this case: > > public class X<T> { > public void foo() { > java.util.List<T> k; > } > } > > It reports "The method foo() from the type X<T> can potentially be declared as > static" but adding "static" yields of course: > "Cannot make a static reference to the non-static type T". > > Is that the meaning of "can potentially" (i.e., a known limitation of the > analysis), or should I file a new bug for that? The only difference between "can potentially" and "can" is that the latter gets reported for methods that won't be overriden. So try private/final methods to toggle between the two. Please add your testcase and anything else you find related to type parameters to bug 378674. Another fup (not a regression) is in Bug 379834. +1 from me, considering that all the issues I found already existed before this patch. |
Build Identifier: 20120216-1857 The "Method can be static" code style warning does not properly account for anonymous inner classes declared within methods. Reproducible: Always Steps to Reproduce: 1. From the menu select Window, Preferences. 2. Go to Java, Compiler, Errors/Warnings. 3. In the Code Style section set "Method can be static" to "Warning". 4. Compile the following code. import java.util.ArrayList; import java.util.Collection; public class CanBeStatic { private Object o = new Object(); public final Collection<Object> go() { return new ArrayList<Object>() { { add(o); } }; } } Notice that you get the "can be declared as static" on the go() method. But the warning is incorrect: if you declare the method as static then it won't compile because it references an instance field.