@Builder.Default and manual constructors

[Jan Rieke]

Having @Builder.Default on a field moves its initializer to a separate method, which is later called only of necessary by the builder's build() method. Lately, lombok significantly improved by also using this initializer method for generated @*ArgsConstructors.

However, as discussed at GitHub, this still has the drawback that manually implemented constructors will not set the initial value. Although this is now documented in the small print of the @Builder documentation, IMHO it still is quite surprising and is likely to lead to hard-to-find bugs, and some people have serious trouble with it.

Therefore, I suggest the following approach: Lombok should also automatically inject an assignment using the initializer method at the very beginning of manual constructors. Only if the manual constructor will always assign a value to the field, such an assignment should not be injected. This will restore the initializing behavior you'd expect when you look at the code also for manually implemented constructors.

Do you think this could be a reasonable and implementable approach?

[Reinier Zwitserloot]

None of the solutions are great.

Messing with constructors invisibly like this also feels bad. But it's probably the least evil choice. However, doing it right seems utterly impossible to me. I'm going to intentionally write some really ugly code to show off how hard this is going to be:

consider this as the base test case for all these:

@Builder @Value

class Example {

private static int counter = 1;

@Builder.Default int x = counter++;

}

CASE: How do you determine if the constructor _definitely_ sets the variable and therefore lombok should NOT inject 'this.x = $builder$defaultX();'?

Example:

consider:

public Constructor1A() {

if (Math.random() < .5) return;

this.x = 0;

}

versus

public Constructor1B() {

if (false) return;

this.x = 0;

}

versus

public Constructor1C() {

if (OtherType.SOME_PSF_BOOLEAN_FIELD_WITH_CONSTANT_VALUE_FALSE) return;

this.x = 0;

}

versus

public Constructor1D {

while (OtherType.someMethod()) {

doStuff();

}

this.x = 0;

}

versus

public Constructor1E {

while (OtherType.someMethod()) {

if (OtherType.foo()) return;

}

this.x = 0;

}

versus

public Constructor1F {

System.out.println(this.x);

while (OtherType.someMethod()) {

if (OtherType.foo()) return;

}

this.x = 0;

}

public Constructor1G {

do {

this.x = 0;

} while (false);

}

I have no idea what's even correct in some of these cases, and in other cases what I'd think is correct, is impossible for lombok to figure out:

1A: Presumably, lombok WILL generate 'this.x = $default()', and places it at the first line. If the dice fall such that x is later 0, counter has still been incremented, but that's more or less expected, I guess.... ? Or perhaps what I expect here is that upon hitting that 'return' statement, javac would have detected that, for this particular code path, this.x has definitely NOT been set, and therefore I expect lombok to silently upgrade that 'return' statement into first setting x. In other words, perhaps I expect counter to be incremented only half the time here. Most likely answer is that lombok detects that 'this.x' is not guaranteed to run and therefore lombok injects a this.x at start. Still, the point is: That's not quite as slam-dunk a case as I wanted it to be.

1B: Much harder; if(false) is defined explicitly in the java lang spec as being voodoo magic which does NOT trigger 'unreachable code!' errors, instead, it completely skips the body content; that return statement won't even be in the generated class file. I kinda expect lombok to act like javac does, and realize that in this particular case, the this.x assignment is guaranteed, and thus, I am not expecting counter to increase here. Lombok can, I guess, detect explicit if(false).

1C: ... whoops. No it can't, because javac can and will detect here that it's unreachable by way of explicit final. But lombok can't do that.

1D: I clearly expect lombok to NOT increment counter here; that statement is definite. The while body is convoluted and may exit abberantly (never finish, System.exit, or throw an exception), but then there is no object pointer anyway.

1E: ... but because a return statement shows up I now expect lombok to generate and increment counter at the start of the method.

1F: Okay, so here I expect the constructor to start out by printing '1' and not '0' (x has already been initialized with the default expr).. but if I later remove the 'return' statement in the while block, all of a suden.. it'll print... 0? That's kinda messed up, that a mix of whether or not there's a return statement in the constructor or a this.x assignment at the 'top level' someplace.

1G: This this.x is no longer at the top level but surely lombok can detect that it is guaranteed to run.. right? I expect no counter increment here.

Some more evil examples:

EvilConstructor() {

foo();

}

private void foo() {

this.x = 10;

}

@Builder.Default int x = counter++;

int greatEvil = foo(); // same foo as above

GreatEvil() {

//intentionally blank

}

