diff --git a/.buildscript/check_git_clean.sh b/.buildscript/check_git_clean.sh index 8668c07c9d..1ffa522f07 100755 --- a/.buildscript/check_git_clean.sh +++ b/.buildscript/check_git_clean.sh @@ -1,3 +1,5 @@ +#!/bin/sh -eux + if [ -n "$(git status --porcelain)" ]; then echo 'warning: source tree contains uncommitted changes; .gitignore patterns may need to be fixed' git status diff --git a/.github/codecov.yml b/.github/codecov.yml new file mode 100644 index 0000000000..0f6ecbeab1 --- /dev/null +++ b/.github/codecov.yml @@ -0,0 +1,4 @@ +codecov: + # Disable report age checking since hits in the build cache can lead to an old timestamp + max_report_age: off + diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 9bb0d03707..acf0d55842 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -14,63 +14,78 @@ jobs: matrix: include: - os: ubuntu-latest - java: 8 + java: 11 epVersion: 2.4.0 - os: ubuntu-latest - java: 11 + java: 17 epVersion: 2.4.0 - os: macos-latest java: 11 - epVersion: 2.18.0 + epVersion: 2.21.1 - os: ubuntu-latest java: 11 - epVersion: 2.18.0 + epVersion: 2.21.1 - os: windows-latest java: 11 - epVersion: 2.18.0 + epVersion: 2.21.1 - os: ubuntu-latest java: 17 - epVersion: 2.18.0 + epVersion: 2.21.1 fail-fast: false runs-on: ${{ matrix.os }} steps: - name: Check out NullAway sources uses: actions/checkout@v3 + - name: 'Set up JDK 21 so it is available' + uses: actions/setup-java@v3 + with: + java-version: '21-ea' + distribution: 'temurin' + - name: 'Set up JDK 17 on Windows' + uses: actions/setup-java@v3 + with: + java-version: '17' + distribution: 'temurin' + if: matrix.os == 'windows-latest' - name: 'Set up JDK ${{ matrix.java }}' uses: actions/setup-java@v3 with: java-version: ${{ matrix.java }} distribution: 'temurin' - - name: Build and test using Java 8 and Error Prone ${{ matrix.epVersion }} + - name: Build and test using Java ${{ matrix.java }} and Error Prone ${{ matrix.epVersion }} env: ORG_GRADLE_PROJECT_epApiVersion: ${{ matrix.epVersion }} uses: gradle/gradle-build-action@v2 with: arguments: build - if: matrix.java == '8' - - name: Build and test using Java 11 and Error Prone ${{ matrix.epVersion }} - env: - ORG_GRADLE_PROJECT_epApiVersion: ${{ matrix.epVersion }} + - name: Run shellcheck uses: gradle/gradle-build-action@v2 with: - arguments: verGJF build - if: matrix.java == '11' - - name: Build and test using Java 17 and Error Prone ${{ matrix.epVersion }} + arguments: shellcheck + if: runner.os == 'Linux' + - name: Aggregate jacoco coverage + id: jacoco_report + uses: gradle/gradle-build-action@v2 env: ORG_GRADLE_PROJECT_epApiVersion: ${{ matrix.epVersion }} - uses: gradle/gradle-build-action@v2 with: - arguments: build -x :sample-app:build -x :jar-infer:jar-infer-lib:build -x :jar-infer:nullaway-integration-test:build -x :jar-infer:test-java-lib-jarinfer:build - if: matrix.java == '17' - - name: Report jacoco coverage - uses: gradle/gradle-build-action@v2 + arguments: codeCoverageReport + continue-on-error: true + if: runner.os == 'Linux' && matrix.java == '11' && matrix.epVersion == '2.21.1' && github.repository == 'uber/NullAway' + - name: Upload coverage reports to Codecov + uses: codecov/codecov-action@v3 + with: + files: ./code-coverage-report/build/reports/jacoco/codeCoverageReport/codeCoverageReport.xml + if: steps.jacoco_report.outcome == 'success' + - name: Test publishToMavenLocal flow env: ORG_GRADLE_PROJECT_epApiVersion: ${{ matrix.epVersion }} - COVERALLS_REPO_TOKEN: ${{ secrets.COVERALLS_REPO_TOKEN }} + ORG_GRADLE_PROJECT_VERSION_NAME: '0.0.0.1-LOCAL' + ORG_GRADLE_PROJECT_RELEASE_SIGNING_ENABLED: 'false' + uses: gradle/gradle-build-action@v2 with: - arguments: coveralls - continue-on-error: true - if: runner.os == 'Linux' && matrix.java == '11' && matrix.epVersion == '2.18.0' && github.repository == 'uber/NullAway' + arguments: publishToMavenLocal + if: matrix.java == '11' - name: Check that Git tree is clean after build and test run: ./.buildscript/check_git_clean.sh publish_snapshot: @@ -81,15 +96,15 @@ jobs: steps: - name: 'Check out repository' uses: actions/checkout@v3 - - name: 'Set up JDK 8' + - name: 'Set up JDK 11' uses: actions/setup-java@v3 with: - java-version: 8 + java-version: 11 distribution: 'temurin' - name: 'Publish' uses: gradle/gradle-build-action@v2 env: - ORG_GRADLE_PROJECT_mavenCentralRepositoryUsername: ${{ secrets.SONATYPE_NEXUS_USERNAME }} - ORG_GRADLE_PROJECT_mavenCentralRepositoryPassword: ${{ secrets.SONATYPE_NEXUS_PASSWORD }} + ORG_GRADLE_PROJECT_mavenCentralUsername: ${{ secrets.SONATYPE_NEXUS_USERNAME }} + ORG_GRADLE_PROJECT_mavenCentralPassword: ${{ secrets.SONATYPE_NEXUS_PASSWORD }} with: arguments: clean publish --no-daemon --no-parallel diff --git a/.github/workflows/gcloud_ssh.sh b/.github/workflows/gcloud_ssh.sh new file mode 100644 index 0000000000..80d9e68795 --- /dev/null +++ b/.github/workflows/gcloud_ssh.sh @@ -0,0 +1,11 @@ +#!/bin/bash + +# This script is used to run commands on a Google Cloud instance via SSH + +# Define the variables for Google Cloud project, zone, username, and instance +PROJECT_ID="ucr-ursa-major-sridharan-lab" +ZONE="us-central1-a" +USER="root" +INSTANCE="nullway-jmh" + +gcloud compute ssh --project=$PROJECT_ID --zone=$ZONE $USER@$INSTANCE --command="$1" diff --git a/.github/workflows/get_repo_details.sh b/.github/workflows/get_repo_details.sh new file mode 100644 index 0000000000..fc448af858 --- /dev/null +++ b/.github/workflows/get_repo_details.sh @@ -0,0 +1,20 @@ +#!/bin/bash + +# This script retrieves the repository and branch details of a GitHub pull request + +# Assign command line arguments to variables +# GH_TOKEN is the GitHub authentication token +# PR_NUMBER is the number of the pull request +# REPO_NAME is the name of the repository +GH_TOKEN="$1" +PR_NUMBER="$2" +REPO_NAME="$3" + +PR_DETAILS=$(curl -s -H "Authorization: token $GH_TOKEN" "https://api.github.com/repos/$REPO_NAME/pulls/$PR_NUMBER") + +REPO_FULL_NAME=$(echo "$PR_DETAILS" | jq -r .head.repo.full_name) +BRANCH_NAME=$(echo "$PR_DETAILS" | jq -r .head.ref) + +# Export vars to GITHUB_ENV so they can be used by later scripts +echo "REPO_FULL_NAME=$REPO_FULL_NAME" >> "$GITHUB_ENV" +echo "BRANCH_NAME=$BRANCH_NAME" >> "$GITHUB_ENV" diff --git a/.github/workflows/jmh-benchmark.yml b/.github/workflows/jmh-benchmark.yml new file mode 100644 index 0000000000..aed98872d7 --- /dev/null +++ b/.github/workflows/jmh-benchmark.yml @@ -0,0 +1,73 @@ +# This GitHub Actions workflow runs JMH benchmarks when a new comment is created on a pull request +name: Run JMH Benchmarks for Pull Request + +on: + issue_comment: # This workflow triggers when a comment is created + types: [created] + +# Only allow one instance of JMH benchmarking to be running at any given time +concurrency: all + +jobs: + benchmarking: + # Only run this job if a comment on a pull request contains '/benchmark' and is a PR on the uber/NullAway repository + if: github.event.issue.pull_request && contains(github.event.comment.body, '/benchmark') && github.repository == 'uber/NullAway' + runs-on: ubuntu-latest + permissions: write-all + + steps: + - name: Add reaction + uses: peter-evans/create-or-update-comment@v3 + with: + comment-id: ${{ github.event.comment.id }} + reactions: '+1' + + - name: Checkout repository + uses: actions/checkout@v3 + + - name: Set branch name + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + chmod +x ./.github/workflows/get_repo_details.sh + ./.github/workflows/get_repo_details.sh "${{ secrets.GITHUB_TOKEN }}" "${{ github.event.issue.number }}" "${{ github.repository }}" + + - id: 'auth' + name: Authenticating + uses: 'google-github-actions/auth@v1' + with: + credentials_json: '${{ secrets.GCP_SA_KEY_1 }}' + + - name: Set up Google Cloud SDK + uses: google-github-actions/setup-gcloud@v1 + + - name: Start VM + run: gcloud compute instances start nullway-jmh --zone=us-central1-a + + - name: Run benchmarks + run: | + chmod +x ./.github/workflows/run_gcp_benchmarks.sh + ./.github/workflows/run_gcp_benchmarks.sh + + - name: Cleanup + # Delete the branch directory on the Google Cloud instance + if: always() + run: | + ./.github/workflows/gcloud_ssh.sh " export BRANCH_NAME=${BRANCH_NAME} && rm -r -f $BRANCH_NAME" + + - name: Formatting Benchmark # Create a text file containing the benchmark results + run: | + (echo 'Main Branch:'; echo '```' ; cat main_text.txt; echo '```'; echo 'With This PR:'; echo '```' ; cat pr_text.txt; echo '```') > benchmark.txt + + - name: Comment Benchmark + uses: mshick/add-pr-comment@v2 + if: always() # This step is for adding the comment + with: + message-path: benchmark.txt # The path to the message file to leave as a comment + message-id: benchmark + - name: Stop VM + if: always() + run: gcloud compute instances stop nullway-jmh --zone=us-central1-a + + + diff --git a/.github/workflows/run_gcp_benchmarks.sh b/.github/workflows/run_gcp_benchmarks.sh new file mode 100644 index 0000000000..9dc8604f87 --- /dev/null +++ b/.github/workflows/run_gcp_benchmarks.sh @@ -0,0 +1,20 @@ +#!/bin/bash + +# This script is responsible for running benchmarks for a GitHub pull request and the main branch on Google Cloud Compute Engine (GCCE). + + +chmod +x ./.github/workflows/gcloud_ssh.sh +./.github/workflows/gcloud_ssh.sh "export BRANCH_NAME=${BRANCH_NAME} && mkdir $BRANCH_NAME" + +# Using gcloud compute scp to copy the bash scripts that will run the benchmarks onto the GCCE +gcloud compute scp ./.github/workflows/run_pr_benchmarks.sh root@nullway-jmh:"$BRANCH_NAME/" --zone=us-central1-a +gcloud compute scp ./.github/workflows/run_main_benchmarks.sh root@nullway-jmh:"$BRANCH_NAME/" --zone=us-central1-a + +# Running the benchmark script for the pull request branch and main branch on GCCE +./.github/workflows/gcloud_ssh.sh " export BRANCH_NAME=${BRANCH_NAME} && export REPO_NAME=${REPO_FULL_NAME} && chmod +x $BRANCH_NAME/run_pr_benchmarks.sh && $BRANCH_NAME/run_pr_benchmarks.sh && cd && chmod +x $BRANCH_NAME/run_main_benchmarks.sh && $BRANCH_NAME/run_main_benchmarks.sh" + +# Copying the benchmark results from GCCE back to the Github runner for the PR branch +gcloud compute scp root@nullway-jmh:"$BRANCH_NAME/pr/NullAway/jmh/build/results/jmh/results.txt" ./pr_text.txt --zone=us-central1-a + +# Copying the benchmark results from GCCE back to the Github runner for the main branch +gcloud compute scp root@nullway-jmh:"$BRANCH_NAME/main/NullAway/jmh/build/results/jmh/results.txt" ./main_text.txt --zone=us-central1-a diff --git a/.github/workflows/run_main_benchmarks.sh b/.github/workflows/run_main_benchmarks.sh new file mode 100644 index 0000000000..fad59dcd0d --- /dev/null +++ b/.github/workflows/run_main_benchmarks.sh @@ -0,0 +1,9 @@ +#!/bin/bash -eux + +cd "$BRANCH_NAME/" +mkdir main +cd main/ +git clone git@github.com:Uber/NullAway.git +cd NullAway/ + +./gradlew jmh --no-daemon diff --git a/.github/workflows/run_pr_benchmarks.sh b/.github/workflows/run_pr_benchmarks.sh new file mode 100644 index 0000000000..83c4a3b20d --- /dev/null +++ b/.github/workflows/run_pr_benchmarks.sh @@ -0,0 +1,9 @@ +#!/bin/bash -eux + +cd "$BRANCH_NAME/" +mkdir pr +cd pr/ +git clone --branch "$BRANCH_NAME" --single-branch git@github.com:"$REPO_NAME".git NullAway +cd NullAway/ + +./gradlew jmh --no-daemon diff --git a/CHANGELOG.md b/CHANGELOG.md index ebf48af143..26c65e8f08 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,88 @@ Changelog ========= +Version 0.10.14 +--------------- +IMPORTANT: This version introduces EXPERIMENTAL JDK21 support. +* Bump Checker Framework dependency to 3.38.0 (#819) + - Note: Not just an internal implementation change. Needed to support JDK 21! +* Treat parameter of generated Record.equals() methods as @Nullable (#825) +* Build / CI tooling for NullAway itself: + - Fixes Codecov Report Expired error (#821) + - Updated Readme.md with Codecov link (#823) + - Remove ASM-related hack in build config (#824) + - Run tests on JDK 21 (#826) + +Version 0.10.13 +--------------- +* Allow library models to define custom stream classes (#807) +* Avoid suggesting castToNonNull fixes in certain cases (#799) +* Ensure castToNonNull insertion/removal suggested fixes do not remove comments (#815) +* Support for JSpecify's 0.3.0 annotation [experimental] + - Generics checks for method overriding (#755) + - Make GenericsChecks methods static (#805) + - Add visitors for handling different types in generic type invariance check (#806) +* Build / CI tooling for NullAway itself: + - Bump versions for some dependencies (#800) + - Update to WALA 1.6.2 (#798) + - Update to Error Prone 2.21.1 (#797) + - Enable contract checking when building NullAway (#802) + - Bump Error Prone Gradle Plugin version (#804) + - Modify JMH Benchmark Workflow For Shellcheck (#813) + - Bump gradle maven publish plugin from 0.21.0 to 0.25.3 (#810) + - Use Spotless to enforce consistent formatting for Gradle build scripts (#809) + - Remove unnecessary compile dependence for jar-infer-cli (#816) + - Added Codecov to CI Pipeline (#820) + +Version 0.10.12 +--------------- +Note: This is the first release built with Java 11. In particular, running + JarInfer now requires a JDK 11 JVM. NullAway is still capable of analyzing JDK 8 + source/target projects, and should be compatible with the Error Prone JDK 9 javac + just as the release before, but a JDK 11 javac is recommended. +* Update to WALA 1.6.1 and remove ability to build on JDK 8 (#777) +* Fix compatibility issue when building on JDK 17 but running on JDK 8 (#779) +* Fix JDK compatibility issue in LombokHandler (#795) +* Improve auto-fixing of unnecessary castToNonNull calls (#796) +* Support for JSpecify's 0.3.0 annotation [experimental] + - JSpecify: avoid crashes when encountering raw types (#792) + - Fix off-by-one error in JSpecify checking of parameter passing (#793) +* Build / CI tooling for NullAway itself: + - Fix Publish Snapshot CI job (#774) + - Add step to create release on GitHub (#775) + - Build the Android sample app on JDK 17 (#776) + - Update to Error Prone 2.20.0 (#772) + - Add tasks to run JDK 8 tests on JDK 11+ (#778) + - Switch to Spotless for formatting Java code (#780) + - Added GCP JMH Benchmark Workflow (#770) + - Set concurrency for JMH benchmarking workflow (#784) + - Disable daemon when running benchmarks (#786) + - Update to Gradle 8.2.1 (#781) + +Version 0.10.11 +--------------- +* NULL_LITERAL expressions may always be null (#749) +* Fix error in Lombok generated code for @Nullable @Builder.Default (#765) +* Support for specific libraries/APIs: + - Added support for Apache Validate (#769) + - Introduce FluentFutureHandler as a workaround for Guava FluentFuture (#771) +* Internal code refactorings: + - [Refactor] Pass resolved Symbols into Handler methods (#729) + - Prepare for Nullable ASTHelpers.getSymbol (#733) + - Refactor: streamline mayBeNullExpr flow (#753) + - Refactor LibraryModelsHandler.onOverrideMayBeNullExpr (#754) + - Refactor simple onOverrideMayBeNullExpr handlers (#747) +* Support for JSpecify's 0.3.0 annotation [experimental] + - JSpecify generics checks for conditional expressions (#739) + - Generics checks for parameter passing (#746) + - Clearer printing of types in errors related to generics (#758) +* NullAwayInfer/Annotator data serialization support [experimental] + - Update path serialization for class files (#752) +* Build / CI tooling for NullAway itself: + - Update to Gradle 8.0.2 (#743) + - Fix CI on Windows (#759) + - Upgrade to Error Prone 2.19.1 (#763) + - Upgrade maven publish plugin to 0.21.0 (#773) + Version 0.10.10 --------------- * Add command line option to skip specific library models. (#741) diff --git a/README.md b/README.md index fa62c4e80b..41d6e21f64 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -## NullAway: Fast Annotation-Based Null Checking for Java [![Build Status](https://github.com/uber/nullaway/actions/workflows/continuous-integration.yml/badge.svg)](https://github.com/uber/nullaway/actions/workflows/continuous-integration.yml) [![Coverage Status](https://coveralls.io/repos/github/uber/NullAway/badge.svg?branch=master)](https://coveralls.io/github/uber/NullAway?branch=master) +## NullAway: Fast Annotation-Based Null Checking for Java [![Build Status](https://github.com/uber/nullaway/actions/workflows/continuous-integration.yml/badge.svg)](https://github.com/uber/nullaway/actions/workflows/continuous-integration.yml) [![Coverage Status](https://codecov.io/github/uber/NullAway/coverage.svg?branch=master)](https://codecov.io/github/uber/NullAway?branch=master) NullAway is a tool to help eliminate `NullPointerException`s (NPEs) in your Java code. To use NullAway, first add `@Nullable` annotations in your code wherever a field, method parameter, or return value may be `null`. Given these annotations, NullAway performs a series of type-based, local checks to ensure that any pointer that gets dereferenced in your code cannot be `null`. NullAway is similar to the type-based nullability checking in the Kotlin and Swift languages, and the [Checker Framework](https://checkerframework.org/) and [Eradicate](https://fbinfer.com/docs/checker-eradicate/) null checkers for Java. @@ -23,7 +23,7 @@ plugins { } dependencies { - annotationProcessor "com.uber.nullaway:nullaway:0.10.10" + annotationProcessor "com.uber.nullaway:nullaway:0.10.13" // Optional, some source of nullability annotations. // Not required on Android if you use the support @@ -75,7 +75,7 @@ The configuration for an Android project is very similar to the Java case, with ```gradle dependencies { - annotationProcessor "com.uber.nullaway:nullaway:0.10.10" + annotationProcessor "com.uber.nullaway:nullaway:0.10.13" errorprone "com.google.errorprone:error_prone_core:2.4.0" errorproneJavac "com.google.errorprone:javac:9+181-r4173-1" } diff --git a/RELEASING.md b/RELEASING.md index 3bcbbd1e98..e9635dbf34 100755 --- a/RELEASING.md +++ b/RELEASING.md @@ -1,7 +1,13 @@ -IMPORTANT: Make sure you are using a JDK 8 JVM by checking `java -version` before any -of the steps below. If you run the steps below on a JDK 11+ JVM, that will break Java -8 support, as the released jars will only run on JDK 11. We do not target Java 8 when -building on JDK 11 since Error Prone has required Java 11 since version 2.11.0. +(For testing only) Publishing an unsigned LOCAL build +===================================================== +By default, we set `RELEASE_SIGNING_ENABLED=true` in `gradle.properties`, which means +published builds must be signed unless they are for a `SNAPSHOT` version. To publish +a non-`SNAPSHOT` build locally without signing (e.g., a `LOCAL` version), use the +following command: + +```bash +ORG_GRADLE_PROJECT_RELEASE_SIGNING_ENABLED=false ./gradlew publishToMavenLocal +``` (Recommended, but optional) Update JarInfer Android SDK Models ============================================================== @@ -26,8 +32,9 @@ Releasing 3. Update the `README.md` with the new version. 4. `git commit -am "Prepare for release X.Y.Z."` (where X.Y.Z is the new version) 5. `git tag -a vX.Y.Z -m "Version X.Y.Z"` (where X.Y.Z is the new version) - 6. `./gradlew clean publish --no-daemon --no-parallel` + 6. `./gradlew clean publish` 7. Update the `gradle.properties` to the next SNAPSHOT version. 8. `git commit -am "Prepare next development version."` 9. `git push && git push --tags` 10. Visit [Sonatype Nexus](https://oss.sonatype.org/) and promote the artifact. + 11. Go to [this page](https://github.com/uber/NullAway/releases/new) to create a new release on GitHub, using the release notes from `CHANGELOG.md`. diff --git a/annotations/build.gradle b/annotations/build.gradle index 5ffa3cec63..ebb8ff5ca6 100644 --- a/annotations/build.gradle +++ b/annotations/build.gradle @@ -16,36 +16,11 @@ import net.ltgt.gradle.errorprone.CheckSeverity plugins { - id 'java-library' - id 'nullaway.jacoco-conventions' + id 'java-library' + id 'nullaway.java-test-conventions' } -sourceCompatibility = 1.8 - dependencies { } -test { - maxHeapSize = "1024m" - if (!JavaVersion.current().java9Compatible) { - jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" - } else { - // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer - jvmArgs += [ - "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", - // Accessed by Lombok tests - "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", - ] - } -} - apply plugin: 'com.vanniktech.maven.publish' diff --git a/build.gradle b/build.gradle index f63ac8f0dc..729ffc2539 100644 --- a/build.gradle +++ b/build.gradle @@ -23,29 +23,21 @@ buildscript { dependencies { classpath 'com.android.tools.build:gradle:7.3.0' - classpath 'com.vanniktech:gradle-maven-publish-plugin:0.14.2' - // This restriction is needed due to our mix of Android and Java modules; - // without it, the build fails with a weird error. - // See https://stackoverflow.com/questions/70217853/how-to-include-android-project-in-a-gradle-multi-project-build - classpath('org.ow2.asm:asm') { - version { - strictly '9.2' - } - } + classpath 'com.vanniktech:gradle-maven-publish-plugin:0.25.3' } } plugins { - id "com.github.sherter.google-java-format" version "0.9" - id "net.ltgt.errorprone" version "3.0.1" apply false - id "com.github.johnrengelman.shadow" version "8.1.0" apply false - id "com.github.kt3k.coveralls" version "2.12.0" apply false - id "me.champeau.jmh" version "0.6.8" apply false - id "com.github.ben-manes.versions" version "0.42.0" + id "com.diffplug.spotless" version "6.21.0" + id "net.ltgt.errorprone" version "3.1.0" apply false + id "com.github.johnrengelman.shadow" version "8.1.1" apply false + id "me.champeau.jmh" version "0.7.1" apply false + id "com.github.ben-manes.versions" version "0.48.0" + id "com.felipefzdz.gradle.shellcheck" version "1.4.6" } repositories { - // to get the google-java-format jar and dependencies - mavenCentral() + // to get the google-java-format jar and dependencies + mavenCentral() } apply from: "gradle/dependencies.gradle" @@ -57,64 +49,41 @@ subprojects { project -> } project.tasks.withType(JavaCompile) { dependsOn(installGitHooks) - if (JavaVersion.current().isJava9Compatible()) { - options.compilerArgs += [ - "-Xlint:deprecation", - "-Xlint:rawtypes", - "-Xlint:unchecked", - "-Werror" - ] - options.errorprone { - // disable warnings in generated code; AutoValue code fails UnnecessaryParentheses check - disableWarningsInGeneratedCode = true - // this check is too noisy - check("StringSplitter", CheckSeverity.OFF) - // https://github.com/google/error-prone/issues/3366 - check("CanIgnoreReturnValueSuggester", CheckSeverity.OFF) - // turn up various checks - check("WildcardImport", CheckSeverity.ERROR) - check("MissingBraces", CheckSeverity.ERROR) - check("TypeToString", CheckSeverity.ERROR) - check("SymbolToString", CheckSeverity.ERROR) - check("MultipleTopLevelClasses", CheckSeverity.ERROR) - check("ClassName", CheckSeverity.ERROR) - check("PackageLocation", CheckSeverity.ERROR) - check("UnnecessaryAnonymousClass", CheckSeverity.ERROR) - check("UnusedException", CheckSeverity.ERROR) - // To enable auto-patching, uncomment the line below, replace [CheckerName] with - // the checker(s) you want to apply patches for (comma-separated), and above, disable - // "-Werror" -// errorproneArgs.addAll("-XepPatchChecks:[CheckerName]", "-XepPatchLocation:IN_PLACE") - } - } else { - // Disable Error Prone checks on JDK 8, as more recent Error Prone versions don't run on JDK 8 - // NOTE: we use disableAllChecks rather than the enabled flag because we still want to use the - // JDK 9 javac packaged with Error Prone, to work around the following bug with JDK 8 javac - // and use of the @NullMarked / @NullUnmarked annotations from jspecify: - // https://github.com/jspecify/jspecify/issues/302 - options.errorprone.disableAllChecks = true + options.compilerArgs += [ + "-Xlint:deprecation", + "-Xlint:rawtypes", + "-Xlint:unchecked", + "-Werror" + ] + options.errorprone { + // disable warnings in generated code; AutoValue code fails UnnecessaryParentheses check + disableWarningsInGeneratedCode = true + // this check is too noisy + check("StringSplitter", CheckSeverity.OFF) + // https://github.com/google/error-prone/issues/3366 + check("CanIgnoreReturnValueSuggester", CheckSeverity.OFF) + // turn up various checks + check("WildcardImport", CheckSeverity.ERROR) + check("MissingBraces", CheckSeverity.ERROR) + check("TypeToString", CheckSeverity.ERROR) + check("SymbolToString", CheckSeverity.ERROR) + check("MultipleTopLevelClasses", CheckSeverity.ERROR) + check("ClassName", CheckSeverity.ERROR) + check("PackageLocation", CheckSeverity.ERROR) + check("UnnecessaryAnonymousClass", CheckSeverity.ERROR) + check("UnusedException", CheckSeverity.ERROR) + // To enable auto-patching, uncomment the line below, replace [CheckerName] with + // the checker(s) you want to apply patches for (comma-separated), and above, disable + // "-Werror" + // errorproneArgs.addAll("-XepPatchChecks:[CheckerName]", "-XepPatchLocation:IN_PLACE") } } - // We target Java 11 when building on JDK 11+, but Java 8 when building on JDK 8, since - // EP 2.11.0+ requires Java 11 - if (JavaVersion.current() >= JavaVersion.VERSION_11) { - tasks.withType(JavaCompile) { - java.sourceCompatibility = "11" - java.targetCompatibility = "11" - } - } else { - tasks.withType(JavaCompile) { - java.sourceCompatibility = "1.8" - java.targetCompatibility = "1.8" - } - } - - // Ensure we are running on Java 8 whenever publishing to remote repos - tasks.withType(PublishToMavenRepository) { - doFirst { - assert JavaVersion.current() == JavaVersion.VERSION_1_8 : "Only publish to remote repos on JDK 1.8" - } + // Target JDK 8. We need to use the older sourceCompatibility / targetCompatibility settings to get + // the build to work on JDK 11+. Once we stop supporting JDK 8, switch to using the javac "release" option + tasks.withType(JavaCompile) { + java.sourceCompatibility = "1.8" + java.targetCompatibility = "1.8" } tasks.withType(Test).configureEach { @@ -126,12 +95,42 @@ subprojects { project -> google() } + // For some reason, spotless complains when applied to the jar-infer folder itself, even + // though there is no top-level :jar-infer project + if (project.name != "jar-infer") { + project.apply plugin: "com.diffplug.spotless" + spotless { + java { + googleJavaFormat() + } + } + } +} + +spotless { + predeclareDeps() + groovyGradle { + target '**/*.gradle' + greclipse() + indentWithSpaces(4) + trimTrailingWhitespace() + endWithNewline() + } +} +spotlessPredeclare { + java { googleJavaFormat('1.17.0') } + groovyGradle { + greclipse() + } } -googleJavaFormat { - toolVersion = "1.14.0" - // don't enforce formatting for generated Java code under buildSrc - exclude 'buildSrc/build/**/*.java' +shellcheck { + useDocker = false + shellcheckBinary = "shellcheck" + sourceFiles = + fileTree(".") { + include("**/*.sh") + } } //////////////////////////////////////////////////////////////////////// @@ -140,9 +139,9 @@ googleJavaFormat { // tasks.register('installGitHooks', Copy) { - from(file('config/hooks/pre-commit-stub')) { - rename 'pre-commit-stub', 'pre-commit' - } - into file('.git/hooks') - fileMode 0777 + from(file('config/hooks/pre-commit-stub')) { + rename 'pre-commit-stub', 'pre-commit' + } + into file('.git/hooks') + fileMode 0777 } diff --git a/buildSrc/src/main/groovy/nullaway.jacoco-conventions.gradle b/buildSrc/src/main/groovy/nullaway.jacoco-conventions.gradle deleted file mode 100644 index 1c7c97c620..0000000000 --- a/buildSrc/src/main/groovy/nullaway.jacoco-conventions.gradle +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Copyright (C) 2021. Uber Technologies - * - * 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. - */ - -// Mostly taken from official Gradle sample: https://docs.gradle.org/current/samples/sample_jvm_multi_project_with_code_coverage.html -plugins { - id 'jacoco' -} - -jacoco { - toolVersion = "0.8.7" -} - -// Do not generate reports for individual projects -tasks.named("jacocoTestReport") { - enabled = false -} - -// Share sources folder with other projects for aggregated JaCoCo reports -configurations.create('transitiveSourcesElements') { - visible = false - canBeResolved = false - canBeConsumed = true - extendsFrom(configurations.implementation) - attributes { - attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage, Usage.JAVA_RUNTIME)) - attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category, Category.DOCUMENTATION)) - attribute(DocsType.DOCS_TYPE_ATTRIBUTE, objects.named(DocsType, 'source-folders')) - } - sourceSets.main.java.srcDirs.forEach { - outgoing.artifact(it) - } -} - -// Share the coverage data to be aggregated for the whole product -configurations.create('coverageDataElements') { - visible = false - canBeResolved = false - canBeConsumed = true - extendsFrom(configurations.implementation) - attributes { - attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage, Usage.JAVA_RUNTIME)) - attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category, Category.DOCUMENTATION)) - attribute(DocsType.DOCS_TYPE_ATTRIBUTE, objects.named(DocsType, 'jacoco-coverage-data')) - } - // This will cause the test task to run if the coverage data is requested by the aggregation task - outgoing.artifact(tasks.named("test").map { task -> - task.extensions.getByType(JacocoTaskExtension).destinationFile - }) -} diff --git a/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle b/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle new file mode 100644 index 0000000000..07f8f2690e --- /dev/null +++ b/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle @@ -0,0 +1,120 @@ +/* + * Copyright (C) 2023. Uber Technologies + * + * 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. + */ + +// Mostly taken from official Gradle sample: https://docs.gradle.org/current/samples/sample_jvm_multi_project_with_code_coverage.html +plugins { + id 'jacoco' +} + +jacoco { + toolVersion = "0.8.10" +} + +// Do not generate reports for individual projects +tasks.named("jacocoTestReport") { + enabled = false +} + +// Share sources folder with other projects for aggregated JaCoCo reports +configurations.create('transitiveSourcesElements') { + visible = false + canBeResolved = false + canBeConsumed = true + extendsFrom(configurations.implementation) + attributes { + attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage, Usage.JAVA_RUNTIME)) + attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category, Category.DOCUMENTATION)) + attribute(DocsType.DOCS_TYPE_ATTRIBUTE, objects.named(DocsType, 'source-folders')) + } + sourceSets.main.java.srcDirs.forEach { + outgoing.artifact(it) + } +} + +// Share the coverage data to be aggregated for the whole product +configurations.create('coverageDataElements') { + visible = false + canBeResolved = false + canBeConsumed = true + extendsFrom(configurations.implementation) + attributes { + attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage, Usage.JAVA_RUNTIME)) + attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category, Category.DOCUMENTATION)) + attribute(DocsType.DOCS_TYPE_ATTRIBUTE, objects.named(DocsType, 'jacoco-coverage-data')) + } + // This will cause the test task to run if the coverage data is requested by the aggregation task + outgoing.artifact(tasks.named("test").map { task -> + task.extensions.getByType(JacocoTaskExtension).destinationFile + }) +} + +test { + maxHeapSize = "1024m" + // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer + jvmArgs += [ + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + // Accessed by Lombok tests + "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", + ] +} + +// Create a task to test on JDK 21 +def testJdk21 = tasks.register("testJdk21", Test) { + onlyIf { + // Only test on JDK 21 when using the latest Error Prone version + deps.versions.errorProneApi == deps.versions.errorProneLatest + } + javaLauncher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.of(21) + } + + description = "Runs the test suite on JDK 21" + group = LifecycleBasePlugin.VERIFICATION_GROUP + + // Copy inputs from normal Test task. + def testTask = tasks.getByName("test") + classpath = testTask.classpath + testClassesDirs = testTask.testClassesDirs + maxHeapSize = "1024m" + // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer + jvmArgs += [ + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + // Accessed by Lombok tests + "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", + ] +} + +tasks.named('check').configure { + dependsOn testJdk21 +} diff --git a/code-coverage-report/build.gradle b/code-coverage-report/build.gradle index 9b4f755322..e9d3da6bcd 100644 --- a/code-coverage-report/build.gradle +++ b/code-coverage-report/build.gradle @@ -18,7 +18,6 @@ plugins { id 'java' id 'jacoco' - id 'com.github.kt3k.coveralls' } // Use JDK 17 for this module, via a toolchain. We need JDK 17 since this module @@ -71,19 +70,6 @@ def codeCoverageReport = tasks.register('codeCoverageReport', JacocoReport) { } } -coveralls { - jacocoReportPath = "build/reports/jacoco/codeCoverageReport/codeCoverageReport.xml" - afterEvaluate { - sourceDirs = sourcesPath.incoming.artifactView { lenient(true) }.files as List - } -} - -def coverallsTask = tasks.named('coveralls') - -coverallsTask.configure { - dependsOn 'codeCoverageReport' -} - // These dependencies indicate which projects have tests or tested code we want to include // when computing overall coverage. We aim to measure coverage for all code that actually ships // in a Maven artifact (so, e.g., we do not measure coverage for the jmh module) diff --git a/config/hooks/pre-commit b/config/hooks/pre-commit index f31a3cf79c..ffdb168862 100755 --- a/config/hooks/pre-commit +++ b/config/hooks/pre-commit @@ -7,6 +7,6 @@ REPO_ROOT_DIR="$(git rev-parse --show-toplevel)" files=$((git diff --cached --name-only --diff-filter=ACMR | grep -Ei "\.java$") || true) if [ ! -z "${files}" ]; then comma_files=$(echo "$files" | paste -s -d "," -) - "${REPO_ROOT_DIR}/gradlew" goJF -DgoogleJavaFormat.include="$comma_files" &>/dev/null + "${REPO_ROOT_DIR}/gradlew" spotlessApply -Pspotless.ratchet.from=HEAD >/dev/null 2>&1 git add $(echo "$files" | paste -s -d " " -) fi diff --git a/gradle.properties b/gradle.properties index 863eb5da2c..452472526a 100644 --- a/gradle.properties +++ b/gradle.properties @@ -12,7 +12,7 @@ org.gradle.caching=true org.gradle.jvmargs=-Xmx2g -XX:MaxMetaspaceSize=512m GROUP=com.uber.nullaway -VERSION_NAME=0.10.11-SNAPSHOT +VERSION_NAME=0.10.15-SNAPSHOT POM_DESCRIPTION=A fast annotation-based null checker for Java @@ -28,3 +28,7 @@ POM_LICENCE_DIST=repo POM_DEVELOPER_ID=uber POM_DEVELOPER_NAME=Uber Technologies POM_DEVELOPER_URL=https://uber.com + +# Publishing configuration for vanniktech/gradle-maven-publish-plugin +SONATYPE_HOST=DEFAULT +RELEASE_SIGNING_ENABLED=true diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 528862b5fc..1e9bb1c99a 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -19,10 +19,9 @@ import org.gradle.util.VersionNumber // The oldest version of Error Prone that we support running on def oldestErrorProneVersion = "2.4.0" // Latest released Error Prone version that we've tested with -def latestErrorProneVersion = "2.18.0" -// Default to using latest tested Error Prone version, except on Java 8, where 2.10.0 is the last version -// that works -def defaultErrorProneVersion = JavaVersion.current() >= JavaVersion.VERSION_11 ? latestErrorProneVersion : "2.10.0" +def latestErrorProneVersion = "2.21.1" +// Default to using latest tested Error Prone version +def defaultErrorProneVersion = latestErrorProneVersion def errorProneVersionToCompileAgainst = defaultErrorProneVersion // If the epApiVersion project property is set, compile and test against that version of Error Prone @@ -33,15 +32,15 @@ if (project.hasProperty("epApiVersion")) { } if (epApiVNum.compareTo(VersionNumber.parse(oldestErrorProneVersion)) < 0) { throw new IllegalArgumentException( - "Error Prone API version " + epApiVersion + " is too old; " - + oldestErrorProneVersion + " is the oldest supported version") + "Error Prone API version " + epApiVersion + " is too old; " + + oldestErrorProneVersion + " is the oldest supported version") } errorProneVersionToCompileAgainst = epApiVersion } def versions = [ asm : "9.3", - checkerFramework : "3.26.0", + checkerFramework : "3.38.0", // for comparisons in other parts of the build errorProneLatest : latestErrorProneVersion, // The version of Error Prone used to check NullAway's code. @@ -49,10 +48,10 @@ def versions = [ // The version of Error Prone that NullAway is compiled and tested against errorProneApi : errorProneVersionToCompileAgainst, support : "27.1.1", - wala : "1.5.8", + wala : "1.6.2", commonscli : "1.4", - autoValue : "1.9", - autoService : "1.0.1", + autoValue : "1.10.2", + autoService : "1.1.1", ] def apt = [ @@ -78,9 +77,11 @@ def build = [ jspecify : "org.jspecify:jspecify:0.3.0", jsr305Annotations : "com.google.code.findbugs:jsr305:3.0.2", commonsIO : "commons-io:commons-io:2.11.0", - wala : ["com.ibm.wala:com.ibm.wala.util:${versions.wala}", - "com.ibm.wala:com.ibm.wala.shrike:${versions.wala}", - "com.ibm.wala:com.ibm.wala.core:${versions.wala}"], + wala : [ + "com.ibm.wala:com.ibm.wala.util:${versions.wala}", + "com.ibm.wala:com.ibm.wala.shrike:${versions.wala}", + "com.ibm.wala:com.ibm.wala.core:${versions.wala}" + ], commonscli : "commons-cli:commons-cli:${versions.commonscli}", // android stuff @@ -97,7 +98,10 @@ def support = [ def test = [ junit4 : "junit:junit:4.13.2", - junit5Jupiter : ["org.junit.jupiter:junit-jupiter-api:5.0.2","org.apiguardian:apiguardian-api:1.0.0"], + junit5Jupiter : [ + "org.junit.jupiter:junit-jupiter-api:5.0.2", + "org.apiguardian:apiguardian-api:1.0.0" + ], jetbrainsAnnotations : "org.jetbrains:annotations:13.0", cfQual : "org.checkerframework:checker-qual:${versions.checkerFramework}", // 2.5.5 is the last release to contain this artifact @@ -109,8 +113,7 @@ def test = [ springBeans : "org.springframework:spring-beans:5.3.7", springContext : "org.springframework:spring-context:5.3.7", grpcCore : "io.grpc:grpc-core:1.15.1", // Should upgrade, but this matches our guava version - mockito : "org.mockito:mockito-core:4.6.1", - mockitoInline : "org.mockito:mockito-inline:4.6.1", + mockito : "org.mockito:mockito-core:5.5.0", javaxAnnotationApi : "javax.annotation:javax.annotation-api:1.3.2", assertJ : "org.assertj:assertj-core:3.23.1", ] diff --git a/gradle/wrapper/gradle-wrapper.jar b/gradle/wrapper/gradle-wrapper.jar index ccebba7710..7f93135c49 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 bdc9a83b1e..864d6c4751 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,6 +1,8 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-8.0.2-bin.zip +distributionSha256Sum=591855b517fc635b9e04de1d05d5e76ada3f89f5fc76f87978d1b245b4f69225 +distributionUrl=https\://services.gradle.org/distributions/gradle-8.3-bin.zip networkTimeout=10000 +validateDistributionUrl=true zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists diff --git a/gradlew b/gradlew index 79a61d421c..0adc8e1a53 100755 --- a/gradlew +++ b/gradlew @@ -83,10 +83,8 @@ done # This is normally unused # shellcheck disable=SC2034 APP_BASE_NAME=${0##*/} -APP_HOME=$( cd "${APP_HOME:-./}" && pwd -P ) || exit - -# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script. -DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"' +# 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 # Use the maximum available, or set MAX_FD != -1 to use that value. MAX_FD=maximum @@ -133,10 +131,13 @@ location of your Java installation." fi else JAVACMD=java - which java >/dev/null 2>&1 || die "ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. + if ! command -v java >/dev/null 2>&1 + then + die "ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. Please set the JAVA_HOME variable in your environment to match the location of your Java installation." + fi fi # Increase the maximum file descriptors if we can. @@ -197,6 +198,10 @@ if "$cygwin" || "$msys" ; then done fi + +# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script. +DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"' + # Collect all arguments for the java command; # * $DEFAULT_JVM_OPTS, $JAVA_OPTS, and $GRADLE_OPTS can contain fragments of # shell script including quotes and variable substitutions, so put them in diff --git a/guava-recent-unit-tests/build.gradle b/guava-recent-unit-tests/build.gradle index f831222651..4d78ac52a3 100644 --- a/guava-recent-unit-tests/build.gradle +++ b/guava-recent-unit-tests/build.gradle @@ -15,7 +15,7 @@ */ plugins { id 'java-library' - id 'nullaway.jacoco-conventions' + id 'nullaway.java-test-conventions' } // We need this separate build target to test newer versions of Guava @@ -31,25 +31,27 @@ dependencies { testImplementation "com.google.guava:guava:31.1-jre" } -test { - maxHeapSize = "1024m" - if (!JavaVersion.current().java9Compatible) { - jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" - } else { - // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer - jvmArgs += [ - "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", - // Accessed by Lombok tests - "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", - ] +// Create a task to test on JDK 8 +def jdk8Test = tasks.register("testJdk8", Test) { + onlyIf { + // Only if we are using a version of Error Prone compatible with JDK 8 + deps.versions.errorProneApi == "2.4.0" } + + javaLauncher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.of(8) + } + + description = "Runs the test suite on JDK 8" + group = LifecycleBasePlugin.VERIFICATION_GROUP + + // Copy inputs from normal Test task. + def testTask = tasks.getByName("test") + classpath = testTask.classpath + testClassesDirs = testTask.testClassesDirs + jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" +} + +tasks.named('check').configure { + dependsOn(jdk8Test) } diff --git a/jar-infer/android-jarinfer-models-sdk28/build.gradle b/jar-infer/android-jarinfer-models-sdk28/build.gradle index 9ff005f6ed..fe50fe8f62 100644 --- a/jar-infer/android-jarinfer-models-sdk28/build.gradle +++ b/jar-infer/android-jarinfer-models-sdk28/build.gradle @@ -2,8 +2,6 @@ plugins { id "java-library" } -sourceCompatibility = 1.8 - repositories { mavenCentral() } diff --git a/jar-infer/android-jarinfer-models-sdk29/build.gradle b/jar-infer/android-jarinfer-models-sdk29/build.gradle index 9ff005f6ed..fe50fe8f62 100644 --- a/jar-infer/android-jarinfer-models-sdk29/build.gradle +++ b/jar-infer/android-jarinfer-models-sdk29/build.gradle @@ -2,8 +2,6 @@ plugins { id "java-library" } -sourceCompatibility = 1.8 - repositories { mavenCentral() } diff --git a/jar-infer/android-jarinfer-models-sdk30/build.gradle b/jar-infer/android-jarinfer-models-sdk30/build.gradle index 9ff005f6ed..fe50fe8f62 100644 --- a/jar-infer/android-jarinfer-models-sdk30/build.gradle +++ b/jar-infer/android-jarinfer-models-sdk30/build.gradle @@ -2,8 +2,6 @@ plugins { id "java-library" } -sourceCompatibility = 1.8 - repositories { mavenCentral() } diff --git a/jar-infer/android-jarinfer-models-sdk31/build.gradle b/jar-infer/android-jarinfer-models-sdk31/build.gradle index 9ff005f6ed..fe50fe8f62 100644 --- a/jar-infer/android-jarinfer-models-sdk31/build.gradle +++ b/jar-infer/android-jarinfer-models-sdk31/build.gradle @@ -2,8 +2,6 @@ plugins { id "java-library" } -sourceCompatibility = 1.8 - repositories { mavenCentral() } diff --git a/jar-infer/jar-infer-cli/build.gradle b/jar-infer/jar-infer-cli/build.gradle index 0b242595be..3245d662b3 100644 --- a/jar-infer/jar-infer-cli/build.gradle +++ b/jar-infer/jar-infer-cli/build.gradle @@ -4,6 +4,12 @@ plugins { id "com.github.johnrengelman.shadow" } +// JarInfer requires JDK 11+, due to its dependence on WALA +tasks.withType(JavaCompile) { + java.sourceCompatibility = JavaVersion.VERSION_11 + java.targetCompatibility = JavaVersion.VERSION_11 +} + repositories { mavenCentral() } @@ -11,8 +17,6 @@ repositories { dependencies { implementation deps.build.commonscli implementation deps.build.guava - compileOnly deps.build.errorProneCheckApi - implementation project(":jar-infer:jar-infer-lib") testImplementation deps.test.junit4 @@ -21,11 +25,14 @@ dependencies { } } +java { + withJavadocJar() + withSourcesJar() +} + jar { manifest { - attributes( - 'Main-Class': 'com.uber.nullaway.jarinfer.JarInfer' - ) + attributes('Main-Class': 'com.uber.nullaway.jarinfer.JarInfer') } // add this classifier so that the output file for the jar task differs from // the output file for the shadowJar task (otherwise they overwrite each other's @@ -35,12 +42,15 @@ jar { shadowJar { mergeServiceFiles() - configurations = [project.configurations.runtimeClasspath] + configurations = [ + project.configurations.runtimeClasspath + ] archiveClassifier = "" } shadowJar.dependsOn jar assemble.dependsOn shadowJar + // We disable the default maven publications to make sure only // our custom shadow publication is used. Since we use the empty // classifier for our fat jar, it would otherwise clash with the @@ -68,7 +78,7 @@ publishing { // `:javadoc` artifacts here. They are also required for Maven Central validation. afterEvaluate { artifact project.sourcesJar - artifact project.javadocsJar + artifact project.javadocJar } // The shadow publication does not auto-configure the pom.xml file for us, so we need to // set it up manually. We use the opportunity to change the name and description from @@ -99,4 +109,33 @@ publishing { } } } + + afterEvaluate { + // Below is a series of hacks needed to get publication to work with + // gradle-maven-publish-plugin >= 0.15.0 (itself needed after the upgrade to Gradle 8.0.2). + // Not sure why e.g. publishShadowPublicationToMavenCentralRepository must depend on signMavenPublication + // (rather than just signShadowPublication) + project.tasks.named('generateMetadataFileForMavenPublication').configure { + dependsOn 'sourcesJar' + dependsOn 'simpleJavadocJar' + } + if (project.tasks.findByName('signShadowPublication')) { + project.tasks.named('signShadowPublication').configure { + dependsOn 'sourcesJar' + dependsOn 'simpleJavadocJar' + } + } + project.tasks.named('publishShadowPublicationToMavenCentralRepository').configure { + if (project.tasks.findByName('signMavenPublication')) { + dependsOn 'signMavenPublication' + } + } + project.tasks.named('publishShadowPublicationToMavenLocal').configure { + dependsOn 'sourcesJar' + dependsOn 'simpleJavadocJar' + if (project.tasks.findByName('signMavenPublication')) { + dependsOn 'signMavenPublication' + } + } + } } diff --git a/jar-infer/jar-infer-lib/build.gradle b/jar-infer/jar-infer-lib/build.gradle index 638ae2de31..2ae8bea7a8 100644 --- a/jar-infer/jar-infer-lib/build.gradle +++ b/jar-infer/jar-infer-lib/build.gradle @@ -15,10 +15,14 @@ */ plugins { id "java-library" - id 'nullaway.jacoco-conventions' + id 'nullaway.java-test-conventions' } -sourceCompatibility = 1.8 +// JarInfer requires JDK 11+, due to its dependence on WALA +tasks.withType(JavaCompile) { + java.sourceCompatibility = JavaVersion.VERSION_11 + java.targetCompatibility = JavaVersion.VERSION_11 +} repositories { mavenCentral() @@ -44,29 +48,17 @@ dependencies { } test { - maxHeapSize = "1024m" - if (!JavaVersion.current().java9Compatible) { - jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" - } else { - // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer - jvmArgs += [ - "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", - // Accessed by Lombok tests - "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", - ] - } - if (JavaVersion.current() >= JavaVersion.VERSION_11) { - dependsOn ':jar-infer:test-android-lib-jarinfer:bundleReleaseAar' - } + dependsOn ':jar-infer:test-android-lib-jarinfer:bundleReleaseAar' +} + +tasks.named('testJdk21', Test).configure { + // Tests fail since WALA does not yet support JDK 21; see https://github.com/uber/NullAway/issues/829 + // So, disable them + onlyIf { false } +} + +tasks.withType(JavaCompile).configureEach { + options.compilerArgs += "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED" } apply plugin: 'com.vanniktech.maven.publish' diff --git a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java index 5deb5cf229..fc852414fb 100644 --- a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java +++ b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java @@ -228,7 +228,7 @@ private void analyzeFile(String pkgName, String inPath, boolean includeNonPublic } else if (!new File(inPath).exists()) { return; } - AnalysisScope scope = AnalysisScopeReader.instance.makePrimordialScope(null); + AnalysisScope scope = AnalysisScopeReader.instance.makeBasePrimordialScope(null); scope.setExclusions( new FileOfClasses( new ByteArrayInputStream(DEFAULT_EXCLUSIONS.getBytes(StandardCharsets.UTF_8)))); @@ -510,6 +510,7 @@ private static String getAstubxSignature(IMethod mtd) { + strArgTypes + ")"; } + /** * Get simple unqualified type name. * diff --git a/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java b/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java index dda6aaf47b..897e96e2ed 100644 --- a/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java +++ b/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java @@ -170,7 +170,7 @@ private void testAnnotationInAarTemplate( * @param result Map of 'method signatures' to their 'inferred list of NonNull parameters'. * @param expected Map of 'method signatures' to their 'expected list of NonNull parameters'. */ - private boolean verify(Map> result, HashMap> expected) { + private boolean verify(Map> result, Map> expected) { for (Map.Entry> entry : result.entrySet()) { String mtd_sign = entry.getKey(); Set ddParams = entry.getValue(); diff --git a/jar-infer/nullaway-integration-test/build.gradle b/jar-infer/nullaway-integration-test/build.gradle index 100e2d81ae..94a880861a 100644 --- a/jar-infer/nullaway-integration-test/build.gradle +++ b/jar-infer/nullaway-integration-test/build.gradle @@ -14,38 +14,15 @@ * limitations under the License. */ plugins { - id "java-library" - id "nullaway.jacoco-conventions" + id "java-library" + id "nullaway.java-test-conventions" } dependencies { - testImplementation deps.test.junit4 - testImplementation(deps.build.errorProneTestHelpers) { + testImplementation deps.test.junit4 + testImplementation(deps.build.errorProneTestHelpers) { exclude group: "junit", module: "junit" } - testImplementation project(":nullaway") - testImplementation project(":jar-infer:test-java-lib-jarinfer") - -} - - -test { - maxHeapSize = "1024m" - if (!JavaVersion.current().java9Compatible) { - jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" - } else { - // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer - jvmArgs += [ - "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", - ] - } + testImplementation project(":nullaway") + testImplementation project(":jar-infer:test-java-lib-jarinfer") } diff --git a/jar-infer/test-android-lib-jarinfer/build.gradle b/jar-infer/test-android-lib-jarinfer/build.gradle index db29e5756a..b79ed03539 100644 --- a/jar-infer/test-android-lib-jarinfer/build.gradle +++ b/jar-infer/test-android-lib-jarinfer/build.gradle @@ -16,8 +16,6 @@ apply plugin: 'com.android.library' -sourceCompatibility = 1.8 - android { compileSdkVersion deps.build.compileSdkVersion diff --git a/jar-infer/test-java-lib-jarinfer/build.gradle b/jar-infer/test-java-lib-jarinfer/build.gradle index 7e60265438..df1a7a0683 100644 --- a/jar-infer/test-java-lib-jarinfer/build.gradle +++ b/jar-infer/test-java-lib-jarinfer/build.gradle @@ -15,7 +15,7 @@ */ plugins { - id "java-library" + id "java-library" } evaluationDependsOn(":jar-infer:jar-infer-cli") @@ -28,15 +28,20 @@ jar { 'Created-By' : "Gradle ${gradle.gradleVersion}", 'Build-Jdk' : "${System.properties['java.version']} (${System.properties['java.vendor']} ${System.properties['java.vm.version']})", 'Build-OS' : "${System.properties['os.name']} ${System.properties['os.arch']} ${System.properties['os.version']}" - ) + ) } } jar.doLast { javaexec { - classpath = files("${rootProject.projectDir}/jar-infer/jar-infer-cli/build/libs/jar-infer-cli-${VERSION_NAME}.jar") + classpath = files("${rootProject.projectDir}/jar-infer/jar-infer-cli/build/libs/jar-infer-cli.jar") main = "com.uber.nullaway.jarinfer.JarInfer" - args = ["-i", jar.archiveFile.get(), "-o", "${jar.destinationDirectory.get()}/${astubxPath}"] + args = [ + "-i", + jar.archiveFile.get(), + "-o", + "${jar.destinationDirectory.get()}/${astubxPath}" + ] } exec { workingDir "./build/libs" diff --git a/jdk17-unit-tests/build.gradle b/jdk17-unit-tests/build.gradle index 506e81dd48..1c9f7f908d 100644 --- a/jdk17-unit-tests/build.gradle +++ b/jdk17-unit-tests/build.gradle @@ -15,7 +15,7 @@ */ plugins { id 'java-library' - id 'nullaway.jacoco-conventions' + id 'nullaway.java-test-conventions' } // Use JDK 17 for this module, via a toolchain @@ -44,22 +44,17 @@ dependencies { } test { - maxHeapSize = "1024m" - // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer jvmArgs += [ - "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", - // Expose a module path for tests as a JVM property. - // Used by com.uber.nullaway.jdk17.NullAwayModuleInfoTests - "-Dtest.module.path=${configurations.testModulePath.asPath}" + // Expose a module path for tests as a JVM property. + // Used by com.uber.nullaway.jdk17.NullAwayModuleInfoTests + "-Dtest.module.path=${configurations.testModulePath.asPath}" ] } +tasks.getByName('testJdk21').configure { + jvmArgs += [ + // Expose a module path for tests as a JVM property. + // Used by com.uber.nullaway.jdk17.NullAwayModuleInfoTests + "-Dtest.module.path=${configurations.testModulePath.asPath}" + ] +} diff --git a/jdk17-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwayRecordTests.java b/jdk17-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwayRecordTests.java index 8135deb27b..d60e264a77 100644 --- a/jdk17-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwayRecordTests.java +++ b/jdk17-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwayRecordTests.java @@ -178,4 +178,36 @@ public void testLocalRecord() { "}") .doTest(); } + + @Test + public void recordEqualsNull() { + defaultCompilationHelper + .addSourceLines( + "Records.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Records {", + " public void recordEqualsNull() {", + " record Rec() {", + " void print(Object o) { System.out.println(o.toString()); }", + " void equals(Integer i1, Integer i2) { }", + " boolean equals(String i1, String i2) { return false; }", + " boolean equals(Long l1) { return false; }", + " }", + " Object o = null;", + " // null can be passed to the generated equals() method taking an Object parameter", + " new Rec().equals(o);", + " // BUG: Diagnostic contains: passing @Nullable parameter 'null'", + " new Rec().print(null);", + " // BUG: Diagnostic contains: passing @Nullable parameter 'null'", + " new Rec().equals(null, Integer.valueOf(100));", + " // BUG: Diagnostic contains: passing @Nullable parameter 'null'", + " new Rec().equals(\"hello\", null);", + " Long l = null;", + " // BUG: Diagnostic contains: passing @Nullable parameter 'l'", + " new Rec().equals(l);", + " }", + "}") + .doTest(); + } } diff --git a/jmh/build.gradle b/jmh/build.gradle index edc2679494..68e3c58446 100644 --- a/jmh/build.gradle +++ b/jmh/build.gradle @@ -15,7 +15,7 @@ */ plugins { id 'java-library' - id 'nullaway.jacoco-conventions' + id 'nullaway.java-test-conventions' id 'me.champeau.jmh' } @@ -105,13 +105,13 @@ def nullawayReleaseProcessorpath = configurations.nullawayReleaseProcessors.asPa // Extra JVM arguments to expose relevant paths for compiling benchmarks def extraJVMArgs = [ - "-Dnullaway.caffeine.sources=${caffeineSourceDir.get()}", - "-Dnullaway.caffeine.classpath=$caffeineClasspath", - "-Dnullaway.autodispose.sources=${autodisposeSourceDir.get()}", - "-Dnullaway.autodispose.classpath=$autodisposeClasspath", - "-Dnullaway.nullawayRelease.sources=${nullawayReleaseSourceDir.get()}", - "-Dnullaway.nullawayRelease.classpath=$nullawayReleaseClasspath", - "-Dnullaway.nullawayRelease.processorpath=$nullawayReleaseProcessorpath", + "-Dnullaway.caffeine.sources=${caffeineSourceDir.get()}", + "-Dnullaway.caffeine.classpath=$caffeineClasspath", + "-Dnullaway.autodispose.sources=${autodisposeSourceDir.get()}", + "-Dnullaway.autodispose.classpath=$autodisposeClasspath", + "-Dnullaway.nullawayRelease.sources=${nullawayReleaseSourceDir.get()}", + "-Dnullaway.nullawayRelease.classpath=$nullawayReleaseClasspath", + "-Dnullaway.nullawayRelease.processorpath=$nullawayReleaseProcessorpath", ] jmh { @@ -128,23 +128,11 @@ jmh { // includes = ['DFlowMicro'] } -// don't run test task on pre-JDK-11 VMs tasks.named('test') { - onlyIf { JavaVersion.current() >= JavaVersion.VERSION_11 } // pass the extra JVM args so we can compile benchmarks in unit tests - jvmArgs extraJVMArgs - // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer - jvmArgs += [ - "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", - ] + jvmArgs += extraJVMArgs } +tasks.getByName('testJdk21').configure { + jvmArgs += extraJVMArgs +} diff --git a/nullaway/build.gradle b/nullaway/build.gradle index 3b4f10f379..026a89094a 100644 --- a/nullaway/build.gradle +++ b/nullaway/build.gradle @@ -16,8 +16,8 @@ import net.ltgt.gradle.errorprone.CheckSeverity plugins { - id 'java-library' - id 'nullaway.jacoco-conventions' + id 'java-library' + id 'nullaway.java-test-conventions' } configurations { @@ -62,12 +62,8 @@ dependencies { testImplementation deps.test.grpcCore testImplementation project(":test-java-lib-lombok") testImplementation deps.test.mockito - testImplementation deps.test.mockitoInline testImplementation deps.test.javaxAnnotationApi testImplementation deps.test.assertJ - - // This ends up being resolved to the NullAway jar under nullaway/build/libs - nullawayJar "com.uber.nullaway:nullaway:$VERSION_NAME" } javadoc { @@ -76,85 +72,110 @@ javadoc { test { - maxHeapSize = "1024m" - if (!JavaVersion.current().java9Compatible) { - jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" - } else { - // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer - jvmArgs += [ - "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", - // Accessed by Lombok tests - "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", - ] - } + if (deps.versions.errorProneApi == "2.4.0" && JavaVersion.current() >= JavaVersion.VERSION_17) { + // This test does not pass on JDK 17 with Error Prone 2.4.0 due to a Mockito incompatibility. Skip it (the + // test passes with more recent Error Prone versions on JDK 17) + filter { + excludeTestsMatching "com.uber.nullaway.NullAwaySerializationTest.suggestNullableArgumentOnBytecodeNoFileInfo" + } + } } apply plugin: 'com.vanniktech.maven.publish' -if (JavaVersion.current() >= JavaVersion.VERSION_11) { - // Required on Java 11+ since Error Prone and NullAway access a bunch of - // JDK-internal APIs that are not exposed otherwise - tasks.withType(JavaCompile).configureEach { - options.compilerArgs += [ - "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.source.tree=ALL-UNNAMED", - ] +// These --add-exports arguments are required when targeting JDK 11+ since Error Prone and NullAway access a bunch of +// JDK-internal APIs that are not exposed otherwise. Since we currently target JDK 8, we do not need to pass the +// arguments, as encapsulation of JDK internals is not enforced on JDK 8. In fact, the arguments cause a compiler error +// when targeting JDK 8. Leaving commented so we can easily add them back once we target JDK 11. +// tasks.withType(JavaCompile).configureEach { +// options.compilerArgs += [ +// "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", +// "--add-exports=jdk.compiler/com.sun.source.tree=ALL-UNNAMED", +// ] +// } + +// Create a task to test on JDK 8 +def jdk8Test = tasks.register("testJdk8", Test) { + onlyIf { + // Only if we are using a version of Error Prone compatible with JDK 8 + deps.versions.errorProneApi == "2.4.0" } - // Create a task to build NullAway with NullAway checking enabled - // For some reason, this doesn't work on Java 8 - tasks.register('buildWithNullAway', JavaCompile) { - onlyIf { - // We only do NullAway checks when compiling against the latest - // version of Error Prone (as nullability annotations on the APIs - // can change between versions) - deps.versions.errorProneApi == deps.versions.errorProneLatest - } - // Configure compilation to run with Error Prone and NullAway - source = sourceSets.main.java - classpath = sourceSets.main.compileClasspath - destinationDirectory = file("$buildDir/ignoredClasses") - def nullawayDeps = configurations.nullawayJar.asCollection() - options.annotationProcessorPath = files( - configurations.errorprone.asCollection(), - sourceSets.main.annotationProcessorPath, - nullawayDeps) - options.errorprone.enabled = true - options.errorprone { - option("NullAway:AnnotatedPackages", "com.uber,org.checkerframework.nullaway") - option("NullAway:CastToNonNullMethod", "com.uber.nullaway.NullabilityUtil.castToNonNull") - option("NullAway:CheckOptionalEmptiness") - option("NullAway:AcknowledgeRestrictiveAnnotations") - } - // Make sure the jar has already been built - dependsOn 'jar' - // Check that the NullAway jar actually exists (without this, - // Gradle will run the compilation even if the jar doesn't exist) - doFirst { - nullawayDeps.forEach { f -> - assert f.exists() - } - } + + javaLauncher = javaToolchains.launcherFor { + languageVersion = JavaLanguageVersion.of(8) } - project.tasks.named('check').configure { - dependsOn 'buildWithNullAway' + description = "Runs the test suite on JDK 8" + group = LifecycleBasePlugin.VERIFICATION_GROUP + + // Copy inputs from normal Test task. + def testTask = tasks.getByName("test") + classpath = testTask.classpath + testClassesDirs = testTask.testClassesDirs + jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" + filter { + // tests cannot run on JDK 8 since Mockito version no longer supports it + excludeTestsMatching "com.uber.nullaway.NullAwaySerializationTest.initializationError" + excludeTestsMatching "com.uber.nullaway.handlers.contract.ContractUtilsTest.getEmptyAntecedent" } } + +tasks.named('check').configure { + dependsOn(jdk8Test) +} + +tasks.named('testJdk21', Test).configure { + filter { + // JSpecify Generics tests do not yet pass on JDK 21 + // See https://github.com/uber/NullAway/issues/827 + excludeTestsMatching "com.uber.nullaway.NullAwayJSpecifyGenericsTests" + } +} + +// Create a task to build NullAway with NullAway checking enabled +tasks.register('buildWithNullAway', JavaCompile) { + onlyIf { + // We only do NullAway checks when compiling against the latest + // version of Error Prone (as nullability annotations on the APIs + // can change between versions) + deps.versions.errorProneApi == deps.versions.errorProneLatest + } + // Configure compilation to run with Error Prone and NullAway + source = sourceSets.main.java + classpath = sourceSets.main.compileClasspath + destinationDirectory = file("$buildDir/ignoredClasses") + options.annotationProcessorPath = files( + configurations.errorprone.asCollection(), + sourceSets.main.annotationProcessorPath, + // This refers to the NullAway jar built from the current source + jar.archiveFile.get(), + sourceSets.main.compileClasspath) + options.errorprone.enabled = true + options.errorprone { + option("NullAway:AnnotatedPackages", "com.uber,org.checkerframework.nullaway") + option("NullAway:CastToNonNullMethod", "com.uber.nullaway.NullabilityUtil.castToNonNull") + option("NullAway:CheckOptionalEmptiness") + option("NullAway:AcknowledgeRestrictiveAnnotations") + option("NullAway:CheckContracts") + } + // Make sure the jar has already been built + dependsOn 'jar' + // Check that the NullAway jar actually exists (without this, + // Gradle will run the compilation even if the jar doesn't exist) + doFirst { + assert jar.archiveFile.get().getAsFile().exists() + } +} + +project.tasks.named('check').configure { + dependsOn 'buildWithNullAway' +} diff --git a/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java b/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java new file mode 100644 index 0000000000..06a91f99c1 --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java @@ -0,0 +1,39 @@ +package com.uber.nullaway; + +import com.sun.tools.javac.code.Symbol; +import java.util.List; + +/** + * Methods backported from {@link com.google.errorprone.util.ASTHelpers} since we do not yet require + * a recent-enough Error Prone version. The methods should be removed once we bump our minimum Error + * Prone version accordingly. + */ +public class ASTHelpersBackports { + + private ASTHelpersBackports() {} + + /** + * Returns true if the symbol is static. Returns {@code false} for module symbols. Remove once we + * require Error Prone 2.16.0 or higher. + */ + @SuppressWarnings("ASTHelpersSuggestions") + public static boolean isStatic(Symbol symbol) { + if (symbol.getKind().name().equals("MODULE")) { + return false; + } + return symbol.isStatic(); + } + + /** + * A wrapper for {@link Symbol#getEnclosedElements} to avoid binary compatibility issues for + * covariant overrides in subtypes of {@link Symbol}. + * + *

