Community
Participate
Working Groups
See example code: public class WarningTest { private static class A { public int getValue() { return 0; } } public Object getA () { return new A(); } } The compiler produces a warning on the declaration getValue(): "The method getValue() from the type WarningTest.A is never used locally". This is inappropriate, as it is quite likely that such a method will be used by code that performs reflection on the type (there are many libraries that will search for public properties of an object using reflection and perform actions on them, including Spring, Hibernate, JSP, Velocity, etc.). At the very least, there should be an option to disable this warning in this scenario without also disabling other unused member warnings (e.g. private members in public types), but I'm not convinced that this warning is really useful for anyone; I suspect it is going to be incorrect in way too many situations to be helpful.
(In reply to comment #0) > [...] > The compiler produces a warning on the declaration getValue(): "The method > getValue() from the type WarningTest.A is never used locally". This is > inappropriate, as it is quite likely that such a method will be used by code > that performs reflection on the type [...] Any program that uses reflection clearly steps out of the scope of what the compiler can check. Therefor, I personally would _never_ consider potential uses via reflection as relevant for the compiler. Note, that reflection could, e.g., also access private methods. OTOH, marking the suspicious method as @SuppressWarnings("unused") gives an excellent location where you could (should!) document that this method is indeed used - via reflection that is - to avoid problems during evolution / refactoring.
My point is, though, that there are several off-the-shelf libraries in common use that use reflection internally, and generally only on public methods. In fact, I suspect the most common use of reflection is to enumerate an manipulate the public properties of an object complying to the JavaBeans specification. Yes, it is true that even private methods are subject to manipulation via reflection, but this is done very rarely. Many people, a lot of whom do not understand reflection, are using reflection on public methods via either Spring, Hibernate, JSP, Struts, or any number of other common libraries/frameworks that use reflection internally to determine how to interact with user supplied objects. The problem with marking the method with @SuppressWarnings("unused") is that this also suppresses warnings relating to variables and/or anonymous classes defined inside the method, and is therefore not a good idea to apply where it is not absolutely necessary. It is also an impractical solution for those, like me, who have upgraded to Galileo with existing projects and suddenly have many thousands of warnings to deal with. It would probably take over a day to fix them all by hand. An option to disable this warning without disabling other unused element warnings would be extremely useful, I'm sure, to many people. The warning as it stands at the moment effectively discourages at least one extremely useful and popular idiom, which is the use of private JavaBean classes to store the results of a servlet request which is then forwarded to a JSP file to display those results.
(In reply to comment #2) > [..] I suspect the most common use of reflection is to enumerate an manipulate > the public properties of an object complying to the JavaBeans specification. That _could_ indicate that we should consider two groups of users with different expectations: those who use JavaBeans-style reflection and those who don't. This seems to call for a configurable option (under compiler preferences), but what exactly would this option say? - "Warn about unused public methods in a private class"? or - "Warn about unused methods that could be accessed via reflection without calling 'setAccessible'"? Technically, we are speaking about a very special situation, and I think in order to turn it into a configurable option we have to come up with a very simple and precise description of this use case. If the first message precisely describes the situation, it might be OK. If we need s.t. like the second method it sounds too complicated for a user-preference, IMHO. > [..] Many people, a lot of whom do not > understand reflection, are using reflection on public methods So, how do they maintain consistency? E.g., what prevents them from deleting a method that is not referenced in the program, i.e., where deletion doesn't cause a compiler error? > but I'm not convinced that this warning is really useful for anyone; It is useful for all those who do not use JavaBeans-style reflection. It could, e.g., signal that an intended override is no longer an override, because the superclass/ifc has changed.
Hi Stephan, Our team is running into this exact problem as well. We have a 250K line code base and have MANY places where we are using the Java Beans specification and running into this. We also don't want to just suppress unused on the method for the reasons Jules listed. For now, we've had to stay with 3.4.x. I have several suggestions for how a preference item could be added to deal with this. 1) Add a 'Ignore unused methods in private classes that are named according to the JavaBeans specification' This would ignore methods starting with get, is, or set. Of course, this approach will likely not flag methods that look like JavaBeans methods but aren't, but it would completely eliminate false positive warnings. 2) Add a 'Ignore unused JavaBeans methods in private classes that are Java Beans' This would attempt to take (1) further by reducing false negatives by having the user specify what Java types are java beans. Of course, this would require that the user be able to specify such types - and for the user to have defined such types. (Note that this is the case for our project, and I believe it would be the case for others as well). 3) Combining (1) and (2). That is, allow the user to check (1), then provide the option to limit this to certain types. Obviously, (3) would provide the most flexibility to Eclipse users, but if (3) is prohibitively difficult to implement, at least (1) would "get us by". Thoughts?
(In reply to comment #4) > Hi Stephan, To avoid undue expectations: I'm not a committer for JDT/Core, I'm just expressing my personal opinion (with knowledge of the compiler source, though). > I have several suggestions for how a preference item could be added to deal > with this. > > 1) Add a 'Ignore unused methods in private classes that are named according to > the JavaBeans specification' > > This would ignore methods starting with get, is, or set. Of course, this > approach will likely not flag methods that look like JavaBeans methods but > aren't, but it would completely eliminate false positive warnings. This sounds straight forward. Eliminating too many warnings shouldn't be much of a concern, since chances are fairly low and no big harm is caused. At the UI it's not clear where this option should appear. Ideally it would be a sub-item of an existing item. Unfortunately, "Unused member of a private class" is not a configurable item. > 2) Add a 'Ignore unused JavaBeans methods in private classes that are Java > Beans' > > This would attempt to take (1) further by reducing false negatives by having > the user specify what Java types are java beans. Of course, this would require > that the user be able to specify such types - and for the user to have defined > such types. (Note that this is the case for our project, and I believe it > would be the case for others as well). Since there is no standard way to mark what is or is not a Java Bean (correct me if I'm wrong), this would create some strange dependency on project specific conventions, IMHO. Out of curiosity: if the class defines methods that shall be invoked from another module, why is the class defined as private?
Hi Stephan, > Comment on non-committer Thank you for clarifying your position in this. How should we proceed getting a change made for this? (hopefully for 3.5.2) I know this will be valuable to many others - JavaBeans is widely used and is a powerful design pattern. > Comment on UI impact Under 'Unnecessary Code', there is an entry titled 'Unused local or private member'. This is the option that would get the checkbox underneath that says something like 'Ignore JavaBeans methods' > Comment on project dependency You are absolutely correct on (2) and (3). These would require a dependency on the project. If no other preferences have such a dependency, this is clearly an inappropriate solution. If this is already being done, it may be worth considering. > Comment on need to have private beans The primary value for this is for testing purposes. We have hundreds of private beans used exclusively inside a test class. There are certainly other uses, though less common, in production source. For example, you may have an implementation that needs to be known only to one class, but that implements a public interface type. In this case, the implementation can and should be private, but other classes can still use it by requiring and working with the interface type. Thanks! Jason
"Out of curiosity: if the class defines methods that shall be invoked from another module, why is the class defined as private?" I generally follow the rule that nothing should be more public than it needs to be. The classes in question are never referred to directly by any code other than the containing class (and correct encapsulation requires this), so there is no need for them to be anything other than private. Regarding Jason's suggestions, (1) seems quite sensible to me. I don't see how (2) could be implemented reliably, so I wouldn't suggest going down that route.
It sounds like (1) is our best course of action. What is the best way for us to proceed to get the fix in?
(In reply to comment #8) > It sounds like (1) is our best course of action. What is the best way for us > to proceed to get the fix in? Just checking : Is any of the interested parties in a position to propose a patch ? Otherwise, we will see what can be done. Olivier, for your attention.
Markus, Before we investigate a fix for this, are you willing to add the corresponding option to the UI? Check comment 4 for the proposed option name. Thanks.
We would not add this to the UI, and I don't think you should support it in Core either. Up to now, we don't have explicit support for JavaBeans, and we should not start to change that here. While I understand the given use cases, this is just not common enough for a general Java IDE. Furthermore, this would inevitably cause false negatives, since it's not clear which classes are to be considered as beans, so it's just not worth the hassle, sorry. The easiest workaround is to make these classes default-visible.
I don't think that's really an appropriate workaround. Apart from the issue that actually performing such a change could be a significant amount of work, it means exposing implementation details that do not need to be public to other classes in the same package, which to my mind violates rules of good design. Also, I disagree that no other JavaBeans-specific functions are exposed in the UI; there are options for automatically creating setter and getter methods, and a refactoring for encapsulating references to properties, both of which are exactly as specific to JavaBeans as the function proposed here, i.e. they work with any class that uses the standard getter and setter naming conventions. I don't see that a warning option along the lines of "warn of unused public getter and setter methods in private classes" ties the UI to JavaBeans any more than it currently is. Finally, as to false positives, I think generating an inappropriate warning is worse than not generating an appropriate one, as long as there is no way to remove the warning without unwanted side effects (as is the case, due to the fact that @SuppressWarnings("unused") has the side effect of removing warnings about unused variables within the method in question as well as the warning about the method itself).
Agree with comment 11. Getter/setter pattern has been there years before JavaBeans and if you e.g. generate private getters and setters they will also get flagged as unused (if indeed unused). You are right that @SuppressWarnings("unused") has the side-effect you mention but that's really not that bad, is it? You can either use the workaround suggested by Markus or you accept the limitations of @SuppressWarnings("unused"). Olivier, I leave it up to you whether you want to add this to the compiler only. Personally, I would close this as WONTFIX.
Here's another pattern to work around the warning, which at the same time documents why compiler and developer have different "opinions": public class BeanHolder { @SuppressWarnings("unused") // class Bean is used only via reflection private static Bean bean; private class Bean { } } This requires more typing but precisely states what is going on - with no false positives nor negatives, right? re comment 6: > ...There are certainly other > uses, though less common, in production source. For example, you may have an > implementation that needs to be known only to one class, but that implements > a public interface type. In this case, the implementation can and should be > private, but other classes can still use it by requiring and working with the > interface type. In that case the private class will be instantiated locally, right? So, the compiler will not complain.
(In reply to comment #14) Forget about my previous comment, I was thinking of a warning against the unused class, not the public method inside the private class as given in comment 0. Sorry.
(In reply to comment #1) > (In reply to comment #0) > > [...] > > The compiler produces a warning on the declaration getValue(): "The method > > getValue() from the type WarningTest.A is never used locally". This is > > inappropriate, as it is quite likely that such a method will be used by code > > that performs reflection on the type [...] > > Any program that uses reflection clearly steps out of the scope of > what the compiler can check. Therefor, I personally would _never_ > consider potential uses via reflection as relevant for the compiler. > Note, that reflection could, e.g., also access private methods. > [...] But this does not need reflection to access the erroneously flagged member: public class EclipseBug { private static class A { public void foo() {} // <-- gets flagged } public static class B extends A {} } Happens with Eclipse Version: 3.6.0, Build id: I20091030-1201.
(In reply to comment #16) Per comment#11 and comment#13, I plan to close this as WONT_FIX. > But this does not need reflection to access the erroneously flagged member: > public class EclipseBug { > private static class A { > public void foo() {} // <-- gets flagged > } > public static class B extends A {} > } > Happens with Eclipse Version: 3.6.0, Build id: I20091030-1201. I plan to follow up & fix this as a separate item.
bug #296660 raised to follow up the issue reported in comment#16
Verified for 3.6M4
