From cb3ff5127a15ab3ed3de6bedd68ff1a98a532400 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 16 Dec 2024 11:09:56 -0800 Subject: [PATCH] Remove `InferredJARModelsHandler` (#1079) Now, all logic for parsing loading `astubx` files is consolidated into the `LibraryModelsHandler.ExternalStubxLibraryModels` class and is uniform for `astubx` files generated via JarInfer or from source code, which will ease future bug fixes and changes. We copied over the logic from `InferredJARModelsHandler` for loading Android SDK models if present. We also update the logic for `ExternalStubxLibraryModels.nonNullParameters()` and `ExternalStubxLibraryModels.nullableReturns()` to pull in all the relevant info from the parsed `astubx` file. Fixes #1072 --- .../java/com/uber/nullaway/LibraryModels.java | 9 +- .../com/uber/nullaway/handlers/Handlers.java | 3 - .../handlers/InferredJARModelsHandler.java | 244 ------------------ .../handlers/LibraryModelsHandler.java | 96 ++++++- 4 files changed, 100 insertions(+), 252 deletions(-) delete mode 100644 nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index 4b06f11745..1cad493399 100644 --- a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java +++ b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java @@ -214,7 +214,14 @@ private MethodRef(String enclosingClass, String methodName, String fullMethodSig this.fullMethodSig = fullMethodSig; } - private static final Pattern METHOD_SIG_PATTERN = Pattern.compile("^(<.*>)?(\\w+)(\\(.*\\))$"); + /** + * Pattern for parsing method signatures. The signature should be in the format: {@code + * methodName(ArgType1,ArgType2,...)}. + * + * @see #methodRef(String, String) + */ + private static final Pattern METHOD_SIG_PATTERN = + Pattern.compile("^(<.*>)?(\\w+|)(\\(.*\\))$"); /** * Construct a method reference. 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 5e8d5ac689..168c4dc18e 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java @@ -51,9 +51,6 @@ public static Handler buildDefault(Config config) { // bytecode annotations handlerListBuilder.add(new RestrictiveAnnotationHandler(config)); } - if (config.isJarInferEnabled()) { - handlerListBuilder.add(new InferredJARModelsHandler()); - } if (config.handleTestAssertionLibraries()) { handlerListBuilder.add(new AssertionHandler(methodNameUtil)); } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java deleted file mode 100644 index aec47b9db9..0000000000 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java +++ /dev/null @@ -1,244 +0,0 @@ -/* - * Copyright (c) 2018 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.handlers; - -import com.google.common.base.Preconditions; -import com.google.errorprone.VisitorState; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.Tree; -import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.util.Context; -import com.uber.nullaway.NullAway; -import com.uber.nullaway.Nullness; -import com.uber.nullaway.dataflow.AccessPath; -import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; -import java.io.InputStream; -import java.util.LinkedHashSet; -import java.util.Map; -import java.util.Set; -import javax.lang.model.element.Modifier; -import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode; -import org.jspecify.annotations.Nullable; - -/** This handler loads inferred nullability model from stubs for methods in unannotated packages. */ -public class InferredJARModelsHandler extends BaseNoOpHandler { - private static boolean DEBUG = false; - private static boolean VERBOSE = false; - - private static void LOG(boolean cond, String tag, String msg) { - if (cond) { - System.out.println("[JI " + tag + "] " + msg); - } - } - - private static final String ANDROID_ASTUBX_LOCATION = "jarinfer.astubx"; - private static final String ANDROID_MODEL_CLASS = - "com.uber.nullaway.jarinfer.AndroidJarInferModels"; - - private static final int RETURN = -1; // '-1' indexes Return type in the Annotation Cache - - private final Map>>> argAnnotCache; - - private final StubxCacheUtil cacheUtil; - - public InferredJARModelsHandler() { - super(); - String jarInferLogName = "JI"; - this.cacheUtil = new StubxCacheUtil(jarInferLogName); - argAnnotCache = cacheUtil.getArgAnnotCache(); - // Load Android SDK JarInfer models - try { - InputStream androidStubxIS = - Class.forName(ANDROID_MODEL_CLASS) - .getClassLoader() - .getResourceAsStream(ANDROID_ASTUBX_LOCATION); - if (androidStubxIS != null) { - cacheUtil.parseStubStream(androidStubxIS, "android.jar: " + ANDROID_ASTUBX_LOCATION); - LOG(DEBUG, "DEBUG", "Loaded Android RT models."); - } - } catch (ClassNotFoundException e) { - LOG( - DEBUG, - "DEBUG", - "Cannot find Android RT models locator class." - + " This is expected if not in an Android project, or the Android SDK JarInfer models Jar has not been set up for this build."); - - } catch (Exception e) { - LOG(DEBUG, "DEBUG", "Cannot load Android RT models."); - } - } - - @Override - public Nullness[] onOverrideMethodInvocationParametersNullability( - Context context, - Symbol.MethodSymbol methodSymbol, - boolean isAnnotated, - Nullness[] argumentPositionNullness) { - if (isAnnotated) { - // We currently do not load JarInfer models for code marked as annotated. - // This is unlikely to change, as the behavior of JarInfer on arguments is to explicitly mark - // as @NonNull those arguments that are shallowly dereferenced within the analyzed method. By - // convention, annotated code has no use for explicit @NonNull annotations, since `T` already - // means `@NonNull T` within annotated code. The only case where we would want to enable this - // for annotated code is if we expect/want JarInfer results to override the results of another - // handler, such as restrictive annotations, but a library models is a safer place to perform - // such an override. - // Additionally, by default, InferredJARModelsHandler is used only to load our Android SDK - // JarInfer models (i.e. `com.uber.nullaway:JarInferAndroidModelsSDK##`), since the default - // model of JarInfer on a normal jar/aar is to add bytecode annotations. - return argumentPositionNullness; - } - Symbol.ClassSymbol classSymbol = methodSymbol.enclClass(); - String className = classSymbol.getQualifiedName().toString(); - if (methodSymbol.getModifiers().contains(Modifier.ABSTRACT)) { - LOG( - VERBOSE, - "Warn", - "Skipping abstract method: " + className + " : " + methodSymbol.getQualifiedName()); - return argumentPositionNullness; - } - if (!argAnnotCache.containsKey(className)) { - return argumentPositionNullness; - } - String methodSign = getMethodSignature(methodSymbol); - Map> methodArgAnnotations = lookupMethodInCache(className, methodSign); - if (methodArgAnnotations == null) { - return argumentPositionNullness; - } - Set jiNonNullParams = new LinkedHashSet<>(); - for (Map.Entry> annotationEntry : methodArgAnnotations.entrySet()) { - if (annotationEntry.getKey() != RETURN - && annotationEntry.getValue().contains("javax.annotation.Nonnull")) { - int nonNullPosition = annotationEntry.getKey(); - jiNonNullParams.add(nonNullPosition); - argumentPositionNullness[nonNullPosition] = Nullness.NONNULL; - } - } - if (!jiNonNullParams.isEmpty()) { - LOG(DEBUG, "DEBUG", "Nonnull params: " + jiNonNullParams.toString() + " for " + methodSign); - } - return argumentPositionNullness; - } - - @Override - public NullnessHint onDataflowVisitMethodInvocation( - MethodInvocationNode node, - Symbol.MethodSymbol symbol, - VisitorState state, - AccessPath.AccessPathContext apContext, - AccessPathNullnessPropagation.SubNodeValues inputs, - AccessPathNullnessPropagation.Updates thenUpdates, - AccessPathNullnessPropagation.Updates elseUpdates, - AccessPathNullnessPropagation.Updates bothUpdates) { - if (isReturnAnnotatedNullable(symbol)) { - return NullnessHint.HINT_NULLABLE; - } - return NullnessHint.UNKNOWN; - } - - @Override - public boolean onOverrideMayBeNullExpr( - NullAway analysis, - ExpressionTree expr, - @Nullable Symbol exprSymbol, - VisitorState state, - boolean exprMayBeNull) { - if (exprMayBeNull) { - return true; - } - if (expr.getKind() == Tree.Kind.METHOD_INVOCATION - && exprSymbol instanceof Symbol.MethodSymbol - && isReturnAnnotatedNullable((Symbol.MethodSymbol) exprSymbol)) { - return true; - } - return false; - } - - private boolean isReturnAnnotatedNullable(Symbol.MethodSymbol methodSymbol) { - Preconditions.checkNotNull(methodSymbol); - Symbol.ClassSymbol classSymbol = methodSymbol.enclClass(); - String className = classSymbol.getQualifiedName().toString(); - if (argAnnotCache.containsKey(className)) { - String methodSign = getMethodSignature(methodSymbol); - Map> methodArgAnnotations = lookupMethodInCache(className, methodSign); - if (methodArgAnnotations != null) { - Set methodAnnotations = methodArgAnnotations.get(RETURN); - if (methodAnnotations != null) { - if (methodAnnotations.contains("javax.annotation.Nullable") - || methodAnnotations.contains("org.jspecify.annotations.Nullable")) { - LOG(DEBUG, "DEBUG", "Nullable return for method: " + methodSign); - return true; - } - } - } - } - return false; - } - - private @Nullable Map> lookupMethodInCache( - String className, String methodSign) { - if (!argAnnotCache.containsKey(className)) { - return null; - } - Map> methodArgAnnotations = argAnnotCache.get(className).get(methodSign); - if (methodArgAnnotations == null) { - LOG( - VERBOSE, - "Warn", - "Cannot find Annotation Cache entry for method: " - + methodSign - + " in class: " - + className); - return null; - } - LOG( - DEBUG, - "DEBUG", - "Found Annotation Cache entry for method: " - + methodSign - + " in class: " - + className - + " -- " - + methodArgAnnotations.toString()); - return methodArgAnnotations; - } - - private String getMethodSignature(Symbol.MethodSymbol method) { - // Generate method signature - String methodSign = - method.enclClass().getQualifiedName().toString() - + ":" - + (method.isStaticOrInstanceInit() ? "" : method.getReturnType() + " ") - + method.getSimpleName() - + "("; - if (!method.getParameters().isEmpty()) { - methodSign += - String.join( - ", ", - method.getParameters().stream().map(p -> p.type.toString()).toArray(String[]::new)); - } - methodSign += ")"; - LOG(DEBUG, "DEBUG", "@ method sign: " + methodSign); - return methodSign; - } -} 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 bc1e49f703..646246275b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -49,6 +49,7 @@ import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import com.uber.nullaway.handlers.stream.StreamTypeRecord; +import java.io.InputStream; import java.util.ArrayList; import java.util.HashSet; import java.util.LinkedHashMap; @@ -1297,6 +1298,13 @@ private NameIndexedMap makeOptimizedLookup( /** Constructs Library Models from stubx files */ private static class ExternalStubxLibraryModels implements LibraryModels { + /** astubx file name used in our Android SDK JarInfer models */ + private static final String ANDROID_ASTUBX_LOCATION = "jarinfer.astubx"; + + /** Class we expect to be present in a jar containing Android SDK JarInfer models */ + private static final String ANDROID_MODEL_CLASS = + "com.uber.nullaway.jarinfer.AndroidJarInferModels"; + private final Map>>> argAnnotCache; private final Set nullMarkedClassesCache; private final Map upperBoundsCache; @@ -1304,6 +1312,25 @@ private static class ExternalStubxLibraryModels implements LibraryModels { ExternalStubxLibraryModels() { String libraryModelLogName = "LM"; StubxCacheUtil cacheUtil = new StubxCacheUtil(libraryModelLogName); + // hardcoded loading of stubx files from android-jarinfer-models-sdkXX artifacts + try { + InputStream androidStubxIS = + Class.forName(ANDROID_MODEL_CLASS) + .getClassLoader() + .getResourceAsStream(ANDROID_ASTUBX_LOCATION); + if (androidStubxIS != null) { + cacheUtil.parseStubStream(androidStubxIS, "android.jar: " + ANDROID_ASTUBX_LOCATION); + astubxLoadLog("Loaded Android RT models."); + } + } catch (ClassNotFoundException e) { + astubxLoadLog( + "Cannot find Android RT models locator class." + + " This is expected if not in an Android project, or the Android SDK JarInfer models Jar has not been set up for this build."); + + } catch (Exception e) { + astubxLoadLog("Cannot load Android RT models."); + } + argAnnotCache = cacheUtil.getArgAnnotCache(); nullMarkedClassesCache = cacheUtil.getNullMarkedClassesCache(); upperBoundsCache = cacheUtil.getUpperBoundCache(); @@ -1338,11 +1365,14 @@ public ImmutableSetMultimap explicitlyNullableParameters() { String className = outerEntry.getKey(); for (Map.Entry>> innerEntry : outerEntry.getValue().entrySet()) { - String methodName = innerEntry.getKey().substring(innerEntry.getKey().indexOf(" ") + 1); + String methodNameAndSignature = + innerEntry.getKey().substring(innerEntry.getKey().indexOf(" ") + 1); for (Map.Entry> entry : innerEntry.getValue().entrySet()) { Integer index = entry.getKey(); if (index >= 0 && entry.getValue().stream().anyMatch(a -> a.contains("Nullable"))) { - mapBuilder.put(methodRef(className, methodName), index); + // remove spaces + methodNameAndSignature = methodNameAndSignature.replaceAll("\\s", ""); + mapBuilder.put(methodRef(className, methodNameAndSignature), index); } } } @@ -1352,7 +1382,37 @@ public ImmutableSetMultimap explicitlyNullableParameters() { @Override public ImmutableSetMultimap nonNullParameters() { - return ImmutableSetMultimap.of(); + ImmutableSetMultimap.Builder mapBuilder = + new ImmutableSetMultimap.Builder<>(); + for (String className : argAnnotCache.keySet()) { + for (Map.Entry>> methodEntry : + argAnnotCache.get(className).entrySet()) { + String methodNameAndSignature = + methodEntry.getKey().substring(methodEntry.getKey().indexOf(" ") + 1); + + for (Map.Entry> argEntry : methodEntry.getValue().entrySet()) { + Integer index = argEntry.getKey(); + if (index >= 0) { + for (String annotation : argEntry.getValue()) { + if (annotation.contains("NonNull") + || annotation.equals("javax.annotation.Nonnull")) { + astubxLoadLog( + "Found non-null parameter: " + + className + + "." + + methodEntry.getKey() + + " arg " + + argEntry.getKey()); + // remove spaces + methodNameAndSignature = methodNameAndSignature.replaceAll("\\s", ""); + mapBuilder.put(methodRef(className, methodNameAndSignature), index); + } + } + } + } + } + } + return mapBuilder.build(); } @Override @@ -1372,7 +1432,27 @@ public ImmutableSetMultimap nullImpliesNullParameters() { @Override public ImmutableSet nullableReturns() { - return ImmutableSet.of(); + ImmutableSet.Builder builder = new ImmutableSet.Builder<>(); + for (String className : argAnnotCache.keySet()) { + for (Map.Entry>> methodEntry : + argAnnotCache.get(className).entrySet()) { + String methodNameAndSignature = + methodEntry.getKey().substring(methodEntry.getKey().indexOf(" ") + 1); + + for (Map.Entry> argEntry : methodEntry.getValue().entrySet()) { + Integer index = argEntry.getKey(); + if (index == -1) { + Set annotations = argEntry.getValue(); + if (annotations.contains("javax.annotation.Nullable") + || annotations.contains("org.jspecify.annotations.Nullable")) { + methodNameAndSignature = methodNameAndSignature.replaceAll("\\s", ""); + builder.add(methodRef(className, methodNameAndSignature)); + } + } + } + } + } + return builder.build(); } @Override @@ -1385,4 +1465,12 @@ public ImmutableSetMultimap castToNonNullMethods() { return ImmutableSetMultimap.of(); } } + + private static boolean DEBUG_ASTUBX_LOADING = false; + + private static void astubxLoadLog(String msg) { + if (DEBUG_ASTUBX_LOADING) { + System.out.println("[JI DEBUG] " + msg); + } + } }