From 195565089f1271c945d5a560e0e8d285ecd8d6a0 Mon Sep 17 00:00:00 2001 From: Eugene Zhuravlev Date: Tue, 29 Jan 2019 14:29:46 +0100 Subject: [PATCH] review follow-up: NotNull instrumentation minor optimizations and tests --- .../AuxiliaryMethodGenerator.java | 7 +- .../NotNullVerifyingInstrumenter.java | 65 ++++++++++++------- .../NoCheckForConstant.java | 6 ++ .../NoCheckForFinalNotNullMethodCall.java | 9 +++ .../NoCheckForNewArray.java | 6 ++ .../NoCheckForNewConstructorCall.java | 10 +++ .../NoCheckForNewMultiArray.java | 6 ++ .../NoCheckForNewObject.java | 6 ++ .../NoCheckForPrivateNotNullMethodCall.java | 9 +++ .../NoCheckForStaticNotNullMethodCall.java | 9 +++ .../NotNullVerifyingInstrumenterTest.java | 59 ++++++++++++++++- 11 files changed, 166 insertions(+), 26 deletions(-) create mode 100644 java/java-tests/testData/compiler/notNullVerification/NoCheckForConstant.java create mode 100644 java/java-tests/testData/compiler/notNullVerification/NoCheckForFinalNotNullMethodCall.java create mode 100644 java/java-tests/testData/compiler/notNullVerification/NoCheckForNewArray.java create mode 100644 java/java-tests/testData/compiler/notNullVerification/NoCheckForNewConstructorCall.java create mode 100644 java/java-tests/testData/compiler/notNullVerification/NoCheckForNewMultiArray.java create mode 100644 java/java-tests/testData/compiler/notNullVerification/NoCheckForNewObject.java create mode 100644 java/java-tests/testData/compiler/notNullVerification/NoCheckForPrivateNotNullMethodCall.java create mode 100644 java/java-tests/testData/compiler/notNullVerification/NoCheckForStaticNotNullMethodCall.java diff --git a/java/compiler/instrumentation-util/src/com/intellij/compiler/notNullVerification/AuxiliaryMethodGenerator.java b/java/compiler/instrumentation-util/src/com/intellij/compiler/notNullVerification/AuxiliaryMethodGenerator.java index adbec94912f5..7f05c787864c 100644 --- a/java/compiler/instrumentation-util/src/com/intellij/compiler/notNullVerification/AuxiliaryMethodGenerator.java +++ b/java/compiler/instrumentation-util/src/com/intellij/compiler/notNullVerification/AuxiliaryMethodGenerator.java @@ -15,7 +15,10 @@ */ package com.intellij.compiler.notNullVerification; -import org.jetbrains.org.objectweb.asm.*; +import org.jetbrains.org.objectweb.asm.ClassReader; +import org.jetbrains.org.objectweb.asm.ClassVisitor; +import org.jetbrains.org.objectweb.asm.Label; +import org.jetbrains.org.objectweb.asm.MethodVisitor; import java.util.*; @@ -65,7 +68,7 @@ class AuxiliaryMethodGenerator { existingMethods.add(name); return null; } - }, 0); + }, ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES); return existingMethods; } diff --git a/java/compiler/instrumentation-util/src/com/intellij/compiler/notNullVerification/NotNullVerifyingInstrumenter.java b/java/compiler/instrumentation-util/src/com/intellij/compiler/notNullVerification/NotNullVerifyingInstrumenter.java index 2916d6eb5707..355fa0a4a983 100644 --- a/java/compiler/instrumentation-util/src/com/intellij/compiler/notNullVerification/NotNullVerifyingInstrumenter.java +++ b/java/compiler/instrumentation-util/src/com/intellij/compiler/notNullVerification/NotNullVerifyingInstrumenter.java @@ -21,9 +21,10 @@ public class NotNullVerifyingInstrumenter extends ClassVisitor implements Opcode private static final String ANNOTATION_DEFAULT_METHOD = "value"; - @SuppressWarnings("SSBasedInspection") private static final String[] EMPTY_STRING_ARRAY = new String[0]; + @SuppressWarnings("SSBasedInspection") + private static final String[] EMPTY_STRING_ARRAY = new String[0]; - private final MethodsData myMethodsData = new MethodsData(); + private final MethodData myMethodData; private String myClassName; private boolean myIsModification = false; private RuntimeException myPostponedError; @@ -37,53 +38,56 @@ public class NotNullVerifyingInstrumenter extends ClassVisitor implements Opcode for (String annotation : notNullAnnotations) { myNotNullAnnotations.add('L' + annotation.replace('.', '/') + ';'); } - collectMethodData(reader, myNotNullAnnotations, myMethodsData); + myMethodData = collectMethodData(reader, myNotNullAnnotations); myAuxGenerator = new AuxiliaryMethodGenerator(reader); } public static boolean processClassFile(FailSafeClassReader reader, ClassVisitor writer, String[] notNullAnnotations) { NotNullVerifyingInstrumenter instrumenter = new NotNullVerifyingInstrumenter(writer, reader, notNullAnnotations); reader.accept(instrumenter, 0); - instrumenter.myAuxGenerator.generateReportingMethod(writer); return instrumenter.myIsModification; } - private static final class MethodsData { + private static final class MethodData { + private String myClassName; final Map> paramNames = new LinkedHashMap>(); - final Set alwaysNotNullMethods = new HashSet(); // methods that 100% guaranteed return a non-null value + final Set alwaysNotNullMethods = new HashSet(); // methods we are 100% sure return a non-null value - static String key(String className, String methodName, String desc) { - return className + '.' + methodName + desc; + public void setClassName(String className) { + myClassName = className; } - String lookupParamName(String className, String methodName, String desc, Integer num) { - final Map names = paramNames.get(key(className, methodName, desc)); + static String key(String methodName, String desc) { + return methodName + desc; + } + + String lookupParamName(String methodName, String desc, Integer num) { + final Map names = paramNames.get(key(methodName, desc)); return names != null? names.get(num) : null; } - void markNotNull(String className, String methodName, String desc) { - alwaysNotNullMethods.add(key(className, methodName, desc)); + void markNotNull(String methodName, String desc) { + alwaysNotNullMethods.add(key(methodName, desc)); } boolean isAlwaysNotNull(String className, String methodName, String desc) { - return alwaysNotNullMethods.contains(key(className, methodName, desc)); + return myClassName.equals(className) && alwaysNotNullMethods.contains(key(methodName, desc)); } } - private static void collectMethodData(ClassReader reader, final Set notNullAnnotations, final MethodsData data) { - + private static MethodData collectMethodData(ClassReader reader, final Set notNullAnnotations) { + final MethodData result = new MethodData(); reader.accept(new ClassVisitor(Opcodes.API_VERSION) { - private String myClassName = null; @Override public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) { - myClassName = name; + result.setClassName(name); } @Override public MethodVisitor visitMethod(int access, final String name, final String desc, String signature, String[] exceptions) { final Map names = new LinkedHashMap(); - data.paramNames.put(MethodsData.key(myClassName, name, desc), names); + result.paramNames.put(MethodData.key(name, desc), names); final Type[] args = Type.getArgumentTypes(desc); final boolean shouldRegisterNotNull = isReferenceType(Type.getReturnType(desc)) && (access & (Opcodes.ACC_FINAL | Opcodes.ACC_STATIC | Opcodes.ACC_PRIVATE)) != 0; @@ -99,11 +103,19 @@ public class NotNullVerifyingInstrumenter extends ClassVisitor implements Opcode @Override public AnnotationVisitor visitAnnotation(String anno, boolean isRuntime) { if (shouldRegisterNotNull && notNullAnnotations.contains(anno)) { - data.markNotNull(myClassName, name, desc); + result.markNotNull(name, desc); } return super.visitAnnotation(anno, isRuntime); } + @Override + public AnnotationVisitor visitTypeAnnotation(int typeRef, TypePath typePath, String anno, boolean visible) { + if (shouldRegisterNotNull && new TypeReference(typeRef).getSort() == TypeReference.METHOD_RETURN && notNullAnnotations.contains(anno)) { + result.markNotNull(name, desc); + } + return super.visitTypeAnnotation(typeRef, typePath, anno, visible); + } + @Override public void visitLocalVariable(String name2, String desc, String signature, Label start, Label end, int slotIndex) { Integer paramIndex = paramSlots.get(slotIndex); @@ -113,7 +125,8 @@ public class NotNullVerifyingInstrumenter extends ClassVisitor implements Opcode } }; } - }, 0); + }, ClassReader.SKIP_FRAMES); + return result; } @Override @@ -263,7 +276,7 @@ public class NotNullVerifyingInstrumenter extends ClassVisitor implements Opcode mv.visitJumpInsn(IFNONNULL, end); NotNullState state = entry.getValue(); - String paramName = myMethodsData.lookupParamName(myClassName, name, desc, param); + String paramName = myMethodData.lookupParamName(name, desc, param); String descrPattern = state.getNullParamMessage(paramName); String[] args = state.message != null ? EMPTY_STRING_ARRAY @@ -313,6 +326,12 @@ public class NotNullVerifyingInstrumenter extends ClassVisitor implements Opcode }; } + @Override + public void visitEnd() { + myAuxGenerator.generateReportingMethod(cv); + super.visitEnd(); + } + private static boolean isStatic(int access) { return (access & ACC_STATIC) != 0; } @@ -445,11 +464,11 @@ public class NotNullVerifyingInstrumenter extends ClassVisitor implements Opcode } private boolean nextCanBeNullValue(int nextMethodCallOpcode, String owner, String name, String descriptor) { - if (nextMethodCallOpcode == Opcodes.INVOKESPECIAL && ("".equals(name) || myMethodsData.isAlwaysNotNull(owner, name, descriptor))) { + if (nextMethodCallOpcode == Opcodes.INVOKESPECIAL && ("".equals(name) || myMethodData.isAlwaysNotNull(owner, name, descriptor))) { // a constructor call or a NotNull marked own method return false; } - if ((nextMethodCallOpcode == Opcodes.INVOKESTATIC || nextMethodCallOpcode == Opcodes.INVOKEVIRTUAL) && myMethodsData.isAlwaysNotNull(owner, name, descriptor)) { + if ((nextMethodCallOpcode == Opcodes.INVOKESTATIC || nextMethodCallOpcode == Opcodes.INVOKEVIRTUAL) && myMethodData.isAlwaysNotNull(owner, name, descriptor)) { return false; } return true; diff --git a/java/java-tests/testData/compiler/notNullVerification/NoCheckForConstant.java b/java/java-tests/testData/compiler/notNullVerification/NoCheckForConstant.java new file mode 100644 index 000000000000..c7ef3da3533e --- /dev/null +++ b/java/java-tests/testData/compiler/notNullVerification/NoCheckForConstant.java @@ -0,0 +1,6 @@ +import org.jetbrains.annotations.NotNull; + +public class NoCheckForConstant { + @NotNull + String method() { return "abc"; } +} \ No newline at end of file diff --git a/java/java-tests/testData/compiler/notNullVerification/NoCheckForFinalNotNullMethodCall.java b/java/java-tests/testData/compiler/notNullVerification/NoCheckForFinalNotNullMethodCall.java new file mode 100644 index 000000000000..e46088c4e333 --- /dev/null +++ b/java/java-tests/testData/compiler/notNullVerification/NoCheckForFinalNotNullMethodCall.java @@ -0,0 +1,9 @@ +import org.jetbrains.annotations.NotNull; + +public class NoCheckForFinalNotNullMethodCall { + @NotNull + final String foo() {return "a";} + + @NotNull + Object method() { return foo(); } +} \ No newline at end of file diff --git a/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewArray.java b/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewArray.java new file mode 100644 index 000000000000..d4eb07861b9a --- /dev/null +++ b/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewArray.java @@ -0,0 +1,6 @@ +import org.jetbrains.annotations.NotNull; + +public class NoCheckForNewArray { + @NotNull + Object method() { return new int[0]; } +} \ No newline at end of file diff --git a/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewConstructorCall.java b/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewConstructorCall.java new file mode 100644 index 000000000000..6c3a1284e23a --- /dev/null +++ b/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewConstructorCall.java @@ -0,0 +1,10 @@ +import org.jetbrains.annotations.NotNull; + +public class NoCheckForNewConstructorCall { + + public NoCheckForNewConstructorCall(int p1, String p2) { + } + + @NotNull + Object method() { return new NoCheckForNewConstructorCall(42, "42"); } +} \ No newline at end of file diff --git a/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewMultiArray.java b/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewMultiArray.java new file mode 100644 index 000000000000..c1d74c81e33c --- /dev/null +++ b/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewMultiArray.java @@ -0,0 +1,6 @@ +import org.jetbrains.annotations.NotNull; + +public class NoCheckForNewMultiArray { + @NotNull + Object method() { return new int[0][0]; } +} \ No newline at end of file diff --git a/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewObject.java b/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewObject.java new file mode 100644 index 000000000000..f45dc454f607 --- /dev/null +++ b/java/java-tests/testData/compiler/notNullVerification/NoCheckForNewObject.java @@ -0,0 +1,6 @@ +import org.jetbrains.annotations.NotNull; + +public class NoCheckForNewObject { + @NotNull + Object method() { return new Object(); } +} \ No newline at end of file diff --git a/java/java-tests/testData/compiler/notNullVerification/NoCheckForPrivateNotNullMethodCall.java b/java/java-tests/testData/compiler/notNullVerification/NoCheckForPrivateNotNullMethodCall.java new file mode 100644 index 000000000000..9fbdb4d37c49 --- /dev/null +++ b/java/java-tests/testData/compiler/notNullVerification/NoCheckForPrivateNotNullMethodCall.java @@ -0,0 +1,9 @@ +import org.jetbrains.annotations.NotNull; + +public class NoCheckForPrivateNotNullMethodCall { + @NotNull + private String foo() {return "a";} + + @NotNull + Object method() { return foo(); } +} \ No newline at end of file diff --git a/java/java-tests/testData/compiler/notNullVerification/NoCheckForStaticNotNullMethodCall.java b/java/java-tests/testData/compiler/notNullVerification/NoCheckForStaticNotNullMethodCall.java new file mode 100644 index 000000000000..753be483b676 --- /dev/null +++ b/java/java-tests/testData/compiler/notNullVerification/NoCheckForStaticNotNullMethodCall.java @@ -0,0 +1,9 @@ +import org.jetbrains.annotations.NotNull; + +public class NoCheckForStaticNotNullMethodCall { + @NotNull + static String foo() {return "a";} + + @NotNull + Object method() { return foo(); } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/compiler/notNullVerification/NotNullVerifyingInstrumenterTest.java b/java/java-tests/testSrc/com/intellij/java/compiler/notNullVerification/NotNullVerifyingInstrumenterTest.java index 29351c09105b..17fead2d9488 100644 --- a/java/java-tests/testSrc/com/intellij/java/compiler/notNullVerification/NotNullVerifyingInstrumenterTest.java +++ b/java/java-tests/testSrc/com/intellij/java/compiler/notNullVerification/NotNullVerifyingInstrumenterTest.java @@ -305,6 +305,54 @@ public abstract class NotNullVerifyingInstrumenterTest { verifyCallThrowsException("Argument for @NotNull parameter 'param' of LocalClassImplicitParameters$Inner. must not be null", instance, test.getMethod("failInner")); } + @Test + public void testNoCheckForConstant() throws Exception { + Class test = prepareTest(true, false, AnnotationUtil.NOT_NULL); + assertNotNull(test); + } + + @Test + public void testNoCheckForNewObject() throws Exception { + Class test = prepareTest(true, false, AnnotationUtil.NOT_NULL); + assertNotNull(test); + } + + @Test + public void testNoCheckForNewConstructorCall() throws Exception { + Class test = prepareTest(true, false, AnnotationUtil.NOT_NULL); + assertNotNull(test); + } + + @Test + public void testNoCheckForNewArray() throws Exception { + Class test = prepareTest(true, false, AnnotationUtil.NOT_NULL); + assertNotNull(test); + } + + @Test + public void testNoCheckForNewMultiArray() throws Exception { + Class test = prepareTest(true, false, AnnotationUtil.NOT_NULL); + assertNotNull(test); + } + + @Test + public void testNoCheckForPrivateNotNullMethodCall() throws Exception { + Class test = prepareTest(true, false, AnnotationUtil.NOT_NULL); + assertNotNull(test); + } + + @Test + public void testNoCheckForFinalNotNullMethodCall() throws Exception { + Class test = prepareTest(true, false, AnnotationUtil.NOT_NULL); + assertNotNull(test); + } + + @Test + public void testNoCheckForStaticNotNullMethodCall() throws Exception { + Class test = prepareTest(true, false, AnnotationUtil.NOT_NULL); + assertNotNull(test); + } + private static void verifyCallThrowsException(String expectedError, @Nullable Object instance, Member member, Object... args) throws Exception { String exceptionText = null; try { @@ -329,6 +377,10 @@ public abstract class NotNullVerifyingInstrumenterTest { } private Class prepareTest(boolean withDebugInfo, String... notNullAnnotations) throws IOException { + return prepareTest(withDebugInfo, true, notNullAnnotations); + } + + private Class prepareTest(boolean withDebugInfo, boolean expectInstrumented, String... notNullAnnotations) throws IOException { String testName = PlatformTestUtil.getTestName(this.testName.getMethodName(), false); File testFile = IdeaTestUtil.findSourceFile((JavaTestUtil.getJavaTestDataPath() + TEST_DATA_PATH) + testName); File classesDir = tempDir.newFolder("output"); @@ -352,7 +404,12 @@ public abstract class NotNullVerifyingInstrumenterTest { mainClass = aClass; } } - assertTrue("Class file not instrumented!", modified); + if (expectInstrumented) { + assertTrue("Class file not instrumented!", modified); + } + else { + assertFalse("Class file instrumented, but should have not!", modified); + } assertNotNull("Class " + testName + " not found!", mainClass); return mainClass; }