From c62c79fa9d5af3beede071925a644c8f367c2b81 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Mon, 4 Apr 2022 16:23:00 +0800 Subject: [PATCH 01/15] build: reformat settings file to KTS format --- settings.gradle | 3 --- settings.gradle.kts | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) delete mode 100644 settings.gradle create mode 100644 settings.gradle.kts diff --git a/settings.gradle b/settings.gradle deleted file mode 100644 index eb46c70..0000000 --- a/settings.gradle +++ /dev/null @@ -1,3 +0,0 @@ -rootProject.name = 'spotbugs-jspecify-plugin' -includeBuild '../jspecify' -includeBuild '../gradle-modules-plugin' diff --git a/settings.gradle.kts b/settings.gradle.kts new file mode 100644 index 0000000..6ff1071 --- /dev/null +++ b/settings.gradle.kts @@ -0,0 +1,3 @@ +rootProject.name = "spotbugs-jspecify-plugin" +includeBuild("../jspecify") +includeBuild("../gradle-modules-plugin") From c49f2bf59bdeb3393c380fb56fd9f789c6e296b5 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Mon, 4 Apr 2022 16:31:51 +0800 Subject: [PATCH 02/15] fix: use SLF4J API instead of System.err --- .../jspecify/nullness/NeedlessAnnotationDetector.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java index 227f0fc..1baa52e 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java @@ -179,7 +179,7 @@ class MethodVisitor extends org.objectweb.asm.MethodVisitor { @Override public void visitParameter(String name, int access) { - System.err.printf("visitParameter: %s%s%n", methodDescriptor.getSignature(), name); + log.info("visitParameter: {}{}", methodDescriptor.getSignature(), name); super.visitParameter(name, access); } @@ -188,8 +188,8 @@ public AnnotationVisitor visitParameterAnnotation( int parameter, String descriptor, boolean visible) { Type[] types = Type.getArgumentTypes(methodDescriptor.getSignature()); Type type = types[parameter]; - System.err.printf( - "visitParameterAnnotation: %s method parameter (%d) is type %s and annotated with %s%n", + log.info( + "visitParameterAnnotation: {} method parameter ({}) is type {} and annotated with {}", methodDescriptor, parameter, type, descriptor); return null; } @@ -202,8 +202,8 @@ public void visitEnd() { if (!canBeNull(returnType) && nullnessOfReturnedValue.isSetExplicitly() && nullnessOfReturnedValue != Nullness.NOT_NULL) { - System.err.printf( - "%s is annotated as nullable, but %s cannot be null%n", methodDescriptor, returnType); + log.info( + "{} is annotated as nullable, but {} cannot be null", methodDescriptor, returnType); bugReporter.reportBug( new BugInstance( "JSPECIFY_NULLNESS_INTRINSICALLY_NOT_NULLABLE", Priorities.HIGH_PRIORITY) From d56ec24721e5ec9de7e3771d0f22c12e3a38d269 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Mon, 4 Apr 2022 16:32:31 +0800 Subject: [PATCH 03/15] refactor: shorten code --- .../nullness/NeedlessAnnotationDetector.java | 5 +++- .../ReturnUnexpectedNullDetector.java | 30 +++++++++---------- .../spotbugs/jspecify/TestWithSample.java | 2 +- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java index 1baa52e..3c77dbe 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java @@ -190,7 +190,10 @@ public AnnotationVisitor visitParameterAnnotation( Type type = types[parameter]; log.info( "visitParameterAnnotation: {} method parameter ({}) is type {} and annotated with {}", - methodDescriptor, parameter, type, descriptor); + methodDescriptor, + parameter, + type, + descriptor); return null; } diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/ReturnUnexpectedNullDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/ReturnUnexpectedNullDetector.java index 231da40..30c450b 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/ReturnUnexpectedNullDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/ReturnUnexpectedNullDetector.java @@ -64,21 +64,21 @@ boolean isTargetMethod() { @Override public void afterOpcode(int code) { - switch (code) { - case Const.INVOKEINTERFACE: - case Const.INVOKESPECIAL: - case Const.INVOKESTATIC: - case Const.INVOKEVIRTUAL: - XMethod methodOperand = getXMethodOperand(); - NullnessDatabase database = Global.getAnalysisCache().getDatabase(NullnessDatabase.class); - Optional optional = - database.findNullnessOf(getXClassOperand(), methodOperand, Global.getAnalysisCache()); - super.afterOpcode(code); - optional.ifPresent(nullness -> stack.getStackItem(0).setUserValue(nullness)); - return; - // constructor has no returned value - default: - super.afterOpcode(code); + super.afterOpcode(code); + + if (code != Const.INVOKEINTERFACE + && code != Const.INVOKESPECIAL + && code != Const.INVOKESTATIC + && code != Const.INVOKEVIRTUAL) { + return; + } + + XMethod methodOperand = getXMethodOperand(); + NullnessDatabase database = Global.getAnalysisCache().getDatabase(NullnessDatabase.class); + Optional optional = + database.findNullnessOf(getXClassOperand(), methodOperand, Global.getAnalysisCache()); + if (methodOperand.isReturnTypeReferenceType() && !stack.isTop()) { + optional.ifPresent(nullness -> stack.getStackItem(0).setUserValue(nullness)); } } } diff --git a/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java b/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java index efc81f0..eb8f4aa 100644 --- a/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java +++ b/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java @@ -74,7 +74,7 @@ private static File[] listSamples() { return Arrays.stream(samples.toFile().listFiles()) .filter(File::isFile) .filter(file -> file.getName().endsWith(".java")) - .toArray(size -> new File[size]); + .toArray(File[]::new); } @ParameterizedTest(name = "Compile and run analysis on {0}") From 44e152091c14c9508ec898ed5a2b1c1c90849b79 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Mon, 4 Apr 2022 18:53:09 +0800 Subject: [PATCH 04/15] test: fix compilation error --- .../com/github/spotbugs/jspecify/TestWithSample.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java b/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java index eb8f4aa..309bae6 100644 --- a/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java +++ b/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java @@ -53,7 +53,7 @@ class TestWithSample { .toAbsolutePath() .normalize(), Paths.get( - System.getenv("user.home"), + System.getProperty("user.home"), ".gradle", "caches", "modules-2", @@ -105,7 +105,8 @@ private void compile( File javaFile, JavaCompiler compiler, StandardJavaFileManager javaFileManager) { Iterable javaFileObjects = javaFileManager.getJavaFileObjects(javaFile); - String classpath = JAR.stream().map(Path::toString).collect(Collectors.joining(",")); + String classpath = + JAR.stream().map(Path::toString).collect(Collectors.joining(File.pathSeparator)); Boolean success = compiler .getTask( @@ -116,6 +117,8 @@ private void compile( null, javaFileObjects) .call(); - Assertions.assertTrue(success); + if (!success) { + Assertions.fail("Compilation failed unexpectedly"); + } } } From 02068b5d73a2bb09d5b05a2e293baa16214467d3 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Mon, 4 Apr 2022 18:58:17 +0800 Subject: [PATCH 05/15] fix: solve a NPE --- .../jspecify/nullness/ReturnUnexpectedNullDetector.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/ReturnUnexpectedNullDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/ReturnUnexpectedNullDetector.java index 30c450b..7f8494e 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/ReturnUnexpectedNullDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/ReturnUnexpectedNullDetector.java @@ -77,7 +77,7 @@ public void afterOpcode(int code) { NullnessDatabase database = Global.getAnalysisCache().getDatabase(NullnessDatabase.class); Optional optional = database.findNullnessOf(getXClassOperand(), methodOperand, Global.getAnalysisCache()); - if (methodOperand.isReturnTypeReferenceType() && !stack.isTop()) { + if ((methodOperand != null && methodOperand.isReturnTypeReferenceType()) && !stack.isTop()) { optional.ifPresent(nullness -> stack.getStackItem(0).setUserValue(nullness)); } } From 27eee5430da75c7228b7f5092a31f44fe4fa23a8 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Mon, 4 Apr 2022 19:08:07 +0800 Subject: [PATCH 06/15] fix: add jsr305 to classpath to resolve compilation error --- .../github/spotbugs/jspecify/TestWithSample.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java b/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java index 309bae6..c4670cf 100644 --- a/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java +++ b/src/test/java/com/github/spotbugs/jspecify/TestWithSample.java @@ -52,6 +52,19 @@ class TestWithSample { Paths.get("..", "jspecify", "build", "libs", "jspecify-0.0.0-SNAPSHOT.jar") .toAbsolutePath() .normalize(), + Paths.get( + System.getProperty("user.home"), + ".gradle", + "caches", + "modules-2", + "files-2.1", + "com.google.code.findbugs", + "jsr305", + "3.0.2", + "25ea2e8b0c338a877313bd4672d3fe056ea78f0d", + "jsr305-3.0.2.jar") + .toAbsolutePath() + .normalize(), Paths.get( System.getProperty("user.home"), ".gradle", From edac7499f648f7e28756db27e56ff90f98c4ab27 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Tue, 5 Apr 2022 07:34:58 +0800 Subject: [PATCH 07/15] chore: make IDEA using JDK11 by default --- .idea/misc.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.idea/misc.xml b/.idea/misc.xml index d0a36e3..2093826 100644 --- a/.idea/misc.xml +++ b/.idea/misc.xml @@ -1,7 +1,7 @@ - + From a790a4d00ea7c555a4b087238f47a60f88e78491 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Wed, 6 Apr 2022 20:26:53 +0800 Subject: [PATCH 08/15] improve JSPECIFY_NULLNESS_INTRINSICALLY_NOT_NULLABLE --- .../nullness/NeedlessAnnotationDetector.java | 84 ++++++++++- .../jspecify/nullness/asm/ParameterType.java | 39 +++++ .../nullness/asm/ParameterTypeFinder.java | 62 ++++++++ .../nullness/asm/SignatureParser.java | 132 +++++++++++++++++ .../jspecify/nullness/asm/TypeArgument.java | 57 +++++++ .../nullness/asm/TypeArgumentWithType.java | 64 ++++++++ .../asm/TypeArgumentWithTypeVariable.java | 60 ++++++++ .../jspecify/nullness/asm/TypeFinder.java | 75 ++++++++++ .../jspecify/nullness/asm/package-info.java | 8 + src/main/resources/messages.xml | 2 +- .../nullness/asm/SignatureParserTest.java | 139 ++++++++++++++++++ 11 files changed, 714 insertions(+), 8 deletions(-) create mode 100644 src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterType.java create mode 100644 src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinder.java create mode 100644 src/main/java/com/github/spotbugs/jspecify/nullness/asm/SignatureParser.java create mode 100644 src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgument.java create mode 100644 src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithType.java create mode 100644 src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithTypeVariable.java create mode 100644 src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeFinder.java create mode 100644 src/main/java/com/github/spotbugs/jspecify/nullness/asm/package-info.java create mode 100644 src/test/java/com/github/spotbugs/jspecify/nullness/asm/SignatureParserTest.java diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java index 3c77dbe..4f56bcc 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java @@ -15,16 +15,13 @@ */ package com.github.spotbugs.jspecify.nullness; +import com.github.spotbugs.jspecify.nullness.asm.*; import edu.umd.cs.findbugs.BugInstance; import edu.umd.cs.findbugs.BugReporter; import edu.umd.cs.findbugs.Priorities; import edu.umd.cs.findbugs.asm.ClassNodeDetector; import edu.umd.cs.findbugs.ba.XClass; -import edu.umd.cs.findbugs.classfile.CheckedAnalysisException; -import edu.umd.cs.findbugs.classfile.ClassDescriptor; -import edu.umd.cs.findbugs.classfile.FieldDescriptor; -import edu.umd.cs.findbugs.classfile.Global; -import edu.umd.cs.findbugs.classfile.MethodDescriptor; +import edu.umd.cs.findbugs.classfile.*; import edu.umd.cs.findbugs.classfile.engine.asm.FindBugsASM; import edu.umd.cs.findbugs.internalAnnotations.SlashedClassName; import java.lang.invoke.MethodHandles; @@ -34,6 +31,8 @@ import org.jspecify.nullness.Nullable; import org.objectweb.asm.AnnotationVisitor; import org.objectweb.asm.Type; +import org.objectweb.asm.TypePath; +import org.objectweb.asm.TypeReference; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -95,6 +94,7 @@ public MethodVisitor visitMethod( return new MethodVisitor( FindBugsASM.ASM_VERSION, methodDescriptor, + signature, nullness == null ? Nullness.NO_EXPLICIT_CONFIG : nullness); } @@ -168,21 +168,91 @@ class MethodVisitor extends org.objectweb.asm.MethodVisitor { private final MethodDescriptor methodDescriptor; /** Default nullness in the current scope. */ private final Nullness defaultNullness; + /** The signature of this method, could be null if the method is not generic. */ + @Nullable private final String signature; /** Nullness specified by the annotation on this method's return value. */ @Nullable private Nullness nullness; - MethodVisitor(int api, MethodDescriptor methodDescriptor, Nullness defaultNullness) { + MethodVisitor( + int api, MethodDescriptor methodDescriptor, String signature, Nullness defaultNullness) { super(api); + this.signature = signature; this.defaultNullness = defaultNullness; this.methodDescriptor = methodDescriptor; } @Override public void visitParameter(String name, int access) { - log.info("visitParameter: {}{}", methodDescriptor.getSignature(), name); + log.debug("visitParameter: signature {}, name {}", methodDescriptor.getSignature(), name); super.visitParameter(name, access); } + private Type getAnnotated(TypeReference typeRef, TypePath typePath) { + TypeArgument tempType = null; + switch (typeRef.getSort()) { + case TypeReference.METHOD_RETURN: + return Type.getReturnType(methodDescriptor.getSignature()); + + case TypeReference.METHOD_FORMAL_PARAMETER: + int typeParamIndex = typeRef.getTypeParameterIndex(); + if (typePath == null) { + // means the type of parameter is directly annotated, like `@Nullness int` + return Type.getArgumentTypes(methodDescriptor.getSignature())[typeParamIndex]; + } + System.err.println("method is " + methodDescriptor + ", signature is " + signature); + ParameterType param = + ParameterTypeFinder.create(api, signature, methodDescriptor) + .getParameterType(typeParamIndex); + + int length = typePath.getLength(); + for (int i = 0; i < length; ++i) { + switch (typePath.getStep(i)) { + case TypePath.TYPE_ARGUMENT: + int index = typePath.getStepArgument(i); + tempType = param.getTypeArguments().get(index); + break; + + case TypePath.ARRAY_ELEMENT: + // means an element of array is annotated, like `@Nullness int[]` + if (tempType instanceof TypeArgumentWithType) { + return ((TypeArgumentWithType) tempType).getType().getElementType(); + } + + Type arrayType = + Type.getArgumentTypes(methodDescriptor.getSignature())[typeParamIndex]; + return arrayType.getElementType(); + } + } + // fall through + default: + log.debug( + ">> sort {}, TypeArgumentIndex {}, TypeParameterIndex {}, TypeParameterBoundIndex {}, typePath {}", + typeRef.getSort(), + typeRef.getTypeArgumentIndex(), + typeRef.getTypeParameterIndex(), + typeRef.getTypeParameterBoundIndex(), + typePath); + } + return null; + } + + @Override + public AnnotationVisitor visitTypeAnnotation( + int typeRef, TypePath typePath, String descriptor, boolean visible) { + TypeReference typeRefObj = new TypeReference(typeRef); + Type annotated = getAnnotated(typeRefObj, typePath); + if (annotated != null + && annotated.getSort() <= Type.DOUBLE + && Nullness.from(descriptor).isPresent()) { + bugReporter.reportBug( + new BugInstance( + "JSPECIFY_NULLNESS_INTRINSICALLY_NOT_NULLABLE", Priorities.HIGH_PRIORITY) + .addType(annotated.getDescriptor()) + .addClassAndMethod(methodDescriptor)); + } + return super.visitTypeAnnotation(typeRef, typePath, descriptor, visible); + } + @Override public AnnotationVisitor visitParameterAnnotation( int parameter, String descriptor, boolean visible) { diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterType.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterType.java new file mode 100644 index 0000000..88d91e7 --- /dev/null +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterType.java @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2021-2022 The SpotBugs team. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.github.spotbugs.jspecify.nullness.asm; + +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import org.objectweb.asm.Type; + +public final class ParameterType { + private final List typeArguments; + private final Type type; + + ParameterType(Type type, List typeArguments) { + this.type = Objects.requireNonNull(type); + this.typeArguments = Collections.unmodifiableList(typeArguments); + } + + public List getTypeArguments() { + return typeArguments; + } + + public Type getType() { + return type; + } +} diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinder.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinder.java new file mode 100644 index 0000000..fb9cd66 --- /dev/null +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinder.java @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2021-2022 The SpotBugs team. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.github.spotbugs.jspecify.nullness.asm; + +import edu.umd.cs.findbugs.classfile.MethodDescriptor; +import java.util.Collections; +import java.util.Objects; +import org.jspecify.nullness.Nullable; +import org.objectweb.asm.Type; + +public abstract class ParameterTypeFinder { + public static ParameterTypeFinder create( + int api, @Nullable String signature, MethodDescriptor methodDescriptor) { + if (signature == null) { + return new BasedOnMethodDescriptor(methodDescriptor); + } else { + return new BasedOnSignature(new SignatureParser(api, signature)); + } + } + + public abstract ParameterType getParameterType(int argument); + + private static class BasedOnSignature extends ParameterTypeFinder { + private final SignatureParser signatureParser; + + public BasedOnSignature(SignatureParser signatureParser) { + this.signatureParser = Objects.requireNonNull(signatureParser); + } + + @Override + public ParameterType getParameterType(int argument) { + return signatureParser.getParameterType(argument); + } + } + + private static class BasedOnMethodDescriptor extends ParameterTypeFinder { + private final Type[] argumentTypes; + + public BasedOnMethodDescriptor(MethodDescriptor methodDescriptor) { + this.argumentTypes = + Type.getArgumentTypes(Objects.requireNonNull(methodDescriptor).getSignature()); + } + + @Override + public ParameterType getParameterType(int argument) { + return new ParameterType(argumentTypes[argument], Collections.emptyList()); + } + } +} diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/SignatureParser.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/SignatureParser.java new file mode 100644 index 0000000..0c3fa78 --- /dev/null +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/SignatureParser.java @@ -0,0 +1,132 @@ +/* + * Copyright (c) 2021-2022 The SpotBugs team. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.github.spotbugs.jspecify.nullness.asm; + +import java.lang.invoke.MethodHandles; +import java.util.*; +import java.util.function.Consumer; +import org.jspecify.nullness.Nullable; +import org.objectweb.asm.Type; +import org.objectweb.asm.signature.SignatureReader; +import org.objectweb.asm.signature.SignatureVisitor; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A helper class that parses signature (in context of the ASM), to split them into {@link + * org.objectweb.asm.Type} instances. It is similar to the {@code + * edu.umd.cs.findbugs.ba.SignatureParser} provided by SpotBugs core, but provides more features + * especially the generics support. + * + * @see edu.umd.cs.findbugs.ba.SignatureParser + */ +class SignatureParser { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + private final MySignatureVisitor visitor; + + SignatureParser(int api, String signature) { + Objects.requireNonNull(signature); + log.debug("analysing signature {}", signature); + SignatureReader reader = new SignatureReader(signature); + visitor = new MySignatureVisitor(api); + reader.accept(visitor); + } + + public Optional getReturnedType() { + return Optional.ofNullable(visitor.returnType); + } + + public ParameterType getParameterType(int argument) { + assert visitor.parameterTypes.size() == visitor.typeArguments.size(); + if (0 <= argument && argument < visitor.parameterTypes.size()) { + TypeArgument typeArgument = visitor.parameterTypes.get(argument); + if (typeArgument instanceof TypeArgumentWithType) { + TypeArgumentWithType withType = (TypeArgumentWithType) typeArgument; + return new ParameterType(withType.getType(), visitor.typeArguments.get(argument)); + } else if (typeArgument instanceof TypeArgumentWithTypeVariable) { + // TODO treat type variable properly + TypeArgumentWithTypeVariable withTypeVariable = (TypeArgumentWithTypeVariable) typeArgument; + return new ParameterType( + Type.getObjectType(withTypeVariable.getTypeVariable()), + visitor.typeArguments.get(argument)); + } + } + throw new IndexOutOfBoundsException( + String.format( + "The target method has only %d arguments but requested to get %dth parameter", + visitor.parameterTypes.size(), argument)); + } + + public Optional getFormalTypeParameter() { + return Optional.ofNullable(visitor.formalTypeParameter); + } + + private static final class MySignatureVisitor extends SignatureVisitor { + @Nullable private String formalTypeParameter; + @Nullable private TypeArgument returnType; + /** + * List of name of parameter types, e.g. {@code + * "CaptureConvertedUnionNullToOtherUnionNull$Lib"}. + */ + private final List parameterTypes = new ArrayList<>(); + + private final List> typeArguments = new ArrayList<>(); + + MySignatureVisitor(int api) { + super(api); + } + + @Override + public SignatureVisitor visitReturnType() { + return new TypeFinder(api, type -> this.returnType = type); + } + + @Override + public SignatureVisitor visitParameterType() { + typeArguments.add(new ArrayList<>()); + + return new TypeFinder( + api, + new Consumer<>() { + private boolean isFirst = true; + + @Override + public void accept(TypeArgument typeArgument) { + if (isFirst) { + parameterTypes.add(typeArgument); + isFirst = false; + } else { + getLastTypeArguments().add(typeArgument); + } + } + }); + } + + private List getLastTypeArguments() { + return typeArguments.get(typeArguments.size() - 1); + } + + @Override + public void visitFormalTypeParameter(String name) { + formalTypeParameter = name; + } + + @Override + public SignatureVisitor visitTypeArgument(char wildcard) { + return new TypeFinder(api, type -> getLastTypeArguments().add(type)); + } + } +} diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgument.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgument.java new file mode 100644 index 0000000..11da536 --- /dev/null +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgument.java @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2021-2022 The SpotBugs team. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.github.spotbugs.jspecify.nullness.asm; + +import java.util.function.Function; + +public abstract class TypeArgument { + private final char wildcard; + + TypeArgument(char wildcard) { + this.wildcard = wildcard; + } + + /** + * Returns '=' for the {@code instanceof}, '+' for {@code extends}, and '-' for {@code super} type + * argument. + * + * @see JVMS + * ยง4.7.9.1. Signatures + */ + public char getWildcard() { + return wildcard; + } + + abstract TypeArgument replaceWildcard(char wildcard); + + abstract TypeArgument replaceType(Function replace); + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + TypeArgument that = (TypeArgument) o; + + return wildcard == that.wildcard; + } + + @Override + public int hashCode() { + return wildcard; + } +} diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithType.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithType.java new file mode 100644 index 0000000..70587f2 --- /dev/null +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithType.java @@ -0,0 +1,64 @@ +/* + * Copyright (c) 2021-2022 The SpotBugs team. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.github.spotbugs.jspecify.nullness.asm; + +import java.util.Objects; +import java.util.function.Function; +import org.objectweb.asm.Type; + +public final class TypeArgumentWithType extends TypeArgument { + private final Type type; + + TypeArgumentWithType(char wildcard, String name) { + super(wildcard); + this.type = Type.getType(Objects.requireNonNull(name)); + } + + TypeArgumentWithType(char wildcard, Type type) { + super(wildcard); + this.type = type; + } + + public Type getType() { + return type; + } + + @Override + TypeArgument replaceWildcard(char wildcard) { + return new TypeArgumentWithType(wildcard, type); + } + + @Override + TypeArgument replaceType(Function replace) { + Type newType = Type.getType(replace.apply(type.getDescriptor())); + return new TypeArgumentWithType(getWildcard(), newType); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + TypeArgumentWithType that = (TypeArgumentWithType) o; + + return Objects.equals(type, that.type); + } + + @Override + public int hashCode() { + return type != null ? type.hashCode() : 0; + } +} diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithTypeVariable.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithTypeVariable.java new file mode 100644 index 0000000..b889e9e --- /dev/null +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithTypeVariable.java @@ -0,0 +1,60 @@ +/* + * Copyright (c) 2021-2022 The SpotBugs team. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.github.spotbugs.jspecify.nullness.asm; + +import java.util.Objects; +import java.util.function.Function; + +public final class TypeArgumentWithTypeVariable extends TypeArgument { + private final String typeVariable; + + TypeArgumentWithTypeVariable(char wildcard, String typeVariable) { + super(wildcard); + this.typeVariable = Objects.requireNonNull(typeVariable); + } + + public String getTypeVariable() { + return typeVariable; + } + + @Override + TypeArgument replaceWildcard(char wildcard) { + return new TypeArgumentWithTypeVariable(wildcard, typeVariable); + } + + @Override + TypeArgument replaceType(Function replace) { + return new TypeArgumentWithTypeVariable(getWildcard(), replace.apply(typeVariable)); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + if (!super.equals(o)) return false; + + TypeArgumentWithTypeVariable that = (TypeArgumentWithTypeVariable) o; + + return Objects.equals(typeVariable, that.typeVariable); + } + + @Override + public int hashCode() { + int result = super.hashCode(); + result = 31 * result + (typeVariable != null ? typeVariable.hashCode() : 0); + return result; + } +} diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeFinder.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeFinder.java new file mode 100644 index 0000000..7826953 --- /dev/null +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeFinder.java @@ -0,0 +1,75 @@ +/* + * Copyright (c) 2021-2022 The SpotBugs team. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.github.spotbugs.jspecify.nullness.asm; + +import java.util.Objects; +import java.util.function.Consumer; +import org.jspecify.nullness.NullMarked; +import org.jspecify.nullness.Nullable; +import org.objectweb.asm.signature.SignatureVisitor; + +/** + * A helper class that find type in signature of returned type, parameter type, etc. It is designed + * to use as returned value of methods in {@link SignatureVisitor}. + */ +@NullMarked +final class TypeFinder extends SignatureVisitor { + private final Consumer<@Nullable TypeArgument> callback; + + TypeFinder(int api, Consumer<@Nullable TypeArgument> callback) { + super(api); + this.callback = Objects.requireNonNull(callback); + } + + @Override + public void visitBaseType(char descriptor) { + callback.accept(new TypeArgumentWithType('?', Character.toString(descriptor))); + } + + @Override + public void visitInnerClassType(String name) { + String innerName = "L" + name + ";"; + callback.accept(new TypeArgumentWithType('?', innerName)); + } + + @Override + public void visitClassType(String name) { + String innerName = "L" + name + ";"; + callback.accept(new TypeArgumentWithType('?', innerName)); + } + + @Override + public void visitTypeVariable(String name) { + callback.accept(new TypeArgumentWithTypeVariable('?', name)); + } + + @Override + public SignatureVisitor visitArrayType() { + return new TypeFinder(api, type -> callback.accept(type.replaceType(t -> "[" + t))); + } + + @Override + public SignatureVisitor visitTypeArgument(char wildcard) { + return new TypeFinder( + api, typeArgument -> callback.accept(typeArgument.replaceWildcard(wildcard))); + } + + @Override + public void visitTypeArgument() { + // TODO treat type argument with a wildcard in a special way? + callback.accept(new TypeArgumentWithType('*', "Ljava/lang/Object;")); + } +} diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/package-info.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/package-info.java new file mode 100644 index 0000000..f740d3b --- /dev/null +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/package-info.java @@ -0,0 +1,8 @@ +/** + * A package serving helper classes that helps to integrate ASM code written in event-oriented style + * with logics written in OOP. + */ +@NullMarked +package com.github.spotbugs.jspecify.nullness.asm; + +import org.jspecify.nullness.NullMarked; diff --git a/src/main/resources/messages.xml b/src/main/resources/messages.xml index 7b8a6ea..4abbc2c 100644 --- a/src/main/resources/messages.xml +++ b/src/main/resources/messages.xml @@ -26,7 +26,7 @@
Nullness annotation used for types that cannot be null.

+

Nullness annotation used for a type ({0}) that cannot be null.

]]>
diff --git a/src/test/java/com/github/spotbugs/jspecify/nullness/asm/SignatureParserTest.java b/src/test/java/com/github/spotbugs/jspecify/nullness/asm/SignatureParserTest.java new file mode 100644 index 0000000..64fbb9c --- /dev/null +++ b/src/test/java/com/github/spotbugs/jspecify/nullness/asm/SignatureParserTest.java @@ -0,0 +1,139 @@ +/* + * Copyright (c) 2021-2022 The SpotBugs team. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.github.spotbugs.jspecify.nullness.asm; + +import edu.umd.cs.findbugs.classfile.engine.asm.FindBugsASM; +import java.util.List; +import java.util.Optional; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.objectweb.asm.Type; + +class SignatureParserTest { + @Nested + static class ParseReturnValueTest { + @Test + void voidMethod() { + SignatureParser parser = new SignatureParser(FindBugsASM.ASM_VERSION, "()V"); + Assertions.assertEquals( + Optional.of(new TypeArgumentWithType('?', Type.VOID_TYPE)), parser.getReturnedType()); + } + + @Test + void primitiveMethod() { + SignatureParser parser = new SignatureParser(FindBugsASM.ASM_VERSION, "()I"); + Assertions.assertEquals( + Optional.of(new TypeArgumentWithType('?', Type.INT_TYPE)), parser.getReturnedType()); + } + } + + @Nested + static class ParseParameterTypeTest { + @Test + void noParameterType() { + SignatureParser parser = + new SignatureParser(FindBugsASM.ASM_VERSION, "(Ljava/lang/Object;)V"); + Assertions.assertTrue(parser.getParameterType(0).getTypeArguments().isEmpty()); + } + + @Test + void singleParameterType() { + SignatureParser parser = + new SignatureParser(FindBugsASM.ASM_VERSION, "(Ljava/util/Set;)V"); + TypeArgumentWithType typeArgument = + (TypeArgumentWithType) parser.getParameterType(0).getTypeArguments().get(0); + Assertions.assertEquals(Type.getType("Ljava/lang/Object;"), typeArgument.getType()); + Assertions.assertEquals('=', typeArgument.getWildcard()); + } + + @Test + void doubleParameterType() { + SignatureParser parser = + new SignatureParser(FindBugsASM.ASM_VERSION, "(Ljava/util/Set;)V"); + List typeArguments = parser.getParameterType(0).getTypeArguments(); + Assertions.assertEquals(2, typeArguments.size()); + Assertions.assertEquals( + "T", ((TypeArgumentWithTypeVariable) typeArguments.get(0)).getTypeVariable()); + } + + @Test + void singleParameterTypeWithWildcard() { + SignatureParser parser = + new SignatureParser(FindBugsASM.ASM_VERSION, "(LAnnotatedWildcard<*>;)V"); + Assertions.assertEquals( + Type.getType("LAnnotatedWildcard;"), parser.getParameterType(0).getType()); + + TypeArgumentWithType typeArgument = + (TypeArgumentWithType) parser.getParameterType(0).getTypeArguments().get(0); + Assertions.assertEquals('*', typeArgument.getWildcard()); + } + + @Test + void singleParameterTypeWithExtends() { + SignatureParser parser = + new SignatureParser(FindBugsASM.ASM_VERSION, "(Ljava/util/Set<+Ljava/lang/Object;>;)V"); + TypeArgumentWithType typeArgument = + (TypeArgumentWithType) parser.getParameterType(0).getTypeArguments().get(0); + Assertions.assertEquals(Type.getType("Ljava/lang/Object;"), typeArgument.getType()); + Assertions.assertEquals('+', typeArgument.getWildcard()); + } + + @Test + void singleParameterTypeWithSuper() { + SignatureParser parser = + new SignatureParser(FindBugsASM.ASM_VERSION, "(Ljava/util/Set<-Ljava/lang/Object;>;)V"); + TypeArgumentWithType typeArgument = + (TypeArgumentWithType) parser.getParameterType(0).getTypeArguments().get(0); + Assertions.assertEquals(Type.getType("Ljava/lang/Object;"), typeArgument.getType()); + Assertions.assertEquals('-', typeArgument.getWildcard()); + } + } + + @Test + void complexCase1() { + SignatureParser parser = + new SignatureParser( + FindBugsASM.ASM_VERSION, + "(LMultiplePathsToTypeVariable$TBounded;)Ljava/lang/Object;"); + List typeArguments = parser.getParameterType(0).getTypeArguments(); + Assertions.assertEquals(2, typeArguments.size()); + Assertions.assertEquals( + "T", ((TypeArgumentWithTypeVariable) typeArguments.get(0)).getTypeVariable()); + Assertions.assertEquals('=', typeArguments.get(0).getWildcard()); + Assertions.assertEquals( + "T", ((TypeArgumentWithTypeVariable) typeArguments.get(0)).getTypeVariable()); + Assertions.assertEquals('+', typeArguments.get(1).getWildcard()); + } + + @Test + void complexCase2() { + SignatureParser parser = + new SignatureParser( + FindBugsASM.ASM_VERSION, + "(LNotNullMarkedAnnotatedInnerOfParameterized.Nested;LNotNullMarkedAnnotatedInnerOfParameterized<*>.Nested;LNotNullMarkedAnnotatedInnerOfParameterized<*>.Nested;LNotNullMarkedAnnotatedInnerOfParameterized<*>.Nested.DoublyNested;LNotNullMarkedAnnotatedInnerOfParameterized<*>.Nested.DoublyNested;LNotNullMarkedAnnotatedInnerOfParameterized<*>.Nested.DoublyNested;LNotNullMarkedAnnotatedInnerOfParameterized$Lib.Nested.DoublyNested;>;LNotNullMarkedAnnotatedInnerOfParameterized$Lib.Nested.DoublyNested;>;LNotNullMarkedAnnotatedInnerOfParameterized$Lib.Nested.DoublyNested;>;)V"); + } + + @Test + void complexCase3() { + SignatureParser parser = + new SignatureParser(FindBugsASM.ASM_VERSION, "(LArraySameType$Lib<[Ljava/lang/Object;>;)V"); + TypeArgumentWithType typeArgument = + (TypeArgumentWithType) parser.getParameterType(0).getTypeArguments().get(0); + Assertions.assertEquals( + Type.getType("Ljava/lang/Object;"), typeArgument.getType().getElementType()); + } +} From 3808b68b82cab5f82378b5f0fe502347ff344a3d Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Thu, 7 Apr 2022 06:19:42 +0800 Subject: [PATCH 09/15] feat: support cases in the EnumAnnotations.java --- .../nullness/NeedlessAnnotationDetector.java | 49 ++++++++++--------- .../spotbugs/jspecify/nullness/Nullness.java | 4 ++ 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java index 4f56bcc..2f34ae1 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java @@ -23,10 +23,9 @@ import edu.umd.cs.findbugs.ba.XClass; import edu.umd.cs.findbugs.classfile.*; import edu.umd.cs.findbugs.classfile.engine.asm.FindBugsASM; -import edu.umd.cs.findbugs.internalAnnotations.SlashedClassName; import java.lang.invoke.MethodHandles; import java.lang.reflect.Modifier; -import java.util.Set; +import java.util.Objects; import org.jspecify.nullness.NullMarked; import org.jspecify.nullness.Nullable; import org.objectweb.asm.AnnotationVisitor; @@ -39,33 +38,16 @@ public class NeedlessAnnotationDetector extends ClassNodeDetector { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - private final Set INTRINSICALLY_NOT_NULLABLE_TYPES = - Set.of( - Type.BOOLEAN_TYPE, - Type.BYTE_TYPE, - Type.CHAR_TYPE, - Type.INT_TYPE, - Type.LONG_TYPE, - Type.FLOAT_TYPE, - Type.DOUBLE_TYPE); - @Nullable private Nullness nullness; - @SlashedClassName @Nullable private ClassDescriptor classDescriptor; + @Nullable private ClassDescriptor classDescriptor; public NeedlessAnnotationDetector(BugReporter bugReporter) { super(bugReporter); } + /** Return true if given type can be null. */ private boolean canBeNull(Type type) { - final XClass clazz; - try { - clazz = Global.getAnalysisCache().getClassAnalysis(XClass.class, classDescriptor); - } catch (CheckedAnalysisException e) { - bugReporter.reportMissingClass(classDescriptor); - return false; - } - return !INTRINSICALLY_NOT_NULLABLE_TYPES.contains(type) - && !clazz.getSuperclassDescriptor().matches(Enum.class); + return type.getSort() > Type.DOUBLE; } @Override @@ -126,12 +108,33 @@ class FieldVisitor extends org.objectweb.asm.FieldVisitor { this.fieldDescriptor = fieldDescriptor; } + /** Return true if visiting firld is an Enum field like {@code enum Foo {FOO;} } */ + private boolean isEnumField() { + if (!fieldDescriptor.isStatic()) { + return false; + } + + try { + XClass clazz = Global.getAnalysisCache().getClassAnalysis(XClass.class, classDescriptor); + if (!clazz.getSuperclassDescriptor().matches(Enum.class)) { + return false; + } + } catch (CheckedAnalysisException e) { + bugReporter.reportMissingClass(classDescriptor); + return false; + } + + return Objects.equals( + Type.getType(fieldDescriptor.getSignature()), + Type.getType(classDescriptor.getSignature())); + } + @Override public void visitEnd() { super.visitEnd(); Nullness nullnessOfReturnedValue = nullness == null ? defaultNullness : nullness; Type type = Type.getType(fieldDescriptor.getSignature()); - if (!canBeNull(type) + if ((isEnumField() || !canBeNull(type)) && nullnessOfReturnedValue.isSetExplicitly() && nullnessOfReturnedValue != Nullness.NOT_NULL) { log.info("{} is annotated as nullable, but {} cannot be null", fieldDescriptor, type); diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/Nullness.java b/src/main/java/com/github/spotbugs/jspecify/nullness/Nullness.java index f305b68..7aaf107 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/Nullness.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/Nullness.java @@ -61,6 +61,10 @@ public static Optional from(@Nullable String descriptor) { case "org/jspecify/nullness/Nullable": case "org.jspecify.nullness.Nullable": return Optional.of(NULLABLE); + case "Lorg/jspecify/nullness/NullnessUnspecified;": + case "org/jspecify/nullness/NullnessUnspecified": + case "org.jspecify.nullness.NullnessUnspecified": + return Optional.of(UNKNOWN); default: return Optional.empty(); } From b2a8a6d32a6459080cf51249b9810f111046e460 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Thu, 7 Apr 2022 06:38:10 +0800 Subject: [PATCH 10/15] feat: support cases in the AnnotatedReceiver.java --- .../jspecify/nullness/NeedlessAnnotationDetector.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java index 2f34ae1..5c083a7 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java @@ -243,6 +243,16 @@ private Type getAnnotated(TypeReference typeRef, TypePath typePath) { public AnnotationVisitor visitTypeAnnotation( int typeRef, TypePath typePath, String descriptor, boolean visible) { TypeReference typeRefObj = new TypeReference(typeRef); + if (typeRefObj.getSort() == TypeReference.METHOD_RECEIVER) { + if (Nullness.from(descriptor).map(Nullness::isSetExplicitly).orElse(Boolean.FALSE)) { + bugReporter.reportBug( + new BugInstance( + "JSPECIFY_NULLNESS_INTRINSICALLY_NOT_NULLABLE", Priorities.HIGH_PRIORITY) + .addType(classDescriptor.getSignature()) + .addClassAndMethod(methodDescriptor)); + } + return super.visitTypeAnnotation(typeRef, typePath, descriptor, visible); + } Type annotated = getAnnotated(typeRefObj, typePath); if (annotated != null && annotated.getSort() <= Type.DOUBLE From a0bf55e8a226d9773048b9bf832a6001a25081a0 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Thu, 7 Apr 2022 18:55:19 +0800 Subject: [PATCH 11/15] test: enable parallel test execution --- src/test/resources/junit-platform.properties | 1 + 1 file changed, 1 insertion(+) create mode 100644 src/test/resources/junit-platform.properties diff --git a/src/test/resources/junit-platform.properties b/src/test/resources/junit-platform.properties new file mode 100644 index 0000000..2115a2f --- /dev/null +++ b/src/test/resources/junit-platform.properties @@ -0,0 +1 @@ +junit.jupiter.execution.parallel.enabled = true From f49bb675b2012e1dda18fe0e700aca121d52b124 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Thu, 7 Apr 2022 18:56:53 +0800 Subject: [PATCH 12/15] chore: adjust log levels --- .../nullness/NeedlessAnnotationDetector.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java index 5c083a7..7208ea8 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java @@ -137,7 +137,7 @@ public void visitEnd() { if ((isEnumField() || !canBeNull(type)) && nullnessOfReturnedValue.isSetExplicitly() && nullnessOfReturnedValue != Nullness.NOT_NULL) { - log.info("{} is annotated as nullable, but {} cannot be null", fieldDescriptor, type); + log.debug("{} is annotated as nullable, but {} cannot be null", fieldDescriptor, type); bugReporter.reportBug( new BugInstance( "JSPECIFY_NULLNESS_INTRINSICALLY_NOT_NULLABLE", Priorities.HIGH_PRIORITY) @@ -202,7 +202,7 @@ private Type getAnnotated(TypeReference typeRef, TypePath typePath) { // means the type of parameter is directly annotated, like `@Nullness int` return Type.getArgumentTypes(methodDescriptor.getSignature())[typeParamIndex]; } - System.err.println("method is " + methodDescriptor + ", signature is " + signature); + log.debug("method is {}, signature is {}", methodDescriptor, signature); ParameterType param = ParameterTypeFinder.create(api, signature, methodDescriptor) .getParameterType(typeParamIndex); @@ -217,6 +217,7 @@ private Type getAnnotated(TypeReference typeRef, TypePath typePath) { case TypePath.ARRAY_ELEMENT: // means an element of array is annotated, like `@Nullness int[]` + // TODO support nested array like int[][] if (tempType instanceof TypeArgumentWithType) { return ((TypeArgumentWithType) tempType).getType().getElementType(); } @@ -254,9 +255,7 @@ public AnnotationVisitor visitTypeAnnotation( return super.visitTypeAnnotation(typeRef, typePath, descriptor, visible); } Type annotated = getAnnotated(typeRefObj, typePath); - if (annotated != null - && annotated.getSort() <= Type.DOUBLE - && Nullness.from(descriptor).isPresent()) { + if (annotated != null && !canBeNull(annotated) && Nullness.from(descriptor).isPresent()) { bugReporter.reportBug( new BugInstance( "JSPECIFY_NULLNESS_INTRINSICALLY_NOT_NULLABLE", Priorities.HIGH_PRIORITY) @@ -271,7 +270,7 @@ public AnnotationVisitor visitParameterAnnotation( int parameter, String descriptor, boolean visible) { Type[] types = Type.getArgumentTypes(methodDescriptor.getSignature()); Type type = types[parameter]; - log.info( + log.debug( "visitParameterAnnotation: {} method parameter ({}) is type {} and annotated with {}", methodDescriptor, parameter, @@ -288,7 +287,7 @@ public void visitEnd() { if (!canBeNull(returnType) && nullnessOfReturnedValue.isSetExplicitly() && nullnessOfReturnedValue != Nullness.NOT_NULL) { - log.info( + log.debug( "{} is annotated as nullable, but {} cannot be null", methodDescriptor, returnType); bugReporter.reportBug( new BugInstance( From 2cea9c657bdad664967b557448dbce96de8d51a6 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Thu, 7 Apr 2022 18:57:31 +0800 Subject: [PATCH 13/15] feat: support cases in the PrimitiveAnnotations.java --- .../jspecify/nullness/NeedlessAnnotationDetector.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java index 7208ea8..c64f894 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java @@ -260,7 +260,9 @@ public AnnotationVisitor visitTypeAnnotation( new BugInstance( "JSPECIFY_NULLNESS_INTRINSICALLY_NOT_NULLABLE", Priorities.HIGH_PRIORITY) .addType(annotated.getDescriptor()) - .addClassAndMethod(methodDescriptor)); + .addClassAndMethod(methodDescriptor) + .addParameterAnnotation( + typeRefObj.getTypeParameterIndex(), "the annotated parameter")); } return super.visitTypeAnnotation(typeRef, typePath, descriptor, visible); } From f866f06bc55a0402a146ab1f867b3f1518ebcee8 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Thu, 7 Apr 2022 20:17:45 +0800 Subject: [PATCH 14/15] feat: support cases in the UnrecognizedLocationsMisc.java --- .../nullness/NeedlessAnnotationDetector.java | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java index c64f894..191ec6e 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/NeedlessAnnotationDetector.java @@ -26,6 +26,7 @@ import java.lang.invoke.MethodHandles; import java.lang.reflect.Modifier; import java.util.Objects; +import org.apache.bcel.Const; import org.jspecify.nullness.NullMarked; import org.jspecify.nullness.Nullable; import org.objectweb.asm.AnnotationVisitor; @@ -80,6 +81,44 @@ public MethodVisitor visitMethod( nullness == null ? Nullness.NO_EXPLICIT_CONFIG : nullness); } + @Override + public AnnotationVisitor visitTypeAnnotation( + int typeRef, TypePath typePath, String descriptor, boolean visible) { + TypeReference typeRefObj = new TypeReference(typeRef); + switch (typeRefObj.getSort()) { + case TypeReference.CLASS_EXTENDS: + if (Nullness.from(descriptor).isPresent() && typePath == null) { + try { + int index = typeRefObj.getSuperTypeIndex(); + ClassDescriptor annotated; + if (index == -1) { + // TODO improve bug description to tell the problem intuitive + annotated = classDescriptor.getXClass().getSuperclassDescriptor(); + log.warn( + "The superclass, {}, is wrongly annotated with a nullness annotation in the class {}", + annotated, + classDescriptor); + } else { + // TODO improve bug description to tell the problem intuitive + annotated = classDescriptor.getXClass().getInterfaceDescriptorList()[index]; + log.warn( + "One of interfaces, {}, is w with a nullness annotation in the class {}", + annotated, + classDescriptor); + } + bugReporter.reportBug( + new BugInstance( + "JSPECIFY_NULLNESS_INTRINSICALLY_NOT_NULLABLE", Priorities.HIGH_PRIORITY) + .addType(annotated) + .addClass(classDescriptor)); + } catch (CheckedAnalysisException e) { + log.error("Could not get XClass from " + classDescriptor, e); + } + } + } + return super.visitTypeAnnotation(typeRef, typePath, descriptor, visible); + } + @Override public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) { Nullness.from(descriptor) @@ -244,7 +283,10 @@ private Type getAnnotated(TypeReference typeRef, TypePath typePath) { public AnnotationVisitor visitTypeAnnotation( int typeRef, TypePath typePath, String descriptor, boolean visible) { TypeReference typeRefObj = new TypeReference(typeRef); - if (typeRefObj.getSort() == TypeReference.METHOD_RECEIVER) { + if (typeRefObj.getSort() == TypeReference.METHOD_RECEIVER + || (Const.CONSTRUCTOR_NAME.equals(methodDescriptor.getName()) + && typeRefObj.getSort() == TypeReference.METHOD_RETURN)) { + // TODO make bug description intuitive if (Nullness.from(descriptor).map(Nullness::isSetExplicitly).orElse(Boolean.FALSE)) { bugReporter.reportBug( new BugInstance( From 3baa9271a1696f4fccaab420b91c86d972fb1095 Mon Sep 17 00:00:00 2001 From: Kengo TODA Date: Fri, 8 Apr 2022 06:28:40 +0800 Subject: [PATCH 15/15] test: reproduce known issue in the SignatureParser --- .../nullness/asm/ParameterTypeFinder.java | 15 ++++---- .../nullness/asm/TypeArgumentWithType.java | 7 +++- .../asm/TypeArgumentWithTypeVariable.java | 11 ++++++ .../nullness/asm/ParameterTypeFinderTest.java | 34 +++++++++++++++++++ 4 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 src/test/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinderTest.java diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinder.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinder.java index fb9cd66..652bc4a 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinder.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinder.java @@ -33,10 +33,10 @@ public static ParameterTypeFinder create( public abstract ParameterType getParameterType(int argument); - private static class BasedOnSignature extends ParameterTypeFinder { + static class BasedOnSignature extends ParameterTypeFinder { private final SignatureParser signatureParser; - public BasedOnSignature(SignatureParser signatureParser) { + BasedOnSignature(SignatureParser signatureParser) { this.signatureParser = Objects.requireNonNull(signatureParser); } @@ -46,12 +46,15 @@ public ParameterType getParameterType(int argument) { } } - private static class BasedOnMethodDescriptor extends ParameterTypeFinder { + static class BasedOnMethodDescriptor extends ParameterTypeFinder { private final Type[] argumentTypes; - public BasedOnMethodDescriptor(MethodDescriptor methodDescriptor) { - this.argumentTypes = - Type.getArgumentTypes(Objects.requireNonNull(methodDescriptor).getSignature()); + BasedOnMethodDescriptor(String signature) { + this.argumentTypes = Type.getArgumentTypes(Objects.requireNonNull(signature)); + } + + BasedOnMethodDescriptor(MethodDescriptor methodDescriptor) { + this(Objects.requireNonNull(methodDescriptor).getSignature()); } @Override diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithType.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithType.java index 70587f2..a614f3c 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithType.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithType.java @@ -22,7 +22,7 @@ public final class TypeArgumentWithType extends TypeArgument { private final Type type; - TypeArgumentWithType(char wildcard, String name) { + public TypeArgumentWithType(char wildcard, String name) { super(wildcard); this.type = Type.getType(Objects.requireNonNull(name)); } @@ -61,4 +61,9 @@ public boolean equals(Object o) { public int hashCode() { return type != null ? type.hashCode() : 0; } + + @Override + public String toString() { + return "TypeArgumentWithType{" + "wildcard=" + getWildcard() + ", type=" + type + '}'; + } } diff --git a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithTypeVariable.java b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithTypeVariable.java index b889e9e..288bd2e 100644 --- a/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithTypeVariable.java +++ b/src/main/java/com/github/spotbugs/jspecify/nullness/asm/TypeArgumentWithTypeVariable.java @@ -57,4 +57,15 @@ public int hashCode() { result = 31 * result + (typeVariable != null ? typeVariable.hashCode() : 0); return result; } + + @Override + public String toString() { + return "TypeArgumentWithTypeVariable{" + + "wildcard=" + + getWildcard() + + ", typeVariable='" + + typeVariable + + '\'' + + '}'; + } } diff --git a/src/test/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinderTest.java b/src/test/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinderTest.java new file mode 100644 index 0000000..f21308c --- /dev/null +++ b/src/test/java/com/github/spotbugs/jspecify/nullness/asm/ParameterTypeFinderTest.java @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2021-2022 The SpotBugs team. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.github.spotbugs.jspecify.nullness.asm; + +import edu.umd.cs.findbugs.classfile.engine.asm.FindBugsASM; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +class ParameterTypeFinderTest { + @Test + void complexCase1() { + String signature = + "(LCaptureConvertedTypeVariableBounded$ImplicitlyObjectBounded.Inner<+Ljava/lang/Object;>;)Ljava/lang/Object;"; + ParameterTypeFinder.BasedOnSignature finder = + new ParameterTypeFinder.BasedOnSignature( + new SignatureParser(FindBugsASM.ASM_VERSION, signature)); + Assertions.assertEquals( + new TypeArgumentWithType('+', "Ljava/lang/Object;"), + finder.getParameterType(0).getTypeArguments().get(0)); + } +}