diff --git a/.github/dependabot.yml b/.github/dependabot.yml index ea03eb6b..b947cc75 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -9,3 +9,13 @@ updates: directory: "/" schedule: interval: "daily" + +- package-ecosystem: "gradle" + directory: "/conformance-test-framework" + schedule: + interval: "daily" + +- package-ecosystem: "gradle" + directory: "/usage-demo" + schedule: + interval: "daily" diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index bbfbbd64..b22f6f9e 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -19,6 +19,12 @@ jobs: with: repository: jspecify/jspecify path: jspecify +# Clone the EISOP CF if necessary: +# - name: Check out eisop/checker-framework +# uses: actions/checkout@v4 +# with: +# repository: eisop/checker-framework +# path: checker-framework - name: Set up Java uses: actions/setup-java@v4 with: @@ -28,17 +34,22 @@ jobs: uses: gradle/gradle-build-action@v3 - name: Build and Test run: ./gradlew build conformanceTests demoTest --include-build ../jspecify +# If a cloned EISOP CF is needed, use the following: +# run: ./gradlew build conformanceTests demoTest --include-build ../jspecify --include-build ../checker-framework env: SHALLOW: 1 - - name: Check out jspecify/samples-google-prototype + JSPECIFY_CONFORMANCE_TEST_MODE: details + - name: Check out jspecify/samples-google-prototype-eisop if: always() run: | - git fetch --depth=1 origin samples-google-prototype - git checkout samples-google-prototype + git fetch --depth=1 origin samples-google-prototype-eisop + git checkout samples-google-prototype-eisop working-directory: jspecify - name: Run Samples Tests if: always() run: ./gradlew jspecifySamplesTest --include-build ../jspecify +# If a cloned EISOP CF is needed, use the following: +# run: ./gradlew jspecifySamplesTest --include-build ../jspecify --include-build ../checker-framework publish-snapshot: name: Publish Conformance Test Framework Snapshot diff --git a/README.md b/README.md index 4b8b8e88..f2129242 100644 --- a/README.md +++ b/README.md @@ -17,13 +17,13 @@ But that's its *only* job. Notably, it is: ## Relationship to Checker Framework and EISOP -The [EISOP project](https://eisop.github.io/) maintains a fork of [Checker Framework](https://checkerframework.org/), and JSpecify conformance is one of its primary goals. +The [EISOP project](https://eisop.github.io/) maintains a fork of the [Checker Framework](https://checkerframework.org/), and JSpecify conformance is one of its goals. -This tool happens to be built on top of another fork of these ([here](https://github.com/jspecify/checker-framework)). However, please view this relationship as **implementation detail** only. Building a reference checker from scratch would simply have been too difficult, so we needed to base it on some existing tool. The choice of which tool was made purely for expediency and is **subject to change**. +This tool is built on top of the [EISOP Checker Framework](https://github.com/eisop/checker-framework). However, please view this relationship as **implementation detail** only. Building a reference checker from scratch would simply have been too difficult, so we needed to base it on some existing tool. The choice of which tool was made purely for expediency and is **subject to change**. ## Usage -Building and running this tool requires building code from several other repositories, but these instructions will take care of that automatically. +Building and running this tool depends on code from several other repositories, but these instructions will take care of that automatically. These instructions might require workarounds or fail outright. Please file an issue if you have any trouble! @@ -31,7 +31,7 @@ These instructions might require workarounds or fail outright. Please file an is Ideally set `JAVA_HOME` to a JDK 11 or JDK 17 installation. -Make sure you have Apache Maven installed and in your PATH, or the Gradle build will fail: +Make sure you have Apache Maven installed and in your PATH, or the `demo` script will fail: ```sh mvn diff --git a/build.gradle b/build.gradle index 491e4a27..1763cc57 100644 --- a/build.gradle +++ b/build.gradle @@ -8,13 +8,26 @@ plugins { id 'com.diffplug.spotless' version '6.25.0' id 'io.github.gradle-nexus.publish-plugin' version '1.3.0' - id 'net.ltgt.errorprone' version '3.0.1' + id 'net.ltgt.errorprone' version '4.1.0' + // To show task list as a tree, run: ./gradlew taskTree + id 'com.dorongold.task-tree' version '4.0.0' } // Nexus Publish plugin requires a group/version at the root project. group = 'org.jspecify.reference' version = '0.0.0-SNAPSHOT' +sourceSets { + main { + resources { + // Minimized jspecify/jdk + srcDirs += [ + "${buildDir}/generated/resources" + ] + } + } +} + repositories { mavenLocal() maven { @@ -35,10 +48,14 @@ nexusPublishing { } ext { - checkerFramework = gradle.includedBuild("checker-framework") + // null if not included with `--include-build path/to/checker-framework` + checkerFramework = gradle.includedBuilds.find { it.name == 'checker-framework' } // null if not included with `--include-build path/to/jspecify` jspecify = gradle.includedBuilds.find { it.name == 'jspecify' } + + // Location of the jspecify/jdk clone, relative to this directory + jspecifyJdkHome = '../jdk' } configurations { @@ -61,8 +78,10 @@ java { dependencies { implementation libs.checkerFramework.checker implementation libs.checkerFramework.checker.qual - implementation libs.checkerFramework.framework - implementation libs.checkerFramework.javacutil + // Eventually, we would want to only depend on `framework` and + // `javacutil` artifacts instead of the entire `checker`. + // implementation libs.checkerFramework.framework + // implementation libs.checkerFramework.javacutil implementation libs.jspecify @@ -78,8 +97,11 @@ dependencies { errorprone libs.errorProne.core } -// Assemble checker-framework when assembling the reference checker. -assemble.dependsOn(checkerFramework.task(":assemble")) +// If built with `--include-build path/to/checker-framework` then +// assemble checker-framework when assembling the reference checker. +if (checkerFramework != null) { + compileJava.dependsOn(checkerFramework.task(":checker:assembleForJavac")) +} // If built with `--include-build path/to/jspecify` then // assemble jspecify when assembling the reference checker. @@ -87,12 +109,19 @@ if (jspecify != null) { assemble.dependsOn(jspecify.task(':assemble')) } +// Enable exec/javaexec +interface InjectedExecOps { + @Inject + ExecOperations getExecOps() +} + tasks.withType(JavaCompile).configureEach { options.compilerArgs.add("-Xlint:all") // ErrorProne makes suppressing these easier options.compilerArgs.add("-Xlint:-fallthrough") options.errorprone.disable("BadImport") + options.errorprone.disable("VoidUsed") options.compilerArgs.addAll( [ @@ -111,6 +140,56 @@ tasks.withType(JavaCompile).configureEach { .collect { "--add-exports=jdk.compiler/com.sun.tools.javac.$it=ALL-UNNAMED" }) } +tasks.register('includeJSpecifyJDK') { + group = 'Build' + dependsOn 'compileJava' + + def srcDir = "${jspecifyJdkHome}/src" + // This directory needs to be stored at the top-level of the resulting .jar file. + // org.checkerframework.framework.stub.AnnotationFileElementTypes will then load + // the JDK classes from here instead of from checker.jar. + def dstDir = "${buildDir}/generated/resources/annotated-jdk/src/" + + inputs.dir file(srcDir) + outputs.dir file(dstDir) + + def injected = project.objects.newInstance(InjectedExecOps) + + doLast { + FileTree srcTree = fileTree(dir: srcDir) + NavigableSet specFiles = new TreeSet<>(); + srcTree.visit { FileVisitDetails fvd -> + if (!fvd.file.isDirectory() && fvd.file.name.matches('.*\\.java')) { + fvd.getFile().readLines().any { line -> + if (line.contains('org.jspecify')) { + specFiles.add(fvd.file.absolutePath) + return true; + } + } + } + } + String absoluteSrcDir = file(srcDir).absolutePath + int srcPrefixSize = absoluteSrcDir.size() + copy { + from(srcDir) + into(dstDir) + for (String specFile : specFiles) { + include specFile.substring(srcPrefixSize) + } + } + injected.execOps.javaexec { + classpath = sourceSets.main.runtimeClasspath + standardOutput = System.out + errorOutput = System.err + + mainClass = 'org.checkerframework.framework.stub.JavaStubifier' + args dstDir + } + } +} + +processResources.dependsOn(includeJSpecifyJDK) + tasks.withType(Test).configureEach { if (!JavaVersion.current().java9Compatible) { jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" @@ -132,14 +211,16 @@ tasks.withType(Test).configureEach { showStackTraces = false showStandardStreams = true events "failed" - exceptionFormat "full" + exceptionFormat = "full" } } test { include '**/NullSpecTest$Minimal.class' - inputs.files("${rootDir}/tests/minimal") + + include '**/NullSpecTest$Regression.class' + inputs.files("${rootDir}/tests/regression") } tasks.register('jspecifySamplesTest', Test) { @@ -217,24 +298,28 @@ tasks.register('demoTest', Exec) { See https://github.com/jspecify/jspecify-reference-checker/issues/81 */ -def cfQualJar = - checkerFramework.projectDir.toPath() - .resolve("checker-qual/build/libs/checker-qual-${libs.versions.checkerFramework.get()}.jar") - -if (!cfQualJar.toFile().exists()) { - mkdir(cfQualJar.parent) - exec { - executable 'jar' - args = [ - 'cf', - cfQualJar, - buildFile.path // Use this build script file! - ] +if (checkerFramework != null) { + def cfQualJar = + checkerFramework.projectDir.toPath() + .resolve("checker-qual/build/libs/checker-qual-${libs.versions.checkerFramework.get()}.jar") + def injected = project.objects.newInstance(InjectedExecOps) + + if (!cfQualJar.toFile().exists()) { + mkdir(cfQualJar.parent) + injected.execOps.exec { + executable 'jar' + args = [ + 'cf', + cfQualJar, + buildFile.path // Use this build script file! + ] + } } } spotless { java { + target '**/*.java' googleJavaFormat() formatAnnotations() } @@ -260,3 +345,47 @@ eclipse.classpath { } } } + +publishing { + publications { + jspecifyReferenceChecker(MavenPublication) { + pom { + groupId = 'org.jspecify.reference' + artifactId = 'checker' + version = project.version + name = 'JSpecify Reference Checker' + description = 'The JSpecify Reference Checker' + url = 'http://jspecify.org/' + from components.java + licenses { + license { + name = 'The Apache License, Version 2.0' + url = 'http://www.apache.org/licenses/LICENSE-2.0.txt' + } + } + scm { + connection = 'scm:git:git@github.com:jspecify/jspecify-reference-checker.git' + developerConnection = 'scm:git:git@github.com:jspecify/jspecify-reference-checker.git' + url = 'https://github.com/jspecify/jspecify-reference-checker' + } + developers { + developer { + id = 'cpovirk' + name = 'Chris Povirk' + email = 'cpovirk@google.com' + } + developer { + id = 'netdpb' + name = 'David P. Baker' + email = 'dpb@google.com' + } + developer { + id = 'wmdietl' + name = 'Werner M. Dietl' + email = 'wdietl@gmail.com' + } + } + } + } + } +} diff --git a/conformance-test-framework/build.gradle b/conformance-test-framework/build.gradle index 633ff854..2065f9ff 100644 --- a/conformance-test-framework/build.gradle +++ b/conformance-test-framework/build.gradle @@ -2,8 +2,8 @@ plugins { id 'java-library' } -group 'org.jspecify.conformance' -version '0.0.0-SNAPSHOT' +group = 'org.jspecify.conformance' +version = '0.0.0-SNAPSHOT' repositories { mavenCentral() diff --git a/demo b/demo index da51b135..496f0b5f 100755 --- a/demo +++ b/demo @@ -15,6 +15,20 @@ if [ ! -e "${jspecify}" ]; then -DoutputDirectory="$(dirname "${jspecify}")" fi fi + +checkerFrameworkDir="${dir}/../checker-framework/" +checkerFrameworkJar="${checkerFrameworkDir}/checker/dist/checker.jar" +if [ ! -e "${checkerFrameworkJar}" ]; then + cfVersion="3.42.0-eisop4" + checkerFrameworkJar="${dir}/build/checker-${cfVersion}-all.jar" + if [ ! -e "${checkerFrameworkJar}" ]; then + echo "Downloading $(basename "${checkerFrameworkJar}") from Maven central" + mvn -q org.apache.maven.plugins:maven-dependency-plugin:3.7.1:copy \ + -Dartifact="io.github.eisop:checker:${cfVersion}:jar:all" \ + -DoutputDirectory="$(dirname "${checkerFrameworkJar}")" + fi +fi + jspecify_reference_checker="${dir}/build/libs/jspecify-reference-checker-0.0.0-SNAPSHOT.jar" if [ ! -e "${jspecify_reference_checker}" ]; then echo "Assembling jspecify-reference-checker" @@ -25,9 +39,11 @@ ourclasspath="${jspecify}:${jspecify_reference_checker}" export CLASSPATH="${ourclasspath}:$CLASSPATH" -$dir/../checker-framework/checker/bin/javac \ +java -jar "${checkerFrameworkJar}" \ -processorpath "${ourclasspath}" \ -processor com.google.jspecify.nullness.NullSpecChecker \ + -checkerQualJar "${checkerFrameworkJar}" \ + -checkerUtilJar "${checkerFrameworkJar}" \ -AcheckImpl \ -AassumePure \ -AsuppressWarnings=contracts.conditional.postcondition.false.methodref,contracts.conditional.postcondition.false.override,contracts.conditional.postcondition.true.methodref,contracts.conditional.postcondition.true.override,purity.methodref,purity.overriding,type.anno.before.decl.anno,type.anno.before.modifier \ diff --git a/docs/development.md b/docs/development.md index 3999d396..74fb3d2d 100644 --- a/docs/development.md +++ b/docs/development.md @@ -1,14 +1,14 @@ # Development -## Codevelopment with Checker Framework fork +## Codevelopment with the EISOP Checker Framework -This project depends on -an [unreleased fork of the Checker Framework][jspecify-checker-framework]. -(The [main-eisop branch] represents ongoing work to depend on a released version -of the [EISOP] fork instead.) +This project depends on the [EISOP Checker Framework][EISOP]. + +To codevelop changes with the EISOP Checker Framework, clone it into the +sibling directory `../checker-framwork` and pass +`--include-build path/to/checker-framework` to Gradle when building +this project. -Because of that dependency, this build clones that unreleased fork into the -sibling directory `../checker-framwork`. _That_ build then clones some other projects into other sibling directories. It expects `../jdk` to contain an annotated JDK, so our build clones [JSpecify's][jspecify-jdk] there. @@ -29,7 +29,7 @@ clone the repo (or your fork) somewhere, and pass `--include-build path/to/jspecify` to Gradle when building. The local clone will be used for both the annotations and the conformance test suite. -By default the reference checker depends on version `0.3.0` of the annotations, +By default the reference checker depends on version `1.0.0` of the annotations, and version `0.0.0-SNAPSHOT` of the conformance test suite. In order to depend on a different published version of either artifact, set @@ -40,7 +40,6 @@ Gradle properties on the command line. of the conformance test suite. [EISOP]: https://github.com/eisop/checker-framework -[jspecify-checker-framework]: https://github.com/jspecify/checker-framework [jspecify-jdk]: https://github.com/jspecify/jdk [jspecify-jspecify]: https://github.com/jspecify/jspecify [main-eisop branch]: https://github.com/jspecify/jspecify-reference-checker/tree/main-eisop diff --git a/gradle.properties b/gradle.properties index 412032de..7315fa5f 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,3 +1,3 @@ -org.jspecify\:jspecify=0.3.0 +org.jspecify\:jspecify=1.0.0 org.jspecify.conformance\:conformance-test-framework=0.0.0-SNAPSHOT org.jspecify.conformance\:conformance-tests=0.0.0-SNAPSHOT diff --git a/gradle/wrapper/gradle-wrapper.jar b/gradle/wrapper/gradle-wrapper.jar index d64cd491..a4b76b95 100644 Binary files a/gradle/wrapper/gradle-wrapper.jar and b/gradle/wrapper/gradle-wrapper.jar differ diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index a80b22ce..cea7a793 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,6 +1,6 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-8.6-bin.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-8.12-bin.zip networkTimeout=10000 validateDistributionUrl=true zipStoreBase=GRADLE_USER_HOME diff --git a/gradlew b/gradlew index 1aa94a42..f3b75f3b 100755 --- a/gradlew +++ b/gradlew @@ -15,6 +15,8 @@ # See the License for the specific language governing permissions and # limitations under the License. # +# SPDX-License-Identifier: Apache-2.0 +# ############################################################################## # @@ -55,7 +57,7 @@ # Darwin, MinGW, and NonStop. # # (3) This script is generated from the Groovy template -# https://github.com/gradle/gradle/blob/HEAD/subprojects/plugins/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt +# https://github.com/gradle/gradle/blob/HEAD/platforms/jvm/plugins-application/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt # within the Gradle project. # # You can find Gradle at https://github.com/gradle/gradle/. @@ -84,7 +86,7 @@ done # shellcheck disable=SC2034 APP_BASE_NAME=${0##*/} # Discard cd standard output in case $CDPATH is set (https://github.com/gradle/gradle/issues/25036) -APP_HOME=$( cd "${APP_HOME:-./}" > /dev/null && pwd -P ) || exit +APP_HOME=$( cd -P "${APP_HOME:-./}" > /dev/null && printf '%s\n' "$PWD" ) || exit # Use the maximum available, or set MAX_FD != -1 to use that value. MAX_FD=maximum diff --git a/gradlew.bat b/gradlew.bat index 7101f8e4..9b42019c 100644 --- a/gradlew.bat +++ b/gradlew.bat @@ -13,6 +13,8 @@ @rem See the License for the specific language governing permissions and @rem limitations under the License. @rem +@rem SPDX-License-Identifier: Apache-2.0 +@rem @if "%DEBUG%"=="" @echo off @rem ########################################################################## diff --git a/initialize-project b/initialize-project index c8783b63..5f04d589 100755 --- a/initialize-project +++ b/initialize-project @@ -11,7 +11,7 @@ # Set SHALLOW=1 to clone sibling projects at depth 1. # # This script automatically tries to download your fork of -# jspecify/checker-framework, jspecify/jspecify, or jspecify/jdk, if they exist. +# eisop/checker-framework, jspecify/jspecify, or jspecify/jdk, if they exist. # It uses the URL of the origin remote (the default remote created when cloning # a repo) to determine that. # @@ -19,8 +19,6 @@ # that contains your forks. For example, FORK=git@github.com:myorg means this # script tries to clone the following before falling back to the JSpecify repos: # -# git@github:myorg/checker-framework.git -# git@github:myorg/jspecify.git # git@github:myorg/jdk.git set -eu @@ -55,14 +53,19 @@ git_clone() { local forking_org forking_org="$(forking_org)" - if [[ -n "${forking_org}" ]]; then + if [[ -n "${forking_org}" ]] \ + && [[ "${forking_org}" != "https://github.com/jspecify" ]] \ + && [[ "${forking_org}" != "git@github.com:jspecify" ]] ; then if run "${git[@]}" "${forking_org}/${repo}.git" "../${repo}"; then return fi fi - run "${git[@]}" "https://github.com/jspecify/${repo}.git" "../${repo}" + if [[ "${repo}" == "checker-framework" ]]; then + forking_org=https://github.com/eisop + else + forking_org=https://github.com/jspecify + fi + run "${git[@]}" "${forking_org}/${repo}.git" "../${repo}" } git_clone jdk --depth 1 --single-branch - -git_clone checker-framework diff --git a/settings.gradle b/settings.gradle index a440bde8..b40f21b7 100644 --- a/settings.gradle +++ b/settings.gradle @@ -6,23 +6,21 @@ include 'conformance-test-framework' // See https://docs.gradle.org/current/userguide/composite_builds.html#included_build_declaring_substitutions includeBuild(".") -exec { +providers.exec { executable './initialize-project' -} - -includeBuild("../checker-framework") +}.result.get() dependencyResolutionManagement { versionCatalogs { libs { - version("checkerFramework", "3.21.5-SNAPSHOT") - - library("checkerFramework-checker", "org.checkerframework", "checker").versionRef("checkerFramework") - library("checkerFramework-checker-qual", "org.checkerframework", "checker-qual").versionRef("checkerFramework") - library("checkerFramework-framework", "org.checkerframework", "framework").versionRef("checkerFramework") - library("checkerFramework-framework-test", "org.checkerframework", "framework-test").versionRef("checkerFramework") - library("checkerFramework-javacutil", "org.checkerframework", "javacutil").versionRef("checkerFramework") - library("errorProne-core", "com.google.errorprone:error_prone_core:2.18.0") + version("checkerFramework", "3.42.0-eisop5") + + library("checkerFramework-checker", "io.github.eisop", "checker").versionRef("checkerFramework") + library("checkerFramework-checker-qual", "io.github.eisop", "checker-qual").versionRef("checkerFramework") + library("checkerFramework-framework", "io.github.eisop", "framework").versionRef("checkerFramework") + library("checkerFramework-framework-test", "io.github.eisop", "framework-test").versionRef("checkerFramework") + library("checkerFramework-javacutil", "io.github.eisop", "javacutil").versionRef("checkerFramework") + library("errorProne-core", "com.google.errorprone:error_prone_core:2.36.0") library("errorProne-javac", "com.google.errorprone:javac:9+181-r4173-1") library("guava", "com.google.guava:guava:31.1-jre") diff --git a/src/main/java/com/google/jspecify/nullness/ConformanceTypeInformationPresenter.java b/src/main/java/com/google/jspecify/nullness/ConformanceTypeInformationPresenter.java new file mode 100644 index 00000000..9bffb729 --- /dev/null +++ b/src/main/java/com/google/jspecify/nullness/ConformanceTypeInformationPresenter.java @@ -0,0 +1,104 @@ +// Copyright 2024 The JSpecify Authors +// +// 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.google.jspecify.nullness; + +import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import java.util.List; +import javax.lang.model.element.ExecutableElement; +import org.checkerframework.framework.type.AnnotatedTypeFactory; +import org.checkerframework.framework.type.AnnotatedTypeFormatter; +import org.checkerframework.framework.type.AnnotatedTypeMirror; +import org.checkerframework.framework.type.AnnotatedTypeMirror.AnnotatedExecutableType; +import org.checkerframework.framework.util.visualize.AbstractTypeInformationPresenter; +import org.checkerframework.framework.util.visualize.TypeOccurrenceKind; +import org.checkerframework.javacutil.TreeUtils; + +/** + * Output "sinkType" and "sourceType" diagnostic warning messages so the conformance tests can look + * for (a subset of) them. + */ +public final class ConformanceTypeInformationPresenter extends AbstractTypeInformationPresenter { + + /** + * Constructs a presenter for the given factory. + * + * @param atypeFactory the AnnotatedTypeFactory for the current analysis + */ + public ConformanceTypeInformationPresenter(AnnotatedTypeFactory atypeFactory) { + super(atypeFactory); + } + + @Override + protected AnnotatedTypeFormatter createTypeFormatter() { + // Use the same type formatter as normal error messages. Look into whether a different format + // would be better here. + return atypeFactory.getAnnotatedTypeFormatter(); + } + + @Override + protected TypeInformationReporter createTypeInformationReporter(ClassTree tree) { + return new ConformanceTypeInformationReporter(tree); + } + + class ConformanceTypeInformationReporter extends TypeInformationReporter { + ConformanceTypeInformationReporter(ClassTree tree) { + super(tree); + } + + @Override + protected void reportTreeType( + Tree tree, AnnotatedTypeMirror type, TypeOccurrenceKind occurrenceKind) { + switch (tree.getKind()) { + case ASSIGNMENT: + AssignmentTree asgn = (AssignmentTree) tree; + AnnotatedTypeMirror varType = + genFactory != null + ? genFactory.getAnnotatedTypeLhs(asgn.getVariable()) + : atypeFactory.getAnnotatedType(asgn.getVariable()); + checker.reportWarning( + asgn.getVariable(), + "sinkType", + typeFormatter.format(varType), + asgn.getVariable().toString()); + break; + case RETURN: + checker.reportWarning(tree, "sinkType", typeFormatter.format(type), "return"); + break; + case METHOD_INVOCATION: + ExecutableElement calledElem = TreeUtils.elementFromUse((MethodInvocationTree) tree); + String methodName = calledElem.getSimpleName().toString(); + AnnotatedExecutableType calledType = (AnnotatedExecutableType) type; + List params = calledType.getParameterTypes(); + + for (int i = 0; i < params.size(); ++i) { + String paramName = calledElem.getParameters().get(i).getSimpleName().toString(); + String paramLocation = String.format("%s#%s", methodName, paramName); + checker.reportWarning( + tree, "sinkType", typeFormatter.format(params.get(i)), paramLocation); + } + break; + default: + // Nothing special for other trees. + } + + if (TreeUtils.isExpressionTree(tree)) { + checker.reportWarning(tree, "sourceType", typeFormatter.format(type), tree.toString()); + } + } + } +} diff --git a/src/main/java/com/google/jspecify/nullness/NullSpecAnalysis.java b/src/main/java/com/google/jspecify/nullness/NullSpecAnalysis.java index 31871566..82476abb 100644 --- a/src/main/java/com/google/jspecify/nullness/NullSpecAnalysis.java +++ b/src/main/java/com/google/jspecify/nullness/NullSpecAnalysis.java @@ -14,12 +14,11 @@ package com.google.jspecify.nullness; -import java.util.Set; -import javax.lang.model.element.AnnotationMirror; import javax.lang.model.type.TypeMirror; import org.checkerframework.common.basetype.BaseTypeChecker; import org.checkerframework.framework.flow.CFAbstractAnalysis; import org.checkerframework.framework.flow.CFValue; +import org.checkerframework.javacutil.AnnotationMirrorSet; final class NullSpecAnalysis extends CFAbstractAnalysis { NullSpecAnalysis(BaseTypeChecker checker, NullSpecAnnotatedTypeFactory factory) { @@ -37,7 +36,7 @@ public NullSpecStore createCopiedStore(NullSpecStore other) { } @Override - public CFValue createAbstractValue(Set annotations, TypeMirror underlyingType) { + public CFValue createAbstractValue(AnnotationMirrorSet annotations, TypeMirror underlyingType) { return defaultCreateAbstractValue(this, annotations, underlyingType); } } diff --git a/src/main/java/com/google/jspecify/nullness/NullSpecAnnotatedTypeFactory.java b/src/main/java/com/google/jspecify/nullness/NullSpecAnnotatedTypeFactory.java index a5bed356..fc8202a4 100644 --- a/src/main/java/com/google/jspecify/nullness/NullSpecAnnotatedTypeFactory.java +++ b/src/main/java/com/google/jspecify/nullness/NullSpecAnnotatedTypeFactory.java @@ -14,10 +14,8 @@ package com.google.jspecify.nullness; -import static com.google.jspecify.nullness.NullSpecAnnotatedTypeFactory.IsDeclaredOrArray.IS_DECLARED_OR_ARRAY; -import static com.google.jspecify.nullness.Util.IMPLEMENTATION_VARIABLE_LOCATIONS; +import static com.google.jspecify.nullness.NullSpecAnnotatedTypeFactory.IsDeclaredOrArrayOrNull.IS_DECLARED_OR_ARRAY_OR_NULL; import static com.google.jspecify.nullness.Util.nameMatches; -import static com.sun.source.tree.Tree.Kind.CONDITIONAL_EXPRESSION; import static com.sun.source.tree.Tree.Kind.IDENTIFIER; import static com.sun.source.tree.Tree.Kind.MEMBER_SELECT; import static com.sun.source.tree.Tree.Kind.NOT_EQUAL_TO; @@ -31,17 +29,12 @@ import static javax.lang.model.element.ElementKind.ENUM_CONSTANT; import static javax.lang.model.type.TypeKind.ARRAY; import static javax.lang.model.type.TypeKind.DECLARED; +import static javax.lang.model.type.TypeKind.NULL; import static javax.lang.model.type.TypeKind.TYPEVAR; import static javax.lang.model.type.TypeKind.WILDCARD; -import static org.checkerframework.framework.qual.TypeUseLocation.CONSTRUCTOR_RESULT; -import static org.checkerframework.framework.qual.TypeUseLocation.EXCEPTION_PARAMETER; -import static org.checkerframework.framework.qual.TypeUseLocation.IMPLICIT_LOWER_BOUND; -import static org.checkerframework.framework.qual.TypeUseLocation.OTHERWISE; -import static org.checkerframework.framework.qual.TypeUseLocation.RECEIVER; +import static org.checkerframework.framework.util.AnnotatedTypes.areCorrespondingTypeVariables; import static org.checkerframework.framework.util.AnnotatedTypes.asSuper; -import static org.checkerframework.framework.util.defaults.QualifierDefaults.AdditionalTypeUseLocation.UNBOUNDED_WILDCARD_UPPER_BOUND; import static org.checkerframework.javacutil.AnnotationUtils.areSame; -import static org.checkerframework.javacutil.AnnotationUtils.areSameByName; import static org.checkerframework.javacutil.TreePathUtil.enclosingClass; import static org.checkerframework.javacutil.TreeUtils.elementFromDeclaration; import static org.checkerframework.javacutil.TreeUtils.elementFromUse; @@ -52,6 +45,7 @@ import com.sun.source.tree.BinaryTree; import com.sun.source.tree.ClassTree; +import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.LambdaExpressionTree; @@ -71,7 +65,6 @@ import java.util.Map; import java.util.Set; import java.util.function.Predicate; -import javax.lang.model.AnnotatedConstruct; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.Element; import javax.lang.model.element.ExecutableElement; @@ -83,6 +76,7 @@ import org.checkerframework.common.basetype.BaseTypeChecker; import org.checkerframework.framework.flow.CFAbstractAnalysis; import org.checkerframework.framework.flow.CFValue; +import org.checkerframework.framework.qual.DefaultQualifier; import org.checkerframework.framework.qual.TypeUseLocation; import org.checkerframework.framework.type.AnnotatedTypeFactory; import org.checkerframework.framework.type.AnnotatedTypeFormatter; @@ -110,14 +104,13 @@ import org.checkerframework.framework.type.typeannotator.TypeAnnotator; import org.checkerframework.framework.type.visitor.AnnotatedTypeVisitor; import org.checkerframework.framework.util.AnnotationFormatter; -import org.checkerframework.framework.util.Contract.ConditionalPostcondition; -import org.checkerframework.framework.util.Contract.Postcondition; -import org.checkerframework.framework.util.Contract.Precondition; import org.checkerframework.framework.util.ContractsFromMethod; import org.checkerframework.framework.util.DefaultAnnotationFormatter; import org.checkerframework.framework.util.DefaultQualifierKindHierarchy; +import org.checkerframework.framework.util.NoContractsFromMethod; import org.checkerframework.framework.util.QualifierKindHierarchy; import org.checkerframework.framework.util.defaults.QualifierDefaults; +import org.checkerframework.javacutil.AnnotationBuilder; final class NullSpecAnnotatedTypeFactory extends GenericAnnotatedTypeFactory< @@ -127,6 +120,7 @@ final class NullSpecAnnotatedTypeFactory private final AnnotationMirror minusNull; private final AnnotationMirror unionNull; private final AnnotationMirror nullnessOperatorUnspecified; + private final AnnotationMirror parametricNull; private final boolean isLeastConvenientWorld; private final NullSpecAnnotatedTypeFactory withLeastConvenientWorld; @@ -134,10 +128,60 @@ final class NullSpecAnnotatedTypeFactory private final AnnotatedDeclaredType javaUtilCollection; + private final ConformanceTypeInformationPresenter conformanceInformationPresenter; + final AnnotatedDeclaredType javaLangClass; final AnnotatedDeclaredType javaLangThreadLocal; final AnnotatedDeclaredType javaUtilMap; + // Ensure that all locations that appear in the `defaultLocations` also appear somewhere in the + // `nullMarkedLocations`. + // As defaults are added and removed to the same environment, we need to ensure that all values + // are correctly changed. + + private static final TypeUseLocation[] defaultLocationsMinusNull = + new TypeUseLocation[] { + TypeUseLocation.CONSTRUCTOR_RESULT, + TypeUseLocation.EXCEPTION_PARAMETER, + TypeUseLocation.IMPLICIT_LOWER_BOUND, + TypeUseLocation.RECEIVER, + }; + + private static final TypeUseLocation[] defaultLocationsUnionNull = + new TypeUseLocation[] { + TypeUseLocation.IMPLICIT_WILDCARD_UPPER_BOUND_SUPER, + TypeUseLocation.LOCAL_VARIABLE, + TypeUseLocation.RESOURCE_VARIABLE, + }; + + private static final TypeUseLocation[] defaultLocationsUnspecified = + new TypeUseLocation[] { + TypeUseLocation.IMPLICIT_WILDCARD_UPPER_BOUND_NO_SUPER, + TypeUseLocation.TYPE_VARIABLE_USE, + TypeUseLocation.OTHERWISE + }; + + private static final TypeUseLocation[] nullMarkedLocationsMinusNull = + new TypeUseLocation[] { + TypeUseLocation.CONSTRUCTOR_RESULT, + TypeUseLocation.EXCEPTION_PARAMETER, + TypeUseLocation.IMPLICIT_LOWER_BOUND, + TypeUseLocation.RECEIVER, + TypeUseLocation.OTHERWISE + }; + private static final TypeUseLocation[] nullMarkedLocationsUnionNull = + new TypeUseLocation[] { + TypeUseLocation.LOCAL_VARIABLE, + TypeUseLocation.RESOURCE_VARIABLE, + TypeUseLocation.IMPLICIT_WILDCARD_UPPER_BOUND_NO_SUPER, + TypeUseLocation.IMPLICIT_WILDCARD_UPPER_BOUND_SUPER, + }; + + private static final TypeUseLocation[] nullMarkedLocationsParametric = + new TypeUseLocation[] {TypeUseLocation.TYPE_VARIABLE_USE}; + + private static final TypeUseLocation[] nullMarkedLocationsUnspecified = new TypeUseLocation[] {}; + /** Constructor that takes all configuration from the provided {@code checker}. */ NullSpecAnnotatedTypeFactory(BaseTypeChecker checker, Util util) { this(checker, util, checker.hasOption("strict"), /* withOtherWorld= */ null); @@ -161,6 +205,7 @@ private NullSpecAnnotatedTypeFactory( minusNull = util.minusNull; unionNull = util.unionNull; nullnessOperatorUnspecified = util.nullnessOperatorUnspecified; + parametricNull = util.parametricNull; addAliasedTypeAnnotation( "org.jspecify.annotations.NullnessUnspecified", nullnessOperatorUnspecified); @@ -173,6 +218,98 @@ private NullSpecAnnotatedTypeFactory( * recognizing annotations by simple class name instead of by fully qualified name. */ + AnnotationMirror nullMarkedDefaultQualMinusNull = + new AnnotationBuilder(processingEnv, DefaultQualifier.class) + .setValue("value", MinusNull.class) + .setValue("locations", nullMarkedLocationsMinusNull) + .setValue("applyToSubpackages", false) + .build(); + AnnotationMirror nullMarkedDefaultQualUnionNull = + new AnnotationBuilder(processingEnv, DefaultQualifier.class) + .setValue("value", Nullable.class) + .setValue("locations", nullMarkedLocationsUnionNull) + .setValue("applyToSubpackages", false) + .build(); + AnnotationMirror nullMarkedDefaultQualParametric = + new AnnotationBuilder(processingEnv, DefaultQualifier.class) + .setValue("value", ParametricNull.class) + .setValue("locations", nullMarkedLocationsParametric) + .setValue("applyToSubpackages", false) + .build(); + AnnotationMirror nullMarkedDefaultQualUnspecified = + new AnnotationBuilder(processingEnv, DefaultQualifier.class) + .setValue("value", NullnessUnspecified.class) + .setValue("locations", nullMarkedLocationsUnspecified) + .setValue("applyToSubpackages", false) + .build(); + AnnotationMirror nullMarkedDefaultQual = + new AnnotationBuilder(processingEnv, DefaultQualifier.List.class) + .setValue( + "value", + new AnnotationMirror[] { + nullMarkedDefaultQualMinusNull, + nullMarkedDefaultQualUnionNull, + nullMarkedDefaultQualParametric, + nullMarkedDefaultQualUnspecified + }) + .build(); + + /* + * XXX: When adding support for aliases, make sure to support them here. But consider how to + * handle @Inherited aliases (https://github.com/jspecify/jspecify/issues/155). In particular, we + * have already edited getDeclAnnotations to remove its inheritance logic, and we needed to do so + * to work around another problem (though perhaps we could have found alternatives). + */ + addAliasedDeclAnnotation( + "org.jspecify.annotations.NullMarked", + DefaultQualifier.List.class.getCanonicalName(), + nullMarkedDefaultQual); + // TODO: does this work as intended? + /* + * We assume that ProtoNonnullApi is like NullMarked in that it guarantees that *all* types + * are non-null, even those that would require type annotations to annotate (e.g., + * type-parameter bounds). This is probably a safe assumption, if only because such types + * might not arise at all in the generated code where ProtoNonnullApi is used. + */ + addAliasedDeclAnnotation( + "com.google.protobuf.Internal.ProtoNonnullApi", + DefaultQualifier.List.class.getCanonicalName(), + nullMarkedDefaultQual); + + AnnotationMirror nullUnmarkedDefaultQualMinusNull = + new AnnotationBuilder(processingEnv, DefaultQualifier.class) + .setValue("value", MinusNull.class) + .setValue("locations", defaultLocationsMinusNull) + .setValue("applyToSubpackages", false) + .build(); + AnnotationMirror nullUnmarkedDefaultQualUnionNull = + new AnnotationBuilder(processingEnv, DefaultQualifier.class) + .setValue("value", Nullable.class) + .setValue("locations", defaultLocationsUnionNull) + .setValue("applyToSubpackages", false) + .build(); + AnnotationMirror nullUnmarkedDefaultQualUnspecified = + new AnnotationBuilder(processingEnv, DefaultQualifier.class) + .setValue("value", NullnessUnspecified.class) + .setValue("locations", defaultLocationsUnspecified) + .setValue("applyToSubpackages", false) + .build(); + AnnotationMirror nullUnmarkedDefaultQual = + new AnnotationBuilder(processingEnv, DefaultQualifier.List.class) + .setValue( + "value", + new AnnotationMirror[] { + nullUnmarkedDefaultQualMinusNull, + nullUnmarkedDefaultQualUnionNull, + nullUnmarkedDefaultQualUnspecified, + }) + .build(); + + addAliasedDeclAnnotation( + "org.jspecify.annotations.NullUnmarked", + DefaultQualifier.List.class.getCanonicalName(), + nullUnmarkedDefaultQual); + this.isLeastConvenientWorld = isLeastConvenientWorld; javaUtilCollection = createType(util.javaUtilCollectionElement); @@ -202,6 +339,9 @@ private NullSpecAnnotatedTypeFactory( withMostConvenientWorld = this; } + conformanceInformationPresenter = + checker.hasOption("showTypes") ? new ConformanceTypeInformationPresenter(this) : null; + if (!givenOtherWorld) { /* * Now the withLeastConvenientWorld and withMostConvenientWorld fields of both `this` and @@ -229,9 +369,51 @@ private NullSpecAnnotatedTypeFactory( } } + /** To ensure setRoot is called on both worlds exactly once. */ + private boolean settingRoot = false; + + /** + * Ensure setRoot is called on both worlds exactly once whenever it is called on one of the + * worlds. + */ + @Override + public void setRoot(@Nullable CompilationUnitTree root) { + if (!settingRoot) { + settingRoot = true; + super.setRoot(root); + if (withLeastConvenientWorld != this) { + withLeastConvenientWorld.setRoot(root); + } else { + withMostConvenientWorld.setRoot(root); + } + settingRoot = false; + } + } + + @Override + protected void addCheckedCodeDefaults(QualifierDefaults defs) { + // TODO: add false for subpackages once overload is added to CF. Shouldn't really matter. + defs.addCheckedCodeDefaults(minusNull, defaultLocationsMinusNull); + defs.addCheckedCodeDefaults(unionNull, defaultLocationsUnionNull); + defs.addCheckedCodeDefaults(nullnessOperatorUnspecified, defaultLocationsUnspecified); + } + + @Override + protected void addUncheckedStandardDefaults(QualifierDefaults defs) { + // TODO: figure out whether we need different defaults here + /* + defs.addUncheckedCodeDefaults(minusNull, defaultLocationsMinusNull); + defs.addUncheckedCodeDefaults(unionNull, defaultLocationsUnionNull); + defs.addUncheckedCodeDefaults(nullnessOperatorUnspecified, defaultLocationsUnspecified); + */ + // defs.addUncheckedCodeDefaults(nullnessOperatorUnspecified, new TypeUseLocation[] { + // TypeUseLocation.ALL }); + } + @Override protected Set> createSupportedTypeQualifiers() { - return new LinkedHashSet<>(asList(Nullable.class, NullnessUnspecified.class, MinusNull.class)); + return new LinkedHashSet<>( + asList(Nullable.class, NullnessUnspecified.class, MinusNull.class, ParametricNull.class)); } @Override @@ -242,11 +424,11 @@ protected QualifierHierarchy createQualifierHierarchy() { private final class NullSpecQualifierHierarchy extends NoElementQualifierHierarchy { NullSpecQualifierHierarchy( Collection> qualifierClasses, Elements elements) { - super(qualifierClasses, elements); + super(qualifierClasses, elements, NullSpecAnnotatedTypeFactory.this); } @Override - public boolean isSubtype(AnnotationMirror subAnno, AnnotationMirror superAnno) { + public boolean isSubtypeQualifiers(AnnotationMirror subAnno, AnnotationMirror superAnno) { if (subAnno == null || superAnno == null) { /* * The stock CF never passes null to this method: It always expose *some* annotation @@ -310,10 +492,16 @@ protected Map> createDirectSuper nameToQualifierKind.get(Nullable.class.getCanonicalName()); DefaultQualifierKind nullnessOperatorUnspecified = nameToQualifierKind.get(NullnessUnspecified.class.getCanonicalName()); + DefaultQualifierKind parametricNullKind = + nameToQualifierKind.get(ParametricNull.class.getCanonicalName()); Map> supers = new HashMap<>(); - supers.put(minusNullKind, singleton(nullnessOperatorUnspecified)); + LinkedHashSet superOfMinusNull = new LinkedHashSet<>(); + superOfMinusNull.add(nullnessOperatorUnspecified); + superOfMinusNull.add(parametricNullKind); + supers.put(minusNullKind, superOfMinusNull); supers.put(nullnessOperatorUnspecified, singleton(unionNullKind)); + supers.put(parametricNullKind, singleton(unionNullKind)); supers.put(unionNullKind, emptySet()); return supers; /* @@ -329,6 +517,24 @@ protected Map> createDirectSuper } }; } + + @Override + public AnnotationMirror getParametricQualifier(AnnotationMirror qualifier) { + return parametricNull; + } + + @Override + public boolean isParametricQualifier(AnnotationMirror qualifier) { + return areSame(parametricNull, qualifier); + } + } + + @Override + public AnnotatedTypeMirror applyCaptureConversion( + AnnotatedTypeMirror type, TypeMirror typeMirror) { + return this == withLeastConvenientWorld + ? super.applyCaptureConversion(type, typeMirror) + : withLeastConvenientWorld.applyCaptureConversion(type, typeMirror); } @Override @@ -458,6 +664,21 @@ private boolean isNullnessSubtype(AnnotatedTypeMirror subtype, AnnotatedTypeMirr && isNullnessSubtype(subtype, ((AnnotatedTypeVariable) supertype).getLowerBound())) { return true; } + if (subtype.getKind() == TYPEVAR && supertype.getKind() == TYPEVAR) { + // Work around wonkyness of CF override checks. + AnnotatedTypeVariable subTV = (AnnotatedTypeVariable) subtype; + if (isCapturedTypeVariable(subTV.getUnderlyingType())) { + AnnotatedTypeMirror subTVBnd = subTV.getUpperBound(); + if (subTVBnd instanceof AnnotatedTypeVariable) { + subTV = (AnnotatedTypeVariable) subTVBnd; + } + } + AnnotatedTypeVariable superTV = (AnnotatedTypeVariable) supertype; + if (areCorrespondingTypeVariables(elements, subTV, superTV)) { + return isSubtype(subTV.getUpperBound(), superTV.getUpperBound()) + && isSubtype(superTV.getLowerBound(), subTV.getLowerBound()); + } + } return isNullInclusiveUnderEveryParameterization(supertype) || isNullExclusiveUnderEveryParameterization(subtype) || (nullnessEstablishingPathExists(subtype, supertype) @@ -495,7 +716,7 @@ && isNullInclusiveUnderEveryParameterization( } boolean isNullExclusiveUnderEveryParameterization(AnnotatedTypeMirror type) { - return nullnessEstablishingPathExists(type, IS_DECLARED_OR_ARRAY); + return nullnessEstablishingPathExists(type, IS_DECLARED_OR_ARRAY_OR_NULL); } private boolean nullnessEstablishingPathExists( @@ -578,6 +799,9 @@ private List getUpperBounds(AnnotatedTypeMirror t * * My only worry is that I always worry about making calls to getAnnotatedType, as discussed in * various comments in this file (e.g., in NullSpecTreeAnnotator.visitMethodInvocation). + * + * This is likely caused by https://github.com/eisop/checker-framework/issues/737. + * Revisit this once that issue is fixed. */ if (type instanceof AnnotatedTypeVariable && !isCapturedTypeVariable(type.getUnderlyingType())) { @@ -733,8 +957,8 @@ protected AnnotatedTypeMirror substituteTypeVariable( substitute.replaceAnnotation(minusNull); } else if (argument.hasAnnotation(unionNull) || use.hasAnnotation(unionNull)) { substitute.replaceAnnotation(unionNull); - } else if (argument.hasAnnotation(nullnessOperatorUnspecified) - || use.hasAnnotation(nullnessOperatorUnspecified)) { + } else if (argument.hasEffectiveAnnotation(nullnessOperatorUnspecified) + || use.hasEffectiveAnnotation(nullnessOperatorUnspecified)) { substitute.replaceAnnotation(nullnessOperatorUnspecified); } @@ -786,131 +1010,16 @@ protected void addCheckedStandardDefaults(QualifierDefaults defs) { */ } - @Override - protected void checkForDefaultQualifierInHierarchy(QualifierDefaults defs) { - /* - * We don't set normal checkedCodeDefaults. This method would report that lack of defaults as a - * problem. That's because CF wants to ensure that every[*] type usage is annotated. - * - * However, we *do* ensure that every[*] type usage is annotated. To do so, we always set a - * default for OTHERWISE on top-level elements. (We do this in populateNewDefaults.) See further - * discussion in addCheckedStandardDefaults. - * - * So, we override this method to not report a problem. - * - * [*] There are a few exceptions that we don't need to get into here. - */ - } - @Override protected QualifierDefaults createQualifierDefaults() { return new NullSpecQualifierDefaults(elements, this); } - private final class NullSpecQualifierDefaults extends QualifierDefaults { + private static final class NullSpecQualifierDefaults extends QualifierDefaults { NullSpecQualifierDefaults(Elements elements, AnnotatedTypeFactory atypeFactory) { super(elements, atypeFactory); } - @Override - protected void populateNewDefaults(Element elt, boolean initialDefaultsAreEmpty) { - /* - * Note: This method does not contain the totality of our defaulting logic. For example, our - * TypeAnnotator has special logic for upper bounds _in the case of `super` wildcards - * specifically_. - * - * Note: Setting a default here affects not only this element but also its descendants in the - * syntax tree. - */ - if (hasNullMarkedOrEquivalent(elt)) { - addElementDefault(elt, unionNull, UNBOUNDED_WILDCARD_UPPER_BOUND); - addElementDefault(elt, minusNull, OTHERWISE); - addDefaultToTopForLocationsRefinedByDataflow(elt); - /* - * (For any TypeUseLocation that we don't set an explicit value for, we inherit any value - * from the enclosing element, which might be a non-null-aware element. That's fine: While - * our non-null-aware setup sets defaults for more locations than just these, it sets those - * locations' defaults to minusNull -- matching the value that we want here.) - */ - } else if (hasNullUnmarked(elt) || initialDefaultsAreEmpty) { - /* - * We need to set defaults appropriate to non-null-aware code. In a normal checker, we would - * expect for such "default defaults" to be set in addCheckedStandardDefaults. But we do - * not, as discussed in our implementation of that method. - */ - - // Here's the big default, the "default default": - addElementDefault(elt, nullnessOperatorUnspecified, OTHERWISE); - /* - * OTHERWISE covers anything that does not have a more specific default inherited from an - * enclosing element (or, of course, a more specific default that we set below in this - * method). If this is a @NullUnmarked element, then it might have a had a @NullMarked - * enclosing element, which would have set a default for UNBOUNDED_WILDCARD_UPPER_BOUND. So - * we make sure to override that here. - */ - addElementDefault(elt, nullnessOperatorUnspecified, UNBOUNDED_WILDCARD_UPPER_BOUND); - - // Some locations are intrinsically non-nullable: - addElementDefault(elt, minusNull, CONSTRUCTOR_RESULT); - addElementDefault(elt, minusNull, RECEIVER); - - // We do want *some* of the CLIMB standard defaults: - addDefaultToTopForLocationsRefinedByDataflow(elt); - addElementDefault(elt, minusNull, IMPLICIT_LOWER_BOUND); - - /* - * But note one difference from the CLIMB defaults: We want the default for implicit upper - * bounds to match the "default default" of nullnessOperatorUnspecified, not to be - * top/unionNull. We accomplished this already simply by not making our - * addCheckedStandardDefaults implementation call its supermethod (which would otherwise - * call addClimbStandardDefaults, which would override the "default default"). - */ - } - } - - private void addDefaultToTopForLocationsRefinedByDataflow(Element elt) { - for (TypeUseLocation location : IMPLEMENTATION_VARIABLE_LOCATIONS) { - /* - * Handling exception parameters correctly is hard, so just treat them as if they're - * restricted to non-null values. Of course the caught exception is already non-null, so all - * this does is forbid users from manually assigning null to an exception parameter. - */ - if (location == EXCEPTION_PARAMETER) { - addElementDefault(elt, minusNull, location); - } else { - addElementDefault(elt, unionNull, location); - } - } - } - - @Override - protected boolean shouldAnnotateOtherwiseNonDefaultableTypeVariable(AnnotationMirror qual) { - /* - * CF usually doesn't apply defaults to type-variable usages. But in non-null-aware code, we - * want our default of nullnessOperatorUnspecified to apply even to type variables. - * - * But there are 2 other things to keep in mind: - * - * - CF *does* apply defaults to type-variable usages *if* they are local variables. That's - * because it will refine their types with dataflow. This CF behavior works fine for us: Since - * we want to apply defaults in strictly more cases, we're happy to accept what CF already - * does for local variables. (We do need to be sure to apply unionNull (our top type) in that - * case, rather than nullnessOperatorUnspecified. We accomplish that in - * addDefaultToTopForLocationsRefinedByDataflow.) - * - * - Non-null-aware code (discussed above) is easy: We apply nullnessOperatorUnspecified to - * everything except local variables. But null-aware code more complex. First, set aside local - * variables, which we handle as discussed above. After that, we need to apply minusNull to - * most types, but we need to *not* apply it to (non-local-variable) type-variable usages. - * (For more on this, see isNullExclusiveUnderEveryParameterization.) This need is weird - * enough that stock CF doesn't appear to support it. Our solution is to introduce this hook - * method into our CF fork and then override it here. Our solution also requires that we set - * up defaulting in a non-standard way, as discussed in addCheckedStandardDefaults and other - * locations. - */ - return areSame(qual, nullnessOperatorUnspecified); - } - @Override public boolean applyConservativeDefaults(Element annotationScope) { /* @@ -926,68 +1035,7 @@ public boolean applyConservativeDefaults(Element annotationScope) { // Disable checking of contracts. @Override protected ContractsFromMethod createContractsFromMethod() { - return new ContractsFromMethod(this) { - @Override - public Set getConditionalPostconditions( - ExecutableElement methodElement) { - return emptySet(); - } - - @Override - public Set getPreconditions(ExecutableElement executableElement) { - return emptySet(); - } - - @Override - public Set getPostconditions(ExecutableElement executableElement) { - return emptySet(); - } - }; - } - - @Override - protected void addComputedTypeAnnotations(Tree tree, AnnotatedTypeMirror type, boolean iUseFlow) { - super.addComputedTypeAnnotations( - tree, - type, - iUseFlow - /* - * TODO(cpovirk): Eliminate this workaround (which may cause problems of its own). But - * currently, it helps in some of our samples and in Guava, though I am unsure why. The - * problem it works around may well be caused by our failure to keep wildcards' - * annotations in sync with their bounds' annotations (whereas stock CF does). - * - * Note that the `type.getKind() != WILDCARD` check appears to be necessary before the - * CF implementation of capture conversion and unnecessary afterward, while the reverse - * is true of the `isCapturedTypeVariable` check. - */ - && type.getKind() != WILDCARD - && !isCapturedTypeVariable(type.getUnderlyingType()) - /* - * TODO(cpovirk): See if we can remove this workaround after merging the fix for - * https://github.com/typetools/checker-framework/issues/5042. - * - * The workaround becomes necessary after the CF implementation of capture conversion. - * Without it, we see dataflow decide that `b ? null : nullable` has a type of - * _non-null_, as in code like the following: - * - * https://github.com/google/guava/blob/156694066b5198740a820c6eef723fb86c054343/guava/src/com/google/common/base/Throwables.java#L470 - * - * (But I haven't been able to reproduce this in a smaller test.) - * - * Fortunately, I think the workaround is harmless: - * TypeFromExpressionVisitor.visitConditionalExpression calls getAnnotatedType on both - * candidate expressions, and getAnnotatedType applies dataflow. So the ternary should - * end up with the dataflow-correct result by virtue of applying lub to those types. - * - * (I think the only exception would be if someone performed a null check _on an entire - * ternary_ and then expected _another appearance of that same ternary_ to be recognized - * as non-null. That seems implausible.) - * - * (Still, it would be good to look into what's going on here in case it's a sign of a - * deeper problem.) - */ - && tree.getKind() != CONDITIONAL_EXPRESSION); + return new NoContractsFromMethod(); } @Override @@ -1490,63 +1538,6 @@ protected NullSpecAnalysis createFlowAnalysis() { return new NullSpecAnalysis(checker, this); } - @Override - public void addDefaultAnnotations(AnnotatedTypeMirror type) { - super.addDefaultAnnotations(type); - /* - * TODO(cpovirk): Find a better solution than this. - * - * The problem I'm working around arises during AnnotatedTypes.leastUpperBound on a - * JSpecify-annotated variant of this code: - * https://github.com/google/guava/blob/39aa77fa0e8912d6bfb5cb9a0bc1ed5135747b6f/guava/src/com/google/common/collect/ImmutableMultiset.java#L205 - * - * CF is unable to infer the right type for `LinkedHashMultiset.create(elements)`: It should - * infer `LinkedHashMultiset`, but instead, it infers `LinkedHashMultiset`. As expected, it sets isUninferredTypeArgument. As *not* expected, it gets to - * AtmLubVisitor.lubTypeArgument with type2Wildcard.extendsBound.lowerBound (a null type) - * missing its annotation. - * - * The part of CF responsible for copying annotations, including those on the extends bound, is - * AsSuperVisitor.visitWildcard_Wildcard. Under stock CF, copyPrimaryAnnos(from, typevar) "also - * sets primary annotations _on the bounds_." Under our CF fork, this is not the case, and we - * end up with an unannotated lower bound on the type-variable usage E (which, again, is itself - * a bound of a wildcard). - * - * (Aside: I haven't looked into how the _upper_ bound of the type-variable usage gets an - * annotation set on it. Could it be happening "accidentally," and if so, might it be wrong - * sometimes?) - * - * The result of an unannotated lower bound is a crash in NullSpecQualifierHierarchy.isSubtype, - * which passes null to areSame. - * - * The workaround: If we see a type-variable usage whose lower bound is a null type that lacks - * an annotation, we annotate that bound as non-null. This workaround shouldn't break any - * working code, but it may or may not be universally the right solution to a missing - * annotation. - * - * I am trying to ignore other questions here, such as: - * - * - Would it make more sense to set the lower bound to match the upper bound, as stock CF does? - * I suspect not under our approach, but I haven't thought about it. - * - * - Does trying to pick correct annotations even matter in the context of an uninferred type - * argument? Does the very idea of "correct annotations" lose meaning in that context? - * - * - Should we fix this in AsSuperVisitor instead? Or would it fix itself if we set bounds on - * our type-variable usages and wildcards in the same way that stock CF does? (Following stock - * CF would likely save us from other problems, too.) - * - * - What's up with the _upper_ bound, as discussed in a parenthetical above? - */ - if (type instanceof AnnotatedTypeVariable) { - AnnotatedTypeMirror lowerBound = ((AnnotatedTypeVariable) type).getLowerBound(); - if (lowerBound instanceof AnnotatedNullType - && !lowerBound.isAnnotatedInHierarchy(unionNull)) { - lowerBound.addAnnotation(minusNull); - } - } - } - @Override protected AnnotationFormatter createAnnotationFormatter() { return new DefaultAnnotationFormatter() { @@ -1823,6 +1814,10 @@ public void postProcessClassTree(ClassTree tree) { * type.invalid.conflicting.annos error, which I have described more in * https://github.com/jspecify/jspecify-reference-checker/commit/d16a0231487e239bc94145177de464b5f77c8b19 */ + + if (conformanceInformationPresenter != null) { + conformanceInformationPresenter.process(tree, getPath(tree)); + } } @Override @@ -1845,42 +1840,11 @@ protected void addAnnotationsFromDefaultForType(Element element, AnnotatedTypeMi } private void addIfNoAnnotationPresent(AnnotatedTypeMirror type, AnnotationMirror annotation) { - if (!type.isAnnotatedInHierarchy(unionNull)) { + if (!type.hasAnnotationInHierarchy(unionNull)) { type.addAnnotation(annotation); } } - /* - * XXX: When adding support for aliases, make sure to support them here. But consider how to - * handle @Inherited aliases (https://github.com/jspecify/jspecify/issues/155). In particular, we - * have already edited getDeclAnnotations to remove its inheritance logic, and we needed to do so - * to work around another problem (though perhaps we could have found alternatives). - */ - private boolean hasNullMarkedOrEquivalent(Element elt) { - return getDeclAnnotations(elt).stream() - .anyMatch(am -> areSameByName(am, "org.jspecify.annotations.NullMarked")) - /* - * We assume that ProtoNonnullApi is like NullMarked in that it guarantees that *all* types - * are non-null, even those that would require type annotations to annotate (e.g., - * type-parameter bounds). This is probably a safe assumption, if only because such types - * might not arise at all in the generated code where ProtoNonnullApi is used. - */ - || hasAnnotationInCode(elt, "ProtoNonnullApi"); - } - - private boolean hasNullUnmarked(Element elt) { - return getDeclAnnotations(elt).stream() - .anyMatch(am -> areSameByName(am, "org.jspecify.annotations.NullUnmarked")); - } - - /** - * Returns whether the given element has an annotation with the given simple name. This method - * does not consider stub files. - */ - private static boolean hasAnnotationInCode(AnnotatedConstruct construct, String name) { - return construct.getAnnotationMirrors().stream().anyMatch(a -> nameMatches(a, name)); - } - @SuppressWarnings("unchecked") // safety guaranteed by API docs private T withMinusNull(T type) { // Remove the annotation from the *root* type, but preserve other annotations. @@ -1911,12 +1875,12 @@ private AnnotatedDeclaredType createType(TypeElement element) { // Avoid lambdas so that our Predicates can have a useful toString() for logging purposes. - enum IsDeclaredOrArray implements Predicate { - IS_DECLARED_OR_ARRAY; + enum IsDeclaredOrArrayOrNull implements Predicate { + IS_DECLARED_OR_ARRAY_OR_NULL; @Override public boolean test(TypeMirror t) { - return t.getKind() == DECLARED || t.getKind() == ARRAY; + return t.getKind() == DECLARED || t.getKind() == ARRAY || t.getKind() == NULL; } } diff --git a/src/main/java/com/google/jspecify/nullness/NullSpecChecker.java b/src/main/java/com/google/jspecify/nullness/NullSpecChecker.java index f89ea601..54d6139d 100644 --- a/src/main/java/com/google/jspecify/nullness/NullSpecChecker.java +++ b/src/main/java/com/google/jspecify/nullness/NullSpecChecker.java @@ -23,7 +23,7 @@ import com.sun.source.util.TreePath; import com.sun.tools.javac.processing.JavacProcessingEnvironment; import com.sun.tools.javac.util.Log; -import java.util.SortedSet; +import java.util.NavigableSet; import java.util.TreeSet; import javax.lang.model.element.TypeElement; import org.checkerframework.common.basetype.BaseTypeChecker; @@ -39,9 +39,10 @@ *
  • "strict": Whether the checker should be a sound, strict type system. Does not imply that * implementation code is checked. *
  • "checkImpl": Whether implementation code should be checked. + *
  • "showTypes": Whether to output type information for the conformance test suite. * */ -@SupportedOptions({"strict", "checkImpl"}) +@SupportedOptions({"strict", "checkImpl", "showTypes"}) public final class NullSpecChecker extends BaseTypeChecker { /* * A non-final field is ugly, but we can't create our Util instance in the constructor because the @@ -62,12 +63,17 @@ public final class NullSpecChecker extends BaseTypeChecker { public NullSpecChecker() {} @Override - public SortedSet getSuppressWarningsPrefixes() { - SortedSet prefixes = new TreeSet<>(); + public NavigableSet getSuppressWarningsPrefixes() { + TreeSet prefixes = new TreeSet<>(); prefixes.add("nullness"); return prefixes; } + @Override + protected String suppressWarningsString(String messageKey) { + return "nullness"; + } + @Override public void initChecker() { super.initChecker(); diff --git a/src/main/java/com/google/jspecify/nullness/NullSpecTransfer.java b/src/main/java/com/google/jspecify/nullness/NullSpecTransfer.java index 171e2f2e..eda309c3 100644 --- a/src/main/java/com/google/jspecify/nullness/NullSpecTransfer.java +++ b/src/main/java/com/google/jspecify/nullness/NullSpecTransfer.java @@ -18,7 +18,6 @@ import static com.sun.source.tree.Tree.Kind.NULL_LITERAL; import static java.util.Arrays.asList; import static java.util.Collections.emptyList; -import static java.util.Collections.singleton; import static java.util.Collections.unmodifiableSet; import static java.util.stream.Collectors.toList; import static org.checkerframework.dataflow.expression.JavaExpression.fromNode; @@ -71,6 +70,7 @@ import org.checkerframework.framework.flow.CFValue; import org.checkerframework.framework.type.AnnotatedTypeMirror; import org.checkerframework.framework.type.AnnotatedTypeMirror.AnnotatedDeclaredType; +import org.checkerframework.javacutil.AnnotationMirrorSet; final class NullSpecTransfer extends CFAbstractTransfer { private final Util util; @@ -1019,7 +1019,8 @@ private boolean valueIsAtLeastAsSpecificAs(CFValue value, CFValue targetDataflow if (target == null) { return false; } - return atypeFactory.getQualifierHierarchy().greatestLowerBound(existing, target) == existing; + return atypeFactory.getQualifierHierarchy().greatestLowerBoundQualifiersOnly(existing, target) + == existing; } private static boolean isNullLiteral(Node node) { @@ -1052,7 +1053,8 @@ private void setResultValue( * if (clazz.cast(foo) != null) { return class.cast(foo); } */ result.setResultValue( - analysis.createAbstractValue(singleton(qual), result.getResultValue().getUnderlyingType())); + analysis.createAbstractValue( + AnnotationMirrorSet.singleton(qual), result.getResultValue().getUnderlyingType())); } private JavaExpression expressionToStoreFor(Node node) { diff --git a/src/main/java/com/google/jspecify/nullness/NullSpecTypeValidator.java b/src/main/java/com/google/jspecify/nullness/NullSpecTypeValidator.java new file mode 100644 index 00000000..bb29fc08 --- /dev/null +++ b/src/main/java/com/google/jspecify/nullness/NullSpecTypeValidator.java @@ -0,0 +1,45 @@ +// Copyright 2020 The JSpecify Authors +// +// 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.google.jspecify.nullness; + +import javax.lang.model.element.AnnotationMirror; +import org.checkerframework.common.basetype.BaseTypeChecker; +import org.checkerframework.common.basetype.BaseTypeValidator; +import org.checkerframework.framework.type.AnnotatedTypeMirror; + +final class NullSpecTypeValidator extends BaseTypeValidator { + private final AnnotationMirror nullnessOperatorUnspecified; + + /** Constructor. */ + NullSpecTypeValidator( + BaseTypeChecker checker, + NullSpecVisitor visitor, + NullSpecAnnotatedTypeFactory atypeFactory, + Util util) { + super(checker, visitor, atypeFactory); + + nullnessOperatorUnspecified = util.nullnessOperatorUnspecified; + } + + @Override + public boolean areBoundsValid(AnnotatedTypeMirror upperBound, AnnotatedTypeMirror lowerBound) { + if (upperBound.hasAnnotation(nullnessOperatorUnspecified) + || lowerBound.hasAnnotation(nullnessOperatorUnspecified)) { + return true; + } else { + return super.areBoundsValid(upperBound, lowerBound); + } + } +} diff --git a/src/main/java/com/google/jspecify/nullness/NullSpecVisitor.java b/src/main/java/com/google/jspecify/nullness/NullSpecVisitor.java index f8d41d58..c6c2f83c 100644 --- a/src/main/java/com/google/jspecify/nullness/NullSpecVisitor.java +++ b/src/main/java/com/google/jspecify/nullness/NullSpecVisitor.java @@ -61,7 +61,6 @@ import com.sun.source.tree.TypeParameterTree; import com.sun.source.tree.VariableTree; import java.util.ArrayList; -import java.util.HashSet; import java.util.List; import java.util.Set; import javax.lang.model.element.AnnotationMirror; @@ -72,10 +71,12 @@ import javax.lang.model.type.DeclaredType; import javax.lang.model.type.TypeMirror; import org.checkerframework.common.basetype.BaseTypeVisitor; +import org.checkerframework.common.basetype.TypeValidator; import org.checkerframework.framework.type.AnnotatedTypeMirror; import org.checkerframework.framework.type.AnnotatedTypeMirror.AnnotatedDeclaredType; import org.checkerframework.framework.type.AnnotatedTypeMirror.AnnotatedExecutableType; import org.checkerframework.javacutil.AnnotationBuilder; +import org.checkerframework.javacutil.AnnotationMirrorSet; final class NullSpecVisitor extends BaseTypeVisitor { private final boolean checkImpl; @@ -101,9 +102,11 @@ private void ensureNonNull(Tree tree, String messageKey) { } } + /* TODO: implement feature to add extra args to return type errors. + * @Override protected String extraArgForReturnTypeError(Tree tree) { - /* + / * We call originStringIfTernary, not originString: * * If the statement is `return foo.bar()`, then the problem is obvious, so we don't want our @@ -116,10 +119,11 @@ protected String extraArgForReturnTypeError(Tree tree) { * the possibly null value (possibly both!). However, this gets tricky: If the branches return * `Foo?` and `Foo*`, then we ideally want to emphasize the `Foo?` branch *but*, at least in * "strict mode," not altogether ignore the `Foo*` branch. - */ + / String origin = originStringIfTernary(tree); return origin.isEmpty() ? "" : (origin + "\n"); } + */ private String originString(Tree tree) { while (tree instanceof ParenthesizedTree) { @@ -625,8 +629,8 @@ private boolean isClassCastAppliedToNonNullableType(MemberReferenceTree tree) { } @Override - protected Set getExceptionParameterLowerBoundAnnotations() { - return new HashSet<>(asList(AnnotationBuilder.fromClass(elements, MinusNull.class))); + protected AnnotationMirrorSet getExceptionParameterLowerBoundAnnotations() { + return new AnnotationMirrorSet(asList(AnnotationBuilder.fromClass(elements, MinusNull.class))); } @Override @@ -634,4 +638,9 @@ protected NullSpecAnnotatedTypeFactory createTypeFactory() { // Reading util this way is ugly but necessary. See discussion in NullSpecChecker. return new NullSpecAnnotatedTypeFactory(checker, ((NullSpecChecker) checker).util); } + + @Override + protected TypeValidator createTypeValidator() { + return new NullSpecTypeValidator(checker, this, atypeFactory, ((NullSpecChecker) checker).util); + } } diff --git a/src/main/java/com/google/jspecify/nullness/ParametricNull.java b/src/main/java/com/google/jspecify/nullness/ParametricNull.java new file mode 100644 index 00000000..0e4c65e9 --- /dev/null +++ b/src/main/java/com/google/jspecify/nullness/ParametricNull.java @@ -0,0 +1,27 @@ +// Copyright 2024 The JSpecify Authors +// +// 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.google.jspecify.nullness; + +import static java.lang.annotation.ElementType.TYPE_USE; + +import java.lang.annotation.Target; +import org.checkerframework.framework.qual.InvisibleQualifier; +import org.checkerframework.framework.qual.ParametricTypeVariableUseQualifier; + +/** Internal implementation detail; not usable in user code. */ +@Target(TYPE_USE) +@InvisibleQualifier +@ParametricTypeVariableUseQualifier(Nullable.class) +@interface ParametricNull {} diff --git a/src/main/java/com/google/jspecify/nullness/Util.java b/src/main/java/com/google/jspecify/nullness/Util.java index e6b7dc0b..955bceb9 100644 --- a/src/main/java/com/google/jspecify/nullness/Util.java +++ b/src/main/java/com/google/jspecify/nullness/Util.java @@ -51,6 +51,7 @@ final class Util { final AnnotationMirror minusNull; final AnnotationMirror unionNull; final AnnotationMirror nullnessOperatorUnspecified; + final AnnotationMirror parametricNull; final TypeElement javaUtilCollectionElement; final ExecutableElement collectionToArrayNoArgElement; @@ -116,6 +117,7 @@ final class Util { minusNull = AnnotationBuilder.fromClass(e, MinusNull.class); unionNull = AnnotationBuilder.fromClass(e, Nullable.class); nullnessOperatorUnspecified = AnnotationBuilder.fromClass(e, NullnessUnspecified.class); + parametricNull = AnnotationBuilder.fromClass(e, ParametricNull.class); /* * Note that all the above annotations must be on the *classpath*, not just the *processorpath*. * That's because, even if we change fromClass to fromName, AnnotationBuilder ultimately calls diff --git a/src/test/java/tests/ConformanceTest.java b/src/test/java/tests/ConformanceTest.java index 8e401988..2745d05a 100644 --- a/src/test/java/tests/ConformanceTest.java +++ b/src/test/java/tests/ConformanceTest.java @@ -29,7 +29,7 @@ import java.io.IOException; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.Objects; +import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -38,7 +38,9 @@ import org.checkerframework.framework.test.TestUtilities; import org.checkerframework.framework.test.TypecheckExecutor; import org.checkerframework.framework.test.TypecheckResult; +import org.checkerframework.framework.test.diagnostics.DetailedTestDiagnostic; import org.checkerframework.framework.test.diagnostics.DiagnosticKind; +import org.checkerframework.framework.test.diagnostics.TestDiagnostic; import org.jspecify.annotations.Nullable; import org.jspecify.conformance.ConformanceTestRunner; import org.jspecify.conformance.ExpectedFact; @@ -142,9 +144,7 @@ private static ImmutableSet analyze( TestUtilities.getShouldEmitDebugInfo()); TypecheckResult result = new TypecheckExecutor().runTest(config); return result.getUnexpectedDiagnostics().stream() - .map(d -> DetailMessage.parse(d.getMessage(), testDirectory)) - .filter(Objects::nonNull) - .map(DetailMessageReportedFact::new) + .map(d -> new DetailMessageReportedFact(testDirectory, d)) .collect(toImmutableSet()); } @@ -155,19 +155,20 @@ static final class DetailMessageReportedFact extends ReportedFact { private static final ImmutableSet CANNOT_CONVERT_KEYS = ImmutableSet.of( - "argument", - "assignment", + "argument.type.incompatible", + "assignment.type.incompatible", "atomicreference.must.include.null", "cast.unsafe", - "lambda.param", - "methodref.receiver.bound", - "methodref.receiver", - "methodref.return", - "override.param", - "override.return", - "return", + "lambda.param.type.incompatible", + "methodref.receiver.bound.invalid", + "methodref.receiver.invalid", + "methodref.return.invalid", + "override.param.invalid", + "override.receiver.invalid", + "override.return.invalid", + "return.type.incompatible", "threadlocal.must.include.null", - "type.argument"); + "type.argument.type.incompatible"); private static final ImmutableSet IRRELEVANT_ANNOTATION_KEYS = ImmutableSet.of( @@ -176,60 +177,67 @@ static final class DetailMessageReportedFact extends ReportedFact { "type.parameter.annotated", "wildcard.annotated"); - private final DetailMessage detailMessage; + private final TestDiagnostic diagnostic; - DetailMessageReportedFact(DetailMessage detailMessage) { - super(detailMessage.file, detailMessage.lineNumber); - this.detailMessage = detailMessage; + DetailMessageReportedFact(@Nullable Path testDirectory, TestDiagnostic diag) { + super( + (testDirectory != null && diag.getFile().startsWith(testDirectory)) + ? testDirectory.relativize(diag.getFile()) + : diag.getFile(), + diag.getLineNumber()); + this.diagnostic = diag; } @Override protected boolean matches(ExpectedFact expectedFact) { if (expectedFact.isNullnessMismatch()) { - return DEREFERENCE.equals(detailMessage.messageKey) - || CANNOT_CONVERT_KEYS.contains(detailMessage.messageKey); + return DEREFERENCE.equals(diagnostic.getMessageKey()) + || CANNOT_CONVERT_KEYS.contains(diagnostic.getMessageKey()); } return super.matches(expectedFact); } @Override protected boolean mustBeExpected() { - return detailMessage.getKind().equals(DiagnosticKind.Error); + return diagnostic.getKind().equals(DiagnosticKind.Error); } @Override protected String getFactText() { - if (CANNOT_CONVERT_KEYS.contains(detailMessage.messageKey)) { - if (detailMessage.messageArguments.size() < 2) { + if (!(diagnostic instanceof DetailedTestDiagnostic)) { + return toString(); + } + List args = ((DetailedTestDiagnostic) diagnostic).getAdditionalTokens(); + if (CANNOT_CONVERT_KEYS.contains(diagnostic.getMessageKey())) { + if (args.size() < 2) { // The arguments must end with sourceType and sinkType. return toString(); } - ImmutableList reversedArguments = detailMessage.messageArguments.reverse(); - String sourceType = fixType(reversedArguments.get(1)); // penultimate - String sinkType = fixType(reversedArguments.get(0)); // last + String sourceType = fixType(args.get(args.size() - 2)); // penultimate + String sinkType = fixType(args.get(args.size() - 1)); // last return cannotConvert(sourceType, sinkType); } - if (IRRELEVANT_ANNOTATION_KEYS.contains(detailMessage.messageKey)) { - if (detailMessage.messageArguments.isEmpty()) { + if (IRRELEVANT_ANNOTATION_KEYS.contains(diagnostic.getMessageKey())) { + if (args.isEmpty()) { // arguments must start with the annotation return toString(); } return irrelevantAnnotation( // Remove the package name (and any enclosing element name); emit just the simple name. - detailMessage.messageArguments.get(0).replaceFirst(".*\\.", "")); + args.get(0).replaceFirst(".*\\.", "")); } - switch (detailMessage.messageKey) { + switch (diagnostic.getMessageKey()) { case "sourceType": { - String expressionType = fixType(detailMessage.messageArguments.get(0)); - String expression = detailMessage.messageArguments.get(1); + String expressionType = fixType(args.get(0)); + String expression = args.get(1); return expressionType(expressionType, expression); } case "sinkType": { - String sinkType = fixType(detailMessage.messageArguments.get(0)); + String sinkType = fixType(args.get(0)); // Remove the simple name of the class and the dot before the method name. - String sink = detailMessage.messageArguments.get(1).replaceFirst("^[^.]+\\.", ""); + String sink = args.get(1).replaceFirst("^[^.]+\\.", ""); return sinkType(sinkType, sink); } } @@ -238,7 +246,7 @@ protected String getFactText() { @Override public String toString() { - return String.format("(%s) %s", detailMessage.messageKey, detailMessage.readableMessage); + return String.format("(%s) %s", diagnostic.getMessageKey(), diagnostic.getMessage()); } /** diff --git a/src/test/java/tests/DetailMessage.java b/src/test/java/tests/DetailMessage.java deleted file mode 100644 index c7deeedf..00000000 --- a/src/test/java/tests/DetailMessage.java +++ /dev/null @@ -1,201 +0,0 @@ -package tests; - -import static com.google.common.base.MoreObjects.toStringHelper; -import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.collect.Iterables.getLast; -import static java.lang.Integer.parseInt; -import static java.util.Arrays.stream; -import static java.util.regex.Pattern.DOTALL; -import static java.util.stream.Collectors.joining; - -import com.google.common.base.Joiner; -import com.google.common.base.Splitter; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.util.List; -import java.util.Objects; -import java.util.regex.Matcher; -import java.util.regex.Pattern; -import org.checkerframework.framework.test.diagnostics.DiagnosticKind; -import org.checkerframework.framework.test.diagnostics.TestDiagnostic; -import org.jspecify.annotations.Nullable; - -/** - * Information about a reported diagnostic. - * - *

    Checker Framework uses a special format to put parseable information about a diagnostic into - * the message text. This object represents that information directly. - */ -final class DetailMessage extends TestDiagnostic { - - private static final Pattern MESSAGE_PATTERN = - Pattern.compile( - "(?\\S+):(?\\d+): (?" - + stream(DiagnosticKind.values()).map(k -> k.parseString).collect(joining("|")) - + "): (?.*)", - DOTALL); - - /** Parser for the output for -Adetailedmsgtext. */ - // Implemented here: org.checkerframework.framework.source.SourceChecker#detailedMsgTextPrefix - private static final Pattern DETAIL_MESSAGE_PATTERN = - Pattern.compile( - Joiner.on(" \\$\\$ ") - .join( - "\\((?[^)]+)\\)", "(?\\d+)", "(?.*)"), - DOTALL); - - private static final Pattern OFFSETS_PATTERN = - Pattern.compile("(\\( (?-?\\d+), (?-?\\d+) \\))?"); - - /** The path to the source file containing the diagnostic. */ - final Path file; - - /** The line number (1-based) of the diagnostic in the {@link #file}. */ - final int lineNumber; - - /** The message key for the user-visible text message that is emitted. */ - final String messageKey; - - /** The arguments to the text message format for the message key. */ - final ImmutableList messageArguments; - - /** The offset within the file of the start of the code covered by the diagnostic. */ - final Integer offsetStart; - - /** The offset within the file of the end (exclusive) of the code covered by the diagnostic. */ - final Integer offsetEnd; - - /** The user-visible message emitted for the diagnostic. */ - final String readableMessage; - - /** - * Returns an object parsed from a diagnostic message, or {@code null} if the message doesn't - * match the expected format. - * - * @param rootDirectory if not null, a root directory prefix to remove from the file part of the - * message - */ - static @Nullable DetailMessage parse(String input, @Nullable Path rootDirectory) { - Matcher messageMatcher = MESSAGE_PATTERN.matcher(input); - if (!messageMatcher.matches()) { - return null; - } - - Path file = Paths.get(messageMatcher.group("file")); - if (rootDirectory != null) { - file = rootDirectory.relativize(file); - } - int lineNumber = parseInt(messageMatcher.group("lineNumber")); - DiagnosticKind kind = DiagnosticKind.fromParseString(messageMatcher.group("kind")); - - String message = messageMatcher.group("message"); - Matcher detailsMatcher = DETAIL_MESSAGE_PATTERN.matcher(message); - if (!detailsMatcher.matches()) { - // Return a message with no key or parts. - return new DetailMessage(file, lineNumber, kind, "", ImmutableList.of(), null, null, message); - } - - int messagePartCount = parseInt(detailsMatcher.group("messagePartCount")); - List messageParts = - Splitter.on("$$") - .trimResults() - .limit(messagePartCount + 2) - .splitToList(detailsMatcher.group("messageParts")); - ImmutableList messageArguments = - ImmutableList.copyOf(Iterables.limit(messageParts, messagePartCount)); - String readableMessage = getLast(messageParts); - - Matcher offsetsMatcher = OFFSETS_PATTERN.matcher(messageParts.get(messagePartCount)); - checkArgument(offsetsMatcher.matches(), "unparseable offsets: %s", input); - - return new DetailMessage( - file, - lineNumber, - kind, - detailsMatcher.group("messageKey"), - messageArguments, - intOrNull(offsetsMatcher.group("start")), - intOrNull(offsetsMatcher.group("end")), - readableMessage); - } - - private static Integer intOrNull(String input) { - return input == null ? null : parseInt(input); - } - - private DetailMessage( - Path file, - int lineNumber, - DiagnosticKind diagnosticKind, - String messageKey, - ImmutableList messageArguments, - Integer offsetStart, - Integer offsetEnd, - String readableMessage) { - super(file.toString(), lineNumber, diagnosticKind, readableMessage, false, true); - this.file = file; - this.lineNumber = lineNumber; - this.messageKey = messageKey; - this.messageArguments = messageArguments; - this.offsetStart = offsetStart; - this.offsetEnd = offsetEnd; - this.readableMessage = readableMessage; - } - - /** The last part of the {@link #file}. */ - String getFileName() { - return file.getFileName().toString(); - } - - /** - * True if this was parsed from an actual {@code -Adetailedmsgtext} message; false if this was - * some other diagnostic. - */ - boolean hasDetails() { - return !messageKey.equals(""); - } - - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - DetailMessage that = (DetailMessage) o; - return lineNumber == that.lineNumber - && file.equals(that.file) - && messageKey.equals(that.messageKey) - && messageArguments.equals(that.messageArguments) - && Objects.equals(offsetStart, that.offsetStart) - && Objects.equals(offsetEnd, that.offsetEnd) - && readableMessage.equals(that.readableMessage); - } - - @Override - public int hashCode() { - return Objects.hash( - file, lineNumber, messageKey, messageArguments, offsetStart, offsetEnd, readableMessage); - } - - @Override - public String toString() { - return String.format("%s:%d: (%s) %s", file, lineNumber, messageKey, readableMessage); - } - - /** String format for debugging use. */ - String toDetailedString() { - return toStringHelper(this) - .add("file", file) - .add("lineNumber", lineNumber) - .add("messageKey", messageKey) - .add("messageArguments", messageArguments) - .add("offsetStart", offsetStart) - .add("offsetEnd", offsetEnd) - .add("readableMessage", readableMessage) - .toString(); - } -} diff --git a/src/test/java/tests/NullSpecTest.java b/src/test/java/tests/NullSpecTest.java index fd198187..3f127d5b 100644 --- a/src/test/java/tests/NullSpecTest.java +++ b/src/test/java/tests/NullSpecTest.java @@ -22,6 +22,7 @@ import java.util.ListIterator; import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest; import org.checkerframework.framework.test.TypecheckResult; +import org.checkerframework.framework.test.diagnostics.DetailedTestDiagnostic; import org.checkerframework.framework.test.diagnostics.DiagnosticKind; import org.checkerframework.framework.test.diagnostics.TestDiagnostic; import org.checkerframework.javacutil.BugInCF; @@ -41,6 +42,30 @@ public static String[] getTestDirs() { } } + /** A small set of regression tests. */ + public static class Regression extends NullSpecTest { + public Regression(List testFiles) { + super(testFiles, false); + } + + @Parameters + public static String[] getTestDirs() { + return new String[] {"regression"}; + } + } + + /** A small set of strict regression tests. */ + public static class RegressionStrict extends NullSpecTest { + public RegressionStrict(List testFiles) { + super(testFiles, true); + } + + @Parameters + public static String[] getTestDirs() { + return new String[] {"regression-strict"}; + } + } + /** A test that ignores cases where there is limited nullness information. */ public static class Lenient extends NullSpecTest { public Lenient(List testFiles) { @@ -78,8 +103,7 @@ private static String[] getSamplesDirs() { private static String[] checkerOptions(boolean strict) { ImmutableList.Builder options = ImmutableList.builder(); - options.add( - "-AassumePure", "-Adetailedmsgtext", "-AcheckImpl", "-AsuppressWarnings=conditional"); + options.add("-AassumePure", "-AcheckImpl", "-AsuppressWarnings=conditional"); if (strict) { options.add("-Astrict"); } @@ -105,10 +129,8 @@ public TypecheckResult adjustTypecheckResult(TypecheckResult testResult) { for (ListIterator i = unexpected.listIterator(); i.hasNext(); ) { TestDiagnostic diagnostic = i.next(); - DetailMessage detailMessage = DetailMessage.parse(diagnostic.getMessage(), null); - if (detailMessage != null && detailMessage.hasDetails()) { - // Replace diagnostics that can be parsed with DetailMessage diagnostics. - i.set(detailMessage); + if (diagnostic instanceof DetailedTestDiagnostic) { + // Keep all detailed test diagnostics. } else if (diagnostic.getKind() != DiagnosticKind.Error) { // Remove warnings like explicit.annotation.ignored and deprecation. i.remove(); @@ -135,18 +157,9 @@ public TypecheckResult adjustTypecheckResult(TypecheckResult testResult) { * unexpected}, a reported diagnostic. */ private boolean corresponds(TestDiagnostic missing, TestDiagnostic unexpected) { - return unexpected instanceof DetailMessage - && corresponds(missing, ((DetailMessage) unexpected)); - } - - /** - * Returns {@code true} if {@code missing} is a JSpecify directive that matches {@code - * unexpected}, a reported diagnostic. - */ - private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) { // First, make sure the two diagnostics are on the same file and line. - if (!missing.getFilename().equals(unexpected.getFileName()) - || missing.getLineNumber() != unexpected.lineNumber) { + if (!missing.getFilename().equals(unexpected.getFilename()) + || missing.getLineNumber() != unexpected.getLineNumber()) { return false; } @@ -155,21 +168,21 @@ private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) { || missing.getMessage().contains("jspecify_nullness_not_enough_information") || missing.getMessage().contains("jspecify_nullness_mismatch") || missing.getMessage().contains("test:cannot-convert")) { - switch (unexpected.messageKey) { - case "argument": - case "assignment": + switch (unexpected.getMessageKey()) { + case "argument.type.incompatible": + case "assignment.type.incompatible": case "atomicreference.must.include.null": case "cast.unsafe": case "dereference": - case "lambda.param": - case "methodref.receiver.bound": - case "methodref.receiver": - case "methodref.return": - case "override.param": - case "override.return": - case "return": + case "lambda.param.type.incompatible": + case "methodref.receiver.bound.invalid": + case "methodref.receiver.invalid": + case "methodref.return.invalid": + case "override.param.invalid": + case "override.return.invalid": + case "return.type.incompatible": case "threadlocal.must.include.null": - case "type.argument": + case "type.argument.type.incompatible": return true; default: return false; @@ -178,7 +191,7 @@ private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) { switch (missing.getMessage()) { case "jspecify_nullness_intrinsically_not_nullable": - switch (unexpected.messageKey) { + switch (unexpected.getMessageKey()) { case "enum.constant.annotated": case "outer.annotated": case "primitive.annotated": @@ -187,7 +200,7 @@ private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) { return false; } case "jspecify_unrecognized_location": - switch (unexpected.messageKey) { + switch (unexpected.getMessageKey()) { /* * We'd rather avoid this `bound` error (in part because it suggests that the annotation * is having some effect, which we don't want!), but the most important thing is that the @@ -196,7 +209,7 @@ private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) { * custom `*.annotated` error. This test probably doesn't confirm that second thing * anymore, but I did manually confirm that it is true as of this writing. */ - case "bound": + case "bound.type.incompatible": case "local.variable.annotated": case "type.parameter.annotated": case "wildcard.annotated": @@ -205,8 +218,9 @@ private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) { return false; } case "jspecify_conflicting_annotations": - switch (unexpected.messageKey) { - case "conflicting.annos": + switch (unexpected.getMessageKey()) { + case "type.invalid.conflicting.annos": + case "type.invalid.super.wildcard": return true; default: return false; diff --git a/tests/ConformanceTestOnSamples-report.txt b/tests/ConformanceTestOnSamples-report.txt index 34f25846..9e5f7006 100644 --- a/tests/ConformanceTestOnSamples-report.txt +++ b/tests/ConformanceTestOnSamples-report.txt @@ -1,4 +1,4 @@ -# 75 pass; 498 fail; 573 total; 13.1% score +# 74 pass; 499 fail; 573 total; 12.9% score FAIL: AnnotatedInnerOfNonParameterized.java: no unexpected facts FAIL: AnnotatedInnerOfParameterized.java: no unexpected facts FAIL: AnnotatedReceiver.java: no unexpected facts @@ -115,7 +115,7 @@ FAIL: ContainmentSuper.java: no unexpected facts FAIL: ContainmentSuperVsExtends.java:24:jspecify_nullness_mismatch FAIL: ContainmentSuperVsExtends.java: no unexpected facts FAIL: ContainmentSuperVsExtendsSameType.java:23:jspecify_nullness_mismatch -PASS: ContainmentSuperVsExtendsSameType.java: no unexpected facts +FAIL: ContainmentSuperVsExtendsSameType.java: no unexpected facts PASS: ContravariantReturns.java:30:jspecify_nullness_mismatch PASS: ContravariantReturns.java:34:jspecify_nullness_mismatch PASS: ContravariantReturns.java:38:jspecify_nullness_mismatch @@ -302,7 +302,7 @@ FAIL: NullLiteralToTypeVariable.java: no unexpected facts FAIL: NullLiteralToTypeVariableUnionNull.java: no unexpected facts FAIL: NullLiteralToTypeVariableUnspec.java: no unexpected facts FAIL: NullMarkedDirectUseOfNotNullMarkedBoundedTypeVariable.java: no unexpected facts -FAIL: NullUnmarkedUndoesNullMarked.java: no unexpected facts +PASS: NullUnmarkedUndoesNullMarked.java: no unexpected facts FAIL: NullUnmarkedUndoesNullMarkedForWildcards.java: no unexpected facts PASS: NullnessDoesNotAffectOverloadSelection.java:23:jspecify_nullness_mismatch PASS: NullnessDoesNotAffectOverloadSelection.java: no unexpected facts @@ -542,7 +542,7 @@ PASS: nullnessUnspecifiedTypeParameter/nullnessunspecifiedtypeparameter/Nullness PASS: nullnessUnspecifiedTypeParameter/nullnessunspecifiedtypeparameter/NullnessUnspecifiedTypeParameter.java:53:jspecify_nullness_mismatch PASS: nullnessUnspecifiedTypeParameter/nullnessunspecifiedtypeparameter/NullnessUnspecifiedTypeParameter.java:57:jspecify_nullness_mismatch PASS: nullnessUnspecifiedTypeParameter/nullnessunspecifiedtypeparameter/NullnessUnspecifiedTypeParameter.java:59:jspecify_nullness_mismatch -PASS: nullnessUnspecifiedTypeParameter/nullnessunspecifiedtypeparameter/NullnessUnspecifiedTypeParameter.java: no unexpected facts +FAIL: nullnessUnspecifiedTypeParameter/nullnessunspecifiedtypeparameter/NullnessUnspecifiedTypeParameter.java: no unexpected facts PASS: packageDefault/packagedefault/Bar.java:23:test:cannot-convert:Object? to Object! PASS: packageDefault/packagedefault/Bar.java: no unexpected facts PASS: packageDefault/packagedefault/package-info.java: no unexpected facts diff --git a/tests/minimal/Demo.java b/tests/minimal/Demo.java index cce60abe..f29690cf 100644 --- a/tests/minimal/Demo.java +++ b/tests/minimal/Demo.java @@ -18,7 +18,7 @@ @NullMarked class Demo { Object mismatch(@Nullable Object o) { - // jspecify_nullness_mismatch + // :: error: jspecify_nullness_mismatch return o; } } diff --git a/tests/regression-strict/Issue177.java b/tests/regression-strict/Issue177.java new file mode 100644 index 00000000..c47c8a9e --- /dev/null +++ b/tests/regression-strict/Issue177.java @@ -0,0 +1,18 @@ +// Copyright 2024 The JSpecify Authors +// +// 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. + +// Test case for Issue 177: +// https://github.com/jspecify/jspecify-reference-checker/issues/177 + +class Issue177 {} diff --git a/tests/regression/Issue159.java b/tests/regression/Issue159.java new file mode 100644 index 00000000..ab44b010 --- /dev/null +++ b/tests/regression/Issue159.java @@ -0,0 +1,26 @@ +// Copyright 2024 The JSpecify Authors +// +// 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. + +// Test case for Issue 159: +// https://github.com/jspecify/jspecify-reference-checker/issues/159 + +import java.util.ArrayList; +import org.jspecify.annotations.NullMarked; + +@NullMarked +class Issue159 extends ArrayList { + Issue159 foo() { + return new Issue159(); + } +} diff --git a/tests/regression/Issue163.java b/tests/regression/Issue163.java new file mode 100644 index 00000000..36de896e --- /dev/null +++ b/tests/regression/Issue163.java @@ -0,0 +1,31 @@ +// Copyright 2024 The JSpecify Authors +// +// 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. + +// Test case for Issue 163: +// https://github.com/jspecify/jspecify-reference-checker/issues/163 + +import org.jspecify.annotations.NullMarked; + +@NullMarked +class Issue163NullForUnspecVoid { + void x(Issue163Value val, Issue163Visitor vis) { + val.accept(vis, null); + } +} + +interface Issue163Value { +

    void accept(Issue163Visitor

    visitor, P param); +} + +interface Issue163Visitor

    {} diff --git a/tests/regression/Issue164.java b/tests/regression/Issue164.java new file mode 100644 index 00000000..10f0d86c --- /dev/null +++ b/tests/regression/Issue164.java @@ -0,0 +1,33 @@ +// Copyright 2024 The JSpecify Authors +// +// 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. + +// Test case for Issue 164: +// https://github.com/jspecify/jspecify-reference-checker/issues/164 + +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@NullMarked +interface Issue164SuperWildcardParent { + void x(Issue164Foo foo); +} + +@NullMarked +interface Issue164SuperWildcardOverride extends Issue164SuperWildcardParent { + @Override + void x(Issue164Foo foo); +} + +@NullMarked +interface Issue164Foo {} diff --git a/tests/regression/Issue164More.java b/tests/regression/Issue164More.java new file mode 100644 index 00000000..2a77d332 --- /dev/null +++ b/tests/regression/Issue164More.java @@ -0,0 +1,32 @@ +// Copyright 2024 The JSpecify Authors +// +// 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. + +// Test case for Issue 164: +// https://github.com/jspecify/jspecify-reference-checker/issues/164 + +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@NullMarked +class Issue164More { + interface Super { + void foo(Lib lib); + } + + interface Sub extends Super { + void foo(Lib lib); + } + + interface Lib {} +} diff --git a/tests/regression/Issue172.java b/tests/regression/Issue172.java new file mode 100644 index 00000000..b782b8df --- /dev/null +++ b/tests/regression/Issue172.java @@ -0,0 +1,37 @@ +// Copyright 2024 The JSpecify Authors +// +// 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. + +// Test case for Issue 172: +// https://github.com/jspecify/jspecify-reference-checker/issues/172 + +import org.jspecify.annotations.NullMarked; + +class Issue172 { + E e() { + return null; + } + + void p(E p) { + p = null; + } +} + +class Issue172UnmarkedUse { + void foo(Issue172 p) {} +} + +@NullMarked +class Issue172MarkedUse { + void foo(Issue172 p) {} +} diff --git a/tests/regression/Issue197.java b/tests/regression/Issue197.java new file mode 100644 index 00000000..eac3ca61 --- /dev/null +++ b/tests/regression/Issue197.java @@ -0,0 +1,33 @@ +// Copyright 2024 The JSpecify Authors +// +// 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. + +// Test case for Issue 197: +// https://github.com/jspecify/jspecify-reference-checker/issues/197 + +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +class Issue197 { + interface Function {} + + interface Super { + void i(Function p); + } + + @NullMarked + interface Sub extends Super { + @Override + void i(Function p); + } +} diff --git a/tests/regression/OverrideTest.java b/tests/regression/OverrideTest.java new file mode 100644 index 00000000..c34e08cf --- /dev/null +++ b/tests/regression/OverrideTest.java @@ -0,0 +1,44 @@ +// Copyright 2024 The JSpecify Authors +// +// 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. + +// Test case based on comment: +// https://github.com/jspecify/jspecify-reference-checker/pull/165#issuecomment-2030038854 + +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@NullMarked +class OverrideTest { + abstract class Super { + abstract void accept(MarkedEntry entry); + } + + abstract class Sub extends Super { + @Override + abstract void accept(MarkedEntry entry); + } + + interface MarkedEntry {} + + abstract class SuperUnmarked { + abstract void accept(OverrideTestUnmarkedEntry entry); + } + + abstract class SubUnmarked extends SuperUnmarked { + @Override + abstract void accept(OverrideTestUnmarkedEntry entry); + } +} + +interface OverrideTestUnmarkedEntry {} diff --git a/tests/regression/UnboundedDefaultsToNullable.java b/tests/regression/UnboundedDefaultsToNullable.java new file mode 100644 index 00000000..6457e3a3 --- /dev/null +++ b/tests/regression/UnboundedDefaultsToNullable.java @@ -0,0 +1,26 @@ +// Copyright 2024 The JSpecify Authors +// +// 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. + +// Test case for Issue 161: +// https://github.com/jspecify/jspecify-reference-checker/issues/161 + +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@NullMarked +class UnboundedDefaultsToNullable { + UnboundedDefaultsToNullable x() { + return this; + } +} diff --git a/usage-demo/README.md b/usage-demo/README.md new file mode 100644 index 00000000..3aaf2233 --- /dev/null +++ b/usage-demo/README.md @@ -0,0 +1,21 @@ +# JSpecify Reference Checker Usage Demo + +This is a simple demonstration for how a gradle project can use the JSpecify Reference Checker. + +Until the JSpecify Reference Checker is released to Maven Central, in the parent directory, +one must first run: + +```` +./gradlew PublishToMavenLocal +```` + +to publish the JSpecify Reference Checker to the local Maven repository. + +Then, in the current `usage-demo` directory, one can run: + +```` +../gradlew assemble +```` + +to assemble the demo project and get a set of three expected error messages +(plus one warning from Error Prone). diff --git a/usage-demo/build.gradle b/usage-demo/build.gradle new file mode 100644 index 00000000..e24b982b --- /dev/null +++ b/usage-demo/build.gradle @@ -0,0 +1,71 @@ +// EISOP Checker Framework and JSpecify Reference Checker example. + +plugins { + id 'java' + id 'com.diffplug.spotless' version '6.25.0' + id 'net.ltgt.errorprone' version '4.1.0' + id 'org.checkerframework' version '0.6.48' apply false +} + +ext { + versions = [ + eisopVersion: '3.42.0-eisop5', + jspecifyVersion: '1.0.0', + jspecifyReferenceCheckerVersion: '0.0.0-SNAPSHOT' + ] +} + +repositories { + mavenLocal() + mavenCentral() +} + +// Project dependencies, e.g. error prone. +dependencies { + errorprone 'com.google.errorprone:error_prone_core:2.36.0' +} + +// Dependency on JSpecify annotations. +dependencies { + implementation "org.jspecify:jspecify:${versions.jspecifyVersion}" +} + + +apply plugin: 'org.checkerframework' + +// Configure EISOP and JSpecify Reference Checker +dependencies { + compileOnly "io.github.eisop:checker-qual:${versions.eisopVersion}" + testCompileOnly "io.github.eisop:checker-qual:${versions.eisopVersion}" + checkerFramework "io.github.eisop:checker-qual:${versions.eisopVersion}" + checkerFramework "io.github.eisop:checker:${versions.eisopVersion}" + + compileOnly "org.jspecify.reference:checker:${versions.jspecifyReferenceCheckerVersion}" + checkerFramework "org.jspecify.reference:checker:${versions.jspecifyReferenceCheckerVersion}" +} + +checkerFramework { + checkers = [ + 'com.google.jspecify.nullness.NullSpecChecker', + ] + extraJavacArgs = [ + // Check implementation code, i.e. method bodies. + '-AcheckImpl', + // Output the EISOP version, to ensure configuration is correct. + '-Aversion' + ] +} + +spotless { + java { + target '**/*.java' + googleJavaFormat() + formatAnnotations() + } + groovyGradle { + target '**/*.gradle' + greclipse() + indentWithSpaces(4) + trimTrailingWhitespace() + } +} diff --git a/usage-demo/settings.gradle b/usage-demo/settings.gradle new file mode 100644 index 00000000..a632f3be --- /dev/null +++ b/usage-demo/settings.gradle @@ -0,0 +1,2 @@ +// Project name is read-only in build scripts, and defaults to directory name. +rootProject.name = "jspecify-reference-checker-usage-demo" diff --git a/usage-demo/src/main/java/demo/Demo.java b/usage-demo/src/main/java/demo/Demo.java new file mode 100644 index 00000000..8f57b853 --- /dev/null +++ b/usage-demo/src/main/java/demo/Demo.java @@ -0,0 +1,26 @@ +package demo; + +import java.lang.reflect.Method; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@NullMarked +class Demo { + // Error about usage on primitive, also a warning from Error Prone. + void conflict(@Nullable int i) {} + + Object incompatible(@Nullable Object in) { + // Error about incompatible return. + return in; + } + + String deref(@Nullable Object in) { + // Error about dereference of nullable reference. + return in.toString(); + } + + void jdkDemo(Method m) throws Exception { + // Demo to ensure the jspecify/jdk is used. No error expected. eisop/jdk would give an error. + m.invoke(null); + } +}