From ca35539f7cd7967bfd8518d0ad0b0015bdd40cfc Mon Sep 17 00:00:00 2001 From: Reinier Zwitserloot Date: Tue, 21 Aug 2018 00:01:40 +0200 Subject: [fixes #1812] `@Singular` marked collections which nevertheless somehow ended up null would cause an NPE during `toBuilder()` invocations. --- .../eclipse/handlers/EclipseHandlerUtil.java | 26 +++++++++ .../lombok/eclipse/handlers/HandleBuilder.java | 34 +++++++++--- .../eclipse/handlers/HandleEqualsAndHashCode.java | 26 --------- src/core/lombok/javac/handlers/HandleBuilder.java | 28 +++++++--- .../BuilderSingularToBuilderWithNull.java | 63 ++++++++++++++++++++++ .../after-delombok/BuilderWithToBuilder.java | 2 +- .../BuilderSingularToBuilderWithNull.java | 57 ++++++++++++++++++++ .../resource/after-ecj/BuilderWithToBuilder.java | 2 +- .../before/BuilderSingularToBuilderWithNull.java | 10 ++++ 9 files changed, 206 insertions(+), 42 deletions(-) create mode 100644 test/transform/resource/after-delombok/BuilderSingularToBuilderWithNull.java create mode 100644 test/transform/resource/after-ecj/BuilderSingularToBuilderWithNull.java create mode 100644 test/transform/resource/before/BuilderSingularToBuilderWithNull.java diff --git a/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java b/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java index 2dce285c..87df6d1b 100644 --- a/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java +++ b/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java @@ -24,6 +24,7 @@ package lombok.eclipse.handlers; import static lombok.core.handlers.HandlerUtil.*; import static lombok.eclipse.Eclipse.*; import static lombok.eclipse.EclipseAugments.*; +import static lombok.eclipse.handlers.EclipseHandlerUtil.setGeneratedBy; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; @@ -1862,4 +1863,29 @@ public class EclipseHandlerUtil { String p = typeDecl.superclass.toString(); return p.equals("Object") || p.equals("java.lang.Object"); } + + public static NameReference generateQualifiedNameRef(ASTNode source, char[]... varNames) { + int pS = source.sourceStart, pE = source.sourceEnd; + long p = (long)pS << 32 | pE; + + NameReference ref; + + if (varNames.length > 1) ref = new QualifiedNameReference(varNames, new long[varNames.length], pS, pE); + else ref = new SingleNameReference(varNames[0], p); + setGeneratedBy(ref, source); + return ref; + } + + public static TypeReference generateQualifiedTypeRef(ASTNode source, char[]... varNames) { + int pS = source.sourceStart, pE = source.sourceEnd; + long p = (long)pS << 32 | pE; + + TypeReference ref; + + long[] poss = Eclipse.poss(source, varNames.length); + if (varNames.length > 1) ref = new QualifiedTypeReference(varNames, poss); + else ref = new SingleTypeReference(varNames[0], p); + setGeneratedBy(ref, source); + return ref; + } } diff --git a/src/core/lombok/eclipse/handlers/HandleBuilder.java b/src/core/lombok/eclipse/handlers/HandleBuilder.java index 4301e51a..9a069c58 100644 --- a/src/core/lombok/eclipse/handlers/HandleBuilder.java +++ b/src/core/lombok/eclipse/handlers/HandleBuilder.java @@ -40,6 +40,7 @@ import org.eclipse.jdt.internal.compiler.ast.Assignment; import org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration; import org.eclipse.jdt.internal.compiler.ast.ConditionalExpression; import org.eclipse.jdt.internal.compiler.ast.ConstructorDeclaration; +import org.eclipse.jdt.internal.compiler.ast.EqualExpression; import org.eclipse.jdt.internal.compiler.ast.Expression; import org.eclipse.jdt.internal.compiler.ast.FalseLiteral; import org.eclipse.jdt.internal.compiler.ast.FieldDeclaration; @@ -47,6 +48,7 @@ import org.eclipse.jdt.internal.compiler.ast.FieldReference; import org.eclipse.jdt.internal.compiler.ast.IfStatement; import org.eclipse.jdt.internal.compiler.ast.MessageSend; import org.eclipse.jdt.internal.compiler.ast.MethodDeclaration; +import org.eclipse.jdt.internal.compiler.ast.NullLiteral; import org.eclipse.jdt.internal.compiler.ast.OperatorIds; import org.eclipse.jdt.internal.compiler.ast.ParameterizedQualifiedTypeReference; import org.eclipse.jdt.internal.compiler.ast.ParameterizedSingleTypeReference; @@ -492,6 +494,7 @@ public class HandleBuilder extends EclipseAnnotationHandler { } } + private static final char[] EMPTY_LIST = "emptyList".toCharArray(); private MethodDeclaration generateToBuilderMethod(String methodName, String builderClassName, EclipseNode type, TypeParameter[] typeParams, List builderFields, boolean fluent, ASTNode source) { // return new ThingieBuilder().setA(this.a).setB(this.b); @@ -511,19 +514,34 @@ public class HandleBuilder extends EclipseAnnotationHandler { for (BuilderFieldData bfd : builderFields) { char[] setterName = fluent ? bfd.name : HandlerUtil.buildAccessorName("set", new String(bfd.name)).toCharArray(); MessageSend ms = new MessageSend(); + Expression[] tgt = new Expression[bfd.singularData == null ? 1 : 2]; + if (bfd.obtainVia == null || !bfd.obtainVia.field().isEmpty()) { char[] fieldName = bfd.obtainVia == null ? bfd.rawName : bfd.obtainVia.field().toCharArray(); - FieldReference fr = new FieldReference(fieldName, 0); - fr.receiver = new ThisReference(0, 0); - ms.arguments = new Expression[] {fr}; + for (int i = 0; i < tgt.length; i++) { + FieldReference fr = new FieldReference(fieldName, 0); + fr.receiver = new ThisReference(0, 0); + tgt[i] = fr; + } } else { String obtainName = bfd.obtainVia.method(); boolean obtainIsStatic = bfd.obtainVia.isStatic(); - MessageSend obtainExpr = new MessageSend(); - obtainExpr.receiver = obtainIsStatic ? new SingleNameReference(type.getName().toCharArray(), 0) : new ThisReference(0, 0); - obtainExpr.selector = obtainName.toCharArray(); - if (obtainIsStatic) obtainExpr.arguments = new Expression[] {new ThisReference(0, 0)}; - ms.arguments = new Expression[] {obtainExpr}; + for (int i = 0; i < tgt.length; i++) { + MessageSend obtainExpr = new MessageSend(); + obtainExpr.receiver = obtainIsStatic ? new SingleNameReference(type.getName().toCharArray(), 0) : new ThisReference(0, 0); + obtainExpr.selector = obtainName.toCharArray(); + if (obtainIsStatic) obtainExpr.arguments = new Expression[] {new ThisReference(0, 0)}; + tgt[i] = obtainExpr; + } + } + if (bfd.singularData == null) { + ms.arguments = tgt; + } else { + Expression ifNull = new EqualExpression(tgt[0], new NullLiteral(0, 0), OperatorIds.EQUAL_EQUAL); + MessageSend emptyList = new MessageSend(); + emptyList.receiver = generateQualifiedNameRef(source, TypeConstants.JAVA, TypeConstants.UTIL, "Collections".toCharArray()); + emptyList.selector = EMPTY_LIST; + ms.arguments = new Expression[] {new ConditionalExpression(ifNull, emptyList, tgt[1])}; } ms.receiver = receiver; ms.selector = setterName; diff --git a/src/core/lombok/eclipse/handlers/HandleEqualsAndHashCode.java b/src/core/lombok/eclipse/handlers/HandleEqualsAndHashCode.java index c99b9b5f..6945e5d9 100644 --- a/src/core/lombok/eclipse/handlers/HandleEqualsAndHashCode.java +++ b/src/core/lombok/eclipse/handlers/HandleEqualsAndHashCode.java @@ -69,7 +69,6 @@ import org.eclipse.jdt.internal.compiler.ast.NullLiteral; import org.eclipse.jdt.internal.compiler.ast.OperatorIds; import org.eclipse.jdt.internal.compiler.ast.ParameterizedQualifiedTypeReference; import org.eclipse.jdt.internal.compiler.ast.ParameterizedSingleTypeReference; -import org.eclipse.jdt.internal.compiler.ast.QualifiedNameReference; import org.eclipse.jdt.internal.compiler.ast.QualifiedTypeReference; import org.eclipse.jdt.internal.compiler.ast.ReturnStatement; import org.eclipse.jdt.internal.compiler.ast.SingleNameReference; @@ -819,29 +818,4 @@ public class HandleEqualsAndHashCode extends EclipseAnnotationHandler 1) ref = new QualifiedNameReference(varNames, new long[varNames.length], pS, pE); - else ref = new SingleNameReference(varNames[0], p); - setGeneratedBy(ref, source); - return ref; - } - - public TypeReference generateQualifiedTypeRef(ASTNode source, char[]... varNames) { - int pS = source.sourceStart, pE = source.sourceEnd; - long p = (long)pS << 32 | pE; - - TypeReference ref; - - long[] poss = Eclipse.poss(source, varNames.length); - if (varNames.length > 1) ref = new QualifiedTypeReference(varNames, poss); - else ref = new SingleTypeReference(varNames[0], p); - setGeneratedBy(ref, source); - return ref; - } } diff --git a/src/core/lombok/javac/handlers/HandleBuilder.java b/src/core/lombok/javac/handlers/HandleBuilder.java index f04ea8b1..edf6f2ae 100644 --- a/src/core/lombok/javac/handlers/HandleBuilder.java +++ b/src/core/lombok/javac/handlers/HandleBuilder.java @@ -492,18 +492,34 @@ public class HandleBuilder extends JavacAnnotationHandler { JCExpression invoke = call; for (BuilderFieldData bfd : builderFields) { Name setterName = fluent ? bfd.name : type.toName(HandlerUtil.buildAccessorName("set", bfd.name.toString())); - JCExpression arg; + JCExpression[] tgt = new JCExpression[bfd.singularData == null ? 1 : 2]; if (bfd.obtainVia == null || !bfd.obtainVia.field().isEmpty()) { - arg = maker.Select(maker.Ident(type.toName("this")), bfd.obtainVia == null ? bfd.rawName : type.toName(bfd.obtainVia.field())); + for (int i = 0; i < tgt.length; i++) { + tgt[i] = maker.Select(maker.Ident(type.toName("this")), bfd.obtainVia == null ? bfd.rawName : type.toName(bfd.obtainVia.field())); + } } else { if (bfd.obtainVia.isStatic()) { - JCExpression c = maker.Select(maker.Ident(type.toName(type.getName())), type.toName(bfd.obtainVia.method())); - arg = maker.Apply(List.nil(), c, List.of(maker.Ident(type.toName("this")))); + for (int i = 0; i < tgt.length; i++) { + JCExpression c = maker.Select(maker.Ident(type.toName(type.getName())), type.toName(bfd.obtainVia.method())); + tgt[i] = maker.Apply(List.nil(), c, List.of(maker.Ident(type.toName("this")))); + } } else { - JCExpression c = maker.Select(maker.Ident(type.toName("this")), type.toName(bfd.obtainVia.method())); - arg = maker.Apply(List.nil(), c, List.nil()); + for (int i = 0; i < tgt.length; i++) { + JCExpression c = maker.Select(maker.Ident(type.toName("this")), type.toName(bfd.obtainVia.method())); + tgt[i] = maker.Apply(List.nil(), c, List.nil()); + } } } + + JCExpression arg; + if (bfd.singularData == null) { + arg = tgt[0]; + } else { + JCExpression eqNull = maker.Binary(CTC_EQUAL, tgt[0], maker.Literal(CTC_BOT, null)); + JCExpression emptyList = maker.Apply(List.nil(), chainDots(type, "java", "util", "Collections", "emptyList"), List.nil()); + arg = maker.Conditional(eqNull, emptyList, tgt[1]); + } + invoke = maker.Apply(List.nil(), maker.Select(invoke, setterName), List.of(arg)); } JCStatement statement = maker.Return(invoke); diff --git a/test/transform/resource/after-delombok/BuilderSingularToBuilderWithNull.java b/test/transform/resource/after-delombok/BuilderSingularToBuilderWithNull.java new file mode 100644 index 00000000..1f472438 --- /dev/null +++ b/test/transform/resource/after-delombok/BuilderSingularToBuilderWithNull.java @@ -0,0 +1,63 @@ +class BuilderSingularToBuilderWithNull { + private java.util.List elems; + public static void test() { + new BuilderSingularToBuilderWithNull(null).toBuilder(); + } + @java.lang.SuppressWarnings("all") + BuilderSingularToBuilderWithNull(final java.util.List elems) { + this.elems = elems; + } + @java.lang.SuppressWarnings("all") + public static class BuilderSingularToBuilderWithNullBuilder { + @java.lang.SuppressWarnings("all") + private java.util.ArrayList elems; + @java.lang.SuppressWarnings("all") + BuilderSingularToBuilderWithNullBuilder() { + } + @java.lang.SuppressWarnings("all") + public BuilderSingularToBuilderWithNullBuilder elem(final String elem) { + if (this.elems == null) this.elems = new java.util.ArrayList(); + this.elems.add(elem); + return this; + } + @java.lang.SuppressWarnings("all") + public BuilderSingularToBuilderWithNullBuilder elems(final java.util.Collection elems) { + if (this.elems == null) this.elems = new java.util.ArrayList(); + this.elems.addAll(elems); + return this; + } + @java.lang.SuppressWarnings("all") + public BuilderSingularToBuilderWithNullBuilder clearElems() { + if (this.elems != null) this.elems.clear(); + return this; + } + @java.lang.SuppressWarnings("all") + public BuilderSingularToBuilderWithNull build() { + java.util.List elems; + switch (this.elems == null ? 0 : this.elems.size()) { + case 0: + elems = java.util.Collections.emptyList(); + break; + case 1: + elems = java.util.Collections.singletonList(this.elems.get(0)); + break; + default: + elems = java.util.Collections.unmodifiableList(new java.util.ArrayList(this.elems)); + } + return new BuilderSingularToBuilderWithNull(elems); + } + @java.lang.Override + @java.lang.SuppressWarnings("all") + public java.lang.String toString() { + return "BuilderSingularToBuilderWithNull.BuilderSingularToBuilderWithNullBuilder(elems=" + this.elems + ")"; + } + } + @java.lang.SuppressWarnings("all") + public static BuilderSingularToBuilderWithNullBuilder builder() { + return new BuilderSingularToBuilderWithNullBuilder(); + } + @java.lang.SuppressWarnings("all") + public BuilderSingularToBuilderWithNullBuilder toBuilder() { + return new BuilderSingularToBuilderWithNullBuilder().elems(this.elems == null ? java.util.Collections.emptyList() : this.elems); + } +} diff --git a/test/transform/resource/after-delombok/BuilderWithToBuilder.java b/test/transform/resource/after-delombok/BuilderWithToBuilder.java index 46387f0f..b644a16f 100644 --- a/test/transform/resource/after-delombok/BuilderWithToBuilder.java +++ b/test/transform/resource/after-delombok/BuilderWithToBuilder.java @@ -86,7 +86,7 @@ class BuilderWithToBuilder { } @java.lang.SuppressWarnings("all") public BuilderWithToBuilderBuilder toBuilder() { - return new BuilderWithToBuilderBuilder().one(this.mOne).two(this.mTwo).foo(BuilderWithToBuilder.rrr(this)).bars(this.bars); + return new BuilderWithToBuilderBuilder().one(this.mOne).two(this.mTwo).foo(BuilderWithToBuilder.rrr(this)).bars(this.bars == null ? java.util.Collections.emptyList() : this.bars); } } class ConstructorWithToBuilder { diff --git a/test/transform/resource/after-ecj/BuilderSingularToBuilderWithNull.java b/test/transform/resource/after-ecj/BuilderSingularToBuilderWithNull.java new file mode 100644 index 00000000..7265e17a --- /dev/null +++ b/test/transform/resource/after-ecj/BuilderSingularToBuilderWithNull.java @@ -0,0 +1,57 @@ +import lombok.Singular; +@lombok.Builder(toBuilder = true) class BuilderSingularToBuilderWithNull { + public static @java.lang.SuppressWarnings("all") class BuilderSingularToBuilderWithNullBuilder { + private @java.lang.SuppressWarnings("all") java.util.ArrayList elems; + @java.lang.SuppressWarnings("all") BuilderSingularToBuilderWithNullBuilder() { + super(); + } + public @java.lang.SuppressWarnings("all") BuilderSingularToBuilderWithNullBuilder elem(String elem) { + if ((this.elems == null)) + this.elems = new java.util.ArrayList(); + this.elems.add(elem); + return this; + } + public @java.lang.SuppressWarnings("all") BuilderSingularToBuilderWithNullBuilder elems(java.util.Collection elems) { + if ((this.elems == null)) + this.elems = new java.util.ArrayList(); + this.elems.addAll(elems); + return this; + } + public @java.lang.SuppressWarnings("all") BuilderSingularToBuilderWithNullBuilder clearElems() { + if ((this.elems != null)) + this.elems.clear(); + return this; + } + public @java.lang.SuppressWarnings("all") BuilderSingularToBuilderWithNull build() { + java.util.List elems; + switch (((this.elems == null) ? 0 : this.elems.size())) { + case 0 : + elems = java.util.Collections.emptyList(); + break; + case 1 : + elems = java.util.Collections.singletonList(this.elems.get(0)); + break; + default : + elems = java.util.Collections.unmodifiableList(new java.util.ArrayList(this.elems)); + } + return new BuilderSingularToBuilderWithNull(elems); + } + public @java.lang.Override @java.lang.SuppressWarnings("all") java.lang.String toString() { + return (("BuilderSingularToBuilderWithNull.BuilderSingularToBuilderWithNullBuilder(elems=" + this.elems) + ")"); + } + } + private @Singular java.util.List elems; + public static void test() { + new BuilderSingularToBuilderWithNull(null).toBuilder(); + } + @java.lang.SuppressWarnings("all") BuilderSingularToBuilderWithNull(final java.util.List elems) { + super(); + this.elems = elems; + } + public static @java.lang.SuppressWarnings("all") BuilderSingularToBuilderWithNullBuilder builder() { + return new BuilderSingularToBuilderWithNullBuilder(); + } + public @java.lang.SuppressWarnings("all") BuilderSingularToBuilderWithNullBuilder toBuilder() { + return new BuilderSingularToBuilderWithNullBuilder().elems(((this.elems == null) ? java.util.Collections.emptyList() : this.elems)); + } +} diff --git a/test/transform/resource/after-ecj/BuilderWithToBuilder.java b/test/transform/resource/after-ecj/BuilderWithToBuilder.java index d304293c..b9cc27dd 100644 --- a/test/transform/resource/after-ecj/BuilderWithToBuilder.java +++ b/test/transform/resource/after-ecj/BuilderWithToBuilder.java @@ -74,7 +74,7 @@ import lombok.Builder; return new BuilderWithToBuilderBuilder(); } public @java.lang.SuppressWarnings("all") BuilderWithToBuilderBuilder toBuilder() { - return new BuilderWithToBuilderBuilder().one(this.mOne).two(this.mTwo).foo(BuilderWithToBuilder.rrr(this)).bars(this.bars); + return new BuilderWithToBuilderBuilder().one(this.mOne).two(this.mTwo).foo(BuilderWithToBuilder.rrr(this)).bars(((this.bars == null) ? java.util.Collections.emptyList() : this.bars)); } } @lombok.experimental.Accessors(prefix = "m") class ConstructorWithToBuilder { diff --git a/test/transform/resource/before/BuilderSingularToBuilderWithNull.java b/test/transform/resource/before/BuilderSingularToBuilderWithNull.java new file mode 100644 index 00000000..92a102cb --- /dev/null +++ b/test/transform/resource/before/BuilderSingularToBuilderWithNull.java @@ -0,0 +1,10 @@ +import lombok.Singular; + +@lombok.Builder(toBuilder = true) +class BuilderSingularToBuilderWithNull { + @Singular private java.util.List elems; + + public static void test() { + new BuilderSingularToBuilderWithNull(null).toBuilder(); + } +} -- cgit