[fixes #2335] ObtainVia(method=) on more than one arg would crash in javac

7 files changed, 207 insertions, 34 deletions

diff --git a/src/core/lombok/eclipse/handlers/HandleBuilder.java b/src/core/lombok/eclipse/handlers/HandleBuilder.java
index 3f711a19..bb031ebc 100755
--- a/src/core/lombok/eclipse/handlers/HandleBuilder.java
+++ b/src/core/lombok/eclipse/handlers/HandleBuilder.java
@@ -565,7 +565,9 @@ public class HandleBuilder extends EclipseAnnotationHandler<Builder> {
 		invoke.type = namePlusTypeParamsToTypeReference(type, builderClassName.toCharArray(), !isStatic, typeParams, p);
 		
 		Expression receiver = invoke;
-		List<Statement> statements = null;
+		List<Statement> preStatements = null;
+		List<Statement> postStatements = null;
+		
 		for (BuilderFieldData bfd : builderFields) {
 			String setterName = new String(bfd.name);
 			String setterPrefix = !prefix.isEmpty() ? prefix : fluent ? "" : "set";
@@ -584,23 +586,31 @@ public class HandleBuilder extends EclipseAnnotationHandler<Builder> {
 			} else {
 				String obtainName = bfd.obtainVia.method();
 				boolean obtainIsStatic = bfd.obtainVia.isStatic();
-				for (int i = 0; i < tgt.length; i++) {
-					MessageSend obtainExpr = new MessageSend();
-					if (obtainIsStatic) {
-						if (typeParams != null && typeParams.length > 0) {
-							obtainExpr.typeArguments = new TypeReference[typeParams.length];
-							for (int j = 0; j<typeParams.length; j++) {
-								obtainExpr.typeArguments[j] = new SingleTypeReference(typeParams[j].name, 0);
-							}
+				MessageSend obtainExpr = new MessageSend();
+				if (obtainIsStatic) {
+					if (typeParams != null && typeParams.length > 0) {
+						obtainExpr.typeArguments = new TypeReference[typeParams.length];
+						for (int j = 0; j<typeParams.length; j++) {
+							obtainExpr.typeArguments[j] = new SingleTypeReference(typeParams[j].name, 0);
 						}
-						obtainExpr.receiver = generateNameReference(type, 0);
-					} else {
-						obtainExpr.receiver = new ThisReference(0, 0);
 					}
-					obtainExpr.selector = obtainName.toCharArray();
-					if (obtainIsStatic) obtainExpr.arguments = new Expression[] {new ThisReference(0, 0)};
-					tgt[i] = obtainExpr;
+					obtainExpr.receiver = generateNameReference(type, 0);
+				} else {
+					obtainExpr.receiver = new ThisReference(0, 0);
 				}
+				obtainExpr.selector = obtainName.toCharArray();
+				if (obtainIsStatic) obtainExpr.arguments = new Expression[] {new ThisReference(0, 0)};
+				for (int i = 0; i < tgt.length; i++) tgt[i] = new SingleNameReference(bfd.name, 0L);
+				
+				// javac appears to cache the type of JCMethodInvocation expressions based on position, meaning, if you have 2 ObtainVia-based method invokes on different types, you get bizarre type mismatch errors.
+				// going via a local variable declaration solves the problem. We copy this behaviour
+				// for ecj so we match what javac's handler does.
+				LocalDeclaration ld = new LocalDeclaration(bfd.name, 0, 0);
+				ld.modifiers = ClassFileConstants.AccFinal;
+				ld.type = EclipseHandlerUtil.copyType(bfd.type, source);
+				ld.initialization = obtainExpr;
+				if (preStatements == null) preStatements = new ArrayList<Statement>();
+				preStatements.add(ld);
 			}
 			
 			ms.selector = setterName.toCharArray();
@@ -612,23 +622,28 @@ public class HandleBuilder extends EclipseAnnotationHandler<Builder> {
 				ms.arguments = new Expression[] {tgt[1]};
 				ms.receiver = new SingleNameReference(BUILDER_TEMP_VAR, p);
 				EqualExpression isNotNull = new EqualExpression(tgt[0], new NullLiteral(pS, pE), OperatorIds.NOT_EQUAL);
-				if (statements == null) statements = new ArrayList<Statement>();
-				statements.add(new IfStatement(isNotNull, ms, pS, pE));
+				if (postStatements == null) postStatements = new ArrayList<Statement>();
+				postStatements.add(new IfStatement(isNotNull, ms, pS, pE));
 			}
 		}
 		
-		if (statements != null) {
-			out.statements = new Statement[statements.size() + 2];
-			for (int i = 0; i < statements.size(); i++) out.statements[i + 1] = statements.get(i);
+		int preSs = preStatements == null ? 0 : preStatements.size();
+		int postSs = postStatements == null ? 0 : postStatements.size();
+		if (postSs > 0) {
+			out.statements = new Statement[preSs + postSs + 2];
+			for (int i = 0; i < preSs; i++) out.statements[i] = preStatements.get(i);
+			for (int i = 0; i < postSs; i++) out.statements[preSs + 1 + i] = postStatements.get(i);
 			LocalDeclaration b = new LocalDeclaration(BUILDER_TEMP_VAR, pS, pE);
-			out.statements[0] = b;
-			b.modifiers |= Modifier.FINAL;
+			out.statements[preSs] = b;
+			b.modifiers |= ClassFileConstants.AccFinal;
 			b.type = namePlusTypeParamsToTypeReference(type, builderClassName.toCharArray(), !isStatic, typeParams, p);
 			b.type.sourceStart = pS; b.type.sourceEnd = pE;
 			b.initialization = receiver;
-			out.statements[out.statements.length - 1] = new ReturnStatement(new SingleNameReference(BUILDER_TEMP_VAR, p), pS, pE);
+			out.statements[preSs + postSs + 1] = new ReturnStatement(new SingleNameReference(BUILDER_TEMP_VAR, p), pS, pE);
 		} else {
-			out.statements = new Statement[] {new ReturnStatement(receiver, pS, pE)};
+			out.statements = new Statement[preSs + 1];
+			for (int i = 0; i < preSs; i++) out.statements[i] = preStatements.get(i);
+			out.statements[preSs] = new ReturnStatement(receiver, pS, pE);
 		}
 		
 		if (cfv.generateUnique()) {
diff --git a/src/core/lombok/javac/handlers/HandleBuilder.java b/src/core/lombok/javac/handlers/HandleBuilder.java
index 1e8b8a22..0bd43ba4 100644
--- a/src/core/lombok/javac/handlers/HandleBuilder.java
+++ b/src/core/lombok/javac/handlers/HandleBuilder.java
@@ -45,6 +45,7 @@ import com.sun.tools.javac.tree.JCTree.JCIdent;
 import com.sun.tools.javac.tree.JCTree.JCIf;
 import com.sun.tools.javac.tree.JCTree.JCLiteral;
 import com.sun.tools.javac.tree.JCTree.JCMethodDecl;
+import com.sun.tools.javac.tree.JCTree.JCMethodInvocation;
 import com.sun.tools.javac.tree.JCTree.JCModifiers;
 import com.sun.tools.javac.tree.JCTree.JCNewClass;
 import com.sun.tools.javac.tree.JCTree.JCPrimitiveTypeTree;
@@ -553,7 +554,9 @@ public class HandleBuilder extends JavacAnnotationHandler<Builder> {
 		
 		JCExpression call = maker.NewClass(null, List.<JCExpression>nil(), namePlusTypeParamsToTypeReference(maker, type, type.toName(builderClassName), !isStatic, typeParams), List.<JCExpression>nil(), null);
 		JCExpression invoke = call;
+		ListBuffer<JCStatement> preStatements = null;
 		ListBuffer<JCStatement> statements = new ListBuffer<JCStatement>();
+		
 		for (BuilderFieldData bfd : builderFields) {
 			String setterPrefix = !prefix.isEmpty() ? prefix : fluent ? "" : "set";
 			String prefixedSetterName = bfd.name.toString();
@@ -566,17 +569,22 @@ public class HandleBuilder extends JavacAnnotationHandler<Builder> {
 					tgt[i] = maker.Select(maker.Ident(type.toName("this")), bfd.obtainVia == null ? bfd.rawName : type.toName(bfd.obtainVia.field()));
 				}
 			} else {
+				String name = bfd.obtainVia.method();
+				JCMethodInvocation inv;
 				if (bfd.obtainVia.isStatic()) {
-					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(typeParameterNames(maker, typeParams), c, List.<JCExpression>of(maker.Ident(type.toName("this"))));
-					}
+					JCExpression c = maker.Select(maker.Ident(type.toName(type.getName())), type.toName(name));
+					inv = maker.Apply(typeParameterNames(maker, typeParams), c, List.<JCExpression>of(maker.Ident(type.toName("this"))));
 				} else {
-					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.<JCExpression>nil(), c, List.<JCExpression>nil());
-					}
+					JCExpression c = maker.Select(maker.Ident(type.toName("this")), type.toName(name));
+					inv = maker.Apply(List.<JCExpression>nil(), c, List.<JCExpression>nil());
 				}
+				for (int i = 0; i < tgt.length; i++) tgt[i] = maker.Ident(bfd.name);
+				
+				// javac appears to cache the type of JCMethodInvocation expressions based on position, meaning, if you have 2 ObtainVia-based method invokes on different types, you get bizarre type mismatch errors.
+				// going via a local variable declaration solves the problem.
+				JCExpression varType = JavacHandlerUtil.cloneType(maker, bfd.type, ast, type.getContext());
+				if (preStatements == null) preStatements = new ListBuffer<JCStatement>();
+				preStatements.append(maker.VarDef(maker.Modifiers(Flags.FINAL), bfd.name, varType, inv));
 			}
 			
 			JCExpression arg;
@@ -589,6 +597,7 @@ public class HandleBuilder extends JavacAnnotationHandler<Builder> {
 				statements.append(maker.If(isNotNull, maker.Exec(invokeBuilder), null));
 			}
 		}
+		
 		if (!statements.isEmpty()) {
 			JCExpression tempVarType = namePlusTypeParamsToTypeReference(maker, type, type.toName(builderClassName), !isStatic, typeParams);
 			statements.prepend(maker.VarDef(maker.Modifiers(Flags.FINAL), type.toName(BUILDER_TEMP_VAR), tempVarType, invoke));
@@ -596,6 +605,11 @@ public class HandleBuilder extends JavacAnnotationHandler<Builder> {
 		} else {
 			statements.append(maker.Return(invoke));
 		}
+		
+		if (preStatements != null) {
+			preStatements.appendList(statements);
+			statements = preStatements;
+		}
 		JCBlock body = maker.Block(0, statements.toList());
 		List<JCAnnotation> annsOnMethod = cfv.generateUnique() ? List.of(maker.Annotation(genTypeRef(type, CheckerFrameworkVersion.NAME__UNIQUE), List.<JCExpression>nil())) : List.<JCAnnotation>nil();
 		return maker.MethodDef(maker.Modifiers(toJavacModifier(access), annsOnMethod), type.toName(toBuilderMethodName), namePlusTypeParamsToTypeReference(maker, type, type.toName(builderClassName), !isStatic, typeParams), List.<JCTypeParameter>nil(), List.<JCVariableDecl>nil(), List.<JCExpression>nil(), body, null);
diff --git a/test/transform/resource/after-delombok/BuilderWithToBuilder.java b/test/transform/resource/after-delombok/BuilderWithToBuilder.java
index 2c4b0531..aa47c430 100644
--- a/test/transform/resource/after-delombok/BuilderWithToBuilder.java
+++ b/test/transform/resource/after-delombok/BuilderWithToBuilder.java
@@ -86,7 +86,8 @@ class BuilderWithToBuilder<T> {
 	}
 	@java.lang.SuppressWarnings("all")
 	public BuilderWithToBuilder.BuilderWithToBuilderBuilder<T> toBuilder() {
-		final BuilderWithToBuilder.BuilderWithToBuilderBuilder<T> builder = new BuilderWithToBuilder.BuilderWithToBuilderBuilder<T>().one(this.mOne).two(this.mTwo).foo(BuilderWithToBuilder.<T>rrr(this));
+		final T foo = BuilderWithToBuilder.<T>rrr(this);
+		final BuilderWithToBuilder.BuilderWithToBuilderBuilder<T> builder = new BuilderWithToBuilder.BuilderWithToBuilderBuilder<T>().one(this.mOne).two(this.mTwo).foo(foo);
 		if (this.bars != null) builder.bars(this.bars);
 		return builder;
 	}
diff --git a/test/transform/resource/after-delombok/I2335_BuilderMultipleObtainVia.java b/test/transform/resource/after-delombok/I2335_BuilderMultipleObtainVia.java
new file mode 100644
index 00000000..f9cd424d
--- /dev/null
+++ b/test/transform/resource/after-delombok/I2335_BuilderMultipleObtainVia.java
@@ -0,0 +1,59 @@
+public class I2335_BuilderMultipleObtainVia {
+	private String theString;
+	private Long theLong;
+	public I2335_BuilderMultipleObtainVia(String theString, Long theLong) {
+		setTheString(theString);
+		setTheLong(theLong);
+	}
+	public String getTheString() {
+		return theString;
+	}
+	public Long getTheLong() {
+		return theLong;
+	}
+	public void setTheString(String theString) {
+		this.theString = theString;
+	}
+	public void setTheLong(Long theLong) {
+		this.theLong = theLong;
+	}
+	@java.lang.SuppressWarnings("all")
+	public static class I2335_BuilderMultipleObtainViaBuilder {
+		@java.lang.SuppressWarnings("all")
+		private String theString;
+		@java.lang.SuppressWarnings("all")
+		private Long theLong;
+		@java.lang.SuppressWarnings("all")
+		I2335_BuilderMultipleObtainViaBuilder() {
+		}
+		@java.lang.SuppressWarnings("all")
+		public I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder theString(final String theString) {
+			this.theString = theString;
+			return this;
+		}
+		@java.lang.SuppressWarnings("all")
+		public I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder theLong(final Long theLong) {
+			this.theLong = theLong;
+			return this;
+		}
+		@java.lang.SuppressWarnings("all")
+		public I2335_BuilderMultipleObtainVia build() {
+			return new I2335_BuilderMultipleObtainVia(this.theString, this.theLong);
+		}
+		@java.lang.Override
+		@java.lang.SuppressWarnings("all")
+		public java.lang.String toString() {
+			return "I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder(theString=" + this.theString + ", theLong=" + this.theLong + ")";
+		}
+	}
+	@java.lang.SuppressWarnings("all")
+	public static I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder builder() {
+		return new I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder();
+	}
+	@java.lang.SuppressWarnings("all")
+	public I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder toBuilder() {
+		final String theString = this.getTheString();
+		final Long theLong = this.getTheLong();
+		return new I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder().theString(theString).theLong(theLong);
+	}
+}
diff --git a/test/transform/resource/after-ecj/BuilderWithToBuilder.java b/test/transform/resource/after-ecj/BuilderWithToBuilder.java
index 131b0ca0..6d935736 100644
--- a/test/transform/resource/after-ecj/BuilderWithToBuilder.java
+++ b/test/transform/resource/after-ecj/BuilderWithToBuilder.java
@@ -74,7 +74,8 @@ import lombok.Builder;
     return new BuilderWithToBuilder.BuilderWithToBuilderBuilder<T>();
   }
   public @java.lang.SuppressWarnings("all") BuilderWithToBuilder.BuilderWithToBuilderBuilder<T> toBuilder() {
-    final BuilderWithToBuilder.BuilderWithToBuilderBuilder<T> builder = new BuilderWithToBuilder.BuilderWithToBuilderBuilder<T>().one(this.mOne).two(this.mTwo).foo(BuilderWithToBuilder.<T>rrr(this));
+    final T foo = BuilderWithToBuilder.<T>rrr(this);
+    final BuilderWithToBuilder.BuilderWithToBuilderBuilder<T> builder = new BuilderWithToBuilder.BuilderWithToBuilderBuilder<T>().one(this.mOne).two(this.mTwo).foo(foo);
     if ((this.bars != null))
         builder.bars(this.bars);
     return builder;
diff --git a/test/transform/resource/after-ecj/I2335_BuilderMultipleObtainVia.java b/test/transform/resource/after-ecj/I2335_BuilderMultipleObtainVia.java
new file mode 100644
index 00000000..7d8b8953
--- /dev/null
+++ b/test/transform/resource/after-ecj/I2335_BuilderMultipleObtainVia.java
@@ -0,0 +1,51 @@
+import lombok.Builder;
+public @Builder class I2335_BuilderMultipleObtainVia {
+  public static @java.lang.SuppressWarnings("all") class I2335_BuilderMultipleObtainViaBuilder {
+    private @java.lang.SuppressWarnings("all") String theString;
+    private @java.lang.SuppressWarnings("all") Long theLong;
+    @java.lang.SuppressWarnings("all") I2335_BuilderMultipleObtainViaBuilder() {
+      super();
+    }
+    public @java.lang.SuppressWarnings("all") I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder theString(final String theString) {
+      this.theString = theString;
+      return this;
+    }
+    public @java.lang.SuppressWarnings("all") I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder theLong(final Long theLong) {
+      this.theLong = theLong;
+      return this;
+    }
+    public @java.lang.SuppressWarnings("all") I2335_BuilderMultipleObtainVia build() {
+      return new I2335_BuilderMultipleObtainVia(this.theString, this.theLong);
+    }
+    public @java.lang.Override @java.lang.SuppressWarnings("all") java.lang.String toString() {
+      return (((("I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder(theString=" + this.theString) + ", theLong=") + this.theLong) + ")");
+    }
+  }
+  private String theString;
+  private Long theLong;
+  public @Builder(toBuilder = true) I2335_BuilderMultipleObtainVia(@Builder.ObtainVia(method = "getTheString") String theString, @Builder.ObtainVia(method = "getTheLong") Long theLong) {
+    super();
+    setTheString(theString);
+    setTheLong(theLong);
+  }
+  public String getTheString() {
+    return theString;
+  }
+  public Long getTheLong() {
+    return theLong;
+  }
+  public void setTheString(String theString) {
+    this.theString = theString;
+  }
+  public void setTheLong(Long theLong) {
+    this.theLong = theLong;
+  }
+  public static @java.lang.SuppressWarnings("all") I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder builder() {
+    return new I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder();
+  }
+  public @java.lang.SuppressWarnings("all") I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder toBuilder() {
+    String theString = this.getTheString();
+    Long theLong = this.getTheLong();
+    return new I2335_BuilderMultipleObtainVia.I2335_BuilderMultipleObtainViaBuilder().theString(theString).theLong(theLong);
+  }
+}
diff --git a/test/transform/resource/before/I2335_BuilderMultipleObtainVia.java b/test/transform/resource/before/I2335_BuilderMultipleObtainVia.java
new file mode 100644
index 00000000..ef3723c0
--- /dev/null
+++ b/test/transform/resource/before/I2335_BuilderMultipleObtainVia.java
@@ -0,0 +1,32 @@
+import lombok.Builder;
+
+@Builder
+public class I2335_BuilderMultipleObtainVia {
+	private String theString;
+	private Long theLong;
+
+	@Builder(toBuilder = true)
+	public I2335_BuilderMultipleObtainVia(
+		@Builder.ObtainVia(method = "getTheString") String theString,
+		@Builder.ObtainVia(method = "getTheLong") Long theLong
+	) {
+		setTheString(theString);
+		setTheLong(theLong);
+	}
+	
+	public String getTheString() {
+		return theString;
+	}
+	
+	public Long getTheLong() {
+		return theLong;
+	}
+	
+	public void setTheString(String theString) {
+		this.theString = theString;
+	}
+	
+	public void setTheLong(Long theLong) {
+		this.theLong = theLong;
+	}
+}