Bug 561621 - Enable @SuppressWarnings for optional errors
Summary: Enable @SuppressWarnings for optional errors
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.16   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-31 14:43 EDT by Stephan Herrmann CLA
Modified: 2020-04-02 03:50 EDT (History)
2 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 2020-03-31 14:43:27 EDT
Some (all?) projects of Platform/UI have pretty strong compiler settings, where several optional problems (like unused local variable) are configured as errors.

This is a nice move for code quality, but it seems to assume 100% correctness of ecj in this regard.

Let me remind you that even for errors mandated by JLS, no compiler has 100% correctness, but perhaps a few nines. For optional problems the guarantees we (JDT/Core) can give are lower than that.

For that reason, projects with optional problems configured as errors are advised to additional configure (in org.eclipse.jdt.core.prefs):

org.eclipse.jdt.core.compiler.problem.suppressOptionalErrors=enabled


This enables the use of @SuppressWarnings() even when optional problems are configured as errors. Of course, @SuppressWarnings must be used sparingly and with good care, but two reasons exist, why this may be the best thing to do:

- Some analyses of ecj are known to raise a false positive in specific, hard-to-discriminate situations.

- Ecj has bugs, and new bugs will occur as its development continues. No project should expect same-day fixes of every bug in JDT, and hence @SuppressWarnings may be a good temporary solution.


I personally would be very much relieved if I knew that Platform can work around bugs in ecj regarding problem detection for some days, until I find the time to fix.
Comment 1 Lars Vogel CLA 2020-03-31 15:18:03 EDT
Maybe you could add platform UI plugin to your runtime workspace to find these issues early during development?
Comment 2 Stephan Herrmann CLA 2020-03-31 15:29:37 EDT
(In reply to Lars Vogel from comment #1)
> Maybe you could add platform UI plugin to your runtime workspace to find
> these issues early during development?

Doing this for every fix in the compiler is quite out of proportion and would significantly slow down development. Running comprehensive tests is the job of jenkins. Feel free to configure an additional build job that I could trigger. If it runs in less then 10 min. we could perhaps add it to our gerrit verification.

By contrast, flipping one setting from disabled to enabled is so many orders of magnitude simpler.
Comment 3 Stephan Herrmann CLA 2020-03-31 15:36:19 EDT
(In reply to Lars Vogel from comment #1)
> Maybe you could add platform UI plugin to your runtime workspace to find
> these issues early during development?

And: perhaps you noticed that I did not promise that no additional problems will ever be raised? Read: even if I tested compiling all code in the world, I might still decide to accept some false positives for the benefit of many more correct positives.
Comment 4 Lars Vogel CLA 2020-03-31 17:26:19 EDT
Can't we flip this setting once needed?
Comment 5 Stephan Herrmann CLA 2020-03-31 17:48:45 EDT
(In reply to Lars Vogel from comment #4)
> Can't we flip this setting once needed?

You must have very strong reasons for avoiding this change.

Fair enough -- if you personally promise to make the change when needed.

Just note that the quickfix to add @SuppressWarnings will not be offered without that change, and hence you will not be aware which problems can potentially be suppressed.
Comment 6 Lars Vogel CLA 2020-03-31 18:07:41 EDT
(In reply to Stephan Herrmann from comment #5)
> (In reply to Lars Vogel from comment #4)
> > Can't we flip this setting once needed?
> 
> You must have very strong reasons for avoiding this change.
> 
> Fair enough -- if you personally promise to make the change when needed.
> 
> Just note that the quickfix to add @SuppressWarnings will not be offered
> without that change, and hence you will not be aware which problems can
> potentially be suppressed.

No I just don't understand why we should change something which is not needed yet. As you fell strong about it, please provide Gerrit and Andrey can review it. Afaics Andrey was affected by the compiler issue.
Comment 7 Rolf Theunissen CLA 2020-04-02 02:50:01 EDT
(In reply to Lars Vogel from comment #6)
> (In reply to Stephan Herrmann from comment #5)
> > (In reply to Lars Vogel from comment #4)
> > > Can't we flip this setting once needed?
> > 
> > You must have very strong reasons for avoiding this change.
> > 
> > Fair enough -- if you personally promise to make the change when needed.
> > 
> > Just note that the quickfix to add @SuppressWarnings will not be offered
> > without that change, and hence you will not be aware which problems can
> > potentially be suppressed.
> 
> No I just don't understand why we should change something which is not
> needed yet. As you fell strong about it, please provide Gerrit and Andrey
> can review it. Afaics Andrey was affected by the compiler issue.

The build of Platform UI was broken for 2-3 days in Gerrit. But also in the workspace, for anybody that was running the latest I-Builds. So the whole platform was affected, not only Andrey.
Depending on the I-Builds of the compiler has a risk. When the JDT project has a advice on how to reduce or manage this risk, I would seriously consider it.

Note that the workaround for the issue as suggested by Andrey would be far more ugly:
https://git.eclipse.org/r/#/c/160250/
Comment 8 Lars Vogel CLA 2020-04-02 03:50:05 EDT
(In reply to Rolf Theunissen from comment #7)
> (In reply to Lars Vogel from comment #6)

AFAIK we did not have lots of compiler bugs affecting platform UI over the years. Hence my impression that we could use Stephans suggestion once needed not up front. IMHO it is not normal that changes in JDT core breaks Platform UI or other repos.
But if someone wants to provide a Gerrit for platform UI, text, resources, runtime, team that is fine for me.