eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
169 stars 133 forks source link

support @SafeVarargs and @SuppressWarnings("varargs") as correct suppression of varargs warnings #453

Open garretwilson opened 2 years ago

garretwilson commented 2 years ago

Summarizing the discussion from java warning: Varargs method could cause heap pollution from non-reifiable varargs parameter, there are two varargs-related warning suppression annotations: 1) @SafeVarargs, which suppresses a warning related to to possible heap pollution from from varargs in a method's contract (signature); and 2) @SuppressWarnings("varargs"), which suppresses a similar varargs warning related to usage within the method.

Unfortunately Eclipse does not recognize @SuppressWarnings("varargs"), and forces us to use @SuppressWarnings("unchecked") instead; moreover when @SuppressWarnings("unchecked") is used, Eclipse produces a warning if we additionally use @SafeVarargs (saying that @SuppressWarnings("unchecked") is redundant), which is ironic seeing that the JDK itself uses @SafeVarargs in methods such as java.util.Collections.addAll(…). In fact support for @SuppressWarnings("varargs") was requested in Eclipse Bug 349669 and Eclipse Bug 344783. (It appears the latter was opened over 10 years ago, is almost identical to this present request, with the same conclusion of this present ticket, and has an unknown status as it was marked "bulk move out of 4.8", and its status is unknown.)

Let me back up and start with an example, as I discussed in correct way to avoid varargs "method could cause heap pollution from non-reifiable varargs parameter iterators" in Eclipse.

public static <E> HashSet<E> createHashSet(final E... elements) {
  final HashSet<E> hashSet = new HashSet<E>(elements.length);
  java.util.Collections.addAll(hashSet, elements);
  return hashSet;
}

If I'm compiling with Java 17 using -Xlint:all, I get the following warning for the method signature:

Varargs method could cause heap pollution from non-reifiable varargs parameter iterators

Eclipse 2022-09 suggests using @SafeVarargs. And indeed Java's own java.util.Collections.addAll(…) itself uses @SafeVarargs! Yet when I add it to the method signature, javac gives me the same warning, only further down in the java.util.Collections.addAll(…) line.

The answers to java warning: Varargs method could cause heap pollution from non-reifiable varargs parameter explain that there is a distinction between suppressing the warning for the method's contract and suppressing it for usage in the message body, suggesting to additionally use @SuppressWarnings("varargs") on the method for IntelliJ. And indeed when I use both @SafeVarargs and @SuppressWarnings("varargs") together, the lint warning goes away with javac. However Eclipse says this annotation value "varargs" is unsupported. So I have just traded one warning in javac for one in Eclipse. That is the crux of the issue.

Eclipse also suggests using @SuppressWarnings("unchecked"). If I add only @SuppressWarnings("unchecked") to the method, then the warning goes away both in Eclipse and in javac with linting! So Eclipse seems to imply that a single @SuppressWarnings("unchecked") is the way to go.

But I don't think it's the way to go because:

In short, I just want a way to prevent the Java 17 javac varargs warnings on a case-by-case basis in a way that is compatible not only with javac with linting, but also with other IDEs such as IntelliJ. If @SuppressWarnings("unchecked") is the appropriate annotation to use, then close this ticket as invalid. But from the references I've mentioned, along with the JDK's own usage, it would seem that a combination of @SuppressWarnings("varargs") for the signature and @SuppressWarnings("varargs") for the implementation are the latest and most appropriate annotations here.

garretwilson commented 2 years ago

As noted in the description, Eclipse Bug 344783 had the same conclusion years and years ago. Quoting Stefan Lindner:

Oracle's javax warning can be suppresses by another annotation

  private ICacheKey<T>[] subCaches;
  @SafeVarargs
  @SuppressWarnings( "varargs" )
  public SimpleCacheKey(final Comparator<T> comparator, final ICacheKey<T>... subCaches) {
      ...
      this.subCaches = subCaches;
  }

but now Eclipse reports "Unsupported @SuppressWarnings("varargs")"

Oracle's javac accepts this annotation and does no longer report any warnings but Eclise says that it's unsupported.

Why?

Indeed—why?

stephan-herrmann commented 2 years ago

