Community
Participate
Working Groups
Build Identifier: I would like an option under Compiler-> Errors/Warnings that said "Methods that could be static" ot something. If a method is using no fields and no calls to non-static functions it could and should be set as a static method. Would like some hints on how to find these methods. Reproducible: Always
This is my last report for a while. I have summerized one year of notations of possible bugs and enhanchment suggestions and got some time from my Company SAAB to check all these notations in new releases. I now have 19 reports listed and I am already amazed about how fast I get response, support and feedback. Thank you very much Eclipse Team!
Thanks for all your suggestions, Lars! By the way, if you have more and you are able, please try to set the component, as well as the project. You've reported them against the "APT" component, which is the Annotation Processing Tooling, which I think was not your intention. With regard to this present suggestion, I agree that would be a good feature. One thing you might want to do is install the FindBugs plugin, which will identify this problem as well as many others. I highly recommend it.
Sounds interesting. Will see what it takes.
Created attachment 180366 [details] proposed fix v1.0 + regression tests + doc This fix introduces the new option JavaCore.COMPILER_PB_METHOD_CAN_BE_STATIC and the new problem org.eclipse.jdt.core.compiler.IProblem.MethodCanBeStatic. The way it is implemented is as follows: We first mark every non-abstract, non-empty method as potentially static using the new bit ASTNode.CanBeStatic. This is done in MethodDeclaration#resolveCode(). Susequently, when each statement of the method is analysed in org.eclipse.jdt.internal.compiler.ast.MethodDeclaration.analyseCode(ClassScope, InitializationFlowContext, FlowInfo), we check for references to non-static fields and methods. This can be in the form of - field reference: ... abc = this.field; (#analyseCode(..)) ... or ... this.field = 1; (#analyseAssignment) ... - QualifiedNameReference: ... abc = X.field; ... or ... X.field = 1; ... - SingleNameReference: ... abc = field; ... or ... field = 1; ... - MessageSend: ... method1(); ... or ... this.method1(); ... or ... X.method1(); ... etc. Basically, since "this" reference is never allowed in static methods, we mark the method as cannot be static if we come across any explicit this reference. (Note that just referencing a method without a receiver viz. method1(), contains an implicit this reference, but this is allowed in static methods). For statements with no this receiver, we just need to check if its a non-static field/method. Tests added in org.eclipse.jdt.core.tests.compiler.regression.ProblemTypeAndMethodTest show different cases of how a method/field could be referenced.
Satyam, please review. TIA
The above patch introduces a new warning "The method {1}({2}) from the type {0} can be declared as static". There are a few aesthetic points pointed out by Satyam that we should probably decide upon: - With this patch, the warning is raised even for methods that override a superclass method and methods that are overriden in subclasses. While for the former cases, we can and we should hide the warning, its not possible to do so in the latter case since we dont have the info about a method's overriders. Is it ok to show the warning in the latter case as well? IMO, its safe to show the warning since it only says "the method can be static" - Should we have just one warning or two warnings saying "should be static" and "potentially static"? In case a class is final, or method is private and the method has passes the analysis to be declared static, we can say with surety that it should be static since it cant be overriden. While in other cases, including the case pointed out in the previous comment, we are not 100% sure since the method may be overriden. Olivier, what's your opinion about these points?
Created attachment 181037 [details] proposed fix v1.1 + regression tests + doc This patch adds a small change to the previous one- for overriding/implementing methods we dont raise this warning.
Created attachment 181082 [details] proposed fix v1.1.2 + regression tests Somehow the previous patch did not contain all the changed files. Submitting a fresh patch with all changes
*** Bug 40536 has been marked as a duplicate of this bug. ***
Markus, it'll be good to have your opinion as well on the points in comment 6.
(In reply to comment #6) > In case a class is final, or method is private and the > method has passes the analysis to be declared static, we can say with surety > that it should be static since it cant be overriden. While in other cases, > including the case pointed out in the previous comment, we are not 100% sure > since the method may be overriden. That's IMO a reason for not adding this "potential" problem. As stated in bug 101849 comment 4, we don't have the infrastructure to find overriding methods efficiently, so this will lead to false positives. And if we add a quick fix, people will rightfully complain that we offer bad advice and the quick fix screws their code. If we add this, then we should have two options (similar to "Null pointer access" and "Potential null pointer access"), such that people can at least disable the one that reports false positives. And someone would have to come up with good names for the options. (In reply to comment #8) > Created an attachment (id=181082) [details] [diff] > proposed fix v1.1.2 + regression tests Typo in Javadoc and help: "a method has not be declared": "be" => "been".
Patch generally looks good. Here are some points to consider. 1. I think empty methods should not be flagged to be static. These are probably in place so that the sub types could override the behavior. 2. As Olivier had mentioned, the message probably can be improved which could indicate that it is not completely sure. Well, I don't have a better message as of now. 3. Having a problem with local classes using the outer class's variable. In the given test case foo should not have been reported as can be static. public class Test { public String name; public void foo() { (new Object() { public boolean foo1() { return Test.this.name == null; } }).foo1(); return; } } 4. If a method uses Type parameters of the given type, that method probably cannot be static. 5. Accesses to super also have to be taken care. 6. In ASTNode.java, CanBeStatic should be declared under the comment for method declaration. Right now, it is declared under the comment type, method and field. As this field is for only methods, it should be declared elsewhere.
Discussing with Markus, since we cannot properly detect the case where a method is overriden in subclass, this check will lead to too many false positives. So we think we should not continue to investigate this issue and close as WONTFIX. Ayushman, do you agree with this ?
(In reply to comment #13) > Discussing with Markus, since we cannot properly detect the case where a method > is overriden in subclass, this check will lead to too many false positives. > So we think we should not continue to investigate this issue and close as > WONTFIX. > Ayushman, do you agree with this ? I dont think we should just give up. I discussed with Satyam and Srikanth as well. There are 2 things here that we need to consider: 1. SHOULD BE STATIC: Methods that are private, or final, or inside a final class, cannot be overriden at all. So atleast in these cases we're sure that if a method passes our analysis to be static, we can and should emit this warning, and maybe make it more concrete to say the method should be marked as static. 2. POTENTIALLY STATIC: These are the cases in which we dont know whether a method is being overriden. Even in these cases I think we should emit a warning, but change the wording a bit. The basic idea is that this is not really a compilation error, but a compiler suggestion to improve code aesthetics. I have seen many developers just care about writing a working code, and aesthetics take the last priority. So in some case where a developer has unintentionally missed out declaring a method as static, this suggestion will atleast alert him. To have a warning which says "The method ... can be declared as static, if it is not overriden elsewhere" (or something similar), will be good and will not be a "false positive" per se! The user can atleast look at the warning and decide to exercise the quick fix *IF* he wants.
(In reply to comment #14) > [..] Olivier/Markus, please let me know your opinion. If the suggested changes seem viable, i'll submit a fresh patch incorporating them. As for the names, how do COMPILER_PB_METHOD_POTENTIALLY_STATIC, and COMPILER_PB_METHOD_SHOULD_BE_STATIC sound?
OK, we can do it if the options are split into a reliable one and a potential hint. But I don't want to rush it into M3, and UI needs more time to add the quick fixes and a Clean Up for the safe option. > As for the names, how do > COMPILER_PB_METHOD_POTENTIALLY_STATIC, and COMPILER_PB_METHOD_SHOULD_BE_STATIC > sound? Please look at the options we already have. The options names should be more modest and describe what problems they find (and not prescribe a solution using works like "should").
(In reply to comment #16) > Please look at the options we already have. The options names should be more > modest and describe what problems they find (and not prescribe a solution using > works like "should"). Hmm. How about COMPILER_PB_MISSING_STATIC_ON_METHOD, and COMPILER_PB_POTENTIAL_MISSING_STATIC_ON_METHOD ? This is on the lines of COMPILER_PB_MISSING_SYNCHRONIZED_ON_INHERITED_METHOD.
COMPILER_PB_MISSING_STATIC_ON_METHOD sounds good. The second was grammatically wrong and should rather be: COMPILER_PB_POTENTIALLY_MISSING_STATIC_ON_METHOD.
I know I'm late to this party, so let my comment not stop you, but ... (In reply to comment #14) > The basic idea is that this is not > really a compilation error, but a compiler suggestion to improve code > aesthetics. IMHO this is strange quality metric. Put bluntly, I would read that warning as: "you are using object-oriented programming, but we advise you not to do so." Sure there are potential reasons for marking a method as static: (a) To be able to call it if you don't have an instance (b) To make sure you can read and understand the code without considering dynamic binding (c) To make the code faster. For (a) you don't need a warning, as you will find the need yourself. Arguing along (c) you're trying to compete with the JVM and its JIT. Do we definitely know that the JVM needs the 'static' mark to avoid dynamic dispatch? Only (b) seems to really help some people who are not used to reading object-oriented code (who use F3 instead of Ctrl-t :) ) Anyway, due to (b) I admit it is a matter of taste and since I don't have to enable the warning I won't mind it. BTW: Has it been considered to offer a *refactoring* for this (which would then check hierarchies up AND down)? Perhaps as part of change method signature? Or is the change too trivial for that?
Created attachment 183447 [details] proposed fix v1.2 + regression tests This patch basically follows the same logic as in the above patch, except for a few changes: 1. Changed the name of the option(s) in JavaCore. There are now 2 options - COMPILER_PB_MISSING_STATIC_ON_METHOD and COMPILER_PB_POTENTIALLY_MISSING_STATIC_ON_METHOD. Updated the documentation and the guide as well. 2. Changed the position of the declared bit CanBeStatic in ASTNode according to point 6 in satyam's comment. 3. Handling super reference case now, with the changes in org.eclipse.jdt.internal.compiler.ast.SuperReference. 4. Making sure that local type's methods are not warned for. (Changes in MethodDeclaration.java:233 onwards). 5. Making sure that we dont warn for empty methods. (Changes in line MethodDeclaration.java:102). 6. Making sure that methods using type parameters declared by enclosing class arent warned for. (Changes in line MethodDeclaration.java:84 onwards) 7. Changed the method org.eclipse.jdt.internal.compiler.lookup.BlockScope.resetEnclosingMethodStaticFlag() to include the case of local type's method accessing a field of the outer type. (point no. 3 in satyam's comment). 8. Added more tests in ProblemTypeAndMethodTest.java Satyam, please review. Thanks!
(In reply to comment #19) > I know I'm late to this party, so let my comment not stop you, but ... >[..] I guess the motivation behind this warning is not really to show any potential problem in the code, but to suggest an alternative way of writing that same code, which would result in more efficiency overall. This would happen if, say, a programmer doesn't declare a method as static, and just to call the method at many places, instantiates an object of the class. (Most coders won't do such a mistake, but a few can when they're running on caffeine overdose! ;) ). In that case, just to call a method, a lot of memory ends up getting wasted in just instantiating objects. This atleast could have been prevented if the method was declared static. The new warning is also on the lines of other "code style" warnings that you see in Java>Compiler>Errors/warnings, viz. "Undocumented empty block", "non static access to static member", etc. - all of which are not programming errors per se. I wouldn't introduce this warning if it were expensive to analyse the code for it. But as you can see in the attached patch, there's minimal overhead in raising this warning, so i think there's no harm in introducing it! :)
Not sure we want this warning on by default.
Sorry for the late review.. Overall, it looks good except for some minor comments. As Olivier had mentioned, I also think this warning should be turned off by default. 1. Main.java: Why is there only one option for both potential and normal error? I think here also there should be two options. What do you think? 2. MethodDeclaration.java: A comment could help when the flag is turned off for empty body.. 3. MethodDeclaration.java: There is a comment telling that the methods should not be static for local types and then I see this line. this.bits |= ASTNode.CanBeStatic; Am I missing something? 4. Though you have fixed the problem where methods could take type arguments of the given class, the method statements or the return type could also refer to the type parameter of the class. In this case also, static warning should be returned. Eg: class X <T> { void foo() { T a;} }
Created attachment 184249 [details] proposed fix v1.3 + regression tests Done. As for the comment in MethodDeclaration, I think it was just confusing, so I changed the wording, the code otherwise is correct. Also added the test for usage of type parameter in ProblemTypeAndMethodTest.test120().
(In reply to comment #24) > Created an attachment (id=184249) [details] [diff] > proposed fix v1.3 + regression tests This patch is incomplete (e.g. misses JavaCore and DefaultCodeFormatter). (In reply to comment #11) > Typo in Javadoc and help: "a method has not be declared": "be" => "been". Still not fixed in the patch from comment 20. Note that I will not have time to add the UI for this for M4 (I don't want to add the preference options without offering the corresponding quick fixes).
(In reply to comment #25) > Note that I will not have time to add the UI for this for M4 (I don't want to > add the preference options without offering the corresponding quick fixes). So this should be moved to M5 all together. Early in M5 looks like a good timing then.
MethodDeclaration is also not part of the patch. Here are some quick comments in LocalDeclaration.java: 1. Probably many computations could be avoided if there is a check of the method not being static yet. 2. You would want to remove those code you had commented. 3. There is a problem in the for loop. If the first type parameter doesn't match, it just marks it static.
(In reply to comment #25) > (In reply to comment #24) > > Created an attachment (id=184249) [details] [diff] [details] [diff] > > proposed fix v1.3 + regression tests > > This patch is incomplete (e.g. misses JavaCore and DefaultCodeFormatter). > > > (In reply to comment #11) > > Typo in Javadoc and help: "a method has not be declared": "be" => "been". > > Still not fixed in the patch from comment 20. > > > Note that I will not have time to add the UI for this for M4 (I don't want to > add the preference options without offering the corresponding quick fixes). CVS is my nemesis! I dont know why it missed out these files in the patch! :) I'll recreate the patch and also take care of Satyam's comments.
(In reply to comment #28) > CVS is my nemesis! I dont know why it missed out these files in the patch! :) > I'll recreate the patch and also take care of Satyam's comments. It's probably not CVS, but the Eclipse CVS Team Provider. Please yell in bug 294650.
Created attachment 184577 [details] proposed fix v1.4 + regression tests This patch adds another check to make sure that if the method returns a type parameter not declared by it, the new warning isnt raised (added ProblemTypeAndMethodTest.test123() for it).
Created attachment 185363 [details] fix 1.4 with cosmetic changes + tests Same fix as above, but changed ASTNode$canBeStatic from bit 6 to bit 9, and shortened the name of a few locals.
Changes look good for me. +1 for this
Released in HEAD for 3.7M5
Verified for 3.7 M5 using build id: I20110124-1800
