From a9b4fb0c685fbc52079d57532c04277e78c95ec2 Mon Sep 17 00:00:00 2001 From: Reinier Zwitserloot Date: Thu, 10 Oct 2013 22:36:16 +0200 Subject: Fix for issues when mixing @NonNull on params with @SneakyThrows or @Synchronized [Issue #588] --- .../lombok/eclipse/handlers/HandleNonNull.java | 175 +++++++++++++++++++++ .../eclipse/handlers/HandleSneakyThrows.java | 2 + .../eclipse/handlers/HandleSynchronized.java | 2 + .../lombok/eclipse/handlers/NonNullHandler.java | 152 ------------------ 4 files changed, 179 insertions(+), 152 deletions(-) create mode 100644 src/core/lombok/eclipse/handlers/HandleNonNull.java delete mode 100644 src/core/lombok/eclipse/handlers/NonNullHandler.java (limited to 'src/core/lombok/eclipse/handlers') diff --git a/src/core/lombok/eclipse/handlers/HandleNonNull.java b/src/core/lombok/eclipse/handlers/HandleNonNull.java new file mode 100644 index 00000000..634cb2d9 --- /dev/null +++ b/src/core/lombok/eclipse/handlers/HandleNonNull.java @@ -0,0 +1,175 @@ +/* + * Copyright (C) 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 + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +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; +import org.eclipse.jdt.internal.compiler.ast.Annotation; +import org.eclipse.jdt.internal.compiler.ast.Argument; +import org.eclipse.jdt.internal.compiler.ast.Block; +import org.eclipse.jdt.internal.compiler.ast.EqualExpression; +import org.eclipse.jdt.internal.compiler.ast.Expression; +import org.eclipse.jdt.internal.compiler.ast.IfStatement; +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; + +@DeferUntilPostDiet +@ProviderFor(EclipseAnnotationHandler.class) +@HandlerPriority(value = 512) // 2^9; onParameter=@__(@NonNull) has to run first. +public class HandleNonNull extends EclipseAnnotationHandler { + @Override public void handle(AnnotationValues 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), + // but in that case those handlers will take care of it. However, we DO check if the annotation is applied to + // a primitive, because those handlers trigger on any annotation named @NonNull and we only want the warning + // behaviour on _OUR_ 'lombok.NonNull'. + + try { + if (isPrimitive(((AbstractVariableDeclaration) annotationNode.up().get()).type)) { + annotationNode.addWarning("@NonNull is meaningless on a primitive."); + } + } catch (Exception ignore) {} + + return; + } + + if (annotationNode.up().getKind() != Kind.ARGUMENT) return; + + Argument arg; + AbstractMethodDeclaration declaration; + + try { + arg = (Argument) annotationNode.up().get(); + declaration = (AbstractMethodDeclaration) annotationNode.up().up().get(); + } catch (Exception e) { + return; + } + + if (isGenerated(declaration)) return; + + if (declaration.isAbstract()) { + 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 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. + + Statement nullCheck = generateNullCheck(arg, ast); + + if (nullCheck == null) { + // @NonNull applied to a primitive. Kinda pointless. Let's generate a warning. + annotationNode.addWarning("@NonNull is meaningless on a primitive."); + return; + } + + if (declaration.statements == null) { + declaration.statements = new Statement[] {nullCheck}; + } else { + char[] expectedName = arg.name; + /* 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) && isNullCheck(stat)) skipOver++; + else break; + } + System.arraycopy(declaration.statements, 0, newStatements, 0, skipOver); + System.arraycopy(declaration.statements, skipOver, newStatements, skipOver + 1, declaration.statements.length - skipOver); + newStatements[skipOver] = nullCheck; + declaration.statements = newStatements; + } + annotationNode.up().up().rebuild(); + } + + private boolean isNullCheck(Statement stat) { + return returnVarNameIfNullCheck(stat) != null; + } + + private char[] returnVarNameIfNullCheck(Statement stat) { + if (!(stat instanceof IfStatement)) return null; + + /* Check that the if's statement is a throw statement, possibly in a block. */ { + Statement then = ((IfStatement) stat).thenStatement; + if (then instanceof Block) { + Statement[] blockStatements = ((Block) then).statements; + if (blockStatements == null || blockStatements.length == 0) return null; + then = blockStatements[0]; + } + + if (!(then instanceof ThrowStatement)) return null; + } + + /* Check that the if's conditional is like 'x == null'. Return from this method (don't generate + a nullcheck) if 'x' is equal to our own variable's name: There's already a nullcheck here. */ { + Expression cond = ((IfStatement) stat).condition; + if (!(cond instanceof EqualExpression)) return null; + EqualExpression bin = (EqualExpression) cond; + int operatorId = ((bin.bits & ASTNode.OperatorMASK) >> ASTNode.OperatorSHIFT); + if (operatorId != OperatorIds.EQUAL_EQUAL) return null; + if (!(bin.left instanceof SingleNameReference)) return null; + if (!(bin.right instanceof NullLiteral)) return null; + return ((SingleNameReference) bin.left).token; + } + } +} 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 { 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 { private static final char[] INSTANCE_LOCK_NAME = "$lock".toCharArray(); private static final char[] STATIC_LOCK_NAME = "$LOCK".toCharArray(); diff --git a/src/core/lombok/eclipse/handlers/NonNullHandler.java b/src/core/lombok/eclipse/handlers/NonNullHandler.java deleted file mode 100644 index 59fda801..00000000 --- a/src/core/lombok/eclipse/handlers/NonNullHandler.java +++ /dev/null @@ -1,152 +0,0 @@ -/* - * Copyright (C) 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 - * in the Software without restriction, including without limitation the rights - * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in - * all copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN - * THE SOFTWARE. - */ -package lombok.eclipse.handlers; - -import java.util.Arrays; - -import org.eclipse.jdt.internal.compiler.ast.ASTNode; -import org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration; -import org.eclipse.jdt.internal.compiler.ast.AbstractVariableDeclaration; -import org.eclipse.jdt.internal.compiler.ast.Annotation; -import org.eclipse.jdt.internal.compiler.ast.Argument; -import org.eclipse.jdt.internal.compiler.ast.Block; -import org.eclipse.jdt.internal.compiler.ast.EqualExpression; -import org.eclipse.jdt.internal.compiler.ast.Expression; -import org.eclipse.jdt.internal.compiler.ast.IfStatement; -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.ThrowStatement; -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 { - @Override public void handle(AnnotationValues 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), - // but in that case those handlers will take care of it. However, we DO check if the annotation is applied to - // a primitive, because those handlers trigger on any annotation named @NonNull and we only want the warning - // behaviour on _OUR_ 'lombok.NonNull'. - - try { - if (isPrimitive(((AbstractVariableDeclaration) annotationNode.up().get()).type)) { - annotationNode.addWarning("@NonNull is meaningless on a primitive."); - } - } catch (Exception ignore) {} - - return; - } - - if (annotationNode.up().getKind() != Kind.ARGUMENT) return; - - Argument arg; - AbstractMethodDeclaration declaration; - - try { - arg = (Argument) annotationNode.up().get(); - declaration = (AbstractMethodDeclaration) annotationNode.up().up().get(); - } catch (Exception e) { - return; - } - - if (isGenerated(declaration)) return; - - if (declaration.isAbstract()) { - 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 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. - - Statement nullCheck = generateNullCheck(arg, ast); - - if (nullCheck == null) { - // @NonNull applied to a primitive. Kinda pointless. Let's generate a warning. - annotationNode.addWarning("@NonNull is meaningless on a primitive."); - return; - } - - if (declaration.statements == null) { - 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; - } - - Statement[] newStatements = new Statement[declaration.statements.length + 1]; - int skipOver = 0; - for (Statement stat : declaration.statements) { - if (isGenerated(stat)) skipOver++; - else break; - } - System.arraycopy(declaration.statements, 0, newStatements, 0, skipOver); - System.arraycopy(declaration.statements, skipOver, newStatements, skipOver + 1, declaration.statements.length - skipOver); - newStatements[skipOver] = nullCheck; - declaration.statements = newStatements; - } - annotationNode.up().up().rebuild(); - } - - private char[] returnVarNameIfNullCheck(Statement stat) { - if (!(stat instanceof IfStatement)) return null; - - /* Check that the if's statement is a throw statement, possibly in a block. */ { - Statement then = ((IfStatement) stat).thenStatement; - if (then instanceof Block) { - Statement[] blockStatements = ((Block) then).statements; - if (blockStatements == null || blockStatements.length == 0) return null; - then = blockStatements[0]; - } - - if (!(then instanceof ThrowStatement)) return null; - } - - /* Check that the if's conditional is like 'x == null'. Return from this method (don't generate - a nullcheck) if 'x' is equal to our own variable's name: There's already a nullcheck here. */ { - Expression cond = ((IfStatement) stat).condition; - if (!(cond instanceof EqualExpression)) return null; - EqualExpression bin = (EqualExpression) cond; - int operatorId = ((bin.bits & ASTNode.OperatorMASK) >> ASTNode.OperatorSHIFT); - if (operatorId != OperatorIds.EQUAL_EQUAL) return null; - if (!(bin.left instanceof SingleNameReference)) return null; - if (!(bin.right instanceof NullLiteral)) return null; - return ((SingleNameReference) bin.left).token; - } - } -} -- cgit