Fixes issue with BuilderDefault used with Builder and NoArgsConstructor.

[philliprower]

Below tests demos the issue this change fixes. Basically if you want to use both @builder and @NoArgsConstructor which we do because of legacy code, the @Builder.Default breaks the default initialization for @NoArgsConstructor. We'd like the defaults values initialized when using both the builder and no arg construction of the object.

public class BuilderDefaultIssueUnitTest {
    @Test
    public void test() {
        Example example1 = Example.builder().build();
        Example example2 = new Example();

        assertEquals(example1.getSomeDefInitInteger(),example2.getSomeDefInitInteger());
        assertEquals(example2.getSomeDefInitString(),example1.getSomeDefInitString());
    }

    @Data
    @Builder
    @AllArgsConstructor
    @NoArgsConstructor
    public static class Example {
        @Builder.Default
        private int someDefInitInteger = 5;
        @Builder.Default
        private String someDefInitString = "Hello";
    }
}

[rspilker]

Well, it fixes one problem and introduces another. If the initializer is a "complex operation", for every object created by the builder it is now executed twice.

Reinier and I are aware of the problem and are considering several solutions, but we have no clear winner yet. Possibly, the best solution is to duplicate the initializer code in the generated no-args constructor, and possible also in a generated required args constructors for fields that are nullable.

For now, the workaround is to type a no-args constructor and initialize the fields with the same expression in the constructor.

[philliprower]

I see... I think your probably right generating the code in the no arg constructor is probably best.

[luolong]

I was just pointed at this issue by a colleague of mine who found this behavior and was unpleasantly surprised by it.

I do believe that the argument that @rspilker pointed out of of optimizing potentially expensive initialization logic is not really in the responsibility of the tool, but rather a responsibility of the developer.

I have no concrete metrics to offer, but my gut feeling based on the kind of code bases I've seen (and believe me -- none of them have been shining examples of Clean Code or beacons of hope for believers in SOLID architecture), most of the field initialization logic has been rather trivial static default values (new empty arrays, constant literal values, null, etc.); Very few involve minimal calculation and extremely rarely is there an expensive computation involved.

Optimising for rare conditions seems like a weird choice given the widespread use and generality of the tool. And one that introduces surprising and rather unwelcome results for the end users.

So, from my completely biased and uninformed position, the current solution breaks more expectations than it solves any existing problems.

So, I would of course prefer if Lombok generated builders would be able to solve the expensive initialization issue in a way that it would be guaranteed to be performed only once and exactly once without much extra effort from the developer, but barring that, I would expect the developer to know how to counter such cases without Lombok breaking existing code.

[Maaartinus]

Very few involve minimal calculation and extremely rarely is there an expensive computation involved.

Sure, it's hardly ever possible to do anything useful in the initializer expression as it's executed before the constructor and can't depend on constructor arguments. Anything except initializing to suitable constant expression is very rare. The most expensive thing, I've ever seen there was a 16 kB buffer allocation and a static AtomicInteger instance counter increment (which is a bad design). Anyway, such cases are rather exceptional and then there's usually no use for a Builder.

[citDennis]

Please fix this. I just lost a day's work when I ran into this after refactoring an extensive codebase. Its not mentioned in the documentation and I made the mistake of trusting that. Another complicating factor was that de-lombok from the GUI (plugin) didn't show it up but tests proved 1.16.16 was broken in this respect. Very very confusing altogether.
I still love Lombok and will continue to recommend it but this really saddens me.

[rzwitserloot]

Fixed in a different way, see issue #1347 .

[citDennis]

Thanks Ronald, we removed most of the offending builders for now. Which maven release do you expect this to be included in?