Same as this ASTHelpers method in Error Prone: + * https://github.com/google/error-prone/blame/a1318e4b0da4347dff7508108835d77c470a7198/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java#L1148 + * TODO: delete this method and switch to ASTHelpers once we can require Error Prone 2.20.0 + */ + @SuppressWarnings("ASTHelpersSuggestions") + public static List getEnclosedElements(Symbol symbol) { + return symbol.getEnclosedElements(); + } +} diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index d35424b9c6..fb7aee6f70 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -22,6 +22,7 @@ package com.uber.nullaway; +import static com.uber.nullaway.ASTHelpersBackports.isStatic; import static com.uber.nullaway.ErrorMessage.MessageTypes.FIELD_NO_INIT; import static com.uber.nullaway.ErrorMessage.MessageTypes.GET_ON_EMPTY_OPTIONAL; import static com.uber.nullaway.ErrorMessage.MessageTypes.METHOD_NO_INIT; @@ -30,8 +31,10 @@ import static com.uber.nullaway.NullAway.INITIALIZATION_CHECK_NAME; import static com.uber.nullaway.NullAway.OPTIONAL_CHECK_NAME; import static com.uber.nullaway.NullAway.getTreesInstance; +import static com.uber.nullaway.Nullness.hasNullableAnnotation; import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -59,6 +62,7 @@ import java.util.stream.StreamSupport; import javax.annotation.Nullable; import javax.lang.model.element.Element; +import javax.lang.model.element.ElementKind; import javax.tools.JavaFileObject; /** A class to construct error message to be displayed after the analysis finds error. */ @@ -132,7 +136,7 @@ public Description createErrorDescription( } if (config.suggestSuppressions() && suggestTree != null) { - builder = addSuggestedSuppression(errorMessage, suggestTree, builder); + builder = addSuggestedSuppression(errorMessage, suggestTree, builder, state); } if (config.serializationIsActive()) { @@ -186,21 +190,30 @@ private boolean hasPathSuppression(TreePath treePath, String subcheckerName) { } private Description.Builder addSuggestedSuppression( - ErrorMessage errorMessage, Tree suggestTree, Description.Builder builder) { + ErrorMessage errorMessage, + Tree suggestTree, + Description.Builder builder, + VisitorState state) { switch (errorMessage.messageType) { case DEREFERENCE_NULLABLE: case RETURN_NULLABLE: case PASS_NULLABLE: case ASSIGN_FIELD_NULLABLE: case SWITCH_EXPRESSION_NULLABLE: - if (config.getCastToNonNullMethod() != null) { - builder = addCastToNonNullFix(suggestTree, builder); + if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree)) { + builder = addCastToNonNullFix(suggestTree, builder, state); } else { - builder = addSuppressWarningsFix(suggestTree, builder, suppressionName); + // When there is a castToNonNull method, suggestTree is set to the expression to be + // casted, which is not suppressible. For simplicity, we just always recompute the + // suppressible node here. + Tree suppressibleNode = suppressibleNode(state.getPath()); + if (suppressibleNode != null) { + builder = addSuppressWarningsFix(suppressibleNode, builder, suppressionName); + } } break; case CAST_TO_NONNULL_ARG_NONNULL: - builder = removeCastToNonNullFix(suggestTree, builder); + builder = removeCastToNonNullFix(suggestTree, builder, state); break; case WRONG_OVERRIDE_RETURN: builder = addSuppressWarningsFix(suggestTree, builder, suppressionName); @@ -315,7 +328,36 @@ private Tree suppressibleNode(@Nullable TreePath path) { .orElse(null); } - private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Builder builder) { + /** + * Checks if it would be appropriate to wrap {@code tree} in a {@code castToNonNull} call. There + * are two cases where this method returns {@code false}: + * + *

    + *
  1. {@code tree} represents the {@code null} literal + *
  2. {@code tree} represents a {@code @Nullable} formal parameter of the enclosing method + *
