Bug 441926 - Improve warning re unused object allocation: easier to suppress and enabled by default
Summary: Improve warning re unused object allocation: easier to suppress and enabled b...
Status: NEW
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.4   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: JDT-Core-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-17 10:39 EDT by Stephan Herrmann CLA
Modified: 2018-05-16 01:38 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Herrmann CLA 2014-08-17 10:39:48 EDT
Follow-up from bug 423639.

I would like to promote the problem "The allocated object is never used" to warning by default.

One thing keeping us from doing so was the inability to suppress this warning per occurrence. The allocation itself cannot be annotated, so @SW doesn't help much (you wouldn't want to annotate the enclosing method).

I'm proposing a new line comment token, which should express what distinguishes situations where the warning is or isn't relevant.

To describe this difference I'm looking at these three properties of an allocation expression:

(1) It is a function providing a fresh new object.
(2) Before returning the fresh object attributes are initialized with their defaults.
(3) If a constructor is defined, its body is executed to perform more initializations.

Judging just from (1) and (2) an allocation would *always* be useless unless the returned reference is used by the caller, because calling a function and ignoring its result has no observable effect.

The difference between cases (1)/(2) and a small group using (3) is whether or not the constructor body changes any state *outside* the freshly created object. If it does, the constructor invocation may create an observable effect. If it doesn't even cases (3) produce no observable effect when ignoring the returned reference.

Hence, the difference can be described by saying: the warning is always relevant unless the constructor produces a side effect outside the fresh object. Therefore, I'm proposing the comment token //$SIDE-EFFECT$ to suppress the warning per location.

Once we have this option for fine tuning, I believe it is fair to enable the diagnostic as warning by default.
Comment 1 Stephan Herrmann CLA 2014-08-17 10:55:54 EDT
Forgot to mention one proposal from bug 423639 comment 10:

Constructors producing a side effect outside the current object could be marked with a declaration annotation @SideEffect.

All allocations using a thusly annotated constructor would be exempted from the warning.

On the one hand this is a more involved enhancement than just evaluating one more line comment token, on the other hand it would greatly reduce the number of changes required to silence irrelevant warnings.
Comment 2 Stephan Herrmann CLA 2015-01-02 17:07:58 EST
Dani mentioned that comment tags like //$NON-NLS-1$ have specific support in refactoring etc., which would be tedious to extend to another similar tag.

Here's the special treatments that I could find:
  - support for $NON-NLS$ is in Scanner & Scribe
  - support for $IDENTITY-COMPARISON$ only in Scanner
  - special space handling for $FALL-THROUGH$ in Parser & Scribe
  - $CASES-OMITTED$ only in Parser

This already looks inconsistent and raises the question, what alignment between these different strategies is recommended already today?

Obviously, the @SideEffect *annotation* does not pose a problem in this regard.
Comment 3 Markus Keller CLA 2015-06-30 10:41:21 EDT
This problem is similar to @SafeVarargs: We want to annotate a declaration, and the effect should be to suppress warnings on all call sites.

"SideEffect" can mean many different things depending on context, so I'm not sure it would be a good idea to use this as a proxy for "OK to not use the allocated object".

I'd rather go for a generic solution, e.g. an annotation @SuppressCallerWarnings("unused") that can be added to the declaration.

@SuppressCallerWarnings("unchecked") would then become a synonym for @SafeVarargs (except for the extra checking requirements from the JLS).
Comment 4 Stephan Herrmann CLA 2015-06-30 11:14:25 EDT
(In reply to Markus Keller from comment #3)
> This problem is similar to @SafeVarargs: We want to annotate a declaration,
> and the effect should be to suppress warnings on all call sites.

I agree.

> "SideEffect" can mean many different things depending on context, so I'm not
> sure it would be a good idea to use this as a proxy for "OK to not use the
> allocated object".

IMHO those many different things should nicely align with the *reasons* why the warning may not be relevant. Do you have a counter example? Are you thinking of sysouts, for which the compiler cannot decide if it's relevant program behavior or just debug output?
 
> I'd rather go for a generic solution, e.g. an annotation
> @SuppressCallerWarnings("unused") that can be added to the declaration.
> 
> @SuppressCallerWarnings("unchecked") would then become a synonym for
> @SafeVarargs (except for the extra checking requirements from the JLS).

More generic solutions sounds good to me.

I see a bit of a mental gap from @SCW("unused") to realizing that this concerns the instance returned by calls to this constructor, but then others might see a similar gap from @SideEffect to its effect on warnings.
A more specific annotation can carry the explanation in its javadoc. Would a @SuppressCallerWarnings annotation enumerate all effects on warning reporting in its javadoc, too?
Assuming any new annotation will go into o.e.jdt.annotation, each new effect of @SCW would require an update of that bundle just to update the javadoc.
Just thinking aloud.

As the main difference I see: declaring the "nature" of a constructor, vs. instructing the compiler to behave one way or other.

Once we'd start considering @SideEffect we could actually perform quite interesting analysis in terms of procedures vs. pure functions etc. But that's a big topic on its own :)
Comment 5 Markus Keller CLA 2015-06-30 11:45:29 EDT
> > "SideEffect" can mean many different things depending on context, [..]
> 
> IMHO those many different things should nicely align with the *reasons* why
> the warning may not be relevant. Do you have a counter example? Are you
> thinking of sysouts, for which the compiler cannot decide if it's relevant
> program behavior or just debug output?

Yes, sysouts, template evaluations (bug 407417), changed random number generator seed, internal caches, caches written to a file, general I/O like audio beeps, inter-process communication, network packets, or even just execution delays can be relevant side effects for some analyses, but may not matter in others.

> A more specific annotation can carry the explanation in its javadoc. Would a
> @SuppressCallerWarnings annotation enumerate all effects on warning
> reporting in its javadoc, too?

No, I see this more like @SuppressWarnings, where the set of options is also somewhat dynamic.

> Assuming any new annotation will go into o.e.jdt.annotation, [..]

First yes, but once proven successful, we could try to standardize it.
Comment 6 Manoj N Palat CLA 2018-05-16 01:38:35 EDT
Bulk move out of 4.8