Skip to content

Instantly share code, notes, and snippets.

@rzwitserloot
Created February 21, 2017 15:57
Show Gist options
  • Save rzwitserloot/8e15182f6d048baacfbbdba7a1dcb268 to your computer and use it in GitHub Desktop.
Save rzwitserloot/8e15182f6d048baacfbbdba7a1dcb268 to your computer and use it in GitHub Desktop.

Purpose

Allow default values (other than null / 0 / false) for builder. That is, if some .fieldName(value) method is never called on the builder, use the provided default instead of null etc.

Snippets to highlight issues and options.

Snippet: final fields with initializers.

@Value @Builder public class Test {
    long created = System.currentTimeMillis();
}

You can interpret this in two ways, and both seem reasonable interpretations:

  1. The NoBuilder interpretation: The builder should not have a created(long) method at all; the above statement remains as is (well, @Value makes it private final). The created field is always initialized by the initializer during construction. The created field therefore holds the current time as it was when you call build() and you can't override it with something else.

  2. The DefaultValue interpretation: System.currentTimeMillis() is merely the default. Clearly the default, if it is to be applied at all, should be applied only when build() is called.

Currently, the above snippet is interpreted as scenario #1, and changing how it works would be a serious breaking change, that's a very big downside to any proposal that changes how this works.

Snippet: non-final fields with initializers.

@Data @Builder public class Test {
    private long created = System.currentTimeMillis();
}

You can interpret this in three ways. Two of these are reasonable.

  1. The NoBuilder interpretation. Slightly odd for a non-final variable to work that way, though.

  2. The DefaultValue interpretation. But this runs into the DoubleInitialize issue.

  3. The IgnoreIt interpretation. The generated builder will never result in created ending up with any sort of currentTimeMillis unless you call .created(System.currentTimeMillis()) yourself. It is consistent with the explanation of @Builder as: This makes an @AllArgsConstructor and then acts as if @Builder was on that.

The third option seems rather silly but it's how lombok works right now. It's sufficiently idiotic that we're okay with breaking backwards compatibility on this one; we're doing anybody that wrote code relying on this behaviour a favour by making it no longer work: It's surprising and that's very bad in code. Also, the consistency with the explanation is not worth much; anytime we see explanations of @Builder, unless it's coming straight from @rspilker or myself, it's not explained that way.

Snippet: the problems with double initialize.

@Data @Builder public class Test {
    private static AtomicInteger counter = new AtomicInteger();
    private int id = counter.incrementAndGet();
}

Going with the DefaultValue interpretation, you run into a serious issue: It would ordinarily seem like a good idea to have the initializer expression remain on the field, and also be copied to the builder. However, that would result in instances made via the builder to double-increase counter, which is rather surprising (and therefore, bad). For what its worth, the DefaultValue interpretation on final variables requires that the actual initializer expression is moved elsewhere, or it's impossible to (pragmatically) generate code that updates the value. Moving the initializer expression around is going to have a noticeable effect if the expression has side effects or is timing dependent.

Proposal

This proposal seems overly complicated and weird, but everything else I came up with has corner cases with very surprising behaviour.

  1. A new annotation, @Builder.Default is created to cater to defaults.

  2. Any fields with initializers without that annotation don't have any behavioural change, except for non-final fields with initializers such that they'd be taken into account by the builder: This gives you the IgnoreIt interpretation which really requires a warning because it is so unexpected. There is no easy way to get rid of the warning; you have to move the initializer expression to a constructor, but then, if you don't do that, there's really no point to that initializing expression in the first place. There's a very slight increase in boilerplate for this in the scenario where you want the initializer expression to be the value for the field if the no-args constructor is used, but not be the default when @Builder is used (as you now have to explicitly make a constructor for it, you can no longer use @NoArgsConstructor, as you'll then be stuck with a warning you can't get rid of). For initializers on final fields, no warnings and no change: You get the NoBuilder interpretation.

  3. If a field does have the @Builder.Default annotation, then it must have an initializer expression. A no-args private static method is created, named $default$fieldName(). It contains just a return statement, returning the (moved) expression. If the expression isn't legal (for example because it mentions non-static members of the class), it's moved anyway and you get an intended compiler error. The field has no initializer at all (it can't; a final field with an initializer can't be written to in any construction, so the builder can't update it at all. Even if it is not final, you can't avoid the expression from being executed even if you're immediately going to overwrite it in the constructor, which is bad if it has side-effects; one would expect that it doesn't get executed at all if you specify a value for it in the builder). Upon calling build() in builder, this static method is invoked to generate the value if needed. A boolean field is generated to track whether it has been set or not. It's not possible to clear out this boolean field once it is set. This boolean field is only made for fields with @Builder.Default. Note, it is not possible to simply initialize the field in the builder by calling this method, because this results in surprises when the initializing expression is timing-sensitive (System.currentTimeMillis() for example), and/or has side-effects, such as AtomicInteger.incrementAndGet(). We're not going to bother trying to scan if the expression has no side-effects. That boolean field is still technically visible if you really want to refer to it, so semi unpredictable rules as to whether it'll be there or not are undesirable, and the performance benefit of avoiding the boolean is negligible; builder objects usually don't live long and thus are efficiently collected.

  4. You cannot mix @Builder.Default with @Singular, at all. Lombok generates a custom error if you try (the default for @Singular fields is the appropriate empty collection, you can't change that; if there is something else as default, does that default still apply if you called addAll with an empty collection?)

  5. It would be possible to stash the 'has this been set?' tracker fields as bits into a long or int, but because of the additional complexity in generated code and the near impossibility of interacting with them if you do want to add stuff to the generated builder, we've decided against doing that. There will be one boolean field in the builder class per field with a default value.

  6. If you add @NoArgsConstructor to your class that also has @Builder on it, the generated no-args constructor will NOT initialize with the stated defaults. Generally @NoArgsConstructor mixed with @Builder is weird and strongly suggests you're using the class in a marshalling library such as jackson or JPA/Hibernate which will soon override all the fields with setters anyway. If you really need to initialize with defaults you can write it yourself, calling the $default$fieldName() static methods to produce the values.

  7. If you write your own constructor, or you put @Builder on a given constructor, the above all still functions; the assumption is then made that the name of the parameter on the constructor matches the field name and the default will be taken from that. Any @Builder.Default on a field that isn't used by any generated builder will result in an unremovable warning.

  8. If @Builder is put on a static method, the return type of that method must be the same as the class you're in, otherwise the @Builder.Default feature cannot be used at all. If the return type is your own class, it works as the previous rule: If the parameter name matches the field name, the default is taken from there.

  9. While we're messing with @Builder: If @Builder is applied to a non-static inner class, we'll generate an explicit error; right now you get a bunch of weird errors because we can assume we can generate static members inside the class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment