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

3 files changed, 44 insertions, 17 deletions

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();