Community
Participate
Working Groups
Our dependency injection engine can inject into private methods from outside the declaring class. Therefore, a private method with an @Inject tag that is not used in the local class should NOT be flagged as an unused warning. (As an aside, I had thought the same argument would hold for private fields, but I believe the warning is still valid for fields. Even though the private field can be set fro outside the class. It can only be used inside the class. And if it is never used, it should be flagged as a warning) Example: import javax.inject.Inject; public class InjectExample { @Inject private String fooField; @Inject private void fooMethod() { System.out.println(fooField); } }
Looks like a duplicate of bug 357902 Btw: I still have plans to provide a patch for that one :-)
I would say we should probably add support to disable this reporting when a member (type/field/method) is annotated with an annotation coming from the javax package. We added support for javax.annotation.PostConstruct and PreDestroy (see bug 346529). I think we should remove the check for the specific annotation name and add a more generic support for annotations coming from javax package. The purpose of this warning is to "clean" up the code from code that is not being used. Of course nothing prevents reflection to be used to access a private method, but when a method is annotated with annotations from javax... package, it is more likely that these methods will be used in a different way at runtime like dependency injection or other runtime mechanisms. Adding support for more specific annotations looks like a wrong solution. So bug 346529 was too specific and the fix should be removed to go with a more general solution.
At least this was useful to find bug 365455.
I'll take a look.
Ayush, please follow up for M5. I'll attach a patch from Olivier (Thanks!) shortly.
Created attachment 207974 [details] Patch from Olivier This is a cumulative patch and includes fix for 365455
I think disabling the warning when an annotation comes from the javax package is too large of a swath. Not every annotation is for the container. For example, if javax.xml.bind.annotation.XmlValue is applied to a method, it must be a JavaBeans (public) property. Applying this to a private method should trigger the warning. I think the previous patch was correct; annotations which only mean something to the container should be suppressed. I know that means maintaining a list of some sort, but that seems superior to the current recommendation.
(In reply to comment #7) > I think the previous patch was correct; annotations which only mean something > to the container should be suppressed. I know that means maintaining a list of > some sort, but that seems superior to the current recommendation. Could you point us to some documentation that would help us put together such a list ? Thanks.
Srikanth, I am not sure if you are asking in jest :-) but there's no such readily available list. Unfortunately, what annotations matter to the container need to be cherry-picked from API documentation. While probably out of scope for this ticket, if the user had a Preference option of selecting WHICH javax.* annotations should be ignored, then things can be exact without Eclipse committers maintaining a list.
(In reply to comment #9) > Srikanth, I am not sure if you are asking in jest :-) but there's no such > readily available list. No, this question was asked in dead earnest, when it comes to javabeans, some of us firmly occupy the don't-know-what-we-don't-know quadrant and don't allow that fact to deter us from shooting off questions :) Ayush, I am OK with the incrementally-fix-as-we-hear-issues approach as these are not critical problems.
(In reply to comment #10) > Ayush, I am OK with the incrementally-fix-as-we-hear-issues approach as > these are not critical problems. Umm...but in the above patch we have switched off the warning completely for any javax.* So are you saying that we should drop that patch and instead only special case javax.inject.Inject in additon to @PostConstruct and @PreDestroy?
(In reply to comment #11) > So are you saying that we should drop that patch and instead only special case > javax.inject.Inject in additon to @PostConstruct and @PreDestroy? Yes, comment #7 points out with specific example why we should not adopt the strategy in the patch.
I went through the JEE 6 API and selected only the annotations which were explicitly documented to: 1) receive a callback 2) applied to methods 3) say something like "method may have any access type" or "method on which XXX is applied MAY be public, protected, package private or private". javax.annotation.PreDestroy javax.annotation.PostConstruct javax.annotation.Resource javax.ejb.AfterBegin javax.ejb.AfterCompletion javax.ejb.BeforeCompletion javax.ejb.PostActivate javax.ejb.PrePassivate javax.inject.Inject Some also apply to fields too, but not all do.
The fix for bug 365455 included with the patch here is being tracked in bug 346175.
(In reply to comment #13) > I went through the JEE 6 API and selected only the annotations which were Thanks for the list Paul. I will exclude these from the unused warnings. Will also try to see if this can be made configurable.
Created attachment 209012 [details] proposed fix v1.0 + regression tests This patch attempts to solve the issue in a more generic way by introducing a new option on preferences page. This option takes a comma separated list of all annotation names (fully qualified) that, when applied to a private method, will suppress the unused warning even if the method is not used. I have kept both @javax.annotation.PreDestroy and @PostConstruct as defaults in this configurable list to preserve the earlier behaviour and backed out the changes done for bug 346529. I have also added a batch compiler option to specify these names which can be used as given below: -warn:unusedPrivateExempt(javax.annotation.Resource|javax.annotation.PreDestroy) The implementation is simple: a special bit is assigned to any annotation that is defined by the user in the list. This is done in org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding.computeId(LookupEnvironment). This bit gets transferred to the tagBits of the method to which the annotation is applied. While reporting the unused error, if this tagBit is found, we skip.
I'm never a big fan of adding more preferences to Eclipse. But in this case, I think it is a particularly bad idea. In point of fact, I do not view these annotations as user preferences. A private method tagged as @Inject must not be flagged as a "never called" warning. This is not a preference, it is a dependency injection truism. The suggested approach would be the same as asking the user to maintain a list of access modifiers that would be considered as "not accessible out side a class", and then making sure that "private" was always included in that preferanece list. I can appreciate that maintaining the list of special annotations is a problem but I don't believe making it the user's problem is the correct approach here.
(In reply to comment #17) > I don't believe making it the user's problem is the correct approach here. That's not my intention at all. The idea behind providing an option is to give some kind of flexibility to the user, in cases he has his own annotations which do something akin to the @Inject annotation, or if in the future other annotations with such meanings are introduced. Do you suggest we keep hard-coding such annotations into the compiler? The whole idea is *not* to burden the user with another preference, but to provide more flexibility. I don't understand why you think thats a 'bad' thing. :) Ofcourse, I can buy the argument that the list of annotations mentioned in comment 13 can be added to the current defaults. But I dont see any de-merits, as such, of the proposed option. Srikanth/Olivier, what do you think?
The fact is that JEE is not the only dependency injection framework. There's nothing special about JEE annotations besides being the standard. Users of the Spring Framework and Google Guice (and the like) also have the ability to annotate private methods with non-standard annotations and would run into the same "unused method" warning. It seems reasonable to me that users should have the power to configure which annotations suppress the warning.
I think in this case we cannot make a call for which annotations should be considered to remove the warnings. This is why we should either use a list defined by the user, or simply remove the warning as soon as the private member is annotated. My concern is that there is a clean-up action to get rid of these unused private members so this warning should be reported only when required. In this case, we end up with false positives that can lead to get the corresponding members removed on the next save. This is bad! The question we should really ask is that "is it a big issue to remove the warning as soon as the method is annotated?". I don't think so. If we remove the warning, the only consequence is that the user doesn't get the automatic cleanup and might keep unused methods in her/his code. If we don't want to completely remove the warning, then this solution seems to be the one to go with. I don't see how we can maintain a list without letting the user set it up. On the patch itself, I would extract the compound name concatenation outside of the for loop in the computeId(..) method.
(In reply to comment #20) > The question we should really ask is that "is it a big issue to remove the > warning as soon as the method is annotated?" I think that may depend on a case to case basis. A user who frequently uses annotations may suffer. Annotations like @SuppressWarnings and now our own Null annotations, which can be expected to be commonly used, will suppress this otherwise useful cleanup diagnostic. So, I'm slightly biased in favour of the option. :) > On the patch itself, I would extract the compound name concatenation outside of > the for loop in the computeId(..) method. Thanks, will do in my next patch.
(In reply to comment #20) > [..], or simply remove the warning as soon as the private member > is annotated. I agree with this. As soon as a member is annotated, we have no clue what an annotation processor (or a reflective tool at runtime) is going to do with it. We can say that the annotation itself establishes a potential reference from an annotation processor, so the member is not definitely unused any more. However, there are a few annotations with well-defined semantics that don't establish a use of the member. These include @Deprecated, @SuppressWarnings, @Nullable, @NonNull. How about hardcoding these few annotations and consider all other annotations as rendering a member used?
(In reply to comment #22) > (In reply to comment #20) > > [..], or simply remove the warning as soon as the private member > > is annotated. > > I agree with this. As soon as a member is annotated, we have no clue what an > annotation processor (or a reflective tool at runtime) is going to do with it. > We can say that the annotation itself establishes a potential reference from an > annotation processor, so the member is not definitely unused any more. > > However, there are a few annotations with well-defined semantics that don't > establish a use of the member. These include @Deprecated, @SuppressWarnings, > @Nullable, @NonNull. > > How about hardcoding these few annotations and consider all other annotations > as rendering a member used? This sounds reasonable to me.
(In reply to comment #23) > (In reply to comment #22) > > (In reply to bug 365437 comment 23) > > > [..], or simply remove the warning as soon as the private member > > > is annotated. > > > > I agree with this. As soon as a member is annotated, we have no clue what an > > annotation processor (or a reflective tool at runtime) is going to do with it. > > We can say that the annotation itself establishes a potential reference from an > > annotation processor, so the member is not definitely unused any more. > > > > However, there are a few annotations with well-defined semantics that don't > > establish a use of the member. These include @Deprecated, @SuppressWarnings, > > @Nullable, @NonNull. > > > > How about hardcoding these few annotations and consider all other annotations > > as rendering a member used? > > This sounds reasonable to me. Same here.
(In reply to comment #22) >These include @Deprecated, @SuppressWarnings, > @Nullable, @NonNull. On the one hand, we're giving the user the option to specify his own Null annotations, on the other, we're hardcoding @org.eclipse.jdt.annotation.Nullable in this particular case. Seems a bit uncanny, no? Apart from these, the user may have so many Marker annotations of his own that don't make a method used, yet we're switching this warning off. Why can't we empower the user?
Also, what about @SafeVarargs? Subsequent java releases may come with more such annotations too.
(In reply to comment #25) > Apart from these, the user may have so many Marker annotations of his own that > don't make a method used, yet we're switching this warning off. Why can't we > empower the user? This was addressed by the earlier comment# 22, an excerpt from which reads: ... As soon as a member is annotated, we have no clue what an annotation processor (or a reflective tool at runtime) is going to do with it. We can say that the annotation itself establishes a potential reference from an annotation processor, so the member is not definitely unused any more ... Obviously this is a conservative assumption, but since the compiler cannot divine the intentions of the programmer, it is a reasonable assumption. (In reply to comment #26) > Also, what about @SafeVarargs? That too. > Subsequent java releases may come with more such annotations too. Those too. @Override doesn't need handling as we never mark a overriding method (relevant only for non top level class) as unused.
Created attachment 209200 [details] proposed fix v2.0 + regression tests Here is the patch which turns off the diagnostic for all annotated methods with any annotation other than @SW, @Deprecated, @SafeVarargs and the user configured null annotations.
Path looks good. Except that I would use a local variable for the resolveType. for (int i = 0; i < annotationsLen; i++) { TypeBinding resolvedType = annotations[i].resolvedType; if (resolvedType != null) { switch (resolvedType.id) { case TypeIds.T_JavaLangSuppressWarnings: case TypeIds.T_JavaLangDeprecated: case TypeIds.T_JavaLangSafeVarargs: case TypeIds.T_ConfiguredAnnotationNonNull: case TypeIds.T_ConfiguredAnnotationNullable: case TypeIds.T_ConfiguredAnnotationNonNullByDefault: break; default: // non-standard annotation found, don't warn return; } } } Do we also need to same code for unused constructors ?
(In reply to comment #29) > Do we also need to same code for unused constructors ? For consistency's sakes, should we exclude annotated fields too ?
Created attachment 209250 [details] proposed fix v2.1 + regression tests Thanks Olivier. This patch adds support for unused private types, fields and constructors as well.
(In reply to comment #31) > This patch adds support for unused private types, fields and constructors as > well. I think the solution being proposed is a good middle ground: once an annotation is applied, it is considered used. But I believe fields should be excluded in this patch. Fields are data points and unless your code is reading them, there's no point in having the data. This is unlike methods which actually get called from a container; but if your fields are being set by a container and you aren't using them, then I think the warning should remain.
(In reply to comment #32) > (In reply to comment #31) > > This patch adds support for unused private types, fields and constructors as > > well. > > I think the solution being proposed is a good middle ground: once an annotation > is applied, it is considered used. But I believe fields should be excluded in > this patch. Fields are data points and unless your code is reading them, > there's no point in having the data. This is unlike methods which actually get > called from a container; but if your fields are being set by a container and > you aren't using them, then I think the warning should remain. This may be true of the container world, but in the most general sense we are in no position to divine what an arbitrary annotation processor would do with some annotated element. So the point made by comment# 22, an excerpt from which reads: ... As soon as a member is annotated, we have no clue what an annotation processor (or a reflective tool at runtime) is going to do with it. We can say that the annotation itself establishes a potential reference from an annotation processor, so the member is not definitely unused any more ... still holds. Notwithstanding the fact that there is no counter example of a valid annotation processor usage being proferred, I suggest we treat fields, methods, types and constructor consistently and go forward with the fix v2.1.
(In reply to comment #33) > Notwithstanding the fact that there is no counter example of > a valid annotation processor usage being proferred, I suggest we > treat fields, methods, types and constructor consistently and > go forward with the fix v2.1. +1
Released in master via commit b0bd844e638d627fddf9ed3dbc5637db4513a8de
Released in master via commit 1d9c797e2d535a723d97129a4995a501f060e8fc to fix side effect in apt pluggable tests.
Verified for 3.8M5 using build I20120122-2000
I'd like to reopen this since private fields that are annotated with @Inject but not referenced anywhere are also ignored by the unused-member check. This does not seem to be right to me.
(In reply to comment #38) > I'd like to reopen this since private fields that are annotated with @Inject > but not referenced anywhere are also ignored by the unused-member check. This > does not seem to be right to me. Please file a new bug.
(In reply to comment #38) > I'd like to reopen this since private fields that are annotated with @Inject > but not referenced anywhere are also ignored by the unused-member check. This > does not seem to be right to me. This has already been discussed in comment 32 and comment 33.
(In reply to comment #40) > (In reply to comment #38) > > I'd like to reopen this since private fields that are annotated with @Inject > > but not referenced anywhere are also ignored by the unused-member check. This > > does not seem to be right to me. > > This has already been discussed in comment 32 and comment 33. The container annotations (@Inject and friends) declare dependencies thus introduce coupling between components. If these dependencies are unused, the tool should warn me about that because I have the chance to reduce coupling simply by removing these unused fields. There is already a special treatment of @Deprecated (I still get the warning about unused fields). From my perception it may be useful to make a distinction between common used annotations that lead to reading or writing a field. Since @Inject will usually be processed by a writing service, I'd expect a warning if no client reads that field.
Sorry, wrong ticket. I move the discussion to the bug 376590