+ */ + private boolean canBeCastToNonNull(Tree tree) { + switch (tree.getKind()) { + case NULL_LITERAL: + // never do castToNonNull(null) + return false; + case IDENTIFIER: + // Don't wrap a @Nullable parameter in castToNonNull, as this misleads callers into thinking + // they can pass in null without causing an NPE. A more appropriate fix would likely be to + // make the parameter @NonNull and add casts at call sites, but that is beyond the scope of + // our suggested fixes + Symbol symbol = ASTHelpers.getSymbol(tree); + return !(symbol != null + && symbol.getKind().equals(ElementKind.PARAMETER) + && hasNullableAnnotation(symbol, config)); + default: + return true; + } + } + + private Description.Builder addCastToNonNullFix( + Tree suggestTree, Description.Builder builder, VisitorState state) { final String fullMethodName = config.getCastToNonNullMethod(); if (fullMethodName == null) { throw new IllegalStateException("cast-to-non-null method not set"); @@ -323,7 +365,7 @@ private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Bu // Add a call to castToNonNull around suggestTree: final String[] parts = fullMethodName.split("\\."); final String shortMethodName = parts[parts.length - 1]; - final String replacement = shortMethodName + "(" + suggestTree.toString() + ")"; + final String replacement = shortMethodName + "(" + state.getSourceForNode(suggestTree) + ")"; final SuggestedFix fix = SuggestedFix.builder() .replace(suggestTree, replacement) @@ -333,26 +375,29 @@ private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Bu } private Description.Builder removeCastToNonNullFix( - Tree suggestTree, Description.Builder builder) { - assert suggestTree.getKind() == Tree.Kind.METHOD_INVOCATION; - final MethodInvocationTree invTree = (MethodInvocationTree) suggestTree; - final Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(invTree); - final String qualifiedName = - ASTHelpers.enclosingClass(methodSymbol) + "." + methodSymbol.getSimpleName().toString(); - if (!qualifiedName.equals(config.getCastToNonNullMethod())) { - throw new RuntimeException("suggestTree should point to the castToNonNull invocation."); - } + Tree suggestTree, Description.Builder builder, VisitorState state) { + // Note: Here suggestTree refers to the argument being cast. We need to find the + // castToNonNull(...) invocation to be replaced by it. Fortunately, state.getPath() + // should be currently pointing at said call. + Tree currTree = state.getPath().getLeaf(); + Preconditions.checkArgument( + currTree.getKind() == Tree.Kind.METHOD_INVOCATION, + String.format("Expected castToNonNull invocation expression, found:\n%s", currTree)); + final MethodInvocationTree invTree = (MethodInvocationTree) currTree; + Preconditions.checkArgument( + invTree.getArguments().contains(suggestTree), + String.format( + "Method invocation tree %s does not contain the expression %s as an argument being cast", + invTree, suggestTree)); // Remove the call to castToNonNull: final SuggestedFix fix = - SuggestedFix.builder() - .replace(suggestTree, invTree.getArguments().get(0).toString()) - .build(); + SuggestedFix.builder().replace(invTree, state.getSourceForNode(suggestTree)).build(); return builder.addFix(fix); } /** - * Reports initialization errors where a constructor fails to guarantee non-fields are initialized - * along all paths at exit points. + * Reports initialization errors where a constructor fails to guarantee non-null fields are + * initialized along all paths at exit points. * * @param methodSymbol Constructor symbol. * @param message Error message. @@ -479,9 +524,7 @@ void reportInitErrorOnField(Symbol symbol, VisitorState state, Description.Build fieldName = flatName.substring(index) + "." + fieldName; } - @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater - boolean isStatic = symbol.isStatic(); - if (isStatic) { + if (isStatic(symbol)) { state.reportMatch( createErrorDescription( new ErrorMessage( diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java index 22ebf13b4b..186b386cde 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java @@ -56,6 +56,8 @@ public enum MessageTypes { ASSIGN_GENERIC_NULLABLE, RETURN_NULLABLE_GENERIC, PASS_NULLABLE_GENERIC, + WRONG_OVERRIDE_RETURN_GENERIC, + WRONG_OVERRIDE_PARAM_GENERIC, } public String getMessage() { diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java index d121e38725..9a189fe821 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java @@ -83,6 +83,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig { static final String FL_JI_REGEX_MODEL_PATH = EP_FL_NAMESPACE + ":JarInferRegexStripModelJar"; static final String FL_JI_REGEX_CODE_PATH = EP_FL_NAMESPACE + ":JarInferRegexStripCodeJar"; static final String FL_ERROR_URL = EP_FL_NAMESPACE + ":ErrorURL"; + /** --- Serialization configs --- */ static final String FL_FIX_SERIALIZATION = EP_FL_NAMESPACE + ":SerializeFixMetadata"; @@ -215,7 +216,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig { "Invalid -XepOpt:" + FL_SUPPRESS_COMMENT + " value. Comment must be single line."); } skippedLibraryModels = getFlagStringSet(flags, FL_SKIP_LIBRARY_MODELS); - /** --- JarInfer configs --- */ + /* --- JarInfer configs --- */ jarInferEnabled = flags.getBoolean(FL_JI_ENABLED).orElse(false); jarInferUseReturnAnnotations = flags.getBoolean(FL_JI_USE_RETURN).orElse(false); // The defaults of these two options translate to: remove .aar/.jar from the file name, and also diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java index 13ffd147f2..a9f3a6d717 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java @@ -1,6 +1,7 @@ package com.uber.nullaway; import static com.uber.nullaway.NullabilityUtil.castToNonNull; +import static java.util.stream.Collectors.joining; import com.google.common.base.Preconditions; import com.google.errorprone.VisitorState; @@ -9,14 +10,20 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotatedTypeTree; import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ArrayTypeTree; import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.ConditionalExpressionTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; import com.sun.source.tree.NewClassTree; import com.sun.source.tree.ParameterizedTypeTree; import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; +import com.sun.source.util.SimpleTreeVisitor; import com.sun.tools.javac.code.Attribute; +import com.sun.tools.javac.code.BoundKind; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.TypeMetadata; @@ -35,15 +42,9 @@ public final class GenericsChecks { private static final Supplier NULLABLE_TYPE_SUPPLIER = Suppliers.typeFromString(NULLABLE_NAME); - private VisitorState state; - private Config config; - private NullAway analysis; - - public GenericsChecks(VisitorState state, Config config, NullAway analysis) { - this.state = state; - this.config = config; - this.analysis = analysis; - } + + /** Do not instantiate; all methods should be static */ + private GenericsChecks() {} /** * Checks that for an instantiated generic type, {@code @Nullable} types are only used for type @@ -125,9 +126,9 @@ private static void reportInvalidAssignmentInstantiationError( ErrorMessage.MessageTypes.ASSIGN_GENERIC_NULLABLE, String.format( "Cannot assign from type " - + rhsType + + prettyTypeForError(rhsType) + " to type " - + lhsType + + prettyTypeForError(lhsType) + " due to mismatched nullability of type parameters")); state.reportMatch( errorBuilder.createErrorDescription( @@ -142,9 +143,9 @@ private static void reportInvalidReturnTypeError( ErrorMessage.MessageTypes.RETURN_NULLABLE_GENERIC, String.format( "Cannot return expression of type " - + returnType + + prettyTypeForError(returnType) + " from method with return type " - + methodType + + prettyTypeForError(methodType) + " due to mismatched nullability of type parameters")); state.reportMatch( errorBuilder.createErrorDescription( @@ -159,16 +160,16 @@ private static void reportMismatchedTypeForTernaryOperator( ErrorMessage.MessageTypes.ASSIGN_GENERIC_NULLABLE, String.format( "Conditional expression must have type " - + expressionType + + prettyTypeForError(expressionType) + " but the sub-expression has type " - + subPartType + + prettyTypeForError(subPartType) + ", which has mismatched nullability of type parameters")); state.reportMatch( errorBuilder.createErrorDescription( errorMessage, analysis.buildDescription(tree), state, null)); } - private void reportInvalidParametersNullabilityError( + private static void reportInvalidParametersNullabilityError( Type formalParameterType, Type actualParameterType, ExpressionTree paramExpression, @@ -179,15 +180,55 @@ private void reportInvalidParametersNullabilityError( new ErrorMessage( ErrorMessage.MessageTypes.PASS_NULLABLE_GENERIC, "Cannot pass parameter of type " - + actualParameterType + + prettyTypeForError(actualParameterType) + ", as formal parameter has type " - + formalParameterType + + prettyTypeForError(formalParameterType) + ", which has mismatched type parameter nullability"); state.reportMatch( errorBuilder.createErrorDescription( errorMessage, analysis.buildDescription(paramExpression), state, null)); } + private static void reportInvalidOverridingMethodReturnTypeError( + Tree methodTree, + Type overriddenMethodReturnType, + Type overridingMethodReturnType, + NullAway analysis, + VisitorState state) { + ErrorBuilder errorBuilder = analysis.getErrorBuilder(); + ErrorMessage errorMessage = + new ErrorMessage( + ErrorMessage.MessageTypes.WRONG_OVERRIDE_RETURN_GENERIC, + "Method returns " + + prettyTypeForError(overridingMethodReturnType) + + ", but overridden method returns " + + prettyTypeForError(overriddenMethodReturnType) + + ", which has mismatched type parameter nullability"); + state.reportMatch( + errorBuilder.createErrorDescription( + errorMessage, analysis.buildDescription(methodTree), state, null)); + } + + private static void reportInvalidOverridingMethodParamTypeError( + Tree formalParameterTree, + Type typeParameterType, + Type methodParamType, + NullAway analysis, + VisitorState state) { + ErrorBuilder errorBuilder = analysis.getErrorBuilder(); + ErrorMessage errorMessage = + new ErrorMessage( + ErrorMessage.MessageTypes.WRONG_OVERRIDE_PARAM_GENERIC, + "Parameter has type " + + prettyTypeForError(methodParamType) + + ", but overridden method has parameter type " + + prettyTypeForError(typeParameterType) + + ", which has mismatched type parameter nullability"); + state.reportMatch( + errorBuilder.createErrorDescription( + errorMessage, analysis.buildDescription(formalParameterTree), state, null)); + } + /** * This method returns the type of the given tree, including any type use annotations. * @@ -197,10 +238,11 @@ private void reportInvalidParametersNullabilityError( * Foo<@Nullable A>}). * * @param tree A tree for which we need the type with preserved annotations. + * @param state the visitor state * @return Type of the tree with preserved annotations. */ @Nullable - private Type getTreeType(Tree tree) { + private static Type getTreeType(Tree tree, VisitorState state) { if (tree instanceof NewClassTree && ((NewClassTree) tree).getIdentifier() instanceof ParameterizedTypeTree) { ParameterizedTypeTree paramTypedTree = @@ -210,9 +252,14 @@ private Type getTreeType(Tree tree) { // TODO: support diamond operators return null; } - return typeWithPreservedAnnotations(paramTypedTree); + return typeWithPreservedAnnotations(paramTypedTree, state); } else { - return ASTHelpers.getType(tree); + Type result = ASTHelpers.getType(tree); + if (result != null && result.isRaw()) { + // bail out of any checking involving raw types for now + return null; + } + return result; } } @@ -224,9 +271,12 @@ private Type getTreeType(Tree tree) { * * @param tree the tree to check, which must be either an {@link AssignmentTree} or a {@link * VariableTree} + * @param analysis the analysis object + * @param state the visitor state */ - public void checkTypeParameterNullnessForAssignability(Tree tree) { - if (!config.isJSpecifyMode()) { + public static void checkTypeParameterNullnessForAssignability( + Tree tree, NullAway analysis, VisitorState state) { + if (!analysis.getConfig().isJSpecifyMode()) { return; } Tree lhsTree; @@ -245,21 +295,33 @@ public void checkTypeParameterNullnessForAssignability(Tree tree) { if (rhsTree == null || rhsTree.getKind().equals(Tree.Kind.NULL_LITERAL)) { return; } - Type lhsType = getTreeType(lhsTree); - Type rhsType = getTreeType(rhsTree); + Type lhsType = getTreeType(lhsTree, state); + Type rhsType = getTreeType(rhsTree, state); if (lhsType instanceof Type.ClassType && rhsType instanceof Type.ClassType) { boolean isAssignmentValid = - compareNullabilityAnnotations((Type.ClassType) lhsType, (Type.ClassType) rhsType); + compareNullabilityAnnotations((Type.ClassType) lhsType, (Type.ClassType) rhsType, state); if (!isAssignmentValid) { reportInvalidAssignmentInstantiationError(tree, lhsType, rhsType, state, analysis); } } } - public void checkTypeParameterNullnessForFunctionReturnType( - ExpressionTree retExpr, Symbol.MethodSymbol methodSymbol) { - if (!config.isJSpecifyMode()) { + /** + * Checks that the nullability of type parameters for a returned expression matches that of the + * type parameters of the enclosing method's return type. + * + * @param retExpr the returned expression + * @param methodSymbol symbol for enclosing method + * @param analysis the analysis object + * @param state the visitor state + */ + public static void checkTypeParameterNullnessForFunctionReturnType( + ExpressionTree retExpr, + Symbol.MethodSymbol methodSymbol, + NullAway analysis, + VisitorState state) { + if (!analysis.getConfig().isJSpecifyMode()) { return; } @@ -268,12 +330,12 @@ public void checkTypeParameterNullnessForFunctionReturnType( if (formalReturnType.getTypeArguments().isEmpty()) { return; } - Type returnExpressionType = getTreeType(retExpr); + Type returnExpressionType = getTreeType(retExpr, state); if (formalReturnType instanceof Type.ClassType && returnExpressionType instanceof Type.ClassType) { boolean isReturnTypeValid = compareNullabilityAnnotations( - (Type.ClassType) formalReturnType, (Type.ClassType) returnExpressionType); + (Type.ClassType) formalReturnType, (Type.ClassType) returnExpressionType, state); if (!isReturnTypeValid) { reportInvalidReturnTypeError( retExpr, formalReturnType, returnExpressionType, state, analysis); @@ -291,58 +353,13 @@ public void checkTypeParameterNullnessForFunctionReturnType( * * @param lhsType type for the lhs of the assignment * @param rhsType type for the rhs of the assignment + * @param state the visitor state */ - private boolean compareNullabilityAnnotations(Type.ClassType lhsType, Type.ClassType rhsType) { - Types types = state.getTypes(); - // The base type of rhsType may be a subtype of lhsType's base type. In such cases, we must - // compare lhsType against the supertype of rhsType with a matching base type. - rhsType = (Type.ClassType) types.asSuper(rhsType, lhsType.tsym); - // This is impossible, considering the fact that standard Java subtyping succeeds before running - // NullAway - if (rhsType == null) { - throw new RuntimeException("Did not find supertype of " + rhsType + " matching " + lhsType); - } - List lhsTypeArguments = lhsType.getTypeArguments(); - List rhsTypeArguments = rhsType.getTypeArguments(); - // This is impossible, considering the fact that standard Java subtyping succeeds before running - // NullAway - if (lhsTypeArguments.size() != rhsTypeArguments.size()) { - throw new RuntimeException( - "Number of types arguments in " + rhsType + " does not match " + lhsType); - } - for (int i = 0; i < lhsTypeArguments.size(); i++) { - Type lhsTypeArgument = lhsTypeArguments.get(i); - Type rhsTypeArgument = rhsTypeArguments.get(i); - boolean isLHSNullableAnnotated = false; - List lhsAnnotations = lhsTypeArgument.getAnnotationMirrors(); - // To ensure that we are checking only jspecify nullable annotations - for (Attribute.TypeCompound annotation : lhsAnnotations) { - if (annotation.getAnnotationType().toString().equals(NULLABLE_NAME)) { - isLHSNullableAnnotated = true; - break; - } - } - boolean isRHSNullableAnnotated = false; - List rhsAnnotations = rhsTypeArgument.getAnnotationMirrors(); - // To ensure that we are checking only jspecify nullable annotations - for (Attribute.TypeCompound annotation : rhsAnnotations) { - if (annotation.getAnnotationType().toString().equals(NULLABLE_NAME)) { - isRHSNullableAnnotated = true; - break; - } - } - if (isLHSNullableAnnotated != isRHSNullableAnnotated) { - return false; - } - // nested generics - if (lhsTypeArgument.getTypeArguments().length() > 0) { - if (!compareNullabilityAnnotations( - (Type.ClassType) lhsTypeArgument, (Type.ClassType) rhsTypeArgument)) { - return false; - } - } - } - return true; + private static boolean compareNullabilityAnnotations( + Type lhsType, Type rhsType, VisitorState state) { + // it is fair to assume rhyType should be the same as lhsType as the Java compiler has passed + // before NullAway. + return lhsType.accept(new CompareNullabilityVisitor(state), rhsType); } /** @@ -351,58 +368,12 @@ private boolean compareNullabilityAnnotations(Type.ClassType lhsType, Type.Class * Type of the tree with the annotations. * * @param tree A parameterized typed tree for which we need class type with preserved annotations. + * @param state the visitor state * @return A Type with preserved annotations. */ - private Type.ClassType typeWithPreservedAnnotations(ParameterizedTypeTree tree) { - Type.ClassType type = (Type.ClassType) ASTHelpers.getType(tree); - Preconditions.checkNotNull(type); - Type nullableType = NULLABLE_TYPE_SUPPLIER.get(state); - List typeArguments = tree.getTypeArguments(); - List newTypeArgs = new ArrayList<>(); - boolean hasNullableAnnotation = false; - for (int i = 0; i < typeArguments.size(); i++) { - AnnotatedTypeTree annotatedType = null; - Tree curTypeArg = typeArguments.get(i); - // If the type argument has an annotation, it will either be an AnnotatedTypeTree, or a - // ParameterizedTypeTree in the case of a nested generic type - if (curTypeArg instanceof AnnotatedTypeTree) { - annotatedType = (AnnotatedTypeTree) curTypeArg; - } else if (curTypeArg instanceof ParameterizedTypeTree - && ((ParameterizedTypeTree) curTypeArg).getType() instanceof AnnotatedTypeTree) { - annotatedType = (AnnotatedTypeTree) ((ParameterizedTypeTree) curTypeArg).getType(); - } - List annotations = - annotatedType != null ? annotatedType.getAnnotations() : Collections.emptyList(); - for (AnnotationTree annotation : annotations) { - if (ASTHelpers.isSameType( - nullableType, ASTHelpers.getType(annotation.getAnnotationType()), state)) { - hasNullableAnnotation = true; - break; - } - } - // construct a TypeMetadata object containing a nullability annotation if needed - com.sun.tools.javac.util.List nullableAnnotationCompound = - hasNullableAnnotation - ? com.sun.tools.javac.util.List.from( - Collections.singletonList( - new Attribute.TypeCompound( - nullableType, com.sun.tools.javac.util.List.nil(), null))) - : com.sun.tools.javac.util.List.nil(); - TypeMetadata typeMetadata = - new TypeMetadata(new TypeMetadata.Annotations(nullableAnnotationCompound)); - Type currentTypeArgType = castToNonNull(ASTHelpers.getType(curTypeArg)); - if (currentTypeArgType.getTypeArguments().size() > 0) { - // nested generic type; recursively preserve its nullability type argument annotations - currentTypeArgType = typeWithPreservedAnnotations((ParameterizedTypeTree) curTypeArg); - } - Type.ClassType newTypeArgType = - (Type.ClassType) currentTypeArgType.cloneWithMetadata(typeMetadata); - newTypeArgs.add(newTypeArgType); - } - Type.ClassType finalType = - new Type.ClassType( - type.getEnclosingType(), com.sun.tools.javac.util.List.from(newTypeArgs), type.tsym); - return finalType; + private static Type.ClassType typeWithPreservedAnnotations( + ParameterizedTypeTree tree, VisitorState state) { + return (Type.ClassType) tree.accept(new PreservedAnnotationTreeVisitor(state), null); } /** @@ -417,32 +388,35 @@ private Type.ClassType typeWithPreservedAnnotations(ParameterizedTypeTree tree) * somewhat confusing; we may want to improve this in the future. * * @param tree A conditional expression tree to check + * @param analysis the analysis object + * @param state the visitor state */ - public void checkTypeParameterNullnessForConditionalExpression(ConditionalExpressionTree tree) { - if (!config.isJSpecifyMode()) { + public static void checkTypeParameterNullnessForConditionalExpression( + ConditionalExpressionTree tree, NullAway analysis, VisitorState state) { + if (!analysis.getConfig().isJSpecifyMode()) { return; } Tree truePartTree = tree.getTrueExpression(); Tree falsePartTree = tree.getFalseExpression(); - Type condExprType = getTreeType(tree); - Type truePartType = getTreeType(truePartTree); - Type falsePartType = getTreeType(falsePartTree); + Type condExprType = getTreeType(tree, state); + Type truePartType = getTreeType(truePartTree, state); + Type falsePartType = getTreeType(falsePartTree, state); // The condExpr type should be the least-upper bound of the true and false part types. To check // the nullability annotations, we check that the true and false parts are assignable to the // type of the whole expression if (condExprType instanceof Type.ClassType) { if (truePartType instanceof Type.ClassType) { if (!compareNullabilityAnnotations( - (Type.ClassType) condExprType, (Type.ClassType) truePartType)) { + (Type.ClassType) condExprType, (Type.ClassType) truePartType, state)) { reportMismatchedTypeForTernaryOperator( truePartTree, condExprType, truePartType, state, analysis); } } if (falsePartType instanceof Type.ClassType) { if (!compareNullabilityAnnotations( - (Type.ClassType) condExprType, (Type.ClassType) falsePartType)) { + (Type.ClassType) condExprType, (Type.ClassType) falsePartType, state)) { reportMismatchedTypeForTernaryOperator( falsePartTree, condExprType, falsePartType, state, analysis); } @@ -457,12 +431,16 @@ public void checkTypeParameterNullnessForConditionalExpression(ConditionalExpres * @param formalParams the formal parameters * @param actualParams the actual parameters * @param isVarArgs true if the call is to a varargs method + * @param analysis the analysis object + * @param state the visitor state */ - public void compareGenericTypeParameterNullabilityForCall( + public static void compareGenericTypeParameterNullabilityForCall( List formalParams, List actualParams, - boolean isVarArgs) { - if (!config.isJSpecifyMode()) { + boolean isVarArgs, + NullAway analysis, + VisitorState state) { + if (!analysis.getConfig().isJSpecifyMode()) { return; } int n = formalParams.size(); @@ -471,14 +449,14 @@ public void compareGenericTypeParameterNullabilityForCall( // all remaining actual arguments in the next loop. n = n - 1; } - for (int i = 0; i < n - 1; i++) { + for (int i = 0; i < n; i++) { Type formalParameter = formalParams.get(i).type; if (!formalParameter.getTypeArguments().isEmpty()) { - Type actualParameter = getTreeType(actualParams.get(i)); + Type actualParameter = getTreeType(actualParams.get(i), state); if (formalParameter instanceof Type.ClassType && actualParameter instanceof Type.ClassType) { if (!compareNullabilityAnnotations( - (Type.ClassType) formalParameter, (Type.ClassType) actualParameter)) { + (Type.ClassType) formalParameter, (Type.ClassType) actualParameter, state)) { reportInvalidParametersNullabilityError( formalParameter, actualParameter, actualParams.get(i), state, analysis); } @@ -491,11 +469,11 @@ public void compareGenericTypeParameterNullabilityForCall( Type varargsElementType = varargsArrayType.elemtype; if (varargsElementType.getTypeArguments().size() > 0) { for (int i = formalParams.size() - 1; i < actualParams.size(); i++) { - Type actualParameter = getTreeType(actualParams.get(i)); + Type actualParameter = getTreeType(actualParams.get(i), state); if (varargsElementType instanceof Type.ClassType && actualParameter instanceof Type.ClassType) { if (!compareNullabilityAnnotations( - (Type.ClassType) varargsElementType, (Type.ClassType) actualParameter)) { + (Type.ClassType) varargsElementType, (Type.ClassType) actualParameter, state)) { reportInvalidParametersNullabilityError( varargsElementType, actualParameter, actualParams.get(i), state, analysis); } @@ -504,4 +482,495 @@ public void compareGenericTypeParameterNullabilityForCall( } } } + + /** + * Visitor that is called from compareNullabilityAnnotations which recursively compares the + * Nullability annotations for the nested generic type arguments. Compares the Type it is called + * upon, i.e. the LHS type and the Type passed as an argument, i.e. The RHS type. + */ + public static class CompareNullabilityVisitor extends Types.DefaultTypeVisitor { + private final VisitorState state; + + CompareNullabilityVisitor(VisitorState state) { + this.state = state; + } + + @Override + public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) { + Types types = state.getTypes(); + // The base type of rhsType may be a subtype of lhsType's base type. In such cases, we must + // compare lhsType against the supertype of rhsType with a matching base type. + rhsType = (Type.ClassType) types.asSuper(rhsType, lhsType.tsym); + // This is impossible, considering the fact that standard Java subtyping succeeds before + // running NullAway + if (rhsType == null) { + throw new RuntimeException("Did not find supertype of " + rhsType + " matching " + lhsType); + } + List lhsTypeArguments = lhsType.getTypeArguments(); + List rhsTypeArguments = rhsType.getTypeArguments(); + // This is impossible, considering the fact that standard Java subtyping succeeds before + // running NullAway + if (lhsTypeArguments.size() != rhsTypeArguments.size()) { + throw new RuntimeException( + "Number of types arguments in " + rhsType + " does not match " + lhsType); + } + for (int i = 0; i < lhsTypeArguments.size(); i++) { + Type lhsTypeArgument = lhsTypeArguments.get(i); + Type rhsTypeArgument = rhsTypeArguments.get(i); + boolean isLHSNullableAnnotated = false; + List lhsAnnotations = lhsTypeArgument.getAnnotationMirrors(); + // To ensure that we are checking only jspecify nullable annotations + for (Attribute.TypeCompound annotation : lhsAnnotations) { + if (annotation.getAnnotationType().toString().equals(NULLABLE_NAME)) { + isLHSNullableAnnotated = true; + break; + } + } + boolean isRHSNullableAnnotated = false; + List rhsAnnotations = rhsTypeArgument.getAnnotationMirrors(); + // To ensure that we are checking only jspecify nullable annotations + for (Attribute.TypeCompound annotation : rhsAnnotations) { + if (annotation.getAnnotationType().toString().equals(NULLABLE_NAME)) { + isRHSNullableAnnotated = true; + break; + } + } + if (isLHSNullableAnnotated != isRHSNullableAnnotated) { + return false; + } + // nested generics + if (!lhsTypeArgument.accept(this, rhsTypeArgument)) { + return false; + } + } + return true; + } + + @Override + public Boolean visitArrayType(Type.ArrayType lhsType, Type rhsType) { + Type.ArrayType arrRhsType = (Type.ArrayType) rhsType; + return lhsType.getComponentType().accept(this, arrRhsType.getComponentType()); + } + + @Override + public Boolean visitType(Type t, Type type) { + return true; + } + } + + /** + * Visitor For getting the preserved Annotation Type for the nested generic type arguments within + * the ParameterizedTypeTree originally passed to TypeWithPreservedAnnotations method, since these + * nested arguments may not always be ParameterizedTypeTrees and may be of different types for + * e.g. ArrayTypeTree. + */ + public static class PreservedAnnotationTreeVisitor extends SimpleTreeVisitor { + + private final VisitorState state; + + PreservedAnnotationTreeVisitor(VisitorState state) { + this.state = state; + } + + @Override + public Type visitArrayType(ArrayTypeTree tree, Void p) { + Type elemType = tree.getType().accept(this, null); + return new Type.ArrayType(elemType, castToNonNull(ASTHelpers.getType(tree)).tsym); + } + + @Override + public Type visitParameterizedType(ParameterizedTypeTree tree, Void p) { + Type.ClassType type = (Type.ClassType) ASTHelpers.getType(tree); + Preconditions.checkNotNull(type); + Type nullableType = NULLABLE_TYPE_SUPPLIER.get(state); + List typeArguments = tree.getTypeArguments(); + List newTypeArgs = new ArrayList<>(); + for (int i = 0; i < typeArguments.size(); i++) { + AnnotatedTypeTree annotatedType = null; + Tree curTypeArg = typeArguments.get(i); + // If the type argument has an annotation, it will either be an AnnotatedTypeTree, or a + // ParameterizedTypeTree in the case of a nested generic type + if (curTypeArg instanceof AnnotatedTypeTree) { + annotatedType = (AnnotatedTypeTree) curTypeArg; + } else if (curTypeArg instanceof ParameterizedTypeTree + && ((ParameterizedTypeTree) curTypeArg).getType() instanceof AnnotatedTypeTree) { + annotatedType = (AnnotatedTypeTree) ((ParameterizedTypeTree) curTypeArg).getType(); + } + List annotations = + annotatedType != null ? annotatedType.getAnnotations() : Collections.emptyList(); + boolean hasNullableAnnotation = false; + for (AnnotationTree annotation : annotations) { + if (ASTHelpers.isSameType( + nullableType, ASTHelpers.getType(annotation.getAnnotationType()), state)) { + hasNullableAnnotation = true; + break; + } + } + // construct a TypeMetadata object containing a nullability annotation if needed + com.sun.tools.javac.util.List nullableAnnotationCompound = + hasNullableAnnotation + ? com.sun.tools.javac.util.List.from( + Collections.singletonList( + new Attribute.TypeCompound( + nullableType, com.sun.tools.javac.util.List.nil(), null))) + : com.sun.tools.javac.util.List.nil(); + TypeMetadata typeMetadata = + new TypeMetadata(new TypeMetadata.Annotations(nullableAnnotationCompound)); + Type currentTypeArgType = curTypeArg.accept(this, null); + Type newTypeArgType = currentTypeArgType.cloneWithMetadata(typeMetadata); + newTypeArgs.add(newTypeArgType); + } + Type.ClassType finalType = + new Type.ClassType( + type.getEnclosingType(), com.sun.tools.javac.util.List.from(newTypeArgs), type.tsym); + return finalType; + } + + /** By default, just use the type computed by javac */ + @Override + protected Type defaultAction(Tree node, Void unused) { + return castToNonNull(ASTHelpers.getType(node)); + } + } + + /** + * Checks that type parameter nullability is consistent between an overriding method and the + * corresponding overridden method. + * + * @param tree A method tree to check + * @param overridingMethod A symbol of the overriding method + * @param overriddenMethod A symbol of the overridden method + * @param analysis the analysis object + * @param state the visitor state + */ + public static void checkTypeParameterNullnessForMethodOverriding( + MethodTree tree, + Symbol.MethodSymbol overridingMethod, + Symbol.MethodSymbol overriddenMethod, + NullAway analysis, + VisitorState state) { + if (!analysis.getConfig().isJSpecifyMode()) { + return; + } + // Obtain type parameters for the overridden method within the context of the overriding + // method's class + Type methodWithTypeParams = + state.getTypes().memberType(overridingMethod.owner.type, overriddenMethod); + + checkTypeParameterNullnessForOverridingMethodReturnType( + tree, methodWithTypeParams, analysis, state); + checkTypeParameterNullnessForOverridingMethodParameterType( + tree, methodWithTypeParams, analysis, state); + } + + /** + * Computes the nullability of the return type of some generic method when seen as a member of + * some class {@code C}, based on type parameter nullability within {@code C}. + * + *

Consider the following example: + * + * 

+   *     interface Fn
 {
+   *         R apply(P p);
+   *     }
+   *     class C implements Fn {
+   *         public @Nullable String apply(String p) {
+   *             return null;
+   *         }
+   *     }
+   *
+ * + * Within the context of class {@code C}, the method {@code Fn.apply} has a return type of + * {@code @Nullable String}, since {@code @Nullable String} is passed as the type parameter for + * {@code R}. Hence, it is valid for overriding method {@code C.apply} to return {@code @Nullable + * String}. + * + * @param method the generic method + * @param enclosingType the enclosing type in which we want to know {@code method}'s return type + * nullability + * @param state Visitor state + * @param config The analysis config + * @return nullability of the return type of {@code method} in the context of {@code + * enclosingType} + */ + public static Nullness getGenericMethodReturnTypeNullness( + Symbol.MethodSymbol method, Type enclosingType, VisitorState state, Config config) { + Type overriddenMethodType = state.getTypes().memberType(enclosingType, method); + if (!(overriddenMethodType instanceof Type.MethodType)) { + throw new RuntimeException("expected method type but instead got " + overriddenMethodType); + } + return getTypeNullness(overriddenMethodType.getReturnType(), config); + } + + /** + * Computes the nullness of the return of a generic method at an invocation, in the context of the + * declared type of its receiver argument. If the return type is a type variable, its nullness + * depends on the nullability of the corresponding type parameter in the receiver's type. + * + *

Consider the following example: + * + * 

+   *     interface Fn
 {
+   *         R apply(P p);
+   *     }
+   *     class C implements Fn {
+   *         public @Nullable String apply(String p) {
+   *             return null;
+   *         }
+   *     }
+   *     static void m() {
+   *         Fn f = new C();
+   *         f.apply("hello").hashCode(); // NPE
+   *     }
+   *
+ * + * The declared type of {@code f} passes {@code Nullable String} as the type parameter for type + * variable {@code R}. So, the call {@code f.apply("hello")} returns {@code @Nullable} and an + * error should be reported. + * + * @param invokedMethodSymbol symbol for the invoked method + * @param tree the tree for the invocation + * @return Nullness of invocation's return type, or {@code NONNULL} if the call does not invoke an + * instance method + */ + public static Nullness getGenericReturnNullnessAtInvocation( + Symbol.MethodSymbol invokedMethodSymbol, + MethodInvocationTree tree, + VisitorState state, + Config config) { + if (!(tree.getMethodSelect() instanceof MemberSelectTree)) { + return Nullness.NONNULL; + } + Type methodReceiverType = + castToNonNull( + ASTHelpers.getType(((MemberSelectTree) tree.getMethodSelect()).getExpression())); + return getGenericMethodReturnTypeNullness( + invokedMethodSymbol, methodReceiverType, state, config); + } + + /** + * Computes the nullness of a formal parameter of a generic method at an invocation, in the + * context of the declared type of its receiver argument. If the formal parameter's type is a type + * variable, its nullness depends on the nullability of the corresponding type parameter in the + * receiver's type. + * + *

Consider the following example: + * + * 

+   *     interface Fn
 {
+   *         R apply(P p);
+   *     }
+   *     class C implements Fn<@Nullable String, String> {
+   *         public String apply(@Nullable String p) {
+   *             return "";
+   *         }
+   *     }
+   *     static void m() {
+   *         Fn<@Nullable String, String> f = new C();
+   *         f.apply(null);
+   *     }
+   *
+ * + * The declared type of {@code f} passes {@code Nullable String} as the type parameter for type + * variable {@code P}. So, it is legal to pass {@code null} as a parameter to {@code f.apply}. + * + * @param paramIndex parameter index + * @param invokedMethodSymbol symbol for the invoked method + * @param tree the tree for the invocation + * @param state the visitor state + * @param config the analysis config + * @return Nullness of parameter at {@code paramIndex}, or {@code NONNULL} if the call does not + * invoke an instance method + */ + public static Nullness getGenericParameterNullnessAtInvocation( + int paramIndex, + Symbol.MethodSymbol invokedMethodSymbol, + MethodInvocationTree tree, + VisitorState state, + Config config) { + if (!(tree.getMethodSelect() instanceof MemberSelectTree)) { + return Nullness.NONNULL; + } + Type methodReceiverType = + castToNonNull( + ASTHelpers.getType(((MemberSelectTree) tree.getMethodSelect()).getExpression())); + return getGenericMethodParameterNullness( + paramIndex, invokedMethodSymbol, methodReceiverType, state, config); + } + + /** + * Computes the nullability of a parameter type of some generic method when seen as a member of + * some class {@code C}, based on type parameter nullability within {@code C}. + * + *

Consider the following example: + * + * 

+   *     interface Fn
 {
+   *         R apply(P p);
+   *     }
+   *     class C implements Fn<@Nullable String, String> {
+   *         public String apply(@Nullable String p) {
+   *             return "";
+   *         }
+   *     }
+   *
+ * + * Within the context of class {@code C}, the method {@code Fn.apply} has a parameter type of + * {@code @Nullable String}, since {@code @Nullable String} is passed as the type parameter for + * {@code P}. Hence, overriding method {@code C.apply} must take a {@code @Nullable String} as a + * parameter. + * + * @param parameterIndex index of the parameter + * @param method the generic method + * @param enclosingType the enclosing type in which we want to know {@code method}'s parameter + * type nullability + * @param state the visitor state + * @param config the analysis config + * @return nullability of the relevant parameter type of {@code method} in the context of {@code + * enclosingType} + */ + public static Nullness getGenericMethodParameterNullness( + int parameterIndex, + Symbol.MethodSymbol method, + Type enclosingType, + VisitorState state, + Config config) { + Type methodType = state.getTypes().memberType(enclosingType, method); + Type paramType = methodType.getParameterTypes().get(parameterIndex); + return getTypeNullness(paramType, config); + } + + /** + * This method compares the type parameter annotations for overriding method parameters with + * corresponding type parameters for the overridden method and reports an error if they don't + * match + * + * @param tree tree for overriding method + * @param overriddenMethodType type of the overridden method + * @param analysis the analysis object + * @param state the visitor state + */ + private static void checkTypeParameterNullnessForOverridingMethodParameterType( + MethodTree tree, Type overriddenMethodType, NullAway analysis, VisitorState state) { + List methodParameters = tree.getParameters(); + List overriddenMethodParameterTypes = overriddenMethodType.getParameterTypes(); + // TODO handle varargs; they are not handled for now + for (int i = 0; i < methodParameters.size(); i++) { + Type overridingMethodParameterType = ASTHelpers.getType(methodParameters.get(i)); + Type overriddenMethodParameterType = overriddenMethodParameterTypes.get(i); + if (overriddenMethodParameterType instanceof Type.ClassType + && overridingMethodParameterType instanceof Type.ClassType) { + if (!compareNullabilityAnnotations( + (Type.ClassType) overriddenMethodParameterType, + (Type.ClassType) overridingMethodParameterType, + state)) { + reportInvalidOverridingMethodParamTypeError( + methodParameters.get(i), + overriddenMethodParameterType, + overridingMethodParameterType, + analysis, + state); + } + } + } + } + + /** + * This method compares the type parameter annotations for an overriding method's return type with + * corresponding type parameters for the overridden method and reports an error if they don't + * match + * + * @param tree tree for overriding method + * @param overriddenMethodType type of the overridden method + * @param analysis the analysis object + * @param state the visitor state + */ + private static void checkTypeParameterNullnessForOverridingMethodReturnType( + MethodTree tree, Type overriddenMethodType, NullAway analysis, VisitorState state) { + Type overriddenMethodReturnType = overriddenMethodType.getReturnType(); + Type overridingMethodReturnType = ASTHelpers.getType(tree.getReturnType()); + if (!(overriddenMethodReturnType instanceof Type.ClassType)) { + return; + } + Preconditions.checkArgument(overridingMethodReturnType instanceof Type.ClassType); + if (!compareNullabilityAnnotations( + (Type.ClassType) overriddenMethodReturnType, + (Type.ClassType) overridingMethodReturnType, + state)) { + reportInvalidOverridingMethodReturnTypeError( + tree, overriddenMethodReturnType, overridingMethodReturnType, analysis, state); + } + } + + /** + * @param type A type for which we need the Nullness. + * @param config The analysis config + * @return Returns the Nullness of the type based on the Nullability annotation. + */ + private static Nullness getTypeNullness(Type type, Config config) { + boolean hasNullableAnnotation = + Nullness.hasNullableAnnotation(type.getAnnotationMirrors().stream(), config); + if (hasNullableAnnotation) { + return Nullness.NULLABLE; + } + return Nullness.NONNULL; + } + + /** + * Returns a pretty-printed representation of type suitable for error messages. The representation + * uses simple names rather than fully-qualified names, and retains all type-use annotations. + */ + public static String prettyTypeForError(Type type) { + return type.accept(PRETTY_TYPE_VISITOR, null); + } + + /** This code is a modified version of code in {@link com.google.errorprone.util.Signatures} */ + private static final Type.Visitor PRETTY_TYPE_VISITOR = + new Types.DefaultTypeVisitor() { + @Override + public String visitWildcardType(Type.WildcardType t, Void unused) { + StringBuilder sb = new StringBuilder(); + sb.append(t.kind); + if (t.kind != BoundKind.UNBOUND) { + sb.append(t.type.accept(this, null)); + } + return sb.toString(); + } + + @Override + public String visitClassType(Type.ClassType t, Void s) { + StringBuilder sb = new StringBuilder(); + for (Attribute.TypeCompound compound : t.getAnnotationMirrors()) { + sb.append('@'); + sb.append(compound.type.accept(this, null)); + sb.append(' '); + } + sb.append(t.tsym.getSimpleName()); + if (t.getTypeArguments().nonEmpty()) { + sb.append('<'); + sb.append( + t.getTypeArguments().stream() + .map(a -> a.accept(this, null)) + .collect(joining(", "))); + sb.append(">"); + } + return sb.toString(); + } + + @Override + public String visitCapturedType(Type.CapturedType t, Void s) { + return t.wildcard.accept(this, null); + } + + @Override + public String visitArrayType(Type.ArrayType t, Void unused) { + // TODO properly print cases like int @Nullable[] + return t.elemtype.accept(this, null) + "[]"; + } + + @Override + public String visitType(Type t, Void s) { + return t.toString(); + } + }; } diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index a69bbcd846..a0816b3712 100644 --- a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java +++ b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java @@ -22,9 +22,11 @@ package com.uber.nullaway; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; import com.sun.tools.javac.code.Symbol; +import com.uber.nullaway.handlers.stream.StreamTypeRecord; import java.util.Objects; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -130,7 +132,23 @@ public interface LibraryModels { ImmutableSetMultimap castToNonNullMethods(); /** - * representation of a method as a qualified class name + a signature for the method + * Get a list of custom stream library specifications. + * + *

This allows users to define filter/map/other methods for APIs which behave similarly to Java + * 8 streams or ReactiveX streams, so that NullAway is able to understand nullability invariants + * across stream API calls. See {@link com.uber.nullaway.handlers.stream.StreamModelBuilder} for + * details on how to construct these {@link com.uber.nullaway.handlers.stream.StreamTypeRecord} + * specs. A full example is available at {@link + * com.uber.nullaway.testlibrarymodels.TestLibraryModels}. + * + * @return A list of StreamTypeRecord specs (usually generated using StreamModelBuilder). + */ + default ImmutableList customStreamNullabilitySpecs() { + return ImmutableList.of(); + } + + /** + * Representation of a method as a qualified class name + a signature for the method * *

The formatting of a method signature should match the result of calling {@link * Symbol.MethodSymbol#toString()} on the corresponding symbol. See {@link @@ -151,6 +169,7 @@ public interface LibraryModels { final class MethodRef { public final String enclosingClass; + /** * we store the method name separately to enable fast comparison with MethodSymbols. See {@link * com.uber.nullaway.handlers.LibraryModelsHandler.OptimizedLibraryModels} diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 9466745eef..bfed0af259 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -28,6 +28,7 @@ import static com.sun.source.tree.Tree.Kind.OTHER; import static com.sun.source.tree.Tree.Kind.PARENTHESIZED; import static com.sun.source.tree.Tree.Kind.TYPE_CAST; +import static com.uber.nullaway.ASTHelpersBackports.isStatic; import static com.uber.nullaway.ErrorBuilder.errMsgForInitializer; import static com.uber.nullaway.NullabilityUtil.castToNonNull; @@ -220,6 +221,11 @@ private enum NullMarking { private final Config config; + /** Returns the configuration being used for this analysis. */ + public Config getConfig() { + return config; + } + private final ErrorBuilder errorBuilder; /** @@ -470,7 +476,7 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) { } // generics check if (lhsType != null && lhsType.getTypeArguments().length() > 0) { - new GenericsChecks(state, config, this).checkTypeParameterNullnessForAssignability(tree); + GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state); } Symbol assigned = ASTHelpers.getSymbol(tree.getVariable()); @@ -621,12 +627,18 @@ public Description matchMethod(MethodTree tree, VisitorState state) { // package) Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree); handler.onMatchMethod(this, tree, state, methodSymbol); - boolean isOverriding = ASTHelpers.hasAnnotation(methodSymbol, Override.class, state); + boolean isOverriding = ASTHelpers.hasAnnotation(methodSymbol, "java.lang.Override", state); boolean exhaustiveOverride = config.exhaustiveOverride(); if (isOverriding || !exhaustiveOverride) { Symbol.MethodSymbol closestOverriddenMethod = NullabilityUtil.getClosestOverriddenMethod(methodSymbol, state.getTypes()); if (closestOverriddenMethod != null) { + if (config.isJSpecifyMode()) { + // Check that any generic type parameters in the return type and parameter types are + // identical (invariant) across the overriding and overridden methods + GenericsChecks.checkTypeParameterNullnessForMethodOverriding( + tree, methodSymbol, closestOverriddenMethod, this, state); + } return checkOverriding(closestOverriddenMethod, methodSymbol, null, state); } } @@ -722,7 +734,14 @@ private Description checkParamOverriding( overriddenMethodArgNullnessMap[i] = Nullness.paramHasNullableAnnotation(overriddenMethod, i, config) ? Nullness.NULLABLE - : Nullness.NONNULL; + : (config.isJSpecifyMode() + ? GenericsChecks.getGenericMethodParameterNullness( + i, + overriddenMethod, + overridingParamSymbols.get(i).owner.owner.type, + state, + config) + : Nullness.NONNULL); } } @@ -808,6 +827,21 @@ static Trees getTreesInstance(VisitorState state) { return Trees.instance(JavacProcessingEnvironment.instance(state.context)); } + private Nullness getMethodReturnNullness( + Symbol.MethodSymbol methodSymbol, VisitorState state, Nullness defaultForUnannotated) { + final boolean isMethodAnnotated = !codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config); + Nullness methodReturnNullness = + defaultForUnannotated; // Permissive default for unannotated code. + if (isMethodAnnotated) { + methodReturnNullness = + Nullness.hasNullableAnnotation(methodSymbol, config) + ? Nullness.NULLABLE + : Nullness.NONNULL; + } + return handler.onOverrideMethodReturnNullability( + methodSymbol, state, isMethodAnnotated, methodReturnNullness); + } + private Description checkReturnExpression( Tree tree, ExpressionTree retExpr, Symbol.MethodSymbol methodSymbol, VisitorState state) { Type returnType = methodSymbol.getReturnType(); @@ -822,8 +856,7 @@ private Description checkReturnExpression( // support) return Description.NO_MATCH; } - if (codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config) - || Nullness.hasNullableAnnotation(methodSymbol, config)) { + if (getMethodReturnNullness(methodSymbol, state, Nullness.NULLABLE).equals(Nullness.NULLABLE)) { return Description.NO_MATCH; } if (mayBeNullExpr(state, retExpr)) { @@ -836,8 +869,8 @@ private Description checkReturnExpression( state, methodSymbol); } - new GenericsChecks(state, config, this) - .checkTypeParameterNullnessForFunctionReturnType(retExpr, methodSymbol); + GenericsChecks.checkTypeParameterNullnessForFunctionReturnType( + retExpr, methodSymbol, this, state); return Description.NO_MATCH; } @@ -908,63 +941,40 @@ private Description checkOverriding( Symbol.MethodSymbol overridingMethod, @Nullable MemberReferenceTree memberReferenceTree, VisitorState state) { - final boolean isOverriddenMethodAnnotated = - !codeAnnotationInfo.isSymbolUnannotated(overriddenMethod, config); - Nullness overriddenMethodReturnNullness = - Nullness.NULLABLE; // Permissive default for unannotated code. - if (isOverriddenMethodAnnotated && !Nullness.hasNullableAnnotation(overriddenMethod, config)) { - overriddenMethodReturnNullness = Nullness.NONNULL; - } - overriddenMethodReturnNullness = - handler.onOverrideMethodInvocationReturnNullability( - overriddenMethod, state, isOverriddenMethodAnnotated, overriddenMethodReturnNullness); - // if the super method returns nonnull, - // overriding method better not return nullable - if (overriddenMethodReturnNullness.equals(Nullness.NONNULL)) { - final boolean isOverridingMethodAnnotated = - !codeAnnotationInfo.isSymbolUnannotated(overridingMethod, config); - // Note that, for the overriding method, the permissive default is non-null. - Nullness overridingMethodReturnNullness = Nullness.NONNULL; - if (isOverridingMethodAnnotated && Nullness.hasNullableAnnotation(overridingMethod, config)) { - overridingMethodReturnNullness = Nullness.NULLABLE; + // if the super method returns nonnull, overriding method better not return nullable + // Note that, for the overriding method, the permissive default is non-null, + // but it's nullable for the overridden one. + if (overriddenMethodReturnsNonNull(overriddenMethod, overridingMethod.owner.type, state) + && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL) + .equals(Nullness.NULLABLE) + && (memberReferenceTree == null + || getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE))) { + String message; + if (memberReferenceTree != null) { + message = + "referenced method returns @Nullable, but functional interface method " + + ASTHelpers.enclosingClass(overriddenMethod) + + "." + + overriddenMethod.toString() + + " returns @NonNull"; + } else { + message = + "method returns @Nullable, but superclass method " + + ASTHelpers.enclosingClass(overriddenMethod) + + "." + + overriddenMethod.toString() + + " returns @NonNull"; } - // We must once again check the handler chain, to allow it to update nullability of the - // overriding method - // (e.g. through AcknowledgeRestrictiveAnnotations=true) - overridingMethodReturnNullness = - handler.onOverrideMethodInvocationReturnNullability( - overridingMethod, state, isOverridingMethodAnnotated, overridingMethodReturnNullness); - if (overridingMethodReturnNullness.equals(Nullness.NULLABLE) - && (memberReferenceTree == null - || getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE))) { - String message; - if (memberReferenceTree != null) { - message = - "referenced method returns @Nullable, but functional interface method " - + ASTHelpers.enclosingClass(overriddenMethod) - + "." - + overriddenMethod.toString() - + " returns @NonNull"; - } else { - message = - "method returns @Nullable, but superclass method " - + ASTHelpers.enclosingClass(overriddenMethod) - + "." - + overriddenMethod.toString() - + " returns @NonNull"; - } - - Tree errorTree = - memberReferenceTree != null - ? memberReferenceTree - : getTreesInstance(state).getTree(overridingMethod); - return errorBuilder.createErrorDescription( - new ErrorMessage(MessageTypes.WRONG_OVERRIDE_RETURN, message), - buildDescription(errorTree), - state, - overriddenMethod); - } + Tree errorTree = + memberReferenceTree != null + ? memberReferenceTree + : getTreesInstance(state).getTree(overridingMethod); + return errorBuilder.createErrorDescription( + new ErrorMessage(MessageTypes.WRONG_OVERRIDE_RETURN, message), + buildDescription(errorTree), + state, + overriddenMethod); } // if any parameter in the super method is annotated @Nullable, // overriding method cannot assume @Nonnull @@ -972,6 +982,23 @@ private Description checkOverriding( overridingMethod.getParameters(), overriddenMethod, null, memberReferenceTree, state); } + private boolean overriddenMethodReturnsNonNull( + Symbol.MethodSymbol overriddenMethod, Type overridingMethodType, VisitorState state) { + Nullness methodReturnNullness = + getMethodReturnNullness(overriddenMethod, state, Nullness.NULLABLE); + if (!methodReturnNullness.equals(Nullness.NONNULL)) { + return false; + } + // In JSpecify mode, for generic methods, we additionally need to check the return nullness + // using the type parameters from the type enclosing the overriding method + if (config.isJSpecifyMode()) { + return GenericsChecks.getGenericMethodReturnTypeNullness( + overriddenMethod, overridingMethodType, state, config) + .equals(Nullness.NONNULL); + } + return true; + } + @Override public Description matchIdentifier(IdentifierTree tree, VisitorState state) { if (!withinAnnotatedCode(state)) { @@ -1013,9 +1040,7 @@ private Description checkForReadBeforeInit(ExpressionTree tree, VisitorState sta } // for static fields, make sure the enclosing init is a static method or block - @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater - boolean isStatic = symbol.isStatic(); - if (isStatic) { + if (isStatic(symbol)) { Tree enclosing = enclosingBlockPath.getLeaf(); if (enclosing instanceof MethodTree && !ASTHelpers.getSymbol((MethodTree) enclosing).isStatic()) { @@ -1124,9 +1149,7 @@ private boolean fieldAlwaysInitializedBeforeRead( Symbol symbol, TreePath pathToRead, VisitorState state, TreePath enclosingBlockPath) { AccessPathNullnessAnalysis nullnessAnalysis = getNullnessAnalysis(state); Set nonnullFields; - @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater - boolean isStatic = symbol.isStatic(); - if (isStatic) { + if (isStatic(symbol)) { nonnullFields = nullnessAnalysis.getNonnullStaticFieldsBefore(pathToRead, state.context); } else { nonnullFields = new LinkedHashSet<>(); @@ -1343,7 +1366,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) { } VarSymbol symbol = ASTHelpers.getSymbol(tree); if (tree.getInitializer() != null) { - new GenericsChecks(state, config, this).checkTypeParameterNullnessForAssignability(tree); + GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state); } if (symbol.type.isPrimitive() && tree.getInitializer() != null) { @@ -1495,8 +1518,7 @@ public Description matchUnary(UnaryTree tree, VisitorState state) { public Description matchConditionalExpression( ConditionalExpressionTree tree, VisitorState state) { if (withinAnnotatedCode(state)) { - new GenericsChecks(state, config, this) - .checkTypeParameterNullnessForConditionalExpression(tree); + GenericsChecks.checkTypeParameterNullnessForConditionalExpression(tree, this, state); doUnboxingCheck(state, tree.getCondition()); } return Description.NO_MATCH; @@ -1621,12 +1643,14 @@ private Description handleInvocation( argumentPositionNullness[i] = Nullness.paramHasNullableAnnotation(methodSymbol, i, config) ? Nullness.NULLABLE - : Nullness.NONNULL; + : ((config.isJSpecifyMode() && tree instanceof MethodInvocationTree) + ? GenericsChecks.getGenericParameterNullnessAtInvocation( + i, methodSymbol, (MethodInvocationTree) tree, state, config) + : Nullness.NONNULL); } } - new GenericsChecks(state, config, this) - .compareGenericTypeParameterNullabilityForCall( - formalParams, actualParams, methodSymbol.isVarArgs()); + GenericsChecks.compareGenericTypeParameterNullabilityForCall( + formalParams, actualParams, methodSymbol.isVarArgs(), this, state); } // Allow handlers to override the list of non-null argument positions @@ -1723,7 +1747,9 @@ private Description checkCastToNonNullTakesNullable( + "at the invocation site, but which are known not to be null at runtime."; return errorBuilder.createErrorDescription( new ErrorMessage(MessageTypes.CAST_TO_NONNULL_ARG_NONNULL, message), - tree, + // The Tree passed as suggestTree is the expression being cast + // to avoid recomputing the arg index: + actual, buildDescription(tree), state, null); @@ -2110,9 +2136,7 @@ private FieldInitEntities collectEntities(ClassTree tree, VisitorState state) { // matchVariable() continue; } - @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater - boolean fieldIsStatic = fieldSymbol.isStatic(); - if (fieldIsStatic) { + if (isStatic(fieldSymbol)) { nonnullStaticFields.add(fieldSymbol); } else { nonnullInstanceFields.add(fieldSymbol); @@ -2290,7 +2314,9 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) { + " for method invocation " + state.getSourceForNode(expr)); } - exprMayBeNull = mayBeNullMethodCall((Symbol.MethodSymbol) exprSymbol); + exprMayBeNull = + mayBeNullMethodCall( + (Symbol.MethodSymbol) exprSymbol, (MethodInvocationTree) expr, state); break; case CONDITIONAL_EXPRESSION: case ASSIGNMENT: @@ -2309,11 +2335,21 @@ private boolean mayBeNullExpr(VisitorState state, ExpressionTree expr) { return exprMayBeNull && nullnessFromDataflow(state, expr); } - private boolean mayBeNullMethodCall(Symbol.MethodSymbol exprSymbol) { + private boolean mayBeNullMethodCall( + Symbol.MethodSymbol exprSymbol, MethodInvocationTree invocationTree, VisitorState state) { if (codeAnnotationInfo.isSymbolUnannotated(exprSymbol, config)) { return false; } - return Nullness.hasNullableAnnotation(exprSymbol, config); + if (Nullness.hasNullableAnnotation(exprSymbol, config)) { + return true; + } + if (config.isJSpecifyMode() + && GenericsChecks.getGenericReturnNullnessAtInvocation( + exprSymbol, invocationTree, state, config) + .equals(Nullness.NULLABLE)) { + return true; + } + return false; } public boolean nullnessFromDataflow(VisitorState state, ExpressionTree expr) { diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 54a4c37710..ba4be9f0f3 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -22,9 +22,6 @@ package com.uber.nullaway; -import static com.sun.tools.javac.code.TypeAnnotationPosition.TypePathEntryKind.ARRAY; -import static com.sun.tools.javac.code.TypeAnnotationPosition.TypePathEntryKind.INNER_TYPE; - import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.errorprone.VisitorState; diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index 1e96a7abd6..c1b8198de0 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -19,6 +19,8 @@ package com.uber.nullaway; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.util.List; import java.util.stream.Stream; import javax.lang.model.element.AnnotationMirror; import org.checkerframework.nullaway.dataflow.analysis.AbstractValue; @@ -210,10 +212,36 @@ public static boolean hasNullableAnnotation(Symbol symbol, Config config) { */ public static boolean paramHasNullableAnnotation( Symbol.MethodSymbol symbol, int paramInd, Config config) { + // We treat the (generated) equals() method of record types to have a @Nullable parameter, as + // the generated implementation handles null (as required by the contract of Object.equals()) + if (isRecordEqualsParam(symbol, paramInd)) { + return true; + } return hasNullableAnnotation( NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd), config); } + private static boolean isRecordEqualsParam(Symbol.MethodSymbol symbol, int paramInd) { + // Here we compare with toString() to preserve compatibility with JDK 11 (records only + // introduced in JDK 16) + if (!symbol.owner.getKind().toString().equals("RECORD")) { + return false; + } + if (!symbol.getSimpleName().contentEquals("equals")) { + return false; + } + // Check for a boolean return type and a single parameter of type java.lang.Object + Type type = symbol.type; + List parameterTypes = type.getParameterTypes(); + if (!(type.getReturnType().toString().equals("boolean") + && parameterTypes != null + && parameterTypes.size() == 1 + && parameterTypes.get(0).toString().equals("java.lang.Object"))) { + return false; + } + return paramInd == 0; + } + /** * Does the parameter of {@code symbol} at {@code paramInd} have a {@code @NonNull} declaration or * type-use annotation? This method works for methods defined in either source or class files. diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java index 4a90c1fbf5..056344a53f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java @@ -191,6 +191,7 @@ private static AccessPath fromVanillaMethodCall( static AccessPath switchRoot(AccessPath origAP, Element newRoot) { return new AccessPath(newRoot, origAP.elements, origAP.mapGetArg); } + /** * Construct the access path given a {@code base.element} structure. * diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java index 85fa9ce61c..6681d69797 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java @@ -16,6 +16,7 @@ package com.uber.nullaway.dataflow; import static com.google.common.base.Preconditions.checkNotNull; +import static com.uber.nullaway.ASTHelpersBackports.isStatic; import static com.uber.nullaway.NullabilityUtil.castToNonNull; import static com.uber.nullaway.Nullness.BOTTOM; import static com.uber.nullaway.Nullness.NONNULL; @@ -28,11 +29,13 @@ import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.suppliers.Suppliers; import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.MethodInvocationTree; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.TypeTag; import com.uber.nullaway.CodeAnnotationInfo; import com.uber.nullaway.Config; +import com.uber.nullaway.GenericsChecks; import com.uber.nullaway.NullabilityUtil; import com.uber.nullaway.Nullness; import com.uber.nullaway.handlers.Handler; @@ -72,6 +75,7 @@ import org.checkerframework.nullaway.dataflow.cfg.node.DoubleLiteralNode; import org.checkerframework.nullaway.dataflow.cfg.node.EqualToNode; import org.checkerframework.nullaway.dataflow.cfg.node.ExplicitThisNode; +import org.checkerframework.nullaway.dataflow.cfg.node.ExpressionStatementNode; import org.checkerframework.nullaway.dataflow.cfg.node.FieldAccessNode; import org.checkerframework.nullaway.dataflow.cfg.node.FloatLiteralNode; import org.checkerframework.nullaway.dataflow.cfg.node.FloatingDivisionNode; @@ -110,7 +114,6 @@ import org.checkerframework.nullaway.dataflow.cfg.node.ReturnNode; import org.checkerframework.nullaway.dataflow.cfg.node.ShortLiteralNode; import org.checkerframework.nullaway.dataflow.cfg.node.SignedRightShiftNode; -import org.checkerframework.nullaway.dataflow.cfg.node.StringConcatenateAssignmentNode; import org.checkerframework.nullaway.dataflow.cfg.node.StringConcatenateNode; import org.checkerframework.nullaway.dataflow.cfg.node.StringConversionNode; import org.checkerframework.nullaway.dataflow.cfg.node.StringLiteralNode; @@ -373,13 +376,6 @@ public TransferResult visitBitwiseXor( return noStoreChanges(NONNULL, input); } - @Override - public TransferResult visitStringConcatenateAssignment( - StringConcatenateAssignmentNode stringConcatenateAssignmentNode, - TransferInput input) { - return noStoreChanges(NULLABLE, input); - } - @Override public TransferResult visitLessThan( LessThanNode lessThanNode, TransferInput input) { @@ -761,10 +757,9 @@ private CodeAnnotationInfo getCodeAnnotationInfo(VisitorState state) { return codeAnnotationInfo; } - @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater private void setReceiverNonnull( AccessPathNullnessPropagation.ReadableUpdates updates, Node receiver, Symbol symbol) { - if (symbol != null && !symbol.isStatic()) { + if ((symbol != null) && !isStatic(symbol)) { setNonnullIfAnalyzeable(updates, receiver); } } @@ -1008,7 +1003,8 @@ Nullness returnValueNullness( nullness = input.getRegularStore().valueOfMethodCall(node, state, NULLABLE, apContext); } else if (node == null || methodReturnsNonNull.test(node) - || !Nullness.hasNullableAnnotation((Symbol) node.getTarget().getMethod(), config)) { + || (!Nullness.hasNullableAnnotation((Symbol) node.getTarget().getMethod(), config) + && !genericReturnIsNullable(node))) { // definite non-null return nullness = NONNULL; } else { @@ -1018,6 +1014,27 @@ Nullness returnValueNullness( return nullness; } + /** + * Computes the nullability of a generic return type in the context of some receiver type at an + * invocation. + * + * @param node the invocation node + * @return nullability of the return type in the context of the type of the receiver argument at + * {@code node} + */ + private boolean genericReturnIsNullable(MethodInvocationNode node) { + if (node != null && config.isJSpecifyMode()) { + MethodInvocationTree tree = node.getTree(); + if (tree != null) { + Nullness nullness = + GenericsChecks.getGenericReturnNullnessAtInvocation( + ASTHelpers.getSymbol(tree), tree, state, config); + return nullness.equals(NULLABLE); + } + } + return false; + } + @Override public TransferResult visitObjectCreation( ObjectCreationNode objectCreationNode, TransferInput input) { @@ -1061,6 +1078,13 @@ public TransferResult visitClassDeclaration( return noStoreChanges(NULLABLE, input); } + @Override + public TransferResult visitExpressionStatement( + ExpressionStatementNode expressionStatementNode, + TransferInput input) { + return noStoreChanges(NULLABLE, input); + } + @Override public TransferResult visitPackageName( PackageNameNode packageNameNode, TransferInput input) { diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java index 3d82d7763e..732ed01920 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java @@ -50,6 +50,7 @@ public class NullnessStore implements Store { private NullnessStore(Map contents) { this.contents = ImmutableMap.copyOf(contents); } + /** * Produce an empty store. * @@ -139,6 +140,7 @@ public AccessPath getMapGetIteratorContentsAccessPath(LocalVariableNode iterator } return null; } + /** * Gets the {@link Nullness} value of an access path. * diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java index bf5070866d..2f955b7b51 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java @@ -187,7 +187,7 @@ public R accept(TreeVisitor visitor, D data) { } }, booleanExpressionNode, - this.getProcessingEnvironment().getTypeUtils()), + this.env.getTypeUtils()), errorType); exNode.setTerminatesExecution(true); this.addLabelForNextNode(endPrecondition); diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java index 2e2df8faee..5ffca638ce 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/FixSerializationConfig.java @@ -49,6 +49,7 @@ public class FixSerializationConfig { * untouched. */ public final boolean suggestEnabled; + /** * If enabled, serialized information of a fix suggest will also include the enclosing method and * class of the element involved in error. Finding enclosing elements is costly and will only be diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java index db6d8c7250..3adb94bbb7 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java @@ -46,10 +46,13 @@ public class Serializer { /** Path to write errors. */ private final Path errorOutputPath; + /** Path to write suggested fix metadata. */ private final Path suggestedFixesOutputPath; + /** Path to write suggested fix metadata. */ private final Path fieldInitializationOutputPath; + /** * Adapter used to serialize outputs. This adapter is capable of serializing outputs according to * the requested serilization version and maintaining backward compatibility with previous @@ -66,6 +69,7 @@ public Serializer(FixSerializationConfig config, SerializationAdapter serializat serializeVersion(outputDirectory); initializeOutputFiles(config); } + /** * Appends the string representation of the {@link SuggestedNullableFixInfo}. * diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java index 75a9a455b9..319c636e48 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java @@ -38,8 +38,10 @@ public abstract class AbstractSymbolLocation implements SymbolLocation { /** Element kind of the targeted symbol */ protected final ElementKind type; + /** Path of the file containing the symbol, if available. */ @Nullable protected final Path path; + /** Enclosing class of the symbol. */ protected final Symbol.ClassSymbol enclosingClass; diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java index ff4f0a542e..7a1c22f9f4 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/location/MethodParameterLocation.java @@ -33,8 +33,10 @@ public class MethodParameterLocation extends AbstractSymbolLocation { /** Symbol of the targeted method. */ private final Symbol.MethodSymbol enclosingMethod; + /** Symbol of the targeted method parameter. */ private final Symbol.VarSymbol paramSymbol; + /** Index of the method parameter in the containing method's argument list. */ private final int index; diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java index e6311c2d94..eb4b21fd57 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java @@ -45,6 +45,7 @@ public class ErrorInfo { * target, and this field is the Symbol for that target. */ @Nullable private final Symbol nonnullTarget; + /** * In cases where {@link ErrorInfo#nonnullTarget} is {@code null}, we serialize this value at its * placeholder in the output tsv file. @@ -54,6 +55,7 @@ public class ErrorInfo { /** Offset of program point where this error is reported. */ private final int offset; + /** Path to the containing source file where this error is reported. */ @Nullable private final Path path; diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/FieldInitializationInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/FieldInitializationInfo.java index dac0c6d117..1a23c81d36 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/FieldInitializationInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/FieldInitializationInfo.java @@ -35,6 +35,7 @@ public class FieldInitializationInfo { /** Symbol of the initializer method. */ private final SymbolLocation initializerMethodLocation; + /** Symbol of the initialized class field. */ private final Symbol field; diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java index fb271cd644..3c5c661239 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java @@ -34,6 +34,7 @@ public class SuggestedNullableFixInfo { /** SymbolLocation of the target element in source code. */ private final SymbolLocation symbolLocation; + /** Error which will be resolved by this type change. */ private final ErrorMessage errorMessage; diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.java index 20ad5cdc62..d90cc80e88 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.java @@ -22,6 +22,7 @@ package com.uber.nullaway.handlers; +import static com.uber.nullaway.ASTHelpersBackports.getEnclosedElements; import static com.uber.nullaway.NullabilityUtil.castToNonNull; import com.google.common.base.Preconditions; @@ -48,6 +49,7 @@ public abstract class AbstractFieldContractHandler extends BaseNoOpHandler { protected static final String THIS_NOTATION = "this."; + /** Simple name of the annotation in {@code String} */ protected final String annotName; @@ -221,7 +223,7 @@ protected boolean validateAnnotationSyntax( public static @Nullable VariableElement getInstanceFieldOfClass( Symbol.ClassSymbol classSymbol, String name) { Preconditions.checkNotNull(classSymbol); - for (Element member : classSymbol.getEnclosedElements()) { + for (Element member : getEnclosedElements(classSymbol)) { if (member.getKind().isField() && !member.getModifiers().contains(Modifier.STATIC)) { if (member.getSimpleName().toString().equals(name)) { return (VariableElement) member; diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java index 2b36cdeb9a..802cedd447 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java @@ -21,7 +21,8 @@ */ package com.uber.nullaway.handlers; -import com.google.common.base.Preconditions; +import static com.uber.nullaway.ASTHelpersBackports.getEnclosedElements; + import com.google.errorprone.VisitorState; import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.suppliers.Suppliers; @@ -31,6 +32,7 @@ import com.sun.tools.javac.code.Types; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; +import com.uber.nullaway.annotations.Initializer; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import java.util.Objects; @@ -52,8 +54,13 @@ public class ApacheThriftIsSetHandler extends BaseNoOpHandler { private static final Supplier TBASE_TYPE_SUPPLIER = Suppliers.typeFromString(TBASE_NAME); - @Nullable private Optional tbaseType; + private Optional tbaseType; + /** + * This method is annotated {@code @Initializer} since it will be invoked when the first class is + * processed, before any other handler methods + */ + @Initializer @Override public void onMatchTopLevelClass( NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) { @@ -142,7 +149,7 @@ private FieldAndGetterElements getFieldAndGetterForProperty( Element getter = null; String fieldName = decapitalize(capPropName); String getterName = "get" + capPropName; - for (Symbol elem : symbol.owner.getEnclosedElements()) { + for (Symbol elem : getEnclosedElements(symbol.owner)) { if (elem.getKind().isField() && elem.getSimpleName().toString().equals(fieldName)) { if (field != null) { throw new RuntimeException("already found field " + fieldName); @@ -171,7 +178,6 @@ private static String decapitalize(String str) { } private boolean thriftIsSetCall(Symbol.MethodSymbol symbol, Types types) { - Preconditions.checkNotNull(tbaseType); // noinspection ConstantConditions return tbaseType.isPresent() && symbol.getSimpleName().toString().startsWith("isSet") diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java index 751f64ffa4..242a96a1dc 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java @@ -109,7 +109,7 @@ public void onMatchReturn(NullAway analysis, ReturnTree tree, VisitorState state } @Override - public Nullness onOverrideMethodInvocationReturnNullability( + public Nullness onOverrideMethodReturnNullability( Symbol.MethodSymbol methodSymbol, VisitorState state, boolean isAnnotated, diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java index d426b12c1c..31617da68b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -123,15 +123,14 @@ public void onMatchReturn(NullAway analysis, ReturnTree tree, VisitorState state } @Override - public Nullness onOverrideMethodInvocationReturnNullability( + public Nullness onOverrideMethodReturnNullability( Symbol.MethodSymbol methodSymbol, VisitorState state, boolean isAnnotated, Nullness returnNullness) { for (Handler h : handlers) { returnNullness = - h.onOverrideMethodInvocationReturnNullability( - methodSymbol, state, isAnnotated, returnNullness); + h.onOverrideMethodReturnNullability(methodSymbol, state, isAnnotated, returnNullness); } return returnNullness; } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java index 0e27408718..82b436fc46 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java @@ -21,9 +21,9 @@ */ package com.uber.nullaway.handlers; +import static com.uber.nullaway.ASTHelpersBackports.getEnclosedElements; import static com.uber.nullaway.NullabilityUtil.castToNonNull; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.errorprone.VisitorState; import com.google.errorprone.suppliers.Supplier; @@ -36,6 +36,7 @@ import com.sun.tools.javac.code.Types; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; +import com.uber.nullaway.annotations.Initializer; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import java.util.ArrayList; @@ -60,9 +61,14 @@ public class GrpcHandler extends BaseNoOpHandler { private static final Supplier GRPC_METADATA_KEY_TYPE_SUPPLIER = Suppliers.typeFromString(GRPC_METADATA_KEY_TNAME); - @Nullable private Optional grpcMetadataType; - @Nullable private Optional grpcKeyType; + private Optional grpcMetadataType; + private Optional grpcKeyType; + /** + * This method is annotated {@code @Initializer} since it will be invoked when the first class is + * processed, before any other handler methods + */ + @Initializer @Override public void onMatchTopLevelClass( NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) { @@ -122,7 +128,7 @@ public ImmutableSet onRegisterImmutableTypes() { private Symbol.MethodSymbol getGetterForMetadataSubtype( Symbol.ClassSymbol classSymbol, Types types) { // Is there a better way than iteration? - for (Symbol elem : classSymbol.getEnclosedElements()) { + for (Symbol elem : getEnclosedElements(classSymbol)) { if (elem.getKind().equals(ElementKind.METHOD)) { Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) elem; if (grpcIsMetadataGetCall(methodSymbol, types)) { @@ -134,8 +140,6 @@ private Symbol.MethodSymbol getGetterForMetadataSubtype( } private boolean grpcIsMetadataContainsKeyCall(Symbol.MethodSymbol symbol, Types types) { - Preconditions.checkNotNull(grpcMetadataType); - Preconditions.checkNotNull(grpcKeyType); // noinspection ConstantConditions return grpcMetadataType.isPresent() && grpcKeyType.isPresent() @@ -148,8 +152,6 @@ private boolean grpcIsMetadataContainsKeyCall(Symbol.MethodSymbol symbol, Types } private boolean grpcIsMetadataGetCall(Symbol.MethodSymbol symbol, Types types) { - Preconditions.checkNotNull(grpcMetadataType); - Preconditions.checkNotNull(grpcKeyType); // noinspection ConstantConditions return grpcMetadataType.isPresent() && grpcKeyType.isPresent() diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java index 7b98fde283..835c01fbfd 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -162,12 +162,12 @@ boolean onOverrideMayBeNullExpr( * @param methodSymbol The method symbol for the method in question. * @param state The current visitor state. * @param isAnnotated A boolean flag indicating whether the called method is considered to be - * within isAnnotated or unannotated code, used to avoid querying for this information - * multiple times within the same handler chain. + * within annotated or unannotated code, used to avoid querying for this information multiple + * times within the same handler chain. * @param returnNullness return nullness computed by upstream handlers or NullAway core. * @return Updated return nullability computed by this handler. */ - Nullness onOverrideMethodInvocationReturnNullability( + Nullness onOverrideMethodReturnNullability( Symbol.MethodSymbol methodSymbol, VisitorState state, boolean isAnnotated, diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java index ba46360a87..22792ddd5f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java @@ -28,6 +28,7 @@ import com.uber.nullaway.handlers.contract.ContractHandler; import com.uber.nullaway.handlers.contract.fieldcontract.EnsuresNonNullHandler; import com.uber.nullaway.handlers.contract.fieldcontract.RequiresNonNullHandler; +import com.uber.nullaway.handlers.temporary.FluentFutureHandler; /** Utility static methods for the handlers package. */ public class Handlers { @@ -56,9 +57,13 @@ public static Handler buildDefault(Config config) { handlerListBuilder.add(new AssertionHandler(methodNameUtil)); } handlerListBuilder.add(new GuavaAssertionsHandler()); - handlerListBuilder.add(new LibraryModelsHandler(config)); + LibraryModelsHandler libraryModelsHandler = new LibraryModelsHandler(config); + handlerListBuilder.add(libraryModelsHandler); handlerListBuilder.add(StreamNullabilityPropagatorFactory.getRxStreamNullabilityPropagator()); handlerListBuilder.add(StreamNullabilityPropagatorFactory.getJavaStreamNullabilityPropagator()); + handlerListBuilder.add( + StreamNullabilityPropagatorFactory.fromSpecs( + libraryModelsHandler.getStreamNullabilitySpecs())); handlerListBuilder.add(new ContractHandler(config)); handlerListBuilder.add(new ApacheThriftIsSetHandler()); handlerListBuilder.add(new GrpcHandler()); @@ -74,6 +79,8 @@ public static Handler buildDefault(Config config) { if (config.checkContracts()) { handlerListBuilder.add(new ContractCheckHandler(config)); } + handlerListBuilder.add(new LombokHandler(config)); + handlerListBuilder.add(new FluentFutureHandler()); return new CompositeHandler(handlerListBuilder.build()); } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java index 954651bccc..e000c4fdcf 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -27,6 +27,7 @@ import static com.uber.nullaway.Nullness.NULLABLE; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; import com.google.errorprone.VisitorState; @@ -46,6 +47,7 @@ import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; +import com.uber.nullaway.handlers.stream.StreamTypeRecord; import java.util.ArrayList; import java.util.HashSet; import java.util.LinkedHashMap; @@ -109,14 +111,16 @@ public Nullness[] onOverrideMethodInvocationParametersNullability( } @Override - public Nullness onOverrideMethodInvocationReturnNullability( + public Nullness onOverrideMethodReturnNullability( Symbol.MethodSymbol methodSymbol, VisitorState state, boolean isAnnotated, Nullness returnNullness) { OptimizedLibraryModels optLibraryModels = getOptLibraryModels(state.context); if (optLibraryModels.hasNonNullReturn(methodSymbol, state.getTypes(), !isAnnotated)) { - return Nullness.NONNULL; + return NONNULL; + } else if (optLibraryModels.hasNullableReturn(methodSymbol, state.getTypes(), !isAnnotated)) { + return NULLABLE; } return returnNullness; } @@ -289,6 +293,25 @@ private void setUnconditionalArgumentNullness( } } + /** + * Get all the stream specifications loaded from any of our library models. + * + *

This is used in Handlers.java to create a StreamNullabilityPropagator handler, which gets + * registered independently of this LibraryModelsHandler itself. + * + *

LibraryModelsHandler is responsible from reading the library models for stream specs, but + * beyond that, checking of the specs falls under the responsibility of the generated + * StreamNullabilityPropagator handler. + * + * @return The list of all stream specifications loaded from any of our library models. + */ + public ImmutableList getStreamNullabilitySpecs() { + // Note: Currently, OptimizedLibraryModels doesn't carry the information about stream type + // records, it is not clear what it means to "optimize" lookup for those and they get accessed + // only once by calling this method during handler setup in Handlers.java. + return libraryModels.customStreamNullabilitySpecs(); + } + private static LibraryModels loadLibraryModels(Config config) { Iterable externalLibraryModels = ServiceLoader.load(LibraryModels.class, LibraryModels.class.getClassLoader()); @@ -450,6 +473,54 @@ private static class DefaultLibraryModels implements LibraryModels { "org.junit.jupiter.api.Assertions", "assertNotNull(java.lang.Object,java.util.function.Supplier)"), 0) + .put(methodRef("org.apache.commons.lang3.Validate", "notNull(T)"), 0) + .put( + methodRef( + "org.apache.commons.lang3.Validate", + "notNull(T,java.lang.String,java.lang.Object...)"), + 0) + .put( + methodRef( + "org.apache.commons.lang3.Validate", + "notEmpty(T[],java.lang.String,java.lang.Object...)"), + 0) + .put(methodRef("org.apache.commons.lang3.Validate", "notEmpty(T[])"), 0) + .put( + methodRef( + "org.apache.commons.lang3.Validate", + "notEmpty(T,java.lang.String,java.lang.Object...)"), + 0) + .put(methodRef("org.apache.commons.lang3.Validate", "notEmpty(T)"), 0) + .put( + methodRef( + "org.apache.commons.lang3.Validate", + "notBlank(T,java.lang.String,java.lang.Object...)"), + 0) + .put(methodRef("org.apache.commons.lang3.Validate", "notBlank(T)"), 0) + .put( + methodRef( + "org.apache.commons.lang3.Validate", + "noNullElements(T[],java.lang.String,java.lang.Object...)"), + 0) + .put(methodRef("org.apache.commons.lang3.Validate", "noNullElements(T[])"), 0) + .put( + methodRef( + "org.apache.commons.lang3.Validate", + "noNullElements(T,java.lang.String,java.lang.Object...)"), + 0) + .put(methodRef("org.apache.commons.lang3.Validate", "noNullElements(T)"), 0) + .put( + methodRef( + "org.apache.commons.lang3.Validate", + "validIndex(T[],int,java.lang.String,java.lang.Object...)"), + 0) + .put(methodRef("org.apache.commons.lang3.Validate", "validIndex(T[],int)"), 0) + .put( + methodRef( + "org.apache.commons.lang3.Validate", + "validIndex(T,int,java.lang.String,java.lang.Object...)"), + 0) + .put(methodRef("org.apache.commons.lang3.Validate", "validIndex(T,int)"), 0) .build(); private static final ImmutableSetMultimap EXPLICITLY_NULLABLE_PARAMETERS = @@ -777,6 +848,8 @@ private static class CombinedLibraryModels implements LibraryModels { private final ImmutableSetMultimap castToNonNullMethods; + private final ImmutableList customStreamNullabilitySpecs; + public CombinedLibraryModels(Iterable models, Config config) { this.config = config; ImmutableSetMultimap.Builder failIfNullParametersBuilder = @@ -795,6 +868,8 @@ public CombinedLibraryModels(Iterable models, Config config) { ImmutableSet.Builder nonNullReturnsBuilder = new ImmutableSet.Builder<>(); ImmutableSetMultimap.Builder castToNonNullMethodsBuilder = new ImmutableSetMultimap.Builder<>(); + ImmutableList.Builder customStreamNullabilitySpecsBuilder = + new ImmutableList.Builder<>(); for (LibraryModels libraryModels : models) { for (Map.Entry entry : libraryModels.failIfNullParameters().entries()) { if (shouldSkipModel(entry.getKey())) { @@ -854,6 +929,9 @@ public CombinedLibraryModels(Iterable models, Config config) { } castToNonNullMethodsBuilder.put(entry); } + for (StreamTypeRecord streamTypeRecord : libraryModels.customStreamNullabilitySpecs()) { + customStreamNullabilitySpecsBuilder.add(streamTypeRecord); + } } failIfNullParameters = failIfNullParametersBuilder.build(); explicitlyNullableParameters = explicitlyNullableParametersBuilder.build(); @@ -864,6 +942,7 @@ public CombinedLibraryModels(Iterable models, Config config) { nullableReturns = nullableReturnsBuilder.build(); nonNullReturns = nonNullReturnsBuilder.build(); castToNonNullMethods = castToNonNullMethodsBuilder.build(); + customStreamNullabilitySpecs = customStreamNullabilitySpecsBuilder.build(); } private boolean shouldSkipModel(MethodRef key) { @@ -914,6 +993,11 @@ public ImmutableSet nonNullReturns() { public ImmutableSetMultimap castToNonNullMethods() { return castToNonNullMethods; } + + @Override + public ImmutableList customStreamNullabilitySpecs() { + return customStreamNullabilitySpecs; + } } /** diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java new file mode 100644 index 0000000000..7069497800 --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java @@ -0,0 +1,89 @@ +package com.uber.nullaway.handlers; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol; +import com.uber.nullaway.Config; +import com.uber.nullaway.NullAway; +import com.uber.nullaway.Nullness; +import java.util.stream.StreamSupport; +import javax.annotation.Nullable; +import javax.lang.model.element.ElementKind; + +/** + * A general handler for Lombok generated code and its internal semantics. + * + *

Currently used to propagate @Nullable in cases where the Lombok annotation processor fails to + * do so consistently. + */ +public class LombokHandler extends BaseNoOpHandler { + + private static String LOMBOK_GENERATED_ANNOTATION_NAME = "lombok.Generated"; + private static String LOMBOK_BUILDER_DEFAULT_METHOD_PREFIX = "$default$"; + + private final Config config; + + public LombokHandler(Config config) { + this.config = config; + } + + private boolean isLombokMethodWithMissingNullableAnnotation( + Symbol.MethodSymbol methodSymbol, VisitorState state) { + if (!ASTHelpers.hasAnnotation(methodSymbol, LOMBOK_GENERATED_ANNOTATION_NAME, state)) { + return false; + } + String methodNameString = methodSymbol.name.toString(); + if (!methodNameString.startsWith(LOMBOK_BUILDER_DEFAULT_METHOD_PREFIX)) { + return false; + } + String originalFieldName = + methodNameString.substring(LOMBOK_BUILDER_DEFAULT_METHOD_PREFIX.length()); + ImmutableList matchingMembers = + StreamSupport.stream(methodSymbol.enclClass().members().getSymbols().spliterator(), false) + .filter( + sym -> + sym.name.contentEquals(originalFieldName) + && sym.getKind().equals(ElementKind.FIELD)) + .collect(ImmutableList.toImmutableList()); + Preconditions.checkArgument( + matchingMembers.size() == 1, + String.format( + "Found %d fields matching Lombok generated builder default method %s", + matchingMembers.size(), methodNameString)); + return Nullness.hasNullableAnnotation(matchingMembers.get(0), config); + } + + @Override + public boolean onOverrideMayBeNullExpr( + NullAway analysis, + ExpressionTree expr, + @Nullable Symbol exprSymbol, + VisitorState state, + boolean exprMayBeNull) { + if (exprMayBeNull) { + return true; + } + Tree.Kind exprKind = expr.getKind(); + if (exprSymbol != null && exprKind == Tree.Kind.METHOD_INVOCATION) { + Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) exprSymbol; + return isLombokMethodWithMissingNullableAnnotation(methodSymbol, state); + } + return false; + } + + @Override + public Nullness onOverrideMethodReturnNullability( + Symbol.MethodSymbol methodSymbol, + VisitorState state, + boolean isAnnotated, + Nullness returnNullness) { + if (isLombokMethodWithMissingNullableAnnotation(methodSymbol, state)) { + return Nullness.NULLABLE; + } + return returnNullness; + } +} diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java index 8e6ac8ee64..561ac6ebae 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java @@ -133,7 +133,7 @@ public Nullness[] onOverrideMethodInvocationParametersNullability( } @Override - public Nullness onOverrideMethodInvocationReturnNullability( + public Nullness onOverrideMethodReturnNullability( Symbol.MethodSymbol methodSymbol, VisitorState state, boolean isAnnotated, diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagatorFactory.java b/nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagatorFactory.java index 2fb0e5a9c2..a3695c67ee 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagatorFactory.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagatorFactory.java @@ -1,4 +1,5 @@ package com.uber.nullaway.handlers; + /* * Copyright (c) 2017 Uber Technologies, Inc. * @@ -28,6 +29,8 @@ import com.uber.nullaway.handlers.stream.StreamTypeRecord; public class StreamNullabilityPropagatorFactory { + + /** Returns a handler for the standard Java 8 stream APIs. */ public static StreamNullabilityPropagator getJavaStreamNullabilityPropagator() { ImmutableList streamModels = StreamModelBuilder.start() @@ -77,6 +80,7 @@ public static StreamNullabilityPropagator getJavaStreamNullabilityPropagator() { return new StreamNullabilityPropagator(streamModels); } + /** Returns a handler for io.reactivex.* stream APIs */ public static StreamNullabilityPropagator getRxStreamNullabilityPropagator() { ImmutableList rxModels = StreamModelBuilder.start() @@ -136,4 +140,19 @@ public static StreamNullabilityPropagator getRxStreamNullabilityPropagator() { return new StreamNullabilityPropagator(rxModels); } + + /** + * Create a new StreamNullabilityPropagator from a list of StreamTypeRecord specs. + * + *

This is used to create a new StreamNullabilityPropagator based on stream API specs provided + * by library models. + * + * @param streamNullabilitySpecs the list of StreamTypeRecord objects defining one or more stream + * APIs (from {@link StreamModelBuilder}). + * @return A handler corresponding to the stream APIs defined by the given specs. + */ + public static StreamNullabilityPropagator fromSpecs( + ImmutableList streamNullabilitySpecs) { + return new StreamNullabilityPropagator(streamNullabilitySpecs); + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/stream/MaplikeMethodRecord.java b/nullaway/src/main/java/com/uber/nullaway/handlers/stream/MaplikeMethodRecord.java index a2abfd0a1b..cb2a34e7e0 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/stream/MaplikeMethodRecord.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/stream/MaplikeMethodRecord.java @@ -1,4 +1,5 @@ package com.uber.nullaway.handlers.stream; + /* * Copyright (c) 2017 Uber Technologies, Inc. * diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/stream/MaplikeToFilterInstanceRecord.java b/nullaway/src/main/java/com/uber/nullaway/handlers/stream/MaplikeToFilterInstanceRecord.java index 908931cc56..74c932e33c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/stream/MaplikeToFilterInstanceRecord.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/stream/MaplikeToFilterInstanceRecord.java @@ -1,4 +1,5 @@ package com.uber.nullaway.handlers.stream; + /* * Copyright (c) 2017 Uber Technologies, Inc. * diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/stream/StreamModelBuilder.java b/nullaway/src/main/java/com/uber/nullaway/handlers/stream/StreamModelBuilder.java index 2423966229..94e0b1a6e8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/stream/StreamModelBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/stream/StreamModelBuilder.java @@ -1,4 +1,5 @@ package com.uber.nullaway.handlers.stream; + /* * Copyright (c) 2017 Uber Technologies, Inc. * @@ -24,6 +25,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.errorprone.predicates.TypePredicate; +import com.google.errorprone.predicates.type.DescendantOf; +import com.google.errorprone.suppliers.Suppliers; import java.util.ArrayList; import java.util.List; import javax.annotation.Nullable; @@ -89,6 +92,16 @@ public StreamModelBuilder addStreamType(TypePredicate tp) { return this; } + /** + * Add a stream type to our models based on the type's fully qualified name. + * + * @param fullyQualifiedName the FQN of the class/interface in our stream-based API. + * @return This builder (for chaining). + */ + public StreamModelBuilder addStreamTypeFromName(String fullyQualifiedName) { + return this.addStreamType(new DescendantOf(Suppliers.typeFromString(fullyQualifiedName))); + } + private void initializeBuilders() { this.filterMethodSigs = ImmutableSet.builder(); this.filterMethodSimpleNames = ImmutableSet.builder(); diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/stream/StreamTypeRecord.java b/nullaway/src/main/java/com/uber/nullaway/handlers/stream/StreamTypeRecord.java index 4167ec9c8b..fc1caf696e 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/stream/StreamTypeRecord.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/stream/StreamTypeRecord.java @@ -1,4 +1,5 @@ package com.uber.nullaway.handlers.stream; + /* * Copyright (c) 2017 Uber Technologies, Inc. * diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/temporary/FluentFutureHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/temporary/FluentFutureHandler.java new file mode 100644 index 0000000000..04ccd6b419 --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/temporary/FluentFutureHandler.java @@ -0,0 +1,91 @@ +package com.uber.nullaway.handlers.temporary; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol; +import com.uber.nullaway.NullabilityUtil; +import com.uber.nullaway.Nullness; +import com.uber.nullaway.handlers.BaseNoOpHandler; +import java.util.Arrays; +import javax.lang.model.element.Name; + +/** + * This handler provides a temporary workaround due to our lack of support for generics, which + * allows natural usage of Futures/FluentFuture Guava APIs. It can potentially introduce false + * negatives, however, and should be deprecated as soon as full generic support is available. + * + *

This works by special casing the return nullability of {@link com.google.common.base.Function} + * and {@link com.google.common.util.concurrent.AsyncFunction} to be e.g. {@code Function<@Nullable + * T>} whenever these functional interfaces are implemented as a lambda expression passed to a list + * of specific methods of {@link com.google.common.util.concurrent.FluentFuture} or {@link + * com.google.common.util.concurrent.Futures}. We cannot currently check that {@code T} for {@code + * FluentFuture} is a {@code @Nullable} type, so this is unsound. However, we have found many + * cases in practice where these lambdas include {@code null} returns, which were already being + * ignored (due to a bug) before PR #765. This handler offers the best possible support for these + * cases, at least until our generics support is mature enough to handle them. + * + *

Note: Package {@code com.uber.nullaway.handlers.temporary} is meant for this sort of temporary + * workaround handler, to be removed as future NullAway features make them unnecessary. This is a + * hack, but the best of a bunch of bad options. + */ +public class FluentFutureHandler extends BaseNoOpHandler { + + private static final String GUAVA_FUNCTION_CLASS_NAME = "com.google.common.base.Function"; + private static final String GUAVA_ASYNC_FUNCTION_CLASS_NAME = + "com.google.common.util.concurrent.AsyncFunction"; + private static final String FLUENT_FUTURE_CLASS_NAME = + "com.google.common.util.concurrent.FluentFuture"; + private static final String FUTURES_CLASS_NAME = "com.google.common.util.concurrent.Futures"; + private static final String FUNCTION_APPLY_METHOD_NAME = "apply"; + private static final String[] FLUENT_FUTURE_INCLUDE_LIST_METHODS = { + "catching", "catchingAsync", "transform", "transformAsync" + }; + + private static boolean isGuavaFunctionDotApply(Symbol.MethodSymbol methodSymbol) { + Name className = methodSymbol.enclClass().flatName(); + return (className.contentEquals(GUAVA_FUNCTION_CLASS_NAME) + || className.contentEquals(GUAVA_ASYNC_FUNCTION_CLASS_NAME)) + && methodSymbol.name.contentEquals(FUNCTION_APPLY_METHOD_NAME); + } + + private static boolean isFluentFutureIncludeListMethod(Symbol.MethodSymbol methodSymbol) { + Name className = methodSymbol.enclClass().flatName(); + return (className.contentEquals(FLUENT_FUTURE_CLASS_NAME) + || className.contentEquals(FUTURES_CLASS_NAME)) + && Arrays.stream(FLUENT_FUTURE_INCLUDE_LIST_METHODS) + .anyMatch(s -> methodSymbol.name.contentEquals(s)); + } + + @Override + public Nullness onOverrideMethodReturnNullability( + Symbol.MethodSymbol methodSymbol, + VisitorState state, + boolean isAnnotated, + Nullness returnNullness) { + // We only care about lambda's implementing Guava's Function + if (!isGuavaFunctionDotApply(methodSymbol)) { + return returnNullness; + } + // Check if we are inside a lambda passed as an argument to a method call: + LambdaExpressionTree enclosingLambda = + ASTHelpers.findEnclosingNode(state.getPath(), LambdaExpressionTree.class); + if (enclosingLambda == null + || !NullabilityUtil.getFunctionalInterfaceMethod(enclosingLambda, state.getTypes()) + .equals(methodSymbol)) { + return returnNullness; + } + MethodInvocationTree methodInvocation = + ASTHelpers.findEnclosingNode(state.getPath(), MethodInvocationTree.class); + if (methodInvocation == null || !methodInvocation.getArguments().contains(enclosingLambda)) { + return returnNullness; + } + // Check if that method call is one of the FluentFuture APIs we care about + Symbol.MethodSymbol lambdaConsumerMethodSymbol = ASTHelpers.getSymbol(methodInvocation); + if (!isFluentFutureIncludeListMethod(lambdaConsumerMethodSymbol)) { + return returnNullness; + } + return Nullness.NULLABLE; + } +} diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java index 5aec8370a2..03ae7e35a3 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java @@ -22,8 +22,12 @@ package com.uber.nullaway; +import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; + import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.ErrorProneFlags; +import com.sun.source.tree.Tree; +import com.uber.nullaway.testlibrarymodels.TestLibraryModels; import java.io.IOException; import org.junit.Before; import org.junit.Rule; @@ -57,6 +61,8 @@ private BugCheckerRefactoringTestHelper makeTestHelper() { .setArgs( "-d", temporaryFolder.getRoot().getAbsolutePath(), + "-processorpath", + TestLibraryModels.class.getProtectionDomain().getCodeSource().getLocation().getPath(), // the remaining args are not needed right now, but they will be necessary when we // switch to the more modern newInstance() API "-XepOpt:NullAway:AnnotatedPackages=com.uber,com.ubercab,io.reactivex", @@ -89,7 +95,8 @@ public void suggestCastToNonNull() throws IOException { "package com.uber;", "import javax.annotation.Nullable;", "class Test {", - " Object test1(@Nullable Object o) {", + " @Nullable Object o;", + " Object test1() {", " return o;", " }", "}") @@ -99,13 +106,65 @@ public void suggestCastToNonNull() throws IOException { "import static com.uber.nullaway.testdata.Util.castToNonNull;", "import javax.annotation.Nullable;", "class Test {", - " Object test1(@Nullable Object o) {", + " @Nullable Object o;", + " Object test1() {", " return castToNonNull(o);", " }", "}") .doTest(); } + /** + * Test for cases where we heuristically decide not to wrap an expression in castToNonNull; see + * {@link ErrorBuilder#canBeCastToNonNull(Tree)} + */ + @Test + public void suppressInsteadOfCastToNonNull() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object f = new Object();", + " Object test1(@Nullable Object o) {", + " return o;", + " }", + " Object test2() {", + " return null;", + " }", + " void test3() {", + " f = null;", + " }", + " @Nullable Object m() { return null; }", + " Object shouldAddCast() {", + " return m();", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "import javax.annotation.Nullable;", + "class Test {", + " Object f = new Object();", + " @SuppressWarnings(\"NullAway\") Object test1(@Nullable Object o) {", + " return o;", + " }", + " @SuppressWarnings(\"NullAway\") Object test2() {", + " return null;", + " }", + " @SuppressWarnings(\"NullAway\") void test3() {", + " f = null;", + " }", + " @Nullable Object m() { return null; }", + " Object shouldAddCast() {", + " return castToNonNull(m());", + " }", + "}") + .doTest(); + } + @Test public void removeUnnecessaryCastToNonNull() throws IOException { makeTestHelper() @@ -132,6 +191,64 @@ public void removeUnnecessaryCastToNonNull() throws IOException { .doTest(); } + @Test + public void removeUnnecessaryCastToNonNullFromLibraryModel() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "class Test {", + " Object test1(Object o) {", + " return castToNonNull(\"CAST_REASON\",o,42);", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "class Test {", + " Object test1(Object o) {", + " return o;", + " }", + "}") + .doTest(); + } + + @Test + public void removeUnnecessaryCastToNonNullMultiLine() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "class Test {", + " static class Foo { Object getObj() { return new Object(); } }", + " Object test1(Foo f) {", + " return castToNonNull(f", + " // comment that should not be deleted", + " .getObj());", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "class Test {", + " static class Foo { Object getObj() { return new Object(); } }", + " Object test1(Foo f) {", + " return f", + " // comment that should not be deleted", + " .getObj();", + " }", + "}") + .doTest(TEXT_MATCH); + } + @Test public void suggestSuppressionOnMethodRef() throws IOException { makeTestHelper() @@ -173,4 +290,113 @@ public void suggestSuppressionOnMethodRef() throws IOException { "}") .doTest(); } + + @Test + public void suggestCastToNonNullPreserveComments() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object x = new Object();", + " static class Foo { @Nullable Object getObj() { return null; } }", + " Object test1(Foo f) {", + " return f", + " // comment that should not be deleted", + " .getObj();", + " }", + " void test2(Foo f) {", + " x = f.getObj(); // comment that should not be deleted", + " }", + " Object test3(Foo f) {", + " return f./* keep this comment */getObj();", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "", + "import javax.annotation.Nullable;", + "class Test {", + " Object x = new Object();", + " static class Foo { @Nullable Object getObj() { return null; } }", + " Object test1(Foo f) {", + " return castToNonNull(f", + " // comment that should not be deleted", + " .getObj());", + " }", + " void test2(Foo f) {", + " x = castToNonNull(f.getObj()); // comment that should not be deleted", + " }", + " Object test3(Foo f) {", + " return castToNonNull(f./* keep this comment */getObj());", + " }", + "}") + .doTest(TEXT_MATCH); + } + + public void suggestInitSuppressionOnConstructor() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object f;", + " Object g;", + " Test() {}", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object f;", + " Object g;", + " @SuppressWarnings(\"NullAway.Init\") Test() {}", + "}") + .doTest(); + } + + @Test + public void suggestInitSuppressionOnField() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object f;", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @SuppressWarnings(\"NullAway.Init\") Object f;", + "}") + .doTest(); + } + + @Test + public void updateExtantSuppressWarnings() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @SuppressWarnings(\"unused\") Object f;", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @SuppressWarnings({\"unused\",\"NullAway.Init\"}) Object f;", + "}") + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java index 0685a6566e..937752c0bc 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java @@ -53,20 +53,22 @@ public void allowLibraryModelsOverrideAnnotations() { temporaryFolder.getRoot().getAbsolutePath(), "-XepOpt:NullAway:AnnotatedPackages=com.uber")) .addSourceLines( - "Foo.java", + "AnnotatedWithModels.java", "package com.uber;", - "public class Foo {", + "public class AnnotatedWithModels {", " Object field = new Object();", - " Object bar() {", - " return new Object();", + " // implicitly @Nullable due to library model", + " Object returnsNullFromModel() {", + " // null is valid here only because of the library model", + " return null;", " }", " Object nullableReturn() {", " // BUG: Diagnostic contains: returning @Nullable", - " return bar();", + " return returnsNullFromModel();", " }", " void run() {", " // just to make sure, flow analysis is also impacted by library models information", - " Object temp = bar();", + " Object temp = returnsNullFromModel();", " // BUG: Diagnostic contains: assigning @Nullable", " this.field = temp;", " }", diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayFrameworkTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayFrameworkTests.java index 93fb62cdeb..ed62eccee9 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayFrameworkTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayFrameworkTests.java @@ -409,6 +409,32 @@ public void testLombokBuilderWithoutGeneratedAsUnannotated() { .doTest(); } + /** + * This test is solely to check if we can run through some of the {@link + * com.uber.nullaway.handlers.LombokHandler} logic without crashing. It does not check that the + * logic is correct. + */ + @Test + public void lombokHandlerRunsWithoutCrashing() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " @Nullable Object test;", + " @lombok.Generated", + " Object $default$test() {", + " return new Object();", + " }", + "}") + .doTest(); + } + @Test public void systemConsoleNullable() { defaultCompilationHelper @@ -491,4 +517,386 @@ public void defaultLibraryModelsClassCast() { "}") .doTest(); } + + @Test + public void apacheValidateNotNull() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String s) {", + " Validate.notNull(s);", + " int l = s.length();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateNotNullWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String s) {", + " Validate.notNull(s, \"Message\");", + " int l = s.length();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateArrayNotEmptyWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String[] s) {", + " Validate.notEmpty(s, \"Message\");", + " int l = s.length;", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateArrayNotEmpty() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String[] s) {", + " Validate.notEmpty(s);", + " int l = s.length;", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateListNotEmptyWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "import java.util.List;", + "public class Foo {", + " public void bar(@Nullable List s) {", + " Validate.notEmpty(s, \"Message\");", + " int l = s.size();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateListNotEmpty() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "import java.util.List;", + "public class Foo {", + " public void bar(@Nullable List s) {", + " Validate.notEmpty(s);", + " int l = s.size();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateMapNotEmptyWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "import java.util.Map;", + "public class Foo {", + " public void bar(@Nullable Map s) {", + " Validate.notEmpty(s, \"Message\");", + " int l = s.size();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateMapNotEmpty() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "import java.util.Map;", + "public class Foo {", + " public void bar(@Nullable Map s) {", + " Validate.notEmpty(s);", + " int l = s.size();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateStringNotEmptyWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String s) {", + " Validate.notEmpty(s, \"Message\");", + " int l = s.length();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateStringNotEmpty() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String s) {", + " Validate.notEmpty(s);", + " int l = s.length();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateStringNotBlankWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String s) {", + " Validate.notBlank(s, \"Message\");", + " int l = s.length();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateStringNotBlank() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String s) {", + " Validate.notBlank(s);", + " int l = s.length();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateArrayNoNullElementsWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String[] s) {", + " Validate.noNullElements(s, \"Message\");", + " int l = s.length;", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateArrayNoNullElements() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String[] s) {", + " Validate.noNullElements(s);", + " int l = s.length;", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateIterableNoNullElementsWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "import java.util.Iterator;", + "public class Foo {", + " public void bar(@Nullable Iterable s) {", + " Validate.noNullElements(s, \"Message\");", + " Iterator l = s.iterator();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateIterableNoNullElements() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "import java.util.Iterator;", + "public class Foo {", + " public void bar(@Nullable Iterable s) {", + " Validate.noNullElements(s);", + " Iterator l = s.iterator();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateArrayValidIndexWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String[] s) {", + " Validate.validIndex(s, 0, \"Message\");", + " int l = s.length;", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateArrayValidIndex() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String[] s) {", + " Validate.validIndex(s, 0);", + " int l = s.length;", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateCollectionValidIndexWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "import java.util.List;", + "public class Foo {", + " public void bar(@Nullable List s) {", + " Validate.validIndex(s, 0, \"Message\");", + " int l = s.size();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateCollectionValidIndex() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "import java.util.List;", + "public class Foo {", + " public void bar(@Nullable List s) {", + " Validate.validIndex(s, 0);", + " int l = s.size();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateStringValidIndexWithMessage() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String s) {", + " Validate.validIndex(s, 0, \"Message\");", + " int l = s.length();", + " }", + "}") + .doTest(); + } + + @Test + public void apacheValidateStringValidIndex() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import org.apache.commons.lang3.Validate;", + "import org.jetbrains.annotations.Nullable;", + "public class Foo {", + " public void bar(@Nullable String s) {", + " Validate.validIndex(s, 0);", + " int l = s.length();", + " }", + "}") + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayFunctionalInterfaceNullabilityTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayFunctionalInterfaceNullabilityTests.java new file mode 100644 index 0000000000..6406b2b733 --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayFunctionalInterfaceNullabilityTests.java @@ -0,0 +1,110 @@ +package com.uber.nullaway; + +import org.junit.Test; + +public class NullAwayFunctionalInterfaceNullabilityTests extends NullAwayTestsBase { + + @Test + public void multipleTypeParametersInstantiation() { + defaultCompilationHelper + .addSourceLines( + "NullableFunction.java", + "package com.uber.unannotated;", // As if a third-party lib, since override is invalid + "import javax.annotation.Nullable;", + "import java.util.function.Function;", + "@FunctionalInterface", + "public interface NullableFunction extends Function {", + " @Override", + " @Nullable", + " T apply(@Nullable F input);", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import java.util.function.Function;", + "import com.uber.unannotated.NullableFunction;", + "class Test {", + " private static void takesNullableFunction(NullableFunction nf) { }", + " private static void takesNonNullableFunction(Function f) { }", + " private static void passesNullableFunction() {", + " takesNullableFunction(s -> { return null; });", + " }", + " private static void passesNullableFunctionToNonNull() {", + " takesNonNullableFunction(s -> { return null; });", + " }", + "}") + .addSourceLines( + "TestGuava.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import com.google.common.base.Function;", + "import com.uber.unannotated.NullableFunction;", + "class TestGuava {", + " private static void takesNullableFunction(NullableFunction nf) { }", + " private static void takesNonNullableFunction(Function f) { }", + " private static void passesNullableFunction() {", + " takesNullableFunction(s -> { return null; });", + " }", + " private static void passesNullableFunctionToNonNull() {", + " // BUG: Diagnostic contains: returning @Nullable expression", + " takesNonNullableFunction(s -> { return null; });", + " }", + "}") + .doTest(); + } + + @Test + public void futuresFunctionLambdas() { + // See FluentFutureHandler + defaultCompilationHelper + .addSourceLines( + "TestGuava.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "import com.google.common.base.Function;", + "import com.google.common.util.concurrent.FluentFuture;", + "import com.google.common.util.concurrent.Futures;", + "import com.google.common.util.concurrent.ListenableFuture;", + "import java.util.concurrent.Executor;", + "class TestGuava {", + " private static ListenableFuture<@Nullable String> fluentFutureCatching(Executor executor) {", + " return FluentFuture", + " .from(Futures.immediateFuture(\"hi\"))", + " .catching(Throwable.class, e -> { return null; }, executor);", + " }", + " private static ListenableFuture<@Nullable String> fluentFutureCatchingAsync(Executor executor) {", + " return FluentFuture", + " .from(Futures.immediateFuture(\"hi\"))", + " .catchingAsync(Throwable.class, e -> { return null; }, executor);", + " }", + " private static ListenableFuture<@Nullable String> fluentFutureTransform(Executor executor) {", + " return FluentFuture", + " .from(Futures.immediateFuture(\"hi\"))", + " .transform(s -> { return null; }, executor);", + " }", + " private static ListenableFuture<@Nullable String> fluentFutureTransformAsync(Executor executor) {", + " return FluentFuture", + " .from(Futures.immediateFuture(\"hi\"))", + " .transformAsync(s -> { return null; }, executor);", + " }", + " private static ListenableFuture fluentFutureTransformNoNull(Executor executor) {", + " return FluentFuture", + " .from(Futures.immediateFuture(\"hi\"))", + " // Should be an error when we have full generics support, false-negative for now", + " .transform(s -> { return s; }, executor);", + " }", + " private static ListenableFuture fluentFutureUnsafe(Executor executor) {", + " return FluentFuture", + " .from(Futures.immediateFuture(\"hi\"))", + " // Should be an error when we have full generics support, false-negative for now", + " .transform(s -> { return null; }, executor);", + " }", + " private static ListenableFuture<@Nullable String> futuresTransform(Executor executor) {", + " return Futures", + " .transform(Futures.immediateFuture(\"hi\"), s -> { return null; }, executor);", + " }", + "}") + .doTest(); + } +} diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index b16dd52cc7..14eeb16cbc 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -45,6 +45,7 @@ public void constructorTypeParamInstantiation() { " // BUG: Diagnostic contains: Generic type parameter", " testBadNonNull(new NonNullTypeParam<@Nullable String>());", " testBadNonNull(", + " // BUG: Diagnostic contains: Cannot pass parameter of type NonNullTypeParam<@Nullable String>", " new NonNullTypeParam<", " // BUG: Diagnostic contains: Generic type parameter", " @Nullable String>());", @@ -234,7 +235,7 @@ public void genericsChecksForAssignments() { "class Test {", " static class NullableTypeParam {}", " static void testPositive(NullableTypeParam<@Nullable String> t1) {", - " // BUG: Diagnostic contains: Cannot assign from type", + " // BUG: Diagnostic contains: Cannot assign from type NullableTypeParam<@Nullable String>", " NullableTypeParam t2 = t1;", " }", " static void testNegative(NullableTypeParam<@Nullable String> t1) {", @@ -255,7 +256,7 @@ public void nestedChecksForAssignmentsMultipleArguments() { " static class SampleClass {}", " static class SampleClassMultipleArguments {}", " static void testPositive() {", - " // BUG: Diagnostic contains: Cannot assign from type", + " // BUG: Diagnostic contains: Cannot assign from type SampleClassMultipleArguments>", " SampleClassMultipleArguments>, String> t1 =", " new SampleClassMultipleArguments>, String>();", " }", @@ -278,7 +279,7 @@ public void superTypeAssignmentChecksSingleInterface() { " interface Fn

{}", " class FnImpl implements Fn<@Nullable String, @Nullable String> {}", " void testPositive() {", - " // BUG: Diagnostic contains: Cannot assign from type", + " // BUG: Diagnostic contains: Cannot assign from type FnImpl", " Fn<@Nullable String, String> f = new FnImpl();", " }", " void testNegative() {", @@ -499,7 +500,7 @@ public void genericFunctionReturnTypeNewClassTree() { "class Test {", " static class A { }", " static A testPositive1() {", - " // BUG: Diagnostic contains: mismatched nullability of type parameters", + " // BUG: Diagnostic contains: Cannot return expression of type A<@Nullable String>", " return new A<@Nullable String>();", " }", " static A<@Nullable String> testPositive2() {", @@ -567,7 +568,7 @@ public void genericsChecksForTernaryOperator() { "class Test {", "static class A { }", " static A testPositive(A a, boolean t) {", - " // BUG: Diagnostic contains: Conditional expression must have type", + " // BUG: Diagnostic contains: Conditional expression must have type A<@Nullable String>", " A<@Nullable String> t1 = t ? new A() : new A<@Nullable String>();", " // BUG: Diagnostic contains: Conditional expression must have type", " return t ? new A<@Nullable String>() : new A<@Nullable String>();", @@ -677,14 +678,20 @@ public void parameterPassing() { " static A sampleMethod2(A> a1, A a2) {", " return a2;", " }", + " static void sampleMethod3(A<@Nullable String> a1) {", + " }", " static void testPositive1(A> a1, A a2) {", - " // BUG: Diagnostic contains: Cannot pass parameter of type", + " // BUG: Diagnostic contains: Cannot pass parameter of type A>", " A a = sampleMethod1(a1, a2);", " }", " static void testPositive2(A> a1, A a2) {", " // BUG: Diagnostic contains: Cannot pass parameter of type", " A a = sampleMethod2(a1, a2);", " }", + " static void testPositive3(A a1) {", + " // BUG: Diagnostic contains: Cannot pass parameter of type", + " sampleMethod3(a1);", + " }", " static void testNegative(A> a1, A a2) {", " A a = sampleMethod1(a1, a2);", " }", @@ -718,6 +725,401 @@ public void varargsParameter() { .doTest(); } + /** + * Currently this test is solely to ensure NullAway does not crash in the presence of raw types. + * Further study of the JSpecify documents is needed to determine whether any errors should be + * reported for these cases. + */ + @Test + public void rawTypes() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class NonNullTypeParam {}", + " static class NullableTypeParam {}", + " static void rawLocals() {", + " NonNullTypeParam t1 = new NonNullTypeParam();", + " NullableTypeParam<@Nullable String> t2 = new NullableTypeParam();", + " NonNullTypeParam t3 = new NonNullTypeParam();", + " NullableTypeParam t4 = new NullableTypeParam<@Nullable String>();", + " NonNullTypeParam t5 = new NonNullTypeParam();", + " NullableTypeParam t6 = new NullableTypeParam();", + " }", + " static void rawConditionalExpression(boolean b, NullableTypeParam<@Nullable String> p) {", + " NullableTypeParam<@Nullable String> t = b ? new NullableTypeParam() : p;", + " }", + " static void doNothing(NullableTypeParam<@Nullable String> p) { }", + " static void rawParameterPassing() { doNothing(new NullableTypeParam()); }", + " static NullableTypeParam<@Nullable String> rawReturn() {", + " return new NullableTypeParam();", + "}", + "}") + .doTest(); + } + + @Test + public void nestedGenericTypeAssignment() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class A { }", + " static void testPositive() {", + " // BUG: Diagnostic contains: Cannot assign from type", + " A[]> var1 = new A[]>();", + " // BUG: Diagnostic contains: Cannot assign from type", + " A[]> var2 = new A[]>();", + " }", + " static void testNegative() {", + " A[]> var1 = new A[]>();", + " A[]> var2 = new A[]>();", + " }", + "}") + .doTest(); + } + + @Test + public void genericPrimitiveArrayTypeAssignment() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class A { }", + " static void testPositive() {", + " // BUG: Diagnostic contains: Cannot assign from type A", + " A x = new A();", + " }", + " static void testNegative() {", + " A x = new A();", + " }", + "}") + .doTest(); + } + + @Test + public void nestedGenericTypeVariables() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class A { }", + " static class B {", + " void foo() {", + " A[]> x = new A[]>();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void nestedGenericWildcardTypeVariables() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class A { }", + " static class B {", + " void foo() {", + " A[]> x = new A[]>();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void overrideReturnTypes() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " R apply(P p);", + " }", + " static class TestFunc1 implements Fn {", + " @Override", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static class TestFunc2 implements Fn {", + " @Override", + " public String apply(String s) {", + " return s;", + " }", + " }", + " static class TestFunc3 implements Fn {", + " @Override", + " // BUG: Diagnostic contains: method returns @Nullable, but superclass", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static class TestFunc4 implements Fn<@Nullable String, String> {", + " @Override", + " // BUG: Diagnostic contains: method returns @Nullable, but superclass", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static void useTestFunc(String s) {", + " Fn f1 = new TestFunc1();", + " String t1 = f1.apply(s);", + " // BUG: Diagnostic contains: dereferenced expression", + " t1.hashCode();", + " TestFunc2 f2 = new TestFunc2();", + " String t2 = f2.apply(s);", + " // There should not be an error here", + " t2.hashCode();", + " Fn f3 = new TestFunc2();", + " String t3 = f3.apply(s);", + " // BUG: Diagnostic contains: dereferenced expression", + " t3.hashCode();", + " // BUG: Diagnostic contains: dereferenced expression", + " f3.apply(s).hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void overrideWithNullCheck() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " R apply(P p);", + " }", + " static class TestFunc1 implements Fn {", + " @Override", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static void useTestFuncWithCast() {", + " Fn f1 = new TestFunc1();", + " if (f1.apply(\"hello\") != null) {", + " String t1 = f1.apply(\"hello\");", + " // no error here due to null check", + " t1.hashCode();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void overrideParameterType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " R apply(P p);", + " }", + " static class TestFunc1 implements Fn<@Nullable String, String> {", + " @Override", + " // BUG: Diagnostic contains: parameter s is", + " public String apply(String s) {", + " return s;", + " }", + " }", + " static class TestFunc2 implements Fn<@Nullable String, String> {", + " @Override", + " public String apply(@Nullable String s) {", + " return \"hi\";", + " }", + " }", + " static class TestFunc3 implements Fn {", + " @Override", + " public String apply(String s) {", + " return \"hi\";", + " }", + " }", + " static class TestFunc4 implements Fn {", + " // this override is legal, we should get no error", + " @Override", + " public String apply(@Nullable String s) {", + " return \"hi\";", + " }", + " }", + " static void useTestFunc(String s) {", + " Fn<@Nullable String, String> f1 = new TestFunc2();", + " // should get no error here", + " f1.apply(null);", + " Fn f2 = new TestFunc3();", + " // BUG: Diagnostic contains: passing @Nullable parameter", + " f2.apply(null);", + " }", + "}") + .doTest(); + } + + @Test + public void nullableGenericTypeVariableReturnType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " @Nullable R apply(P p);", + " }", + " static class TestFunc implements Fn {", + " @Override", + " //This override is fine and is handled by the current code", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static void useTestFunc(String s) {", + " Fn f = new TestFunc();", + " String t = f.apply(s);", + " // BUG: Diagnostic contains: dereferenced expression", + " t.hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void overrideWithNestedTypeParametersInReturnType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " class P{}", + " interface Fn, T4 extends @Nullable Object> {", + " T3 apply();", + " }", + " class TestFunc1 implements Fn, @Nullable String> {", + " @Override", + " // BUG: Diagnostic contains: Method returns P<@Nullable String, @Nullable String>, but overridden method", + " public P<@Nullable String, @Nullable String> apply() {", + " return new P<@Nullable String, @Nullable String>();", + " }", + " }", + " class TestFunc2 implements Fn, @Nullable String> {", + " @Override", + " // BUG: Diagnostic contains: Method returns P<@Nullable String, String>, but overridden method returns", + " public P<@Nullable String, String> apply() {", + " return new P<@Nullable String, String>();", + " }", + " }", + " class TestFunc3 implements Fn, @Nullable String> {", + " @Override", + " public P<@Nullable String, @Nullable String> apply() {", + " return new P<@Nullable String, @Nullable String>();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void overrideWithNestedTypeParametersInParameterType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " class P{}", + " interface Fn, R extends @Nullable Object> {", + " String apply(T t, String s);", + " }", + " class TestFunc implements Fn, String> {", + " @Override", + " // BUG: Diagnostic contains: Parameter has type P<@Nullable String, String>, but overridden method has parameter type P", + " public String apply(P<@Nullable String, String> p, String s) {", + " return s;", + " }", + " }", + " class TestFunc2 implements Fn, String> {", + " @Override", + " public String apply(P p, String s) {", + " return s;", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void interactionWithContracts() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "import org.jetbrains.annotations.Contract;", + "class Test {", + " interface Fn1

{", + " R apply(P p);", + " }", + " static class TestFunc1 implements Fn1<@Nullable String, @Nullable String> {", + " @Override", + " @Contract(\"!null -> !null\")", + " public @Nullable String apply(@Nullable String s) {", + " return s;", + " }", + " }", + " interface Fn2

{", + " @Contract(\"!null -> !null\")", + " R apply(P p);", + " }", + " static class TestFunc2 implements Fn2<@Nullable String, @Nullable String> {", + " @Override", + " public @Nullable String apply(@Nullable String s) {", + " return s;", + " }", + " }", + " static class TestFunc2_Bad implements Fn2<@Nullable String, @Nullable String> {", + " @Override", + " public @Nullable String apply(@Nullable String s) {", + " // False negative: with contract checking enabled, this should be rejected", + " // See https://github.com/uber/NullAway/issues/803", + " return null;", + " }", + " }", + " static void testMethod() {", + " // No error due to @Contract", + " (new TestFunc1()).apply(\"hello\").hashCode();", + " Fn1<@Nullable String, @Nullable String> fn1 = new TestFunc1();", + " // BUG: Diagnostic contains: dereferenced expression fn1.apply(\"hello\")", + " fn1.apply(\"hello\").hashCode();", + " // BUG: Diagnostic contains: dereferenced expression (new TestFunc2())", + " (new TestFunc2()).apply(\"hello\").hashCode();", + " Fn2<@Nullable String, @Nullable String> fn2 = new TestFunc2();", + " // No error due to @Contract", + " fn2.apply(\"hello\").hashCode();", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( diff --git a/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayStreamSupportNegativeCases.java b/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayStreamSupportNegativeCases.java index d67a94d02e..742a638b7e 100644 --- a/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayStreamSupportNegativeCases.java +++ b/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayStreamSupportNegativeCases.java @@ -24,6 +24,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.uber.nullaway.testdata.unannotated.CustomStream; import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.DoubleStream; @@ -288,6 +289,26 @@ private void filterThenForEachOrdered(Stream> stream) stream.filter(s -> s.get() != null).forEachOrdered(s -> System.out.println(s.get().length())); } + // CustomStream is modeled in TestLibraryModels + private CustomStream filterThenMapLambdasCustomStream(CustomStream stream) { + return stream.filter(s -> s != null).map(s -> s.length()); + } + + private CustomStream filterThenMapNullableContainerLambdasCustomStream( + CustomStream> stream) { + return stream + .filter(c -> c.get() != null) + .map(c -> c.get().length()); + } + + private CustomStream filterThenMapMethodRefsCustomStream( + CustomStream> stream) { + return stream + .filter(c -> c.get() != null && perhaps()) + .map(NullableContainer::get) + .map(String::length); + } + private static class CheckFinalBeforeStream { @Nullable private final T ref; diff --git a/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayStreamSupportPositiveCases.java b/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayStreamSupportPositiveCases.java index e6c306e98c..82a12a00b9 100644 --- a/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayStreamSupportPositiveCases.java +++ b/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayStreamSupportPositiveCases.java @@ -23,6 +23,7 @@ package com.uber.nullaway.testdata; import com.google.common.base.Preconditions; +import com.uber.nullaway.testdata.unannotated.CustomStreamWithoutModel; import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.DoubleStream; @@ -153,6 +154,28 @@ private void forEachOrdered(Stream> stream) { stream.forEachOrdered(s -> System.out.println(s.get().length())); } + // CustomStreamWithoutModel is NOT modeled in TestLibraryModels + private CustomStreamWithoutModel filterThenMapLambdasCustomStream(CustomStreamWithoutModel stream) { + // Safe because generic is String, not @Nullable String + return stream.filter(s -> s != null).map(s -> s.length()); + } + + private CustomStreamWithoutModel filterThenMapNullableContainerLambdasCustomStream( + CustomStreamWithoutModel> stream) { + return stream + .filter(c -> c.get() != null) + // BUG: Diagnostic contains: dereferenced expression + .map(c -> c.get().length()); + } + + private CustomStreamWithoutModel filterThenMapMethodRefsCustomStream( + CustomStreamWithoutModel> stream) { + return stream + .filter(c -> c.get() != null && perhaps()) + .map(NullableContainer::get) // CSWoM> -> CSWoM<@Nullable String> + .map(String::length); // Should be an error with proper generics support! + } + private static class CheckNonfinalBeforeStream { @Nullable private T ref; diff --git a/nullaway/src/test/resources/com/uber/nullaway/testdata/unannotated/CustomStream.java b/nullaway/src/test/resources/com/uber/nullaway/testdata/unannotated/CustomStream.java new file mode 100644 index 0000000000..22ff34ab8c --- /dev/null +++ b/nullaway/src/test/resources/com/uber/nullaway/testdata/unannotated/CustomStream.java @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2023 Uber Technologies, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package com.uber.nullaway.testdata.unannotated; + +import java.util.function.Function; +import java.util.function.Predicate; + +/** + * A class representing a custom implementation of java.util.stream.Stream to test the ability to + * define stream handler specs through library models. + */ +public class CustomStream { + + private CustomStream() {} + + public CustomStream filter(Predicate predicate) { + throw new UnsupportedOperationException(); + } + + public CustomStream map(Function mapper) { + throw new UnsupportedOperationException(); + } +} diff --git a/nullaway/src/test/resources/com/uber/nullaway/testdata/unannotated/CustomStreamWithoutModel.java b/nullaway/src/test/resources/com/uber/nullaway/testdata/unannotated/CustomStreamWithoutModel.java new file mode 100644 index 0000000000..1fd7037980 --- /dev/null +++ b/nullaway/src/test/resources/com/uber/nullaway/testdata/unannotated/CustomStreamWithoutModel.java @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2023 Uber Technologies, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package com.uber.nullaway.testdata.unannotated; + +import java.util.function.Function; +import java.util.function.Predicate; + +/** + * A copy of {@link CustomStream} but for which our test library models have no spec/model, to test that errors still + * get reported in the absence of a model. + */ +public class CustomStreamWithoutModel { + + private CustomStreamWithoutModel() {} + + public CustomStreamWithoutModel filter(Predicate predicate) { + throw new UnsupportedOperationException(); + } + + public CustomStreamWithoutModel map(Function mapper) { + throw new UnsupportedOperationException(); + } +} diff --git a/sample-app/build.gradle b/sample-app/build.gradle index 7b64db306e..6c4deb2f8b 100644 --- a/sample-app/build.gradle +++ b/sample-app/build.gradle @@ -39,7 +39,7 @@ android { lintOptions { abortOnError false } - + DomainObjectSet variants = getApplicationVariants() // or getLibraryVariants() in libraries variants.addAll(getTestVariants()) variants.addAll(getUnitTestVariants()) @@ -51,17 +51,17 @@ android { } } } - + // If you want to disable NullAway in just tests, you can do the below -// DomainObjectSet testVariants = getTestVariants() -// testVariants.addAll(getUnitTestVariants()) -// testVariants.configureEach { variant -> -// variant.getJavaCompileProvider().configure { -// options.errorprone { -// check("NullAway", CheckSeverity.OFF) -// } -// } -// } + // DomainObjectSet testVariants = getTestVariants() + // testVariants.addAll(getUnitTestVariants()) + // testVariants.configureEach { variant -> + // variant.getJavaCompileProvider().configure { + // options.errorprone { + // check("NullAway", CheckSeverity.OFF) + // } + // } + // } } dependencies { @@ -70,5 +70,10 @@ dependencies { annotationProcessor project(path: ":sample-library-model") testImplementation deps.test.junit4 +} +spotless { + java { + target 'src/*/java/**/*.java' + } } diff --git a/sample-library-model/build.gradle b/sample-library-model/build.gradle index 01ca784f99..8feee697c9 100644 --- a/sample-library-model/build.gradle +++ b/sample-library-model/build.gradle @@ -17,7 +17,7 @@ import net.ltgt.gradle.errorprone.CheckSeverity plugins { - id "java-library" + id "java-library" } dependencies { @@ -29,10 +29,10 @@ dependencies { } tasks.withType(JavaCompile) { - if (!name.toLowerCase().contains("test")) { - options.errorprone { - check("NullAway", CheckSeverity.ERROR) - option("NullAway:AnnotatedPackages", "com.uber") + if (!name.toLowerCase().contains("test")) { + options.errorprone { + check("NullAway", CheckSeverity.ERROR) + option("NullAway:AnnotatedPackages", "com.uber") + } } - } -} \ No newline at end of file +} diff --git a/sample/build.gradle b/sample/build.gradle index 2586c8dee7..58ddd90185 100644 --- a/sample/build.gradle +++ b/sample/build.gradle @@ -17,12 +17,9 @@ import net.ltgt.gradle.errorprone.CheckSeverity plugins { - id "java-library" + id "java-library" } -sourceCompatibility = "1.8" -targetCompatibility = "1.8" - dependencies { annotationProcessor project(":nullaway") annotationProcessor project(path: ":sample-library-model") @@ -32,10 +29,10 @@ dependencies { } tasks.withType(JavaCompile) { - if (!name.toLowerCase().contains("test")) { - options.errorprone { - check("NullAway", CheckSeverity.ERROR) - option("NullAway:AnnotatedPackages", "com.uber") + if (!name.toLowerCase().contains("test")) { + options.errorprone { + check("NullAway", CheckSeverity.ERROR) + option("NullAway:AnnotatedPackages", "com.uber") + } } - } -} +} diff --git a/settings.gradle b/settings.gradle index 4683c7b94c..b1fe598c56 100644 --- a/settings.gradle +++ b/settings.gradle @@ -13,7 +13,6 @@ include ':sample' include ':test-java-lib' include ':test-java-lib-lombok' include ':test-library-models' -include ':compile-bench' include ':jar-infer:android-jarinfer-models-sdk28' include ':jar-infer:android-jarinfer-models-sdk29' include ':jar-infer:android-jarinfer-models-sdk30' @@ -25,10 +24,6 @@ include ':jar-infer:nullaway-integration-test' include ':jmh' include ':guava-recent-unit-tests' include ':jdk17-unit-tests' - -// The following modules require JDK 11 and fail during Gradle configuration on JDK 8 -if (JavaVersion.current() >= JavaVersion.VERSION_11) { - include ':code-coverage-report' - include ':sample-app' - include ':jar-infer:test-android-lib-jarinfer' -} +include ':code-coverage-report' +include ':sample-app' +include ':jar-infer:test-android-lib-jarinfer' diff --git a/test-java-lib-lombok/build.gradle b/test-java-lib-lombok/build.gradle index 8e8d7c5a94..c70f54828d 100644 --- a/test-java-lib-lombok/build.gradle +++ b/test-java-lib-lombok/build.gradle @@ -17,12 +17,9 @@ import net.ltgt.gradle.errorprone.CheckSeverity plugins { - id "java-library" + id "java-library" } -sourceCompatibility = "1.8" -targetCompatibility = "1.8" - dependencies { annotationProcessor project(":nullaway") annotationProcessor deps.test.lombok @@ -34,21 +31,16 @@ dependencies { } tasks.withType(JavaCompile) { - if (!name.toLowerCase().contains("test")) { - options.errorprone { - check("NullAway", CheckSeverity.ERROR) - option("NullAway:AnnotatedPackages", "com.uber") - option("NullAway:UnannotatedSubPackages", "com.uber.lib.unannotated") - if (JavaVersion.current() == JavaVersion.VERSION_1_8) { - // false positive warnings, only on Java 8 - check("MissingSummary", CheckSeverity.OFF) - check("SameNameButDifferent", CheckSeverity.OFF) - } + if (!name.toLowerCase().contains("test")) { + options.errorprone { + check("NullAway", CheckSeverity.ERROR) + option("NullAway:AnnotatedPackages", "com.uber") + option("NullAway:UnannotatedSubPackages", "com.uber.lib.unannotated") + } } - } - if (JavaVersion.current().java9Compatible) { // We need to fork on JDK 16+ since Lombok accesses internal compiler APIs options.fork = true - options.forkOptions.jvmArgs += ["--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED"] - } + options.forkOptions.jvmArgs += [ + "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED" + ] } diff --git a/test-java-lib-lombok/src/main/java/com/uber/lombok/LombokDTO.java b/test-java-lib-lombok/src/main/java/com/uber/lombok/LombokDTO.java index 37bedb4c14..44d6b50709 100644 --- a/test-java-lib-lombok/src/main/java/com/uber/lombok/LombokDTO.java +++ b/test-java-lib-lombok/src/main/java/com/uber/lombok/LombokDTO.java @@ -36,4 +36,5 @@ public class LombokDTO { private String field; @Builder.Default private String fieldWithDefault = "Default"; @Nullable private String nullableField; + @Nullable @Builder.Default private String fieldWithNullDefault = null; } diff --git a/test-java-lib/build.gradle b/test-java-lib/build.gradle index 9fda6c3bef..79323d6ebf 100644 --- a/test-java-lib/build.gradle +++ b/test-java-lib/build.gradle @@ -17,12 +17,9 @@ import net.ltgt.gradle.errorprone.CheckSeverity plugins { - id "java-library" + id "java-library" } -sourceCompatibility = "1.8" -targetCompatibility = "1.8" - dependencies { annotationProcessor project(":nullaway") implementation deps.build.jspecify @@ -33,11 +30,11 @@ dependencies { } tasks.withType(JavaCompile) { - if (!name.toLowerCase().contains("test")) { - options.errorprone { - check("NullAway", CheckSeverity.ERROR) - option("NullAway:AnnotatedPackages", "com.uber") - option("NullAway:UnannotatedSubPackages", "com.uber.lib.unannotated") + if (!name.toLowerCase().contains("test")) { + options.errorprone { + check("NullAway", CheckSeverity.ERROR) + option("NullAway:AnnotatedPackages", "com.uber") + option("NullAway:UnannotatedSubPackages", "com.uber.lib.unannotated") + } } - } } diff --git a/test-library-models/build.gradle b/test-library-models/build.gradle index 01ca784f99..8feee697c9 100644 --- a/test-library-models/build.gradle +++ b/test-library-models/build.gradle @@ -17,7 +17,7 @@ import net.ltgt.gradle.errorprone.CheckSeverity plugins { - id "java-library" + id "java-library" } dependencies { @@ -29,10 +29,10 @@ dependencies { } tasks.withType(JavaCompile) { - if (!name.toLowerCase().contains("test")) { - options.errorprone { - check("NullAway", CheckSeverity.ERROR) - option("NullAway:AnnotatedPackages", "com.uber") + if (!name.toLowerCase().contains("test")) { + options.errorprone { + check("NullAway", CheckSeverity.ERROR) + option("NullAway:AnnotatedPackages", "com.uber") + } } - } -} \ No newline at end of file +} diff --git a/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java b/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java index cd6aad1d40..c94dfef501 100644 --- a/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java +++ b/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java @@ -18,9 +18,12 @@ import static com.uber.nullaway.LibraryModels.MethodRef.methodRef; import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; import com.uber.nullaway.LibraryModels; +import com.uber.nullaway.handlers.stream.StreamModelBuilder; +import com.uber.nullaway.handlers.stream.StreamTypeRecord; @AutoService(LibraryModels.class) public class TestLibraryModels implements LibraryModels { @@ -66,7 +69,7 @@ public ImmutableSetMultimap nullImpliesNullParameters() { @Override public ImmutableSet nullableReturns() { return ImmutableSet.of( - methodRef("com.uber.Foo", "bar()"), + methodRef("com.uber.AnnotatedWithModels", "returnsNullFromModel()"), methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated()"), methodRef("com.uber.lib.unannotated.UnannotatedWithModels", "returnsNullUnannotated2()")); } @@ -87,4 +90,36 @@ public ImmutableSetMultimap castToNonNullMethods() { 1) .build(); } + + @Override + public ImmutableList customStreamNullabilitySpecs() { + // Identical to the default model for java.util.stream.Stream, but with the original type + // renamed + return StreamModelBuilder.start() + .addStreamTypeFromName("com.uber.nullaway.testdata.unannotated.CustomStream") + .withFilterMethodFromSignature("filter(java.util.function.Predicate)") + .withMapMethodFromSignature( + "map(java.util.function.Function)", + "apply", + ImmutableSet.of(0)) + .withMapMethodFromSignature( + "mapToInt(java.util.function.ToIntFunction)", + "applyAsInt", + ImmutableSet.of(0)) + .withMapMethodFromSignature( + "mapToLong(java.util.function.ToLongFunction)", + "applyAsLong", + ImmutableSet.of(0)) + .withMapMethodFromSignature( + "mapToDouble(java.util.function.ToDoubleFunction)", + "applyAsDouble", + ImmutableSet.of(0)) + .withMapMethodFromSignature( + "forEach(java.util.function.Consumer)", "accept", ImmutableSet.of(0)) + .withMapMethodFromSignature( + "forEachOrdered(java.util.function.Consumer)", "accept", ImmutableSet.of(0)) + .withMapMethodAllFromName("flatMap", "apply", ImmutableSet.of(0)) + .withPassthroughMethodFromSignature("distinct()") + .end(); + } }