From e69a991fcb141fb24de8afb433c753d35821b1c3 Mon Sep 17 00:00:00 2001 From: Reinier Zwitserloot Date: Mon, 6 May 2019 22:48:11 +0200 Subject: [fixes #2120] ecj was not generating explicit nullchecks for builder-setters. --- doc/changelog.markdown | 1 + .../eclipse/handlers/EclipseHandlerUtil.java | 11 ++++++ src/core/lombok/eclipse/handlers/HandleSetter.java | 4 +-- src/core/lombok/javac/handlers/HandleSetter.java | 4 +-- .../lombok/javac/handlers/JavacHandlerUtil.java | 9 +++++ .../after-delombok/BuilderWithNonNull.java | 40 ++++++++++++++++++++++ .../resource/after-ecj/BuilderWithNonNull.java | 34 ++++++++++++++++++ .../after-ecj/SuperBuilderWithNonNull.java | 8 +++++ .../resource/before/BuilderWithNonNull.java | 5 +++ 9 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 test/transform/resource/after-delombok/BuilderWithNonNull.java create mode 100644 test/transform/resource/after-ecj/BuilderWithNonNull.java create mode 100644 test/transform/resource/before/BuilderWithNonNull.java diff --git a/doc/changelog.markdown b/doc/changelog.markdown index acaa74de..0ed8afe1 100644 --- a/doc/changelog.markdown +++ b/doc/changelog.markdown @@ -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) diff --git a/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java b/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java index 010dc9d8..ddb2f198 100644 --- a/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java +++ b/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java @@ -725,6 +725,17 @@ public class EclipseHandlerUtil { return false; } + public static boolean hasNonNullAnnotations(EclipseNode node, List 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]; /** diff --git a/src/core/lombok/eclipse/handlers/HandleSetter.java b/src/core/lombok/eclipse/handlers/HandleSetter.java index 529a7d19..d70d4acd 100644 --- a/src/core/lombok/eclipse/handlers/HandleSetter.java +++ b/src/core/lombok/eclipse/handlers/HandleSetter.java @@ -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 @@ -238,7 +238,7 @@ public class HandleSetter extends EclipseAnnotationHandler { Annotation[] copyableAnnotations = findCopyableAnnotations(fieldNode); List statements = new ArrayList(5); - if (!hasNonNullAnnotations(fieldNode)) { + if (!hasNonNullAnnotations(fieldNode) && !hasNonNullAnnotations(fieldNode, onParam)) { statements.add(assignment); } else { Statement nullCheck = generateNullCheck(field, sourceNode); diff --git a/src/core/lombok/javac/handlers/HandleSetter.java b/src/core/lombok/javac/handlers/HandleSetter.java index 28f5318d..c661ab89 100644 --- a/src/core/lombok/javac/handlers/HandleSetter.java +++ b/src/core/lombok/javac/handlers/HandleSetter.java @@ -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 @@ -234,7 +234,7 @@ public class HandleSetter extends JavacAnnotationHandler { 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); diff --git a/src/core/lombok/javac/handlers/JavacHandlerUtil.java b/src/core/lombok/javac/handlers/JavacHandlerUtil.java index 509a7397..e923c5c0 100644 --- a/src/core/lombok/javac/handlers/JavacHandlerUtil.java +++ b/src/core/lombok/javac/handlers/JavacHandlerUtil.java @@ -1418,6 +1418,15 @@ public class JavacHandlerUtil { return false; } + public static boolean hasNonNullAnnotations(JavacNode node, List 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). */ diff --git a/test/transform/resource/after-delombok/BuilderWithNonNull.java b/test/transform/resource/after-delombok/BuilderWithNonNull.java new file mode 100644 index 00000000..bee7d415 --- /dev/null +++ b/test/transform/resource/after-delombok/BuilderWithNonNull.java @@ -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(); + } +} \ No newline at end of file diff --git a/test/transform/resource/after-ecj/BuilderWithNonNull.java b/test/transform/resource/after-ecj/BuilderWithNonNull.java new file mode 100644 index 00000000..a8ef93f0 --- /dev/null +++ b/test/transform/resource/after-ecj/BuilderWithNonNull.java @@ -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(); + } +} \ No newline at end of file diff --git a/test/transform/resource/after-ecj/SuperBuilderWithNonNull.java b/test/transform/resource/after-ecj/SuperBuilderWithNonNull.java index 4b5cb188..616d7083 100644 --- a/test/transform/resource/after-ecj/SuperBuilderWithNonNull.java +++ b/test/transform/resource/after-ecj/SuperBuilderWithNonNull.java @@ -10,6 +10,10 @@ public class SuperBuilderWithNonNull { 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(); @@ -57,6 +61,10 @@ public class SuperBuilderWithNonNull { 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(); } diff --git a/test/transform/resource/before/BuilderWithNonNull.java b/test/transform/resource/before/BuilderWithNonNull.java new file mode 100644 index 00000000..03a54326 --- /dev/null +++ b/test/transform/resource/before/BuilderWithNonNull.java @@ -0,0 +1,5 @@ +@lombok.Builder +class BuilderWithNonNull { + @lombok.NonNull + private final String id; +} -- cgit