False positive EI_EXPOSE_REP when using Lombok @Singular annotation

[mmy97]

This code using Lombok annotations causes what I think is a false positive EI_EXPOSE_REP:

import java.util.Map;
import lombok.Builder;
import lombok.Singular;
import lombok.Value;

@Value
@Builder
public class FalsePositive {

  @Singular
  Map<Integer, String> falsePositives;
}

Some explanation of the magic lombok is doing behind the scenes in case you aren't familiar with lombok:

• Value makes all fields private final and generates getters (but not setters) for all fields.
• Builder makes all constructors private and makes it so the class can only be constructed using the Builder pattern.
• Singular can only be used in conjunction with Builder. It makes it so that you can add things to a Collection line by line in the builder, instead of supplying the entire Collection at once. The key property of Singular is that it ensures the created collection is immutable.

If we delombok-ify the code (I did this using IntelliJ lombok plugin), we can see that it generates this .build() method:

    public FalsePositive build() {
      Map<Integer, String> falsePositives;
      switch (this.falsePositives$key == null ? 0 : this.falsePositives$key.size()) {
        case 0:
          falsePositives = java.util.Collections.emptyMap();
          break;
        case 1:
          falsePositives = java.util.Collections.singletonMap(this.falsePositives$key.get(0),
              this.falsePositives$value.get(0));
          break;
        default:
          falsePositives = new java.util.LinkedHashMap<Integer, String>(
              this.falsePositives$key.size() < 1073741824 ? 1 + this.falsePositives$key.size()
                  + (this.falsePositives$key.size() - 3) / 3 : Integer.MAX_VALUE);
          for (int $i = 0; $i < this.falsePositives$key.size(); $i++) {
            falsePositives.put(this.falsePositives$key.get($i),
                (String) this.falsePositives$value.get($i));
          }
          falsePositives = java.util.Collections.unmodifiableMap(falsePositives);
      }

      return new FalsePositive(falsePositives);
    }

falsePositives can be created using these 3 methods Collections.emptyMap, Collections.singletonMap and Collections.unmodifiableMap.

All 3 of these return immutable collections, so it should not be possible to modify the collection after retrieving it with a getter, therefore there is no risk here (unless I've missed something).

This also happens with other types of collections such as List and Set.

[welcome]

Thanks for opening your first issue here! 😃
Please check our contributing guideline. Especially when you report a problem, make sure you share a Minimal, Complete, and Verifiable example to reproduce it in this issue.

[baloghadamsoftware]

Thank you for reporting this. You are completely right, this is indeed a false positive. However, it is extremely difficult to fix it with the current infrastructure.

By default, SpotBugs uses the static type of the fields as their real type. However, if all the assignments to that (private) field are of the same dynamic type then that type can be used instead of the static type. If the assigned value is the return value of a method then SpotBugs uses its return type from its signature. However, some functions return a more specific type, such as the static methods of java.util.Collections class. To address this issue, #2141 adds these functions to a list where we override the type in the signature by the real type of the returned object, registers these types as immutable and changes the FindReturnRef detector to check for the real type.

However, this unfortunately does not help here since the build() method uses three different types to create the same field so in this case SpotBugs reverts to the static type of the field which is java.util.Map. To fix this issue we should do massive changes to keep track the whole set of possible types instead of a single type.