eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
168 stars 133 forks source link

Confusing "At least one of the problems in category 'resource' is not analysed due to a compiler option being ignored" #2279

Open laeubi opened 8 months ago

laeubi commented 8 months ago

ECJ emits a warning here:

grafik

The reason seems that the @SuppressWarnings("resource") is not needed (anymore?) at that place, but I find the message highly confusing, because it does not tell what option is responsible for that if I go to the problem severity

grafik

it only talks about "a relevant option is set to ignore" but again here its hard to tell what that should be.

The quickfix suggest to remove the token but I can't really match that to the actual message, because if it is not needed I would expect something like "remove unnecessary suppress warning" or similar:

grafik

stephan-herrmann commented 7 months ago

I'm not happy with that message either, but let's go back a few steps:

What you see is the result of long discussions concerning the following conflict:

Once a user A wrote:

    @SuppressWarnings("unchecked")
    public static <T> T asClassUnchecked(Object object, T requiredClassObject) {
        return (T) object;
    }

When the compiler informed him Unnecessary @SuppressWarnings("unchecked") he filed a bug. It turned out that suppressing was deemed unnecessary, because in the preferences Unchecked generic type operations was set to ignore.

Since that day we have two interpretations of what makes as @SuppressWarnings() annotation "used":

  1. the compiler detected a problem and matched it against the annotation
  2. the compiler would theoretically detect the problem that matched the annotation, even though in the current compiler settings that problem may be set to ignore

For mitigation the following strategy was implemented (in 2007): never signal @SuppressWarnings as unnecessary, if the corresponding compiler option was set to ignore. Nice.

This ensured that when different users compile the same sources with different compiler options, they do not see different suggestions to remove @SuppressWarnings (which otherwise could end in an edit war between them).

This relies on one assumption: that the compiler can detect whether or not the corresponding option was enabled.

That assumption turned out to be wrong, because "the corresponding option" is no longer a useful concept, because the same suppress token may suppress different problems, controlled separately by different compiler options.

Consider the following code:

public class Unused {
    @SuppressWarnings("unused")
    private void m1() {
        String s = "";
    }
    @SuppressWarnings("unused")
    void m2() {
        String s = "";
    }
    @SuppressWarnings("unused")
    private void m3() {
        String s = "";
        System.out.println(s);
    }
}

Assume that the compiler is configured to warn about unused local variables, but it should ignore unused private members:

Other user compiles the same code telling the compiler to ignore unused local variables, but wants to be warned about unused private members:

So each user would want to remove a different annotation. Not good.

Hence we implemented the info (in 2016), which should tell you something like: "I was about to suggest removing this annotation, but then I found that my analysis was incomplete, so perhaps it is needed? Unfortunately, you didn't tell me, whether you want to see warnings suppressable as 'unused', some you want to see, others you don't?".

Obviously, the message had to be shortened, and even in it's long form it is problematic.

Aside from these user-facing issues, there is one technical issue shining through: when you configure a certain option to "Ignore" then the compiler (for some options) doesn't even perform the necessary analysis, and in any case it doesn't record any ignored problems, and hence at the time when @SuppressWarnings is analyzed (at the end), there is no information about any ignored problems, that might justify the annotation. In fact ability to skip a given analysis by setting the option to ignore is a very useful concept.

Specifically regarding "resource" warnings: please see that "Potential resource leaks" are ignored by default. Perhaps the compiler learned that your resource problem is in fact not a definitive problem, but only potential. Now the problem got under the radar of the compiler with default configuration, and necessity of suppressing is no longer known.

stephan-herrmann commented 7 months ago

After that walk through 17 years of history, back to your comments:

it only talks about "a relevant option is set to ignore" but again here its hard to tell what that should be.

Perhaps that's the main usability bug here: we don't have a means to show which options are covered by the "resource" token.

The quickfix suggest to remove the token but I can't really match that to the actual message, because if it is not needed I would expect something like "remove unnecessary suppress warning" or similar:

That should be a lesser problem. Once you try the quickfix, you will see that the annotation is removed, too. There is one quirk though: put the following on any warning-free code: @SuppressWarnings({"discouraged", "unchecked"})

The quickfix can be invoked twice (once per token), and you end up with @SuppressWarnings({}) - which is probably not what you want.