Address some minor Error Prone warnings.

[eamonnmcmanus]

No description provided.

[Marcono1234]

Out of curiosity, which Error Prone bug pattern reported this? And what is the issue with Set.of() here?

[eamonnmcmanus]

Internally, at least, we prefer for people to use ImmutableSet.of(...) instead of Set.of(...). The reasoning is that the spec List.of(...) doesn't have guarantees about what the returned list is like. (Is it mutable? thread safe?) I've personally never been very convinced by that argument, but in this case it's easy enough to use Collections.emptySet() to shut the warning up. If considerably more verbose. Of course we can't use ImmutableSet.of() here because we don't depend on Guava.

[MukjepScarlet]

Set.of only works on Java 9+.
By the way, I viewed the code of OpenJDK 17:

    @SafeVarargs
    @SuppressWarnings("varargs")
    static <E> Set<E> of(E... elements) {
        switch (elements.length) { // implicit null check of elements
            case 0:
                @SuppressWarnings("unchecked")
                var set = (Set<E>) ImmutableCollections.EMPTY_SET;
                return set;
            case 1:
                return new ImmutableCollections.Set12<>(elements[0]);
            case 2:
                return new ImmutableCollections.Set12<>(elements[0], elements[1]);
            default:
                return new ImmutableCollections.SetN<>(elements);
        }
    }

And Set.of will always return an immutable set (also thread-safe because we only read from it)

[eamonnmcmanus]

We only run tests on JDK ≥11, so the fact that this test currently uses Set.of() is not an issue for JDK-compatibility reasons.

I'm realizing that the explanation I gave wasn't right. I was thinking of the explanation for avoiding Stream.collect(toSet()) and the like. The set returned by Set.of is explicitly specified to be unmodifiable, which basically means immutable here. And therefore it is thread-safe too. The reason for preferring ImmutableSet.of() is mostly that it reflects the immutability explicitly in the type. I'm not really convinced by that argument either, and it applies just as well to Collections.emptySet(). But there is another thing that I am convinced by, and that is that Set.of().contains(null) throws NullPointerException. I think that is a giant wart which is a legitimate reason to avoid Set.of and the like. Collections.emptySet() doesn't do that.

[MukjepScarlet]

Yeah, Collections.EMPTY_SET and ImmutableCollections.EMPTY_SET follow different design patterns. All immutable collections introduced from Java 9 don't allow null elements (or K/V for maps), so I approve to use Collections.emptySet here.

[eamonnmcmanus]

(Not allowing null elements is fine. Throwing with contains(null) is not fine. It doesn't make a difference in this test context, but it's enough to make me not want to use Set.of despite its convenience.)

[Marcono1234]

@eamonnmcmanus do you want to merge these changes? Or is there a reason why you have waited with it?

[eamonnmcmanus]

[Google context: cl/745573037.]