ImmutableEnumChecker on a list coming from List.of(...) which is immutable

[kamilgregorczyk]

Description of the problem / feature request:

I'm getting ImmutableEnumChecker errors on such enum:

public enum SomeEnum {

    SOME_VALUE(List.of(TYPE_1, TYPE_2))

    public final List<SomeType> types;

    SomeEnum(List<SomeType> types) {
        this.types = types;
    }
}

ErrorProne should not trigger ImmutableEnumChecker on collections created from ImmutableCollections

[jan25]

https://github.com/google/error-prone/blob/master/docs/bugpattern/ImmutableEnumChecker.md this talks about using ImmutableList.of to fix the error. Perhaps List.of bit in your snippet is the issue?

[kamilgregorczyk]

Of course, but List.of uses immutable implementation of a list

[kamilgregorczyk]

Since JDK9 there are built in immutable collections with nice constructors so there's no need to use guava's ones

[Stephan202]

There is still a good reason to use the Guava variants: to communicate immutability at the type level. From the ImmutableCollection documentation:

Expressing the immutability guarantee directly in the type that user code references is a powerful advantage. Although Java offers certain immutable collection factory methods, such as Collections.singleton(Object) and Set.of, we recommend using these classes instead for this reason (as well as for consistency).

[kamilgregorczyk]

When you create a class that's immutable you call it SomethingSomething not ImmutableSomethingSomething.

JDK finally has immutable collections with nice way of constructing them, just because some ppl prefer to have ImmutableList and guava as a dependency does not mean that error-prone should not ack the fact that JDK evolved and enforce guava everywhere.

[cushon]

There's some related discussion in the docs for MutableMethodReturnType.

It's OK to prefer List.of, but having a static Immutable* type does have advantages: it makes it clear to users of the type that what they're getting is actually immutable, and also enables additional static checking with Error Prone (e.g. ImmutableModification).

[cykl]

It's OK to prefer List.of

Does error-prone support it?

[ChristianCiach]

There doesn't seem to be a way to properly avoid this warning (without suppressing it) without using Guava, which we rarely need anymore since we upgraded to newer JDK releases. It's just not worth it to include Guava just for the immutable collection types.

Also, the warning message says:

[ImmutableEnumChecker] enums should be immutable

But.. it is immutable when using List.of! The checker is all about "immutability", not about communicating that fact to the caller. Even the checker description says nothing about a necessity to communicate the immutability to the caller.

I would clarify this as a bug. The checker just fails to detect the immutability of the enum. It's a clear case of a false-positive.

[agentgt]

I agree with others and I don't think it should be a requirement of any checker that is turned on by default to require use of Guava (or I guess some interface naming scheme).

[delanym]

Was hoping this rule could help tighten things up but there are many enums with a List which is safely handled with Collections.unmodifiableList

[agentgt]

It is my number one @SuppressWarnings (other than some Eclipse issues with null).

I use parameterized testing with enums so I have lots of enums that take lists.

[agentgt]

I think the best compromise if Error Prone is going to be adamant about the list having "Immutable" in the name is perhaps this is only applied if the enum is public.

I use enums all the time particularly for unit test and I find the EnumOrdinal and this checker quite noisy.

So if an Enum is private or package friendly perhaps not make those two checkers warn by default (e.g. make it opt in)?