Community
Participate
Working Groups
I20080609-1311 Given: public void foo() { if (bar()) new IllegalArgumentException("You must not bar!"); } Is: Compiler does not warn that this code is most probably a bug because the throw is missing Should: Add a potential programming problem which flags this code and warns that the throw is missing See bug 236332 for a real world use case.
Created attachment 110739 [details] proposed implementation and tests I suggest to check whether a throwable is instantiated but not used in any way. IMHO, the following should not trigger a warning, although not directly throwing: new RuntimeException("How come?").printStackTrace(); handleError(new Error("SNAFU")); whenDidThisHappen = new Exception("Now!"); return new Exception("Do you still want to continue?"); Only if the allocation is actually used as a statement, it is very unlikely (side-effect in a ctor??) that the code makes any sense. The good news: Implementation of such "analysis" is really easy: 2 lines ;-) See attached patch. A few mundane things I wasn't quite sure about: * what codes to assign (IProblem, CompilerOptions) * where to add the tests My patch makes it a configurable warning * suppressable by token "unused" * separate new compiler option - not yet integrated with Batch nor UI A side note regarding irritants in CompilerOptions: those constants are approaching the ceiling of the 64 bits in a long. For a while I was going to ask you devs to use these bits more sparingly, guess what: in our variant we currently have 9 additional irritants. (58+9=67) But rest assured, by now I found a way to handle 126 irritants :)
Created attachment 111358 [details] proposed tests I just noticed that the patch wizard ignores may additions in org.eclipse.jdt.core.tests.compiler :( So here's a patch containing the proposed tests, created on the command line within project org.eclipse.jdt.core.tests.compiler. Any comments?
> But rest assured, by now I found a way to handle 126 irritants :) Being curious. How did you do it ? I was thinking of holding onto an array of long, instead of just one.
(In reply to comment #3) > > But rest assured, by now I found a way to handle 126 irritants :) > Being curious. :) the irritant encoding is still a single long, bit 64 switches between two sets. Remaining 63 bits are used as before. (2 x 63 = 126) (so an irritant may have at most 2 bits set). I now keep separate fields warningThreshold and errorThreshold for each of these two sets. Switching over an irritant is now cascaded in three steps: 1 check the switch bit 2 check for int vs. long range 3 switch over the individual constants Computing the severity inspects the switch bit to select the correct threshold value. You can see the details here: http://trac.objectteams.org/ot/changeset/18817 HTH
This is similar to what I had in mind, reserve high bits for irritant to select group of irritants. The issue is that irritants cannot be composed any longer blindly. Also I think you need to fix CompilationUnitDeclaration#suppressWarningIrritants
(In reply to comment #5) > The issue is that irritants cannot be composed any longer blindly. Yup, this applies to CompilerOptions#warningTokenToIrritants, which can only group irritants from the same set. I wouldn't see any other places that would want to compose irritants. Do you see more? > Also I think you need to fix > CompilationUnitDeclaration#suppressWarningIrritants Gee wiz, you caught me here. Yes, thanks, you're right. Hm, that is a little trickier, and in this case I see your point of using arrays. I even tried both orderings of indices, but replacing the "long" leaves with "pairs of long" seems most "natural". So this patch: http://trac.objectteams.org/ot/changeset/18958 is currently under test on our build machine. Thanks for the hint. BTW: have you had a chance to look at the patch in this bug? (it's really simple)
I will look at it. I am more skeptical about the general utility of such a warning.
(In reply to comment #7) > I am more skeptical about the general utility of such a warning. My equation was: - only report in a very limited set of situations => extremely unlikely to raise false alarms - implementation is trivial + IF it triggers the user will be very grateful
See bug 245973 for issue with 64bit limitation with irritants. A patch is attached there with my current thinking.
Re: patch The situation where it will report a warning is extremely limited. Currently, only when codegen decides to optimize out the value. Note that this may arise in other situations than just statement expressions. Codegen optimizations do make certain decisions: e.g. if (false && new Exception()) { } Moreover, how is this more useful than flagging useless allocation in general ? e.g. new Integer(0); // how useful is that ? i.e. shouldn't the warning simply be reporting unused allocation ?
BTW, checking allocated type for subtype of Throwable should rather be written as: if (allocatedType.findSuperTypeOriginatingFrom(TypeIds.T_JavaLangThrowable, true) != null) { } as it will not force loading of Throwable in name environment. We want to minimize type loading for easing use of compiler when bootstrapping class library development (you do not need Throwable to be loaded).
(In reply to comment #10) > The situation where it will report a warning is extremely limited. it is limited, yes. > Currently, only when codegen decides to optimize out the value. Note that this > may arise in other situations than just statement expressions. > > Codegen optimizations do make certain decisions: > e.g. > if (false && new Exception()) { > } You mean a user would expect the warning also for: e = false ? new Exception() : null; But the primary bad smell here is the constant condition, not the ignored value. In order to create a situation where the value would still be ignored after replacing "false" with some useful value, you would need to use the conditional expression as a statement, but that's a syntax error to begin with. > Moreover, how is this more useful than flagging useless allocation in general ? > e.g. > new Integer(0); // how useful is that ? > > i.e. shouldn't the warning simply be reporting unused allocation ? Yes, might as well consider, but then - error message would be more general (can no longer suggest to add "throw") - this assumes that ctors are not used to create global side-effects, an assumption which I considered fair for Throwables, but will perhaps not hold for all classes ever.
> You mean a user would expect the warning also for: > e = false ? new Exception() : null; Your implementation would raise a warning in this case, as codegen valueRequired is false there. I was only writing a dumb example demonstrating that it may occur in various situations. Re: flagging unused allocation in general over exception allocation. This at least would be more consistent with the limitation of the warning, I think.
(In reply to comment #13) > > You mean a user would expect the warning also for: > > e = false ? new Exception() : null; > > Your implementation would raise a warning in this case, as codegen > valueRequired is false there. No, generateCode is skipped for the then expression ;-) > I was only writing a dumb example demonstrating that it may occur in various > situations. Currently I don't see any situations that would produce truely unexpected results. I may have to look harder for a valid counter example. Unfortunately I have to leave the discussion at this point - won't have internet the next days ...
Here my 2 cents: 1. Issue a warning whenever an object is created without been used (assigned/thrown/member invoked) 2. Make this option configurable (of course) and off by default At least in my code instantiating an object without using it is almost always certainly a bug because IMHO any side effect in a constructor, other then initializing the object, is bad programming style.
(In reply to comment #15) > 1. Issue a warning whenever an object is created without been used > (assigned/thrown/member invoked) +1 for this since I just hit this problem and I remembered this enhancement request. I was trying to extend getAdapter(Class) but ended up doing nothing... public Object getAdapter(Class adapter) { if (adapter.equals(X.class)) { new X(); // whoops! :( } return super.getAdapter(adapter); }
As said in comment 13, I would support the idea of yet another warning for diagnosing unnecessary object allocations, but I am pretty sure there are valid usecases out there some will tell us, which we will have to add special support for (given certain libs maybe); so this could be more time involving in the future. Nice to have, but low prio to me.
(In reply to comment #17) So, what does this mean? 3 people would like to see this warning implemented, a patch plus tests exist. The only jdt-core committer in this discussion states: > Nice to have, but low prio to me. Is there any workflow, how a feature that is low prio to the core committers can still be committed eventually? If your judgement is: the given patch and tests are only the lesser part of the effort, the larger part is still to come, please be more specific, so I can make up my mind whether I feel it's worth investing even more in this issue. And you know that I won't run away after the patch is committed, i.e., I am willing to help also in case people complain about this feature in the future. (youre worries seem to be connected to recent discussions, e.g., regarding dead-code warnings, where people wish certain patterns to be excluded from the analysis? Maybe considering all these unnecessary-code warnings together will eventually produce an even better way of fine-tuning such warnings??) Also note, that I was careful NOT to say: > instantiating an object without using it is almost always > certainly a bug because IMHO any side effect in a constructor, other then > initializing the object, is bad programming style. I understand that a compiler must not be too harsh in deciding what is or isn't bad programming style. Some people are likely to feel offended by any such decision, so yes, I'm open to looking for even better ways how we prevent style-related warnings from getting in some people's ways. But this issue also has the potential of exposing real bugs as several of us have observed in real life.
-1 to only handle unused Throwable creation but +1 to to detected the creation of any unused object. And yes, it's bad coding style ;-) Philippe, maybe we should adjust the summary?
(In reply to comment #19) ups, there _is_ another jdt-core committer in this discussion ;-) > -1 to only handle unused Throwable creation but +1 to to detected the creation > of any unused object. d'accord > And yes, it's bad coding style ;-) I don't disagree ;-) only: experience tells, that it's easy to create border line cases where "bad" would be too strong a word - but that's just the general reason why warnings should be configurable. I assume this one would rank as "unused"?
>(In reply to comment #19) >ups, there _is_ another jdt-core committer in this discussion ;-) Almost (I'm not a JDT Core committer but overall JDT co-lead).
This new warning should go under the "unused" umbrella. This means that if users are using 1.5 code they can use the @SuppressWarning annotation to get rid of the warning in some corner cases. I opened bug 285000 for the UI side of this option. I would rename the option in the patch for UnusedObjectAllocation instead of UnusedThrowableAllocation.
Philippe, any comment? I think this fits well inside the "unused" warnings.
(In reply to comment #22) > This new warning should go under the "unused" umbrella. This means that if > users are using 1.5 code they can use the @SuppressWarning annotation to get > rid of the warning in some corner cases. Right (it's is already included in patch & tests). > I opened bug 285000 for the UI side of this option. > > I would rename the option in the patch for UnusedObjectAllocation instead of > UnusedThrowableAllocation. sure. Just let me know, if you'd like me to update the patch and/or add more tests.
Updating title. Reassigning. Philippe, let me know if this is fine for you.
Created attachment 153869 [details] updated patch I guess we have sufficient authoritative +1 on this bug to proceed ;-) So here is an updated patch that is also more complete wrt. tests. The warning has been broadened beyond Throwable and the constants etc. renamed accordingly. I was wondering whether "allocating" would be the best term in the message (vs. "instantiating" or "creating"), but it seems that "allocating" is indeed consistently used by compiler messages.
I didn't review the patch but quickly tried it out: - the default severity needs to be 'Ignore' i.e. off by default - the message should be closer to unused variables: ==> "The allocated object is never used"
(In reply to comment #27) > I didn't review the patch but quickly tried it out: > - the default severity needs to be 'Ignore' i.e. off by default This probably won't change your mind, but I do think that Eclipse is hiding a few valuable features from users who'll never notice that something has been added. My favorite example in this regard is "Java Type Indicators". Many Eclipse users have no idea that this feature exists, yet it's wonderful ;-) We certainly don't want to get flooded with complaints "what's this new warning here, it's wrong and I can't even turn it off". Assuming that this warning will have its individual checkbox in the UI, which is better: 1. user is surprised to see a new warning, can decide to unsubscribe if he doesn't like the warning (most users will never notice, until some distant date when they first make this mistake). 2. user is unaware of this warning, user is probably also unaware that he may occasionally forget a word like throw or return etc. Only few users will carefully track the N&N to find this feature, so they can explicitly enable. -> Majority is not helped at all. I'd prefer #1 because I think we're more trying to help the careless users. The eager ones are better able to help themselves anyway. my 2c, still I'd rather see this adopted with default 'Ignore' than not adopted at all. Wasn't it you, Dani, who insisted that side-effects in ctor are BAD STYLE and unused allocation thus has NO justification? ;-) > - the message should be closer to unused variables: > ==> "The allocated object is never used" OK.
>Wasn't it you, Dani, who insisted that side-effects in ctor are >BAD STYLE and unused allocation thus has NO justification? ;-) Correct but there is nor rule with exceptions. Cases where this is not bad style is for example when creating UI elements like a Label where you really don't care to store it in a variable or when you simply want to validate a URL by creating it and catching the exception. Another issue is that almost all SDK code base is 1.4 and hence we cannot use @suppressWarnings to silent the cases where the code is intended as is.
Ok, several small things: 1) If this is part of the unused support I think the category should be: CategorizedProblem.CAT_UNNECESSARY_CODE and not CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM. 2) Implement comment 27 3) I would like to see a support for the batch compiler with a new warning token. I am ready to release once this is addressed. Thanks Stephan for the patch.
Created attachment 155923 [details] patch v3 (In reply to comment #30) > Ok, several small things: > 1) If this is part of the unused support I think the category should be: > CategorizedProblem.CAT_UNNECESSARY_CODE and not > CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM. Done. > 2) Implement comment 27 Done, although I'm still not convinced that default=ignore is a good idea. > 3) I would like to see a support for the batch compiler with a new warning > token. Done. (token is "unusedAllocation") > I am ready to release once this is addressed. Sorry for the delay. > Thanks Stephan for the patch. You're welcome ;-)
Last patch looks good except for the regression tests where we set the corresponding option to ERROR instead of WARNING. I'll fix that part.
Released for 3.6M5. Regression tests added in: org.eclipse.jdt.core.tests.compiler.regression.FlowAnalysisTest#test063 org.eclipse.jdt.core.tests.compiler.regression.FlowAnalysisTest#test064 org.eclipse.jdt.core.tests.compiler.regression.FlowAnalysisTest#test065 org.eclipse.jdt.core.tests.compiler.regression.FlowAnalysisTest#test066 org.eclipse.jdt.core.tests.compiler.regression.FlowAnalysisTest#test067 org.eclipse.jdt.core.tests.compiler.regression.FlowAnalysisTest#test068 Updated other existing tests. I also updated the copyrights for the provided patch.
Created attachment 155983 [details] Updated patch This is the patch that ended up being released.
Thank you Stephan and Olivier! I'll be turning this on once it's in the build. :)
I forgot to mention that unused anonymous class declaration are not detected. I expect this warning to also handle that case. I'll fix it.
Created attachment 155987 [details] Proposed fix for unused anonymous types Patch released to HEAD
Created attachment 155989 [details] Same patch with a regression test
(In reply to comment #30) > 1) If this is part of the unused support I think the category should be: > CategorizedProblem.CAT_UNNECESSARY_CODE and not > CategorizedProblem.CAT_POTENTIAL_PROGRAMMING_PROBLEM. It's good that @SuppressWarnings("unused") works for this problem, but I think it should go back to the CAT_POTENTIAL_PROGRAMMING_PROBLEM category. Up to now, code that was flagged with CAT_UNNECESSARY_CODE could be removed without any semantic change, so it was definitely unnecessary. But unusedObjectAllocation is only a potential problem.
I missed this subtlety between the two categories. Sorry, Stephan you were right. I'll fix it.
Created attachment 156030 [details] Category change Category change released in HEAD.
verified for 3.6M5 using build I20100122-0800
Verified.
