Community
Participate
Working Groups
import com.google.inject.Inject; public class Empty { @Inject private String unusedButNotFlagged; private String unusedAnfFlagged; } This is a regression since bug 365437 was fixed (which is great!) Fields that are annotated with @Inject but never read should be flagged as unused fields.
In light of bug 365437 comment 32 and 33, I'd say this is intentional and not a regression
Copied new comment from original ticket to move the discussion to this one here: 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.
Hm, maybe for @Inject the best way is indeed to make an exception from the exception from the rule. But on the long run, maintaining an implementation that hard-codes the subtleties of semantics of lots of annotations looks like a battle we cannot win. In bug 365437 making these things configurable by user options was deemed as too arbitrary an approach. I wonder, if this information should actually come from additional plug-ins? If JDT/Core would provide an extension point someone can provide a plug-in that knows the rules for DI, someone else do the same for persistence annotations etc. The point is: you need more than superficial knowledge about an annotation to tell whether it creates a relevant usage relationship. A user option makes it too easy for users to misconfigure (and they'll blame the compiler that it gave wrong advice). A plug-in providing this info via an extension can be reviewed once and for all. The only downside I can see: this won't directly work for the batch compiler.
(In reply to comment #1) > In light of bug 365437 comment 32 and 33, I'd say this is intentional and > not a regression I believe it is a regression and the fix for 365437 actually made things worse: - old behaviour: field is actually used -> just silence the warning - new behaviour: field is actually unused -> no indication anywhere
*** Bug 386692 has been marked as a duplicate of this bug. ***
(In reply to comment #4) > (In reply to comment #1) > > In light of bug 365437 comment 32 and 33, I'd say this is intentional and > > not a regression > > [..] the fix for 365437 actually made things > worse: It really depends on how you look at things. I would say that the problem of fields being marked as unused even when they're actually used in some way is worse than being silent on unused fields. Ofcourse, its good to have the warning on the above cases to guarantee leaner code, but its not like the code will break at runtime or anything. I still don't know how we can fix this bug to accomodate all the hundreds of annotations people will want to have excluded. And this list will never stop growing!
(In reply to comment #6) > I would say that the problem of fields being marked > as unused even when they're actually used in some way is > worse than being silent on unused fields. I agree when you put it that way. However, you can easily silence the spurious warning once you determined the field is indeed used. I do not have a work-around to accomplish the analysis that I would want to have – yet. Regarding the possibly infinite number of annotations to consider and to make exceptions for (although I’d say that @Inject, @In and @Autowired should cover a lot of ground), if you want to avoid that, make it configurable. For me, make the old behaviour an option.
import javax.annotation.Nonnull; public class Nulls { private String unusedAndFlagged() { return null; } @Nonnull private String usedButNotFlagged() { return ""; } } With this example it’s even more obvious that the approach to consider each annotation as use is hurting. I can now trade in null analysis for unused analysis. You say “And this list will never stop growing!” and you’re quite right. You suppress way too many warnings when it is so easy to suppress the few actually necessary places with @SuppressWarnings.
*** Bug 391842 has been marked as a duplicate of this bug. ***
Where do we stand in this discussion? In bug 383371 comment 48 we have a request to respect @Inject also during null analysis. If we agree that Guice's @Inject is worth hard-coding special treatment in our analysis, I'd be happy to use that information for both analyses ("unused" and "null") alike. Brainstorming about the situation I could imagine that we classify the known effect of an annotation into: 1. read/write 2. guaranteed non-null write 3. unknown Here, @Inject would be classified as (2), right? (Unless Guice is told to tolerate a null value(?)). (2) would *not* prevent "unused" warnings...
(In reply to comment #10) > Where do we stand in this discussion? > > In bug 383371 comment 48 we have a request to respect @Inject also during > null analysis. Yes, that would be great! > If we agree that Guice's @Inject is worth hard-coding special treatment in > our analysis, I'd be happy to use that information for both analyses > ("unused" and "null") alike. The solution should allow other to add other annotations (preference?, extension point?), e.g. to cover com.google.inject.Inject and javax.inject.Inject. Other frameworks may use different annotations for that purpose. > > Brainstorming about the situation I could imagine that we classify the known > effect of an annotation into: > 1. read/write > 2. guaranteed non-null write > 3. unknown > > Here, @Inject would be classified as (2), right? (Unless Guice is told to > tolerate a null value(?)). (2) would *not* prevent "unused" warnings... Guice's @Inject has a notion of 'boolean optional()'. option=true implies that that there is no guaranteed non-null write. @Inject(optional=true) Type name = new Type(); should be definitely non-null whereas @Inject(optional=true) Type name; is possibly null. I'd like to see warnings for unused injected fields, so (2) carries the right semantics for my use case. Please note that it is possible to annotate private methods with @Inject which implies that Guice will invoke that methods, so I don't expect an unused warning there.
Released for 4.3 M6 via commit fc2bbd994a305724f9f5d9645163d8f7329cd965. The patch handles both @javax.inject.Inject and @com.google.inject.Inject.
Verified for 4.3 M6 using Build id: I20130310-2000
Could you not support @Autowired too, which is probably also very popular? org.springframework.beans.factory.annotation.Autowired
(In reply to comment #14) > Could you not support @Autowired too, which is probably also very popular? > > org.springframework.beans.factory.annotation.Autowired See bug 386692 comment 3. Bug 386692 should be reopened and then either be closed as WONTFIX or get fixed.
FYI: interested parties may want to join me discussing bug 547451 comment 24.
