Recognize OpenDaylight's Immutable interface contract #1697

rovarga · 2021-09-10T22:32:32Z

During migration to spotbugs-4.4.1 in OpenDaylight we are noticing a number of new EI_EXPOSE_REP and MS_EXPOSE_REP warnings.
A large number of these are reported against types which are known to be immutable as part of their interface contract. This contract is not driven by annotations, but rather by implementing two dedicated interfaces,
Mutable and Immutable. These cannot be implemented simultaneously and can therefore be used to specify API expectations, such as (made up)

interface ImmutableMapBuilder<K extends Immutable, V extends Immutable> extends Mutable {
  ImmutableMapBuilder<K, V> put(K key, V value);
}

as well as runtime decisions without going to java.lang.reflect:

public void logAsync(Object obj) {
  final Object state;
  if (obj instanceof Immutable) {
    // Can safely defer toString()
    state = obj;
  } else {
    state = obj.toString();
  }
  enqueueToLog(state);
}

Since findbugs.util.MutableClasses also reacts to any annotation named Immutable being attached to a class, it would be beneficial if it would also recognize /Immutable; being extended or implemented in a type's hierarchy.

The text was updated successfully, but these errors were encountered:

Aside from recognizing annotations, also treat any class that implements an interface named Immutable as an immutable class. Fixes spotbugs#1697. Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

OpenDaylight has a type-safe marker interface which acts as API contract specification of effective immutability. Recognize this contract as equivalent to an inherited @immutable annotation. Fixes spotbugs#1697. Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

victorwss · 2021-09-15T15:53:46Z

The problem with non-sealed interfaces is that their implementation can be anything, including something that bears Immutable as part of its name but isn't immutable, like this:

public final class NotImmutableMapBuilder<K extends Immutable, V extends Immutable> implements ImmutableMapBuilder<K, V> {
    private final java.util.Map<K, V> reallyVeryImmutableMapButNot = new java.util.HashMap<>();

    @Override
    public ImmutableMapBuilder<K, V> put(K key, V value) {
        reallyVeryImmutableMapButNot.put(key, value);
        return this;
    }

    // Really really very helpful, informative and heuristic-friendly name!
    public V xxGroslynicateFooRqwz(K key) {
        return reallyVeryImmutableMapButNot.get(key);
    }
}

With this, SpotBugs would see the interface and say "oh, so this is an immutable interface, fine then, I will mark all of its use as safe with regards of immutability".

But then, some other module far from the place where the interface is defined, somewhere very far, far away down the road of a large multi-module application's build script, features that implementation.

To be worse, every non-final class also has the very same problem about subclassing, and this is the reason why immutable classes must be final before anything else. However final is practically the opposite of interface, so I think that it is hard to reconcile that.

rovarga · 2021-09-15T19:53:32Z

Agreed, I am specifically looking to understand OpenDaylight's contract in the context of determining mutability of classes on the class path, not in the project being analyzed. I believe we need to make that distinction, as otherwise SpotBugs would have to analyze the entire world in each run.

In the specific example you listed, though, SpotBugs would only understand, for example, that key objects are indeed immutable and therefore are safe to use as a key, not to infer anything about NotImmutableMapBuilder.

So this issue is really about trusting an explicit contract rather than trying to outsmart it through some heuristic. We already trust annotations, so I do not quite see how this is worse :)

OpenDaylight has a type-safe marker interface which acts as API contract specification of effective immutability. Recognize this contract as equivalent to an inherited @immutable annotation. Fixes spotbugs#1697. Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

rovarga linked a pull request Sep 14, 2021 that will close this issue

Recognize OpenDaylight's Immutable interface contract #1703

Open

1 task

rovarga mentioned this issue Sep 14, 2021

EI_EXPOSE_REP and EI_EXPOSE_REP2 occur if using Guava Immutable objects #1601

Open

rovarga changed the title ~~Recognize interface-defined immutability~~ Recognize OpenDaylight's Immutable interface contract Sep 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recognize OpenDaylight's Immutable interface contract #1697

Recognize OpenDaylight's Immutable interface contract #1697

rovarga commented Sep 10, 2021 •

edited

victorwss commented Sep 15, 2021 •

edited

rovarga commented Sep 15, 2021

Recognize OpenDaylight's Immutable interface contract #1697

Recognize OpenDaylight's Immutable interface contract #1697

Comments

rovarga commented Sep 10, 2021 • edited

victorwss commented Sep 15, 2021 • edited

rovarga commented Sep 15, 2021

rovarga commented Sep 10, 2021 •

edited

victorwss commented Sep 15, 2021 •

edited