Skip to content

Commit

Permalink
[fixes #2120] ecj was not generating explicit nullchecks for builder-…
Browse files Browse the repository at this point in the history
…setters.
  • Loading branch information
rzwitserloot committed May 6, 2019
1 parent d41e804 commit e69a991
Show file tree
Hide file tree
Showing 9 changed files with 112 additions and 4 deletions.
1 change: 1 addition & 0 deletions doc/changelog.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Lombok Changelog
* BUGFIX: If you use `@Builder` and manually write the `build()` method in your builder class, javac would error out instead of deferring to your implementation. [Issue #2050](https://github.com/rzwitserloot/lombok/issues/2050) [Issue #2061](https://github.com/rzwitserloot/lombok/issues/2061)
* BUGFIX: `@SuperBuilder` together with `@Singular` on non-lists would produce an erroneous `emptyList` call. [Issue #2104](https://github.com/rzwitserloot/lombok/issues/2104).
* IMPROBABLE BREAKING CHANGE: For fields and parameters marked non-null, if the method body starts with an assert statement to ensure the value isn't null, no code to throw an exception will be generated.
* IMPROBABLE BREAKING CHANGE: When using `ecj` to compile java code with `@Builder` or `@SuperBuilder` in it, and a builder setter method was generated for a `@NonNull`-marked method, no explicit null check would be present. However, running `javac` on the exact same file _would_ produce the null check. Now ecj also produces this null check. [Issue #2120](https://github.com/rzwitserloot/lombok/issues/2120).

### v1.18.6 (February 12th, 2019)
* FEATURE: Javadoc on fields will now also be copied to the Builders' setters. Thanks for the contribution, Emil Lundberg. [Issue #2008](https://github.com/rzwitserloot/lombok/issues/2008)
Expand Down
11 changes: 11 additions & 0 deletions src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,17 @@ public static boolean hasNonNullAnnotations(EclipseNode node) {
return false;
}

public static boolean hasNonNullAnnotations(EclipseNode node, List<Annotation> anns) {
if (anns == null) return false;
for (Annotation annotation : anns) {
TypeReference typeRef = annotation.type;
if (typeRef != null && typeRef.getTypeName() != null) {
for (String bn : NONNULL_ANNOTATIONS) if (typeMatches(bn, node, typeRef)) return true;
}
}
return false;
}

private static final Annotation[] EMPTY_ANNOTATIONS_ARRAY = new Annotation[0];

/**
Expand Down
4 changes: 2 additions & 2 deletions src/core/lombok/eclipse/handlers/HandleSetter.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2009-2017 The Project Lombok Authors.
* Copyright (C) 2009-2019 The Project Lombok Authors.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -238,7 +238,7 @@ static MethodDeclaration createSetter(TypeDeclaration parent, boolean deprecate,

Annotation[] copyableAnnotations = findCopyableAnnotations(fieldNode);
List<Statement> statements = new ArrayList<Statement>(5);
if (!hasNonNullAnnotations(fieldNode)) {
if (!hasNonNullAnnotations(fieldNode) && !hasNonNullAnnotations(fieldNode, onParam)) {
statements.add(assignment);
} else {
Statement nullCheck = generateNullCheck(field, sourceNode);
Expand Down
4 changes: 2 additions & 2 deletions src/core/lombok/javac/handlers/HandleSetter.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2009-2017 The Project Lombok Authors.
* Copyright (C) 2009-2019 The Project Lombok Authors.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -234,7 +234,7 @@ public static JCMethodDecl createSetter(long access, boolean deprecate, JavacNod
long flags = JavacHandlerUtil.addFinalIfNeeded(Flags.PARAMETER, field.getContext());
JCVariableDecl param = treeMaker.VarDef(treeMaker.Modifiers(flags, annsOnParam), fieldDecl.name, fieldDecl.vartype, null);

if (!hasNonNullAnnotations(field)) {
if (!hasNonNullAnnotations(field) && !hasNonNullAnnotations(field, onParam)) {
statements.append(treeMaker.Exec(assign));
} else {
JCStatement nullCheck = generateNullCheck(treeMaker, field, source);
Expand Down
9 changes: 9 additions & 0 deletions src/core/lombok/javac/handlers/JavacHandlerUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -1418,6 +1418,15 @@ public static boolean hasNonNullAnnotations(JavacNode node) {
return false;
}

public static boolean hasNonNullAnnotations(JavacNode node, List<JCAnnotation> anns) {
if (anns == null) return false;
for (JCAnnotation ann : anns) {
for (String nn : NONNULL_ANNOTATIONS) if (typeMatches(nn, node, ann)) return true;
}

return false;
}

/**
* Searches the given field node for annotations and returns each one that is 'copyable' (either via configuration or from the base list).
*/
Expand Down
40 changes: 40 additions & 0 deletions test/transform/resource/after-delombok/BuilderWithNonNull.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
class BuilderWithNonNull {
@lombok.NonNull
private final String id;
@java.lang.SuppressWarnings("all")
BuilderWithNonNull(@lombok.NonNull final String id) {
if (id == null) {
throw new java.lang.NullPointerException("id is marked non-null but is null");
}
this.id = id;
}
@java.lang.SuppressWarnings("all")
public static class BuilderWithNonNullBuilder {
@java.lang.SuppressWarnings("all")
private String id;
@java.lang.SuppressWarnings("all")
BuilderWithNonNullBuilder() {
}
@java.lang.SuppressWarnings("all")
public BuilderWithNonNullBuilder id(@lombok.NonNull final String id) {
if (id == null) {
throw new java.lang.NullPointerException("id is marked non-null but is null");
}
this.id = id;
return this;
}
@java.lang.SuppressWarnings("all")
public BuilderWithNonNull build() {
return new BuilderWithNonNull(id);
}
@java.lang.Override
@java.lang.SuppressWarnings("all")
public java.lang.String toString() {
return "BuilderWithNonNull.BuilderWithNonNullBuilder(id=" + this.id + ")";
}
}
@java.lang.SuppressWarnings("all")
public static BuilderWithNonNullBuilder builder() {
return new BuilderWithNonNullBuilder();
}
}
34 changes: 34 additions & 0 deletions test/transform/resource/after-ecj/BuilderWithNonNull.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
@lombok.Builder class BuilderWithNonNull {
public static @java.lang.SuppressWarnings("all") class BuilderWithNonNullBuilder {
private @java.lang.SuppressWarnings("all") String id;
@java.lang.SuppressWarnings("all") BuilderWithNonNullBuilder() {
super();
}
public @java.lang.SuppressWarnings("all") BuilderWithNonNullBuilder id(final @lombok.NonNull String id) {
if ((id == null))
{
throw new java.lang.NullPointerException("id is marked non-null but is null");
}
this.id = id;
return this;
}
public @java.lang.SuppressWarnings("all") BuilderWithNonNull build() {
return new BuilderWithNonNull(id);
}
public @java.lang.Override @java.lang.SuppressWarnings("all") java.lang.String toString() {
return (("BuilderWithNonNull.BuilderWithNonNullBuilder(id=" + this.id) + ")");
}
}
private final @lombok.NonNull String id;
@java.lang.SuppressWarnings("all") BuilderWithNonNull(final @lombok.NonNull String id) {
super();
if ((id == null))
{
throw new java.lang.NullPointerException("id is marked non-null but is null");
}
this.id = id;
}
public static @java.lang.SuppressWarnings("all") BuilderWithNonNullBuilder builder() {
return new BuilderWithNonNullBuilder();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ public ParentBuilder() {
protected abstract @java.lang.SuppressWarnings("all") B self();
public abstract @java.lang.SuppressWarnings("all") C build();
public @java.lang.SuppressWarnings("all") B nonNullParentField(final @lombok.NonNull String nonNullParentField) {
if ((nonNullParentField == null))
{
throw new java.lang.NullPointerException("nonNullParentField is marked non-null but is null");
}
this.nonNullParentField = nonNullParentField;
nonNullParentField$set = true;
return self();
Expand Down Expand Up @@ -57,6 +61,10 @@ public ChildBuilder() {
protected abstract @java.lang.Override @java.lang.SuppressWarnings("all") B self();
public abstract @java.lang.Override @java.lang.SuppressWarnings("all") C build();
public @java.lang.SuppressWarnings("all") B nonNullChildField(final @lombok.NonNull String nonNullChildField) {
if ((nonNullChildField == null))
{
throw new java.lang.NullPointerException("nonNullChildField is marked non-null but is null");
}
this.nonNullChildField = nonNullChildField;
return self();
}
Expand Down
5 changes: 5 additions & 0 deletions test/transform/resource/before/BuilderWithNonNull.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
@lombok.Builder
class BuilderWithNonNull {
@lombok.NonNull
private final String id;
}

0 comments on commit e69a991

Please sign in to comment.