projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.83k stars 2.37k forks source link

@With uses `==` for object equality, which can cause unexpected issue with Strings #2856

Open mjsmcp opened 3 years ago

mjsmcp commented 3 years ago

Generated code from Lombok's @With annotation uses == to compare objects, which does not necessarily work as intended for Strings. See FindBugs , where this is called out as a bad practice.

@With
@AllArgsConstructor
public class SampleClass {
    private String foo;
    private Object bar;
}

Processes to something along these lines:

public SampleClass withFoo(String foo) {
    return this.foo == foo ? this : new SampleClass(foo, this.bar);
}

For potential solutions:

  1. Use the Objects.equals method instead. (null-safe equals)
  2. Use the .equals method instead (non-null-safe equals)
  3. Provide a configuration option for the @With to select the equality mechanism you want (equality = Equality.NULL_SAFE)

I would suggest approach 3 as it maintains backwards compatibility with existing behavior for a non-experimental feature.

Rawi01 commented 3 years ago

Can you explain why you think this is a problem and should be changed?

mjsmcp commented 3 years ago

Sure. There are two reasons to fix this:

  1. Testing for string equality is a very common bug in Java since the == operator compares the references for Strings, not values. The current implementation hides this issue and could result in difficult to debug code for users instead of the consistently reliable code Lombok produces otherwise
  2. If you're using Lombok alongside a code quality toolchain including Findbugs, @With becomes unusable unless you configure FindBugs to ignore every instance of an @With generated method, which could be impractical. The other option is to Delombok @With, which removes the benefit of using Lombok.
Rawi01 commented 3 years ago

I know that string comparison using == doesn't work as expected and is a common bug (it is actually way harder to spot if you use something like Long because it works fine for the first 128 longs). I should have mentioned that in my question, sorry.

What I actually want to know is why you think it is a problem in this specific case. If we call withFoo and passing exactly the same reference there is no reason to create a copy of the object because the output will be the same object. If it is a different reference the method returns a new instance. This behaviour is also part of the documentation and lombok generates a comment for the withX methods that also explains it.

If I understand you correctly you want that the output is something like this:

public SampleClass withFoo(String foo) {
    return (this.foo == foo || (this.foo != null && this.foo.equals(foo)) ? this : new SampleClass(foo, this.bar);
}

This is possible but also requires some special handling for primitive types and arrays so it gets more complex.

If you're using Lombok alongside a code quality toolchain including Findbugs, @With becomes unusable unless you configure FindBugs to ignore every instance of an @With generated method, which could be impractical. The other option is to Delombok @With, which removes the benefit of using Lombok.

You can use the lombok configuration system to add @SuppressFBWarnings for all generated methods by adding lombok.extern.findbugs.addSuppressFBWarnings = true to the lombok.config.

antfie commented 1 year ago

Please note that at this time Veracode SAST reports CWE-597 (Use of Wrong Operator in String Comparison) for every @With wrapped class instance variable of type String. This is due to the usage of the equality operator (==) rather than the .equals() method, as has been discussed in the thread.