If you read the existing bugs you probably saw, that there was confusion about the specification of these things. Explanations had appeared in comment sections of draft specifications, but never made it into the JLS proper. So this leaves us with just the recommendation within JLS 9.6.4.5 saying: "Compiler vendors are encouraged to document the strings they support for @SuppressWarnings, and to cooperate to ensure that the same strings are recognized across multiple compilers."

I've tried to initiate such cooperation several times but with very limited results.

Given that JLS only specifies a total of 4 @SW tokens, could you try to state in specificationish words:

Please understand that one or two examples don't suffice to ensure that ecj will implement the right thing :)

howlger commented 2 years ago

javac with -Xlint:varargs checks whether @SafeVarargs has been correctly added. There doesn't seem to be anything like this in the Eclipse Compiler for Java (ecj) yet.

The following example, javac -Xlint:varargs ... of Java 17 gives five warnings, but Eclipse 4.25 gives none. The warning of the varargs escaping to another @SafeVarargs method (see 3), as in your case, does not seem correct to me.

/////////////////////////////////////////////////////////////////////////////////////////////////
// warning: [varargs] Redundant SafeVarargs annotation. Varargs element type String is reifiable.
@SafeVarargs 
static <E> void foo(String... elements1) { }

//////////////////////////////////////////////////////////////////////////////////////////////////////////////
// warning: [varargs] Varargs method could cause heap pollution from non-reifiable varargs parameter elements2
@SafeVarargs 
static <E> E[] foo(E[] elements1, int i, E... elements2) {
    Object[] objects1= elements1;
    objects1[0]= new Object();
    Object[] objects2= elements2; // 1
    objects2[0]= new Object();
    array(elements1);
    array(elements2); // 2
    safeVararg(elements1);
    safeVararg(elements2); // 3 false positive?
    return i > 0
           ? elements1
           : elements2; // 4
}

@SafeVarargs
static <E> void safeVararg(E... elements) { }

static <E> void array(E[] elements) { }

Unfortunately, in javac only @SuppressWarnings("varargs") works, but not @SuppressWarnings("all") to suppress those varargs warnings (which seems to me a bug of javac). So @SuppressWarnings("all") cannot be used here as workaround, but @SafeVarargs @SuppressWarnings({"all", "varargs"}) makes both compilers happy.

See also:

garretwilson commented 1 year ago

Hi, everyone. Is it my imagination, or does @SuppressWarnings("varargs") now work under Eclipse 2023-06? Was there a ticket or pull request for this change? Was it accidental? 😄 Or am I mistaken in what this ticket was about? (I had to read my own bug report to remember the details.)

stephan-herrmann commented 1 year ago

Is it my imagination, or does @SuppressWarnings("varargs") now work under Eclipse 2023-06?

How did you test?

Looking at https://github.com/eclipse-jdt/eclipse.jdt.core/blob/478aff48e9d6377ab43b9b9658db02780a06dbdd/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/CompilerOptions.java#L560 I see no token varargs, so it doesn't seem to be implemented :)

Also https://github.com/eclipse-jdt/eclipse.jdt.core/blob/478aff48e9d6377ab43b9b9658db02780a06dbdd/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/impl/CompilerOptions.java#L1056

garretwilson commented 1 year ago

How did you test?

I just used it in code, like this:

https://github.com/globalmentor/globalmentor-web/blob/64ca42a2905bf7897849a66a97e761d357f73475/vocab/src/main/java/com/globalmentor/vocab/VocabularyRegistry.java#L82

According to my ticket here, Eclipse isn't supposed to like it. But I don't see any Eclipse warnings.

In my Eclipse "Java > Compiler > Errors/Warnings" preferences, I see "Unhandled token in '@SuppressWarnings':" is set to "Ignore". I clicked on "Restore Defaults" and it says that the default is "Warning". If I set it back to "Warning", I get the warnings again in Eclipse.

I guess maybe I turned off warnings so that I could use the best annotation. 🤦‍♂️ And I just published a library to Maven Central using @SuppressWarnings("varargs"). Drat.

I see no token varargs, so it doesn't seem to be implemented :)

Thanks for confirming that.