External Library Models Integration #922

akulk022 · 2024-02-24T23:13:37Z

The newly added library-model module consists of a CLI process that takes an input directory with annotated java source files as a command line parameter and uses com.github.javaparser APIS to generate libmodels.astubx file containing method stubs for methods that return @nullable. This can be run using the existing JarInferEnabled and JarInferUseReturnAnnotations flags.

This allows us to be able catch issues as shown in the below example from externally annotated source code:

@NullMarked
public class AnnotationExample {
    @Nullable
    public String makeUpperCase(String inputString) {
        if (inputString == null || inputString.isEmpty()) {
            return null;
        } else {
            return inputString.toUpperCase();
        }
    }
}

class Test {
    static AnnotationExample annotationExample = new AnnotationExample();
    static void test(String value){}
    static void testPositive() {
        // BUG: Diagnostic contains: passing @Nullable parameter 'annotationExample.makeUpperCase(\"nullaway\")'
        test(annotationExample.makeUpperCase(\"nullaway\"));
    }    
}

…o library_model_stub_abhijitk

…hFileException

codecov · 2024-02-26T06:29:46Z

Codecov Report

Attention: Patch coverage is 9.70874% with 93 lines in your changes are missing coverage. Please review.

Project coverage is 86.08%. Comparing base (5acd394) to head (80be0d4).

Files	Patch %	Lines
.../uber/nullaway/libmodel/LibraryModelGenerator.java	0.00%	92 Missing ⚠️
...er/nullaway/handlers/InferredJARModelsHandler.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #922      +/-   ##
============================================
- Coverage     87.15%   86.08%   -1.07%     
- Complexity     2011     2018       +7     
============================================
  Files            78       79       +1     
  Lines          6522     6612      +90     
  Branches       1265     1280      +15     
============================================
+ Hits           5684     5692       +8     
- Misses          422      510      +88     
+ Partials        416      410       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… for nullable returns

msridhar

A few quick comments to start

lib-model/lib-model-consume/build.gradle

lib-model/lib-model-consume/src/main/java/com/uber/nullaway/libmodel/LibModelInfoExtractor.java

...odel/lib-model-consume/src/main/java/com/uber/nullaway/libmodel/MethodAnnotationsRecord.java

lib-model/lib-model-consume/src/main/java/com/uber/nullaway/libmodel/StubxFileWriter.java

lib-model/lib-model-consume/src/main/java/com/uber/nullaway/libmodel/LibModelInfoExtractor.java

…bmodel/LibModelInfoExtractor.java Co-authored-by: Manu Sridharan <[email protected]>

lib-model/lib-model-consume/src/main/java/com/uber/nullaway/libmodel/LibModelInfoExtractor.java

… thread safe

…o library_model_stub_abhijitk

msridhar · 2024-03-06T02:36:34Z

At a high level I'd like to do some renaming of the new modules. Let's use library-model instead of lib-model, consistently. Also:

lib-model-consume -> library-model-generator
lib-model-consume-cli -> library-model-generator-cli
lib-model-test -> library-model-generator-integration-test

msridhar

Overall this looks great now! I have one more comment which should be an easy fix. I think the remaining high-level question, for me, is whether we should rename classes related to JarInfer inside NullAway (since they now serve a more general purpose), and whether we should add extra command-line flags instead of just having JarInferEnabled. I think we can fix those things in a follow-up PR but we should decide what we want to do.

Otherwise, I'm going to ask @lazaroclapp to review this PR as he originally wrote the stubx generation code.

msridhar · 2024-03-14T04:24:40Z

.../library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

+                if (a.getNameAsString().equalsIgnoreCase(NULL_MARKED)) {
+                  this.isNullMarked = true;
+                }


This logic won't handle cases where @NullMarked appears on some nested classes but not others. But, I think for now, we can just say we only support @NullMarked on the top-level class. Can we add a comment to this effect?

It also needs @NullMarked on the same file, correct? We don't have a way to add this to the package somewhere and have the rest of the traversal be aware of that (that seems more likely than mixed marked/unmarked code in the models, but I am not sure)

Yes, you're correct, @lazaroclapp, we are assuming an explicit @NullMarked on the top-level class in a source file, and that there is only one top-level class per source file. We won't find anything on package-info.java or module-info.java for now. Both these assumptions hold for jspecify/jdk

@akulk022 can you update the comment in light of the above?

lazaroclapp

Did a pass. Had a bunch of comments, but they are mostly questions and notes for the future. Overall this looks good to me.

build.gradle

lazaroclapp · 2024-03-20T03:01:29Z

...ibrary-model-generator/src/main/java/com/uber/nullaway/libmodel/MethodAnnotationsRecord.java

+import com.google.common.collect.ImmutableSet;
+
+/** A record describing the annotations associated with a java method and its arguments. */
+@AutoValue


Are we otherwise using @AutoValue for something or what's the advantage of it here?

Sorry this is my fault. I thought this was new code and thought that @AutoValue made it a bit cleaner. I didn't realize it had been moved from somewhere else. @lazaroclapp would you like us to undo the @AutoValue-ization of this file?

Never mind, seems we have been using @AutoValue in at least some parts of the codebase, since the beginning, so I am fine having it here too, at least until we can do JDK 17 records.

lazaroclapp · 2024-03-20T03:04:31Z

library-model/library-model-generator-cli/build.gradle

@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2017. Uber Technologies


Not that I care about this anymore, but I suspect Uber would want these copyright headers to be updated for the right years 😉 (e.g. 2024 here and 20[...]-2024 for the files that were altered, I think)

lazaroclapp · 2024-03-20T03:06:39Z

...y-model-generator-cli/src/main/java/com/uber/nullaway/libmodel/LibraryModelGeneratorCLI.java

+   */
+  public static void main(String[] args) {
+    LibraryModelGenerator libraryModelGenerator = new LibraryModelGenerator();
+    libraryModelGenerator.generateAstubxForLibraryModels(args[0], args[1]);


Check the number of args and print a basic usage message? (Not sure if we do that for JarInfer, and I don't think we need a full argument parser here yet, a check on the length of args would suffice for now)

lazaroclapp · 2024-03-20T03:12:03Z

...r-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java

+                "-XepOpt:NullAway:JarInferEnabled=true",
+                "-XepOpt:NullAway:JarInferUseReturnAnnotations=true"))


I agree with @yuxincs that overloading the meaning of the JarInfer[...] CLI options for NullAway for this is bad practice, specially because other than the Android SDK, JarInfer no longer uses .astubx but actually modifies the bytecode of jars to add annotations (so this options aren't used with JarInfer except for that Android SDK jar).

It can certainly be a follow up PR, but I'd be in favor of rethinking these CLI flags. Maybe -XepOpt:NullAway:JarInferEnabled is actually something like -XepOpt:NullAway:ScanClassPathForAstubXModels? Is there a better name? A nicer way to specify which .astubx files we want to load without breaking things in our build internally? (I think there is! We only really care about that Android SDK astubx file right now, so it's not like this changes for each target like it used to, cc: @yuxincs )

I am in favor of changing these, but maybe in a follow-up PR. I don't think we need to go as far as keeping the old flag but deprecating it, as long as we document properly in release notes. But we should probably check as to what this breaks (if anything) internally at Uber.

@akulk022 can you open an issue on renaming these flags?

I've created the issue #940

lazaroclapp · 2024-03-20T03:34:30Z

.../library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

+      return (annotation.getNameAsString().equalsIgnoreCase(NULLABLE)
+              && this.isJspecifyNullableImportPresent)
+          || annotation.getNameAsString().equalsIgnoreCase(JSPECIFY_NULLABLE_IMPORT);
+    }


Do we care about star imports or other nonsense or is it fine here to only count either FQN or an import of the FQN?

I think this logic is fine but we should add a comment that we don't consider star imports @akulk022

lazaroclapp · 2024-03-20T03:40:02Z

.../library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

+    Map<String, String> importedAnnotations =
+        ImmutableMap.of(
+            "Nonnull", "javax.annotation.Nonnull",
+            "Nullable", "javax.annotation.Nullable");


Why are we using the javax annotations here and not JSpecify? Some limitation with StubxWriter?

NullAway/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java

Line 221 in 3bde26c

if (methodAnnotations.contains("javax.annotation.Nullable")) {

No limitation with StubxWriter 🙂, just used it because the check here considers the javax annotation and I probably didn't consider adding an OR and including the jspecify annotation in the condition. I've updated it.

lazaroclapp · 2024-03-20T03:44:09Z

.../library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

+                if (a.getNameAsString().equalsIgnoreCase(NULL_MARKED)) {
+                  this.isNullMarked = true;
+                }


It also needs @NullMarked on the same file, correct? We don't have a way to add this to the package somewhere and have the rest of the traversal be aware of that (that seems more likely than mixed marked/unmarked code in the models, but I am not sure)

lazaroclapp · 2024-03-20T03:47:45Z

.../library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

+      String parentClassName = "";
+      if (parentClassNode.isPresent()) {
+        if (parentClassNode.get() instanceof ClassOrInterfaceDeclaration) {
+          parentClassName = ((ClassOrInterfaceDeclaration) parentClassNode.get()).getNameAsString();


This is just the simple name, right? So currently this breaks when dealing with nested/inner classes? i.e. the concatenation below:

packageName + "." + parentClassName + ":" + getMethodReturnTypeString(md) [...]

Ends up producing com.example.Bar:String baz() for:

@NullMarked class Foo { public class Bar { @Nullable public String baz() {...} } }

correct?

Yes, that is correct. I'll look into making this work.

@akulk022 is this easy to fix? Or should it wait for a follow up?

@msridhar I'm working on a fix since it seems like it should be easy enough.

@akulk022 be sure to add a test for it as well

@msridhar @lazaroclapp I've updated the logic to handle this scenario, I've also added a test for it. Does this work and are there more scenarios I should test?

lazaroclapp · 2024-03-20T03:58:55Z

.../library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

+     */
+    private String getMethodReturnTypeString(MethodDeclaration md) {
+      if (md.getType() instanceof ClassOrInterfaceType) {
+        return md.getType().getChildNodes().get(0).toString();


Note that using simple names is something we inherited from the CF stub format + us not explicitly handling imports, I think. So we might want to change these to FQNs at some point (but probably not in this PR)

Co-authored-by: Lázaro Clapp <[email protected]>

…Handler.java and updating some comments

…erEnabled and JarInferUseReturnAnnotations flags.

…o library_model_stub_abhijitk

… defined in the jar's manifest

msridhar · 2024-03-27T21:47:11Z

.../library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

@@ -157,7 +155,7 @@ public Result process(Path localPath, Path absolutePath, ParseResult<Compilation

  private static class AnnotationCollectionVisitor extends VoidVisitorAdapter<Void> {

-    private String packageName = "";
+    private StringBuilder parentName;


Probably better to just make this a String? I think making it a StringBuilder is an unnecessary optimization and makes the code more brittle. You can just overwrite it with new String values and I think it'll be fine

…o library_model_stub_abhijitk

lazaroclapp

LGTM! Definitely some potential future improvements are possible (and renaming some stuff so it's less tied to JarInfer), but as far as initial support for building JSpecify JDK astubx files, this is excellent!

msridhar · 2024-04-09T18:35:38Z

Going to merge this now, and we will build on it in future PRs

akulk022 added 12 commits January 31, 2024 15:45

fixes

1d48e6e

stubx file writer logic

b0631cb

changing method returns to nullable

e5353d1

Adding safe cast for EnumDeclaration as parent for method

ea7d117

updating logic for method declaration

e9a06dc

moving lib model to separate module

19b020b

moving code

31f569c

infrastructure code for lib model

a26f856

Merge branch 'uber:master' into library_model_stub_abhijitk

a863363

adding sample_jdk source files in resources

ee56362

Merge remote-tracking branch 'origin/library_model_stub_abhijitk' int…

3806c80

…o library_model_stub_abhijitk

adding logic to create directories if they don't exist to avoid NoSuc…

32f0990

…hFileException

akulk022 added 3 commits February 26, 2024 16:11

changing logic for ArrayTypes and adding Scanner findInLine test case…

3dedc77

… for nullable returns

replacing with synthetic test case files

07586ae

unnecessary formatting change

4b7891e

msridhar reviewed Feb 29, 2024

View reviewed changes

msridhar and others added 3 commits February 28, 2024 18:53

minor cleanup

10c36e3

Update lib-model/lib-model-consume/src/main/java/com/uber/nullaway/li…

36b5a21

…bmodel/LibModelInfoExtractor.java Co-authored-by: Manu Sridharan <[email protected]>

fixes

a6c8204

msridhar reviewed Feb 29, 2024

View reviewed changes

lib-model/lib-model-consume/src/main/java/com/uber/nullaway/libmodel/LibModelInfoExtractor.java Outdated Show resolved Hide resolved

akulk022 and others added 6 commits February 29, 2024 15:21

Generating MethodAnnotationsRecord using AutoValue

0c2cd44

Removing Redundant files for StubxWriter and MethodAnnotationsRecord

ac14c99

Merge branch 'master' into library_model_stub_abhijitk

2dbc283

making methodRecords local variable and passing as a param to make it…

32e80d5

… thread safe

Merge remote-tracking branch 'origin/library_model_stub_abhijitk' int…

2b637f1

…o library_model_stub_abhijitk

separating lib model cli module

888c1b2

akulk022 requested a review from msridhar March 6, 2024 00:02

removing unnecessary dependencies

fb81869

akulk022 added 2 commits March 13, 2024 14:45

Refactoring test setup

1137aa9

Merge branch 'master' into library_model_stub_abhijitk

6e0ef33

msridhar marked this pull request as ready for review March 14, 2024 04:19

msridhar approved these changes Mar 14, 2024

View reviewed changes

adding comment for NullMarked functionality

41bc90f

msridhar requested a review from lazaroclapp March 19, 2024 16:13

lazaroclapp reviewed Mar 20, 2024

View reviewed changes

akulk022 and others added 2 commits March 22, 2024 03:24

adding simple usage message

26c4907

review suggestion

dc0618a

Co-authored-by: Lázaro Clapp <[email protected]>

akulk022 mentioned this pull request Mar 23, 2024

Rename the Jar Infer Flags JarInferEnabled,JarInferUseReturnAnnotations #940

Open

akulk022 and others added 9 commits March 23, 2024 16:55

adding condition to consider jspecify annotation in InferredJARModels…

5fa73da

…Handler.java and updating some comments

Merge branch 'master' into library_model_stub_abhijitk

14e39b6

try to incorporate more code coverage info

f390cd1

Adding a test to check if the functionality is off without the JarInf…

ad44788

…erEnabled and JarInferUseReturnAnnotations flags.

Merge remote-tracking branch 'origin/library_model_stub_abhijitk' int…

2a1c2da

…o library_model_stub_abhijitk

Handling the scenario for inner class: uber#922 (comment)

8f86b36

minor comment modifications.

35b988c

Removing redundant main class specification in javaexec; it's already…

c1fe36b

… defined in the jar's manifest

Merge branch 'master' into library_model_stub_abhijitk

d301e28

akulk022 requested a review from lazaroclapp March 24, 2024 21:07

msridhar reviewed Mar 27, 2024

View reviewed changes

akulk022 added 5 commits March 27, 2024 19:07

changing parentName from StringBuilder to String

439ca14

Merge branch 'master' into library_model_stub_abhijitk

e2a9c60

updating year in copyright header for newly created files

a072bbd

Merge remote-tracking branch 'origin/library_model_stub_abhijitk' int…

75785fa

…o library_model_stub_abhijitk

Merge branch 'master' into library_model_stub_abhijitk

80be0d4

lazaroclapp approved these changes Mar 31, 2024

View reviewed changes

msridhar merged commit 09db47a into uber:master Apr 9, 2024
10 of 12 checks passed

msridhar mentioned this pull request Apr 16, 2024

Tracking issue: making jspecify/jdk library models available to NullAway users #950

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External Library Models Integration #922

External Library Models Integration #922

akulk022 commented Feb 24, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading

msridhar left a comment

msridhar commented Mar 6, 2024

msridhar left a comment

msridhar Mar 14, 2024

lazaroclapp Mar 20, 2024

msridhar Mar 23, 2024

msridhar Mar 23, 2024

lazaroclapp left a comment

lazaroclapp Mar 20, 2024

msridhar Mar 23, 2024

lazaroclapp Mar 31, 2024

lazaroclapp Mar 20, 2024

lazaroclapp Mar 20, 2024

lazaroclapp Mar 20, 2024

msridhar Mar 23, 2024

akulk022 Mar 23, 2024

lazaroclapp Mar 20, 2024

msridhar Mar 24, 2024

lazaroclapp Mar 20, 2024

akulk022 Mar 20, 2024 •

edited

Loading

lazaroclapp Mar 20, 2024

lazaroclapp Mar 20, 2024

akulk022 Mar 21, 2024 •

edited

Loading

msridhar Mar 24, 2024

akulk022 Mar 24, 2024

msridhar Mar 24, 2024

akulk022 Mar 24, 2024

lazaroclapp Mar 20, 2024

msridhar Mar 27, 2024 •

edited

Loading

lazaroclapp left a comment

msridhar commented Apr 9, 2024

		"-XepOpt:NullAway:JarInferEnabled=true",
		"-XepOpt:NullAway:JarInferUseReturnAnnotations=true"))

External Library Models Integration #922

External Library Models Integration #922

Conversation

akulk022 commented Feb 24, 2024 • edited Loading

codecov bot commented Feb 26, 2024 • edited Loading

Codecov Report

msridhar left a comment

Choose a reason for hiding this comment

msridhar commented Mar 6, 2024

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akulk022 Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akulk022 Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

msridhar commented Apr 9, 2024

akulk022 commented Feb 24, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading

akulk022 Mar 20, 2024 •

edited

Loading

akulk022 Mar 21, 2024 •

edited

Loading

msridhar Mar 27, 2024 •

edited

Loading