In both cases, our eyeballs can trivially see that x will always be explicitly set, but in the EvilConstructor case, it is set during the constructor run, whereas with the GreatEvil case, it's set during the initializer run. If lombok generates a this.x = $def(); statement at the top of all explicit constructors, in one case x ends up being '10', but in the GreatEvil case, it'd end up being counter++, and not 10. What would I expect? Hard to say; I realize that lombok cannot possibly detect this and thus I expect lombok to generate the default, but the fact that in the GreatEvil case I still end up with x NOT being 10.. is very surprising to me!

I don't think this problem is solveable, so the real question is: Can we live with this clusterbomb, and file it away as 'oh come on nobody writes code THIS ugly, they deserve the mess!'? Should there be an opt-out mechanism? opt-out should probably be visible, so should the opt-out be.. on @Builder.Default itself? What if I want to opt-out but only for a specific constructor? Can I do something like annotate my explicitly written constructor with @Builder.Default.DontTouchMe?

Another example:

public class InstanceInit {

private static int counter = 1;

@Builder.Default private int x = counter++;

{

 System.out.println(x);

}

}

}

If you remove the @Default annotation from the above code, that'd print 1. However, pretty much no matter how we're going to go about it, it would now print 0 with the @Default annotation. That's still 'surprising', but even if we were capable of detecting this scenario (which seems difficult to me), there isn't really anything we can do about it; there's no way to know if there's a point in pre-initializing x here, because that depends on which constructor is going to be invoked later. I guess we can detect that apparently it is going to matter so in this case, x should ALWAYS start out as resolving 'counter++' and becoming equal to the result, and if then later a constructor always overwrites x, so be it. Therefore, removal of the sysout line will change the behaviour (a constructor invocation would no longer increment counter, whereas with the print, it always does). This is a hairy situation: I have no idea what to expect. Pile on to that that it's hard to detect that this is happening. Imagine the instance initializer calls a method.

[Martin Grajcar]

Taken literary, no, as "always assign" is undecidable.

Definite Assignment according to https://docs.oracle.com/javase/specs/jls/se8/html/jls-16.html would be a good approximation,

but even this is pretty complicated.

I guess, we should start with something simple.

• If we can determine that a variable is definitely assigned in the constructor, then do not inject the initializer.
  
• If we can determine that a variable is definitely unassigned in the constructor, then do inject the initializer.
  
• Otherwise, produce a warning.
  

This allows to keep the determination overhead arbitrary low, which should nonetheless cover most uses.

For example, only a top-level assignment before any return statement should count as "definitely assigned",
while any assignment precludes "definitely unassigned".

This sounds rather simple and given that Definite Assignment is no perfect solution either, I'd go for it.

The downside is the need to make it possible to suppress the warning and let the user decide.

But how? Given that there may be multiple @Builder.Default annotations and multiple user-defined constructors,

we'd need something allowing us e.g. to specify what should happen for each combination of them.

I can't imagine how to avoid it to be stringly typed, e.g.,

@Builder.Default.Initialize({"var1", "var2"})

@Builder.Default.DoNotInitialize({"var3", "var4"})

on the constructor (or something even more terrible on the fields).

Another idea: the user could prepend var1 = null; to the constructor to tell Lombok that the variable is definitely assigned.

However, this fails for final fields whenever there's any other assignment and possibly produces warnings otherwise.

Maybe Lombok could remove such lines... they're usually no-ops anyway (but not always: the super-class ctor can call a virtual method setting the field to non-null before the class ctor gets invoked, but that's a bad design anyway).

Maybe we should just keep it simple and do not try to solve all possible problems.

We have to avoid people being surprised by what happens and this happens currently in trivial cases.

So I'd suggest the following: For every constructor

• When there's a top-level assignment to var1 before any return statement, do not inject the var1 initializer.
• Otherwise, when there's no assignment to var1, do inject the var1 initializer.
• Otherwise, when there's @Builder.Default(copyToManualConstructor=false) on var1, do not inject the var1 initializer.
• Otherwise, when there's @Builder.Default(copyToManualConstructor=true) on var1, do inject the var1 initializer.
• Otherwise, produce a warning.

People writing multiple constructors, which should behave differently and there's a no top-level assignment, but there's some non-top-level assignment....,

are just out of luck. They'll get a warning and solve it or complain.... and we'll see what to do - maybe add some more fine-grained tuning.

However, I guess this won't happen anytime soon.

--
You received this message because you are subscribed to the Google Groups "Project Lombok" group.
To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Martin Grajcar]

I don't think this problem is solveable, so the real question is: Can we live with this clusterbomb, and file it away as 'oh come on nobody writes code THIS ugly, they deserve the mess!'?

IMHO, they deserve a warning. That's about all we can reasonably do. Let me check what happens using my (relative) simple proposal (assuming that copyToManualConstructor doesn't get specified):

Constructor1A - warning; IMHO OK

Constructor1B - warning; not perfect but as we can't solve 1C, I wouldn't bother

Constructor1C - warning; unfixable, therefore OK

Constructor1D - no initializer injection; OK

Constructor1E - warning; OK

Constructor1F - warning; OK

Constructor1F without the return statement - no initializer injection, should probably produce a warning of using a variable before it gets assigned

Constructor1G - warning; we could do better, but IMHO OK as not worth the effort

In case of a single manual constructor, the warning can be easily avoided by specifying the desired behavior. In case of multiple crazy manual constructors... bad luck.

Should there be an opt-out mechanism?

IMHO yes. I intentionally wrote just "warning" without specifying whether the initializer gets injected as I don't care  (for me, warning=error), so we could call it opt-in instead.

 

opt-out should probably be visible, so should the opt-out be.. on @Builder.Default itself?

Yes.

What if I want to opt-out but only for a specific constructor?

Bad luck, but such a case is pretty rare. 

Can I do something like annotate my explicitly written constructor with @Builder.Default.DontTouchMe?

Such an annotation could be the next step, but it's less visible and not better than @Builder.Default(copyToManualConstructor=...)

as it handles multiple constructors but not multiple fields. We'd need a way of specifying the behavior for each constructor-field pair.

Or to document the name of the initializer method. Then the user could opt-out for all constructors and call the method manually where needed.

Concerning the non-static initializer block

   {

      System.out.println(x);

  }

I'm rather lost. I can't imagine detecting such a situation and I can't imagine that doing it would be right.

I could imagine something like

@Builder.Default(executeImmediately=true) private int x = counter++;

translating to

private int x;

{

    x = $lombokInit$x();

}

but I'm unsure if such an option would get ever used.

--

[Jan Rieke]

Well, that are some nasty examples ;)

But we could also take the easy route and always inject the initializer. This would be (nearly) what also happens if we didn't move the initializing expression to a method, see https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.5:

"4. Execute the instance initializers and instance variable initializers for this class, assigning the values of instance variable initializers to the corresponding instance variables, in the left-to-right order in which they appear textually in the source code for the class."

(It's only "nearly" because we mess with the textual order.)

So maybe we simply should not try to be more intelligent than the Java compiler.

[Martin Grajcar]

I guess, this goes too far as it'd make using it with final fields impossible. Javac needn't care as instead of

final String title = ""; // game over

you initialize title in the constructor. This is verbose, but the workaround for a broken @Builder.Default would be even more verbose.

What's worse: Adding a new constructor to a class using @Builder.Default would force you to rewrite it.

--

[Reinier Zwitserloot]

Going with:

top-level assignment before a return statement = definitely-set wins; we don't set it.

no assignment anywhere = definitely-unset wins, we definitely set it.

everything else: Check for the annotation parameter about this on the @Builder.Default anno about what to do. If it is not there, don't do it, and generate a warning.

The one problem I have with this is that we ignore the annotation parameter in the first 2 cases. This makes sense there where you have multiple manual constructors,  but it CAN matter... but only in weird edge cases (for example when you read the field prior to setting it and you expected it to be the builderdefault at that point; you always overwrite the value later but let's say you intentionally want to read the default first, and write after).

To account for that and also this ugliness: "this.x = x+1", hey, we kinda have to do a pretty deep scan anyway: We also have to scan for declaration of a variable named 'x' (and parameters), to tell if "x = 5" is setting the field or a shadowing localvar/parameter. So if we're doing that anyway, why not scan if lexically speaking, x is potentially read prior to it being set, in which case we revert to the annotation parameter rule (warn if it is not there, otherwise follow the annotation parameter).

However, I'm still more in favour of an annotation on your manual constructor and not on @Builder.Default.

To be clear this is highly unlikely to make our priority list anytime soon, but it's good to work out what it ought to be first so we know what we're prioritizing (or what we're kicking down the list a bit).

To unsubscribe from this group and stop receiving emails from it, send an email to project-lombok+unsubscribe@googlegroups.com.

[Jan Rieke]

Another idea for a quicker mitigation: Lombok could leave the code as it is right now, but only emit a warning if there are manual constructors that do not assign a value in any case.

Detecting that shouldn't be too hard: We do not have to perform any kind of path analysis, just visit every instruction in the manual constructor.

This will cover the most likely bug cause: Users are simply not aware of the removal of the initializer. Of course, a conditional set will avoid the warning too, but I think a warning at least in some cases is better than the current situation.

[Reinier Zwitserloot]

We can, but we need to make an annotation or some such to allow people to remove the warning. Might as well go whole hog then, no?

[Jan Rieke]

Or advising the user to simply add an assignment for that field to the manual constructor? I think it's really uncommon that users intentionally add an initializer, but don't want that field to be initialized in their manual constructors. Furthermore, adding an explicit assignment doesn't really hurt and would help lombok newbies to not misunderstand the code.

[Martin Grajcar]

This should help most of time. It's only problematic, when the initializer is complicated - then the user should not be forced to repeat the lengthy expression.

Anyway, I'd suggest the following:

(1) top-level assignment before a return statement = definitely-set wins; we don't set it.

(2) no assignment anywhere = definitely-unset wins, we definitely set it.

(3) otherwise, generate a warning linking to an explanation

The steps (1) and (3) are simple and should be implemented ASAP as they alone suffice to make the feature no more error-prone.

Step (2) should come next as it needs no annotation and will improbably change in the future.

The annotations are much more complicated:

• The steps (1) and (2) seems pretty obvious and should probably take precedence over any annotations, but this may be confusing. In case, there's just one constructor, and the annotation gets ignored, we can produce a warning on the annotation as it's useless. In case of multiple constructors, the annotation may be effective with exactly one of them and then, there's no warning.
  
• This is an argument for placing the annotations on the constructor instead, but then there's the problem of multiple fields.

Maybe we should use no annotations. Could something like

class X {

    @lombok.Builder.Default private long x = System.nanoTime();

    @lombok.Builder.Default private final long y = System.currentTimeMillis();

    X() {

        lombok.Builder.Default.doNotInitialize(x);

        lombok.Builder.Default.initialize(y);

    }

}

work? The two static methods should not really exist.

The former would just prevent the warning and get removed.

The latter would get replaced by the statement y = System.currentTimeMillis().

An unrelated simple idea: Add an argument like

@Builder.Default(keepOnField=boolean)

and make it mandatory in case there's any manual constructor.

Both the javadoc and the error would point to an explanation.

This is not perfectly backwards compatible, but the incompatibility comes in the problematic cases only.

Most problem could be solved just by setting keepOnField=true.

On Wed, Sep 12, 2018 at 3:24 PM Jan Rieke <jan.rieke...@gmail.com> wrote:

Or advising the user to simply add an assignment for that field to the manual constructor? I think it's really uncommon that users intentionally add an initializer, but don't want that field to be initialized in their manual constructors. Furthermore, adding an explicit assignment doesn't really hurt and would help lombok newbies to not misunderstand the code.

--

You received this message because you are subscribed to the Google Groups "Project Lombok" group.

To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.

[Jan Rieke]

This issue is still discussed on GitHub, so there are still people running into it. I think we need some kind of solution here to prevent at least the most frequent bugs.

Thus, I suggest the following approach, as described previously:

Lombok does not change anything in the code of manual constructors. Instead, it emits a warning if there is no assignment to a @Builder.Default field in the manual constructor at all (not analyzing execution paths here).

If you want to remove that warning, you'd have to add an assignment to the field (so there will be no remove-the-warning annotation or similar things).

Pro:

• The assignment is what you want anyway in the vast majority of the cases.
• Would prevent most cases where users forget to manually initialize the field.

Con:

• In those rare cases where you want that field to be uninitialized intentionally, you'd have to add an otherwise unnecessary (default value) assignment. However, this makes the code easier to understand for non-expert lombok users.
• If you have a field that is not initialized in all possible execution paths, you still run into the issue. (Lombok could theoretically perform a full execution path analysis, but this seems too much to me.)
  

Would you accept a PR with that approach?

[bushwakko]

What I'd expect is that the default value is passed to the constructor by the builder, if the value is not set in the builder. I don't think Lombok should care what happens to that value in the constructor after that. Am I missing something here (I might be)? Since this works with finals, I think it's pretty explicit that what you are writing is lombok-notation and not actual java.

[Mat Jaggard]

Yes, you are missing something. This is about default values in other constructors - the builder pattern works but other manually written constructors don't.

On Tue, 19 Feb 2019, 09:41 bushwakko, <bush...@gmail.com> wrote:

What I'd expect is that the default value is passed to the constructor by the builder, if the value is not set in the builder. I don't think Lombok should care what happens to that value in the constructor after that. Am I missing something here (I might be)? Since this works with finals, I think it's pretty explicit that what you are writing is lombok-notation and not actual java.

--