Fix for issues when mixing @NonNull on params with @SneakyThrows or @Synchronized [Issue #588]

7 files changed, 81 insertions, 27 deletions

diff --git a/src/core/lombok/core/Version.java b/src/core/lombok/core/Version.java
index 24faf821..318075d3 100644
--- a/src/core/lombok/core/Version.java
+++ b/src/core/lombok/core/Version.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2009-2011 The Project Lombok Authors.
+ * Copyright (C) 2009-2013 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
diff --git a/src/core/lombok/eclipse/handlers/NonNullHandler.java b/src/core/lombok/eclipse/handlers/HandleNonNull.java
index 59fda801..634cb2d9 100644
--- a/src/core/lombok/eclipse/handlers/NonNullHandler.java
+++ b/src/core/lombok/eclipse/handlers/HandleNonNull.java
@@ -21,8 +21,19 @@
  */
 package lombok.eclipse.handlers;
 
+import static lombok.eclipse.Eclipse.isPrimitive;
+import static lombok.eclipse.handlers.EclipseHandlerUtil.*;
+
 import java.util.Arrays;
 
+import lombok.NonNull;
+import lombok.core.AST.Kind;
+import lombok.core.AnnotationValues;
+import lombok.core.HandlerPriority;
+import lombok.eclipse.DeferUntilPostDiet;
+import lombok.eclipse.EclipseAnnotationHandler;
+import lombok.eclipse.EclipseNode;
+
 import org.eclipse.jdt.internal.compiler.ast.ASTNode;
 import org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration;
 import org.eclipse.jdt.internal.compiler.ast.AbstractVariableDeclaration;
@@ -36,22 +47,15 @@ import org.eclipse.jdt.internal.compiler.ast.NullLiteral;
 import org.eclipse.jdt.internal.compiler.ast.OperatorIds;
 import org.eclipse.jdt.internal.compiler.ast.SingleNameReference;
 import org.eclipse.jdt.internal.compiler.ast.Statement;
+import org.eclipse.jdt.internal.compiler.ast.SynchronizedStatement;
 import org.eclipse.jdt.internal.compiler.ast.ThrowStatement;
+import org.eclipse.jdt.internal.compiler.ast.TryStatement;
 import org.mangosdk.spi.ProviderFor;
 
-import lombok.NonNull;
-import lombok.core.AST.Kind;
-import lombok.core.AnnotationValues;
-import lombok.eclipse.DeferUntilPostDiet;
-import lombok.eclipse.EclipseAnnotationHandler;
-import lombok.eclipse.EclipseNode;
-
-import static lombok.eclipse.Eclipse.*;
-import static lombok.eclipse.handlers.EclipseHandlerUtil.*;
-
 @DeferUntilPostDiet
 @ProviderFor(EclipseAnnotationHandler.class)
-public class NonNullHandler extends EclipseAnnotationHandler<NonNull> {
+@HandlerPriority(value = 512) // 2^9; onParameter=@__(@NonNull) has to run first.
+public class HandleNonNull extends EclipseAnnotationHandler<NonNull> {
 	@Override public void handle(AnnotationValues<NonNull> annotation, Annotation ast, EclipseNode annotationNode) {
 		if (annotationNode.up().getKind() == Kind.FIELD) {
 			// This is meaningless unless the field is used to generate a method (@Setter, @RequiredArgsConstructor, etc),
@@ -86,7 +90,7 @@ public class NonNullHandler extends EclipseAnnotationHandler<NonNull> {
 			annotationNode.addWarning("@NonNull is meaningless on a parameter of an abstract method.");
 			return;
 		}
-
+		
 		// Possibly, if 'declaration instanceof ConstructorDeclaration', fetch declaration.constructorCall, search it for any references to our parameter,
 		// and if they exist, create a new method in the class: 'private static <T> T lombok$nullCheck(T expr, String msg) {if (expr == null) throw NPE; return expr;}' and
 		// wrap all references to it in the super/this to a call to this method.
@@ -103,16 +107,31 @@ public class NonNullHandler extends EclipseAnnotationHandler<NonNull> {
 			declaration.statements = new Statement[] {nullCheck};
 		} else {
 			char[] expectedName = arg.name;
-			for (Statement stat : declaration.statements) {
-				char[] varNameOfNullCheck = returnVarNameIfNullCheck(stat);
-				if (varNameOfNullCheck == null) break;
-				if (Arrays.equals(expectedName, varNameOfNullCheck)) return;
+			/* Abort if the null check is already there, delving into try and synchronized statements */ {
+				Statement[] stats = declaration.statements;
+				int idx = 0;
+				while (stats != null && stats.length > idx) {
+					Statement stat = stats[idx++];
+					if (stat instanceof TryStatement) {
+						stats = ((TryStatement) stat).tryBlock.statements;
+						idx = 0;
+						continue;
+					}
+					if (stat instanceof SynchronizedStatement) {
+						stats = ((SynchronizedStatement) stat).block.statements;
+						idx = 0;
+						continue;
+					}
+					char[] varNameOfNullCheck = returnVarNameIfNullCheck(stat);
+					if (varNameOfNullCheck == null) break;
+					if (Arrays.equals(varNameOfNullCheck, expectedName)) return;
+				}
 			}
 			
 			Statement[] newStatements = new Statement[declaration.statements.length + 1];
 			int skipOver = 0;
 			for (Statement stat : declaration.statements) {
-				if (isGenerated(stat)) skipOver++;
+				if (isGenerated(stat) && isNullCheck(stat)) skipOver++;
 				else break;
 			}
 			System.arraycopy(declaration.statements, 0, newStatements, 0, skipOver);
@@ -123,6 +142,10 @@ public class NonNullHandler extends EclipseAnnotationHandler<NonNull> {
 		annotationNode.up().up().rebuild();
 	}
 	
+	private boolean isNullCheck(Statement stat) {
+		return returnVarNameIfNullCheck(stat) != null;
+	}
+	
 	private char[] returnVarNameIfNullCheck(Statement stat) {
 		if (!(stat instanceof IfStatement)) return null;
 		
diff --git a/src/core/lombok/eclipse/handlers/HandleSneakyThrows.java b/src/core/lombok/eclipse/handlers/HandleSneakyThrows.java
index aa78ca3b..d3a95db8 100644
--- a/src/core/lombok/eclipse/handlers/HandleSneakyThrows.java
+++ b/src/core/lombok/eclipse/handlers/HandleSneakyThrows.java
@@ -30,6 +30,7 @@ import java.util.List;
 
 import lombok.SneakyThrows;
 import lombok.core.AnnotationValues;
+import lombok.core.HandlerPriority;
 import lombok.eclipse.DeferUntilPostDiet;
 import lombok.eclipse.EclipseAnnotationHandler;
 import lombok.eclipse.EclipseNode;
@@ -60,6 +61,7 @@ import org.mangosdk.spi.ProviderFor;
  */
 @ProviderFor(EclipseAnnotationHandler.class)
 @DeferUntilPostDiet
+@HandlerPriority(value = 1024) // 2^10; @NonNull must have run first, so that we wrap around the statements generated by it.
 public class HandleSneakyThrows extends EclipseAnnotationHandler<SneakyThrows> {
 	
 	private static class DeclaredException {
diff --git a/src/core/lombok/eclipse/handlers/HandleSynchronized.java b/src/core/lombok/eclipse/handlers/HandleSynchronized.java
index e4c58eab..f76f06ed 100644
--- a/src/core/lombok/eclipse/handlers/HandleSynchronized.java
+++ b/src/core/lombok/eclipse/handlers/HandleSynchronized.java
@@ -27,6 +27,7 @@ import java.lang.reflect.Modifier;
 
 import lombok.Synchronized;
 import lombok.core.AnnotationValues;
+import lombok.core.HandlerPriority;
 import lombok.core.AST.Kind;
 import lombok.eclipse.DeferUntilPostDiet;
 import lombok.eclipse.EclipseAnnotationHandler;
@@ -52,6 +53,7 @@ import org.mangosdk.spi.ProviderFor;
  */
 @ProviderFor(EclipseAnnotationHandler.class)
 @DeferUntilPostDiet
+@HandlerPriority(value = 1024) // 2^10; @NonNull must have run first, so that we wrap around the statements generated by it.
 public class HandleSynchronized extends EclipseAnnotationHandler<Synchronized> {
 	private static final char[] INSTANCE_LOCK_NAME = "$lock".toCharArray();
 	private static final char[] STATIC_LOCK_NAME = "$LOCK".toCharArray();
diff --git a/src/core/lombok/javac/handlers/NonNullHandler.java b/src/core/lombok/javac/handlers/HandleNonNull.java
index acf1588e..21611a39 100644
--- a/src/core/lombok/javac/handlers/NonNullHandler.java
+++ b/src/core/lombok/javac/handlers/HandleNonNull.java
@@ -36,21 +36,24 @@ import com.sun.tools.javac.tree.JCTree.JCLiteral;
 import com.sun.tools.javac.tree.JCTree.JCMethodDecl;
 import com.sun.tools.javac.tree.JCTree.JCParens;
 import com.sun.tools.javac.tree.JCTree.JCStatement;
+import com.sun.tools.javac.tree.JCTree.JCSynchronized;
 import com.sun.tools.javac.tree.JCTree.JCThrow;
+import com.sun.tools.javac.tree.JCTree.JCTry;
 import com.sun.tools.javac.tree.JCTree.JCVariableDecl;
 import com.sun.tools.javac.util.List;
 
 import lombok.NonNull;
 import lombok.core.AnnotationValues;
+import lombok.core.HandlerPriority;
 import lombok.core.AST.Kind;
 import lombok.javac.JavacAnnotationHandler;
 import lombok.javac.JavacNode;
-
 import static lombok.javac.JavacTreeMaker.TypeTag.*;
 import static lombok.javac.JavacTreeMaker.TreeTag.*;
 
 @ProviderFor(JavacAnnotationHandler.class)
-public class NonNullHandler extends JavacAnnotationHandler<NonNull> {
+@HandlerPriority(value = 512) // 2^9; onParameter=@__(@NonNull) has to run first.
+public class HandleNonNull extends JavacAnnotationHandler<NonNull> {
 	@Override public void handle(AnnotationValues<NonNull> annotation, JCAnnotation ast, JavacNode annotationNode) {
 		if (annotationNode.up().getKind() == Kind.FIELD) {
 			// This is meaningless unless the field is used to generate a method (@Setter, @RequiredArgsConstructor, etc),
@@ -77,7 +80,7 @@ public class NonNullHandler extends JavacAnnotationHandler<NonNull> {
 			return;
 		}
 		
-		if (JavacHandlerUtil.isGenerated(declaration)) return;
+//		if (JavacHandlerUtil.isGenerated(declaration)) return;
 		
 		if (declaration.body == null) {
 			annotationNode.addWarning("@NonNull is meaningless on a parameter of an abstract method.");
@@ -99,17 +102,33 @@ public class NonNullHandler extends JavacAnnotationHandler<NonNull> {
 		List<JCStatement> statements = declaration.body.stats;
 		
 		String expectedName = annotationNode.up().getName();
-		for (JCStatement stat : statements) {
-			if (JavacHandlerUtil.isConstructorCall(stat)) continue;
-			String varNameOfNullCheck = returnVarNameIfNullCheck(stat);
-			if (varNameOfNullCheck == null) break;
-			if (varNameOfNullCheck.equals(expectedName)) return;
+		
+		/* Abort if the null check is already there, delving into try and synchronized statements */ {
+			List<JCStatement> stats = statements;
+			int idx = 0;
+			while (stats.size() > idx) {
+				JCStatement stat = stats.get(idx++);
+				if (JavacHandlerUtil.isConstructorCall(stat)) continue;
+				if (stat instanceof JCTry) {
+					stats = ((JCTry) stat).body.stats;
+					idx = 0;
+					continue;
+				}
+				if (stat instanceof JCSynchronized) {
+					stats = ((JCSynchronized) stat).body.stats;
+					idx = 0;
+					continue;
+				}
+				String varNameOfNullCheck = returnVarNameIfNullCheck(stat);
+				if (varNameOfNullCheck == null) break;
+				if (varNameOfNullCheck.equals(expectedName)) return;
+			}
 		}
 		
 		List<JCStatement> tail = statements;
 		List<JCStatement> head = List.nil();
 		for (JCStatement stat : statements) {
-			if (JavacHandlerUtil.isConstructorCall(stat) || JavacHandlerUtil.isGenerated(stat)) {
+			if (JavacHandlerUtil.isConstructorCall(stat) || (JavacHandlerUtil.isGenerated(stat) && isNullCheck(stat))) {
 				tail = tail.tail;
 				head = head.prepend(stat);
 				continue;
@@ -122,6 +141,10 @@ public class NonNullHandler extends JavacAnnotationHandler<NonNull> {
 		declaration.body.stats = newList;
 	}
 	
+	private boolean isNullCheck(JCStatement stat) {
+		return returnVarNameIfNullCheck(stat) != null;
+	}
+	
 	/**
 	 * Checks if the statement is of the form 'if (x == null) {throw WHATEVER;},
 	 * where the block braces are optional. If it is of this form, returns "x".
diff --git a/src/core/lombok/javac/handlers/HandleSneakyThrows.java b/src/core/lombok/javac/handlers/HandleSneakyThrows.java
index 69d2b45d..b41277c3 100644
--- a/src/core/lombok/javac/handlers/HandleSneakyThrows.java
+++ b/src/core/lombok/javac/handlers/HandleSneakyThrows.java
@@ -29,6 +29,7 @@ import java.util.Collections;
 
 import lombok.SneakyThrows;
 import lombok.core.AnnotationValues;
+import lombok.core.HandlerPriority;
 import lombok.javac.JavacAnnotationHandler;
 import lombok.javac.JavacNode;
 import lombok.javac.JavacTreeMaker;
@@ -49,6 +50,7 @@ import com.sun.tools.javac.util.List;
  * Handles the {@code lombok.SneakyThrows} annotation for javac.
  */
 @ProviderFor(JavacAnnotationHandler.class)
+@HandlerPriority(value = 1024) // 2^10; @NonNull must have run first, so that we wrap around the statements generated by it.
 public class HandleSneakyThrows extends JavacAnnotationHandler<SneakyThrows> {
 	@Override public void handle(AnnotationValues<SneakyThrows> annotation, JCAnnotation ast, JavacNode annotationNode) {
 		deleteAnnotationIfNeccessary(annotationNode, SneakyThrows.class);
diff --git a/src/core/lombok/javac/handlers/HandleSynchronized.java b/src/core/lombok/javac/handlers/HandleSynchronized.java
index b173f8fb..661a7c2a 100644
--- a/src/core/lombok/javac/handlers/HandleSynchronized.java
+++ b/src/core/lombok/javac/handlers/HandleSynchronized.java
@@ -26,6 +26,7 @@ import static lombok.javac.handlers.JavacHandlerUtil.*;
 import lombok.Synchronized;
 import lombok.core.AST.Kind;
 import lombok.core.AnnotationValues;
+import lombok.core.HandlerPriority;
 import lombok.javac.JavacAnnotationHandler;
 import lombok.javac.JavacNode;
 import lombok.javac.JavacTreeMaker;
@@ -46,6 +47,7 @@ import com.sun.tools.javac.util.List;
  * Handles the {@code lombok.Synchronized} annotation for javac.
  */
 @ProviderFor(JavacAnnotationHandler.class)
+@HandlerPriority(value = 1024) // 2^10; @NonNull must have run first, so that we wrap around the statements generated by it.
 public class HandleSynchronized extends JavacAnnotationHandler<Synchronized> {
 	private static final String INSTANCE_LOCK_NAME = "$lock";
 	private static final String STATIC_LOCK_NAME = "$LOCK";