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

12 files changed, 125 insertions, 31 deletions

diff --git a/doc/changelog.markdown b/doc/changelog.markdown
index 7782f1fa..1ee3504c 100644
--- a/doc/changelog.markdown
+++ b/doc/changelog.markdown
@@ -1,8 +1,13 @@
 Lombok Changelog
 ----------------
 
-### v0.12.1 "Edgy Guinea Pig"
-* PLATFORM: Initial (not quite totally finished) JDK8 support, without affecting existing support for JDK6 and 7. [Issue #451](https://code.google.com/p/projectlombok/issues/detail?id=451).
+### v1.12.2 (October 10th, 2013)
+* PLATFORM: Initial JDK8 support, without affecting existing support for JDK6 and 7. [Issue #451](https://code.google.com/p/projectlombok/issues/detail?id=451). While lombok will now work on JDK8 / javac8, and netbeans 7.4 and up, lombok does not (yet) support new language features introduced with java8, such as lambda expressions. Support for these features will be added in a future version.
+* PLATFORM: Running javac on IBM J9 VM would cause NullPointerExceptions when compiling with lombok. These issues should be fixed. [Issue #554](https://code.google.com/p/projectlombok/issues/detail?id=554).
+* CHANGE: [JDK8-related] The canonical way to write onMethod / onParameter / onConstructor annotation now uses a double underscore instead of a single underscore, so, now, the proper way to use this feature is `@RequiredArgsConstructor(onConstructor=@__(@Inject))`. The old way (single underscore) still works, but generates warnings on javac 8.
+* BUGFIX: Using `@NonNull` on an abstract method used to cause exceptions during compilation. [Issue #559](https://code.google.com/p/projectlombok/issues/detail?id=559).
+* BUGFIX: Using `@NonNull` on methods that also have `@SneakyThrows` or `@Synchronized` caused arbitrary behaviour. [Issue #588](https://code.google.com/p/projectlombok/issues/detail?id=588).
+* GERMANY: Major version bumped from 0 to 1, because allegedly this is important. Rest assured, this change is nevertheless backwards compatible.
 
 ### v0.12.0 "Angry Butterfly" (July 16th, 2013)
 * FEATURE: javadoc on fields will now be copied to generated getters / setters / withers. There are ways to specify separate javadoc for the field, the setter, and the getter, and `@param` and `@return` are handled appropriately. Addresses feature request [Issue #59](https://code.google.com/p/projectlombok/issues/detail?id=59). [@Getter and @Setter documentation](http://projectlombok.org/features/GetterSetter.html). [@Wither documentation](http://projectlombok.org/features/experimental/Wither.html).
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";
diff --git a/test/transform/resource/after-delombok/NonNullWithSneakyThrows.java b/test/transform/resource/after-delombok/NonNullWithSneakyThrows.java
new file mode 100644
index 00000000..91646468
--- /dev/null
+++ b/test/transform/resource/after-delombok/NonNullWithSneakyThrows.java
@@ -0,0 +1,12 @@
+class NonNullWithSneakyThrows {
+	void test(@lombok.NonNull String in) {
+		try {
+			if (in == null) {
+				throw new java.lang.NullPointerException("in");
+			}
+			System.out.println(in);
+		} catch (final java.lang.Throwable $ex) {
+			throw lombok.Lombok.sneakyThrow($ex);
+		}
+	}
+}
\ No newline at end of file
diff --git a/test/transform/resource/after-ecj/InjectField.java b/test/transform/resource/after-ecj/InjectField.java
index 83d9e5fa..f9ea9142 100644
--- a/test/transform/resource/after-ecj/InjectField.java
+++ b/test/transform/resource/after-ecj/InjectField.java
@@ -4,9 +4,9 @@ import lombok.Synchronized;
 @Log enum InjectField1 {
   A(),
   B(),
+  private static final java.util.logging.Logger log = java.util.logging.Logger.getLogger(InjectField1.class.getName());
   private final java.lang.Object $lock = new java.lang.Object[0];
   private static final java.lang.Object $LOCK = new java.lang.Object[0];
-  private static final java.util.logging.Logger log = java.util.logging.Logger.getLogger(InjectField1.class.getName());
   private static final String LOG_MESSAGE = "static initializer";
   private String fieldA;
   static {
@@ -32,8 +32,8 @@ import lombok.Synchronized;
   }
 }
 @Log class InjectField2 {
-  private final java.lang.Object $lock = new java.lang.Object[0];
   private static final java.util.logging.Logger log = java.util.logging.Logger.getLogger(InjectField2.class.getName());
+  private final java.lang.Object $lock = new java.lang.Object[0];
   private static final String LOG_MESSAGE = "static initializer";
   static {
     log.log(Level.FINE, LOG_MESSAGE);
diff --git a/test/transform/resource/after-ecj/NonNullWithSneakyThrows.java b/test/transform/resource/after-ecj/NonNullWithSneakyThrows.java
new file mode 100644
index 00000000..fac8dcdd
--- /dev/null
+++ b/test/transform/resource/after-ecj/NonNullWithSneakyThrows.java
@@ -0,0 +1,18 @@
+class NonNullWithSneakyThrows {
+  NonNullWithSneakyThrows() {
+    super();
+  }
+  @lombok.SneakyThrows void test(@lombok.NonNull String in) {
+    try 
+      {
+        if ((in == null))
+            {
+              throw new java.lang.NullPointerException("in");
+            }
+        System.out.println(in);
+      }
+    catch (final java.lang.Throwable $ex)       {
+        throw lombok.Lombok.sneakyThrow($ex);
+      }
+  }
+}
\ No newline at end of file
diff --git a/test/transform/resource/before/NonNullWithSneakyThrows.java b/test/transform/resource/before/NonNullWithSneakyThrows.java
new file mode 100644
index 00000000..35f78a7f
--- /dev/null
+++ b/test/transform/resource/before/NonNullWithSneakyThrows.java
@@ -0,0 +1,5 @@
+class NonNullWithSneakyThrows {
+	@lombok.SneakyThrows void test(@lombok.NonNull String in) {
+		System.out.println(in);
+	}
+}