Update region computation. #91

nimakarimipour · 2022-11-11T16:55:49Z

CI

~~Please note that CI will fail for this PR as it requires release of NullAway:0.10.5.~~
NullAway:0.10.5 is released and CI should pass.

This PR updates the region computation in Type-Annotator-Scanner according to latest changes in NullAway following PR658.

Also in this PR the infrastructure for keeping compatibility with older versions of NullAway (0.10.4 or less) is implemented.

msridhar

What is here LGTM, though I know we plan to update this one to keep compatibility with NullAway 0.10.4. I guess we can approve after this PR is finalized.

msridhar

Mostly LGTM, just a few comments

msridhar · 2022-11-17T17:44:05Z

.github/workflows/continuous-integration.yml

+        with:
+          java-version: 11
+          distribution: 'adopt'


Nit: 'adopt' is out of date, use 'temurin'

msridhar · 2022-11-17T17:45:09Z

.github/workflows/continuous-integration.yml

+          distribution: 'adopt'
+      - name: Build with Gradle
+        run: ./gradlew :core:test --scan --no-daemon -Pnullaway-test-version=0


Probably don't need --no-daemon here?

Yes, that doesn't seem necessary. 08d9e2c

msridhar · 2022-11-17T17:51:37Z

core/src/main/java/edu/ucr/cs/riple/core/adapters/Adapter.java

+ * Responsible for performing tasks related to NullAway / Type Annotator Scanner serialization
+ * features.
+ */
+public interface Adapter {


Let's be more verbose here and call this NullAwayVersionAdapter

msridhar · 2022-11-17T17:52:16Z

core/src/main/java/edu/ucr/cs/riple/core/adapters/AdapterAbstractClass.java

+import edu.ucr.cs.riple.core.Config;
+
+/** Base class for all adapters. */
+public abstract class AdapterAbstractClass implements Adapter {


I don't think it's worth creating this abstract class just to hold a single protected field. I would get rid of it.

Deleted in: b51475a

msridhar · 2022-11-17T17:53:01Z

core/src/main/java/edu/ucr/cs/riple/core/adapters/NullAwayAdapterVersion0.java

+ *   <li>Type Annotator Scanner: 1.3.3 or below
+ * </ul>
+ */
+public class NullAwayAdapterVersion0 extends AdapterAbstractClass {


If we go with NullAwayVersionAdapter for the interface, this can be NullAwayVersion0Adapter, or simply NullAwayV0Adapter

Sure, sound good. b51475a

Thank you for the suggestions in renaming.

core/src/main/java/edu/ucr/cs/riple/core/adapters/NullAwayAdapterVersion1.java

core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/Region.java

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

nimakarimipour · 2022-11-18T16:45:58Z

@msridhar Thank you for the review. I resolved all comments.

lazaroclapp

Would like to see CI passing on the recently released NullAway 0.10.5 before landing this, but here are my comments so far :)

lazaroclapp · 2022-11-18T18:38:35Z

.github/workflows/continuous-integration.yml

I assume either now or in a follow up diff, we want to change all the above distribution: 'adopt' to temurin. Not really needed for this PR (and it's already quite large) but worth keeping in mind.

lazaroclapp · 2022-11-18T18:44:05Z

.github/workflows/continuous-integration.yml

+          distribution: 'temurin'
+      - name: Build with Gradle
+        run: ./gradlew :core:test --scan -Pnullaway-test-version=0


Can we call this something more accurate like nullaway-serialization-format-version=0. It is not directly the NullAway version itself.

Sure makes sense. 69e537b

lazaroclapp · 2022-11-18T18:49:46Z

type-annotator-scanner/src/test/java/edu/ucr/cs/riple/scanner/tools/TrackerNodeDisplay.java

@@ -29,14 +29,14 @@
 public class TrackerNodeDisplay implements Display {

  public final String callerClass;
-  public final String callerMethod;
+  public final String callerMember;
  public final String calleeClass;
  public final String member;


Shouldn't this be renamed to calleeMember or similar? Or maybe even calleeMethod? Is it ever anything other than a method? Can it be a field? (If so, then the term "callee" is not exactly correct either...)

Wondering if destinationClass, destinationMember, sourceClass, sourceMember would be better names here... hard to tell without having the description of the algorithm, so maybe we can keep these names for now, but they are a bit misleading (cc: @msridhar for potential naming suggestions)

Yes, they it should be calleeMember and yes it can be a field as well. I understand that calleeMember is not accurate as it can be a field as well. I am going to change it for now to calleeMember but it should be changed in future. 851f8eb

I think "caller" here refers to the region containing a use of some member, and "callee" refers to the used member. So maybe regionClass, regionMember, usedClass, usedMember? @nimakarimipour if you agree we can just make the change. Otherwise, we can leave a TODO comment and fix in a follow-up.

@msridhar Sure, we are already renaming a member of this class in this PR, I will update it now. Thank you for the suggestions.

lazaroclapp · 2022-11-18T18:54:35Z

type-annotator-scanner/src/test/java/edu/ucr/cs/riple/scanner/FieldTrackerTest.java

+            "}")
+        .setExpectedOutputs(
+            new TrackerNodeDisplay("edu.ucr.A", "f0", "edu.ucr.B", "foo"),
+            new TrackerNodeDisplay("edu.ucr.A", "f0", "edu.ucr.B", "foo"),


Why are some entries, like the one just above, repeated?

Methods matchIdentifier and matchMemberSelect might produce the same output, I just thought it might not worth it to keep all the outputs in a set and check for every output if it is already serialized as they will be deserialized and converted into a set in core. Also I didn't want to change the test infra in this PR. Please let me know if you think I should change the infra or make a list and prevent duplicates serialize in output.

Makes sense to me, just wanted to understand this. I do think eventually you might want tests to not need this repetition, but it's low priority and not needed for this PR.

lazaroclapp · 2022-11-18T19:20:48Z

type-annotator-scanner/src/main/java/edu/ucr/cs/riple/scanner/out/TrackerNode.java

+          // Can be enclosed by a field declaration tree.
+          VariableTree fieldDeclTree = ASTHelpers.findEnclosingNode(path, VariableTree.class);
+          if (fieldDeclTree != null) {
+            fieldSymbol = ASTHelpers.getSymbol(fieldDeclTree);


So, in this path, we aren't actually checking that ASTHelpers.getSymbol(fieldDeclTree) is a field, right?

What happens on a test case like:

public class A { private static Object f; static { Object a = B.foo(); Object b = B.bar(); if (a.hashCode() > b.hashCode()) { f = a; } else { f = b; } } } public class B { public static Object foo() {...} public static Object bar() {...} }

Wouldn't that generate spurious A.a and A.b regions, even though a and b are locals and not members of A?

Yes, we are creating A.b and A.a. In previous approach the enclosing class of these fields (a and b) was still considered to be A and we would have generated region A.null. In the new approach we are making A.a and A.b. I am not sure if they are actually wrong as long as NullAway considers the identical region for triggered error and fix. Please let me know if we should still change this.

@nimakarimipour the issue here is that there are no fields A.a and A.b; a and b are local variables inside the static initializer. Ideally, we would not create regions for entities that are not fields. I would think you could easily add a check that the symbol is actually for a field? You can check symbol.getKind().equals(ElementKind.FIELD)

@msridhar @lazaroclapp Sure, it is wrong to create a region for A.a and A.b conceptually. To address this issue the suggested change will work and it will create correct results. But if we apply this change, it will require a follow up change in NullAway as well. If we do not apply this change, we will not get inaccurate results as NullAway and Type-Annotator-Scanner uses the same logic to label regions. And if we apply this change here in this PR, we might get inaccurate results using NullAway:0.10.5. Please let me know if I should apply the change anyway and make a follow up on NullAway as well. Thank you.

I think we should at least track this issue somewhere. I would be fine with it not being fixed in this version of the auto-annotator, so we can keep compatibility with NullAway 0.10.5 (though this is again kind of an issue where we are breaking fix serialization format compatibility... wonder how common these breaking changes will be in the future). The main problem is that I am not sure what the auto-annotator will try to do with the code above then: will it try to add @Nullable to those local variable declarations? Will it just consider it an error where it can't make a fix on the file? I am also wondering if this can cause broken code to be generated by the auto-annotator if e.g. the @Nullable annotation is not applicable to local variable declarations...

@msridhar @lazaroclapp It is still capable of examining @Nullable annotations on a and b, however NullAway will not suggest a fix for these variables since they are local variables.

The only case that will not be able to handle is seeing a dereference error in their initialization expression.

Class Bar { static { @Nullable Foo a; Foo b = a.deref(); } }

With current labeling, the region for dereferencing a.bar() is Bar#b and we cannot add a SuppressWarnings on that.

But in the suggested version which is labeling this region as Bar#null it is not still possible to silence this error with @SuppressWarnings as well. (For both cases the only possible solution is to add the SuppressWarnings at the class level).

In summary, the current implementation does not produce inaccurate results and it will not leave an error which the proposed approach can resolve.

(For both cases the only possible solution is to add the SuppressWarnings at the class level).

So, just to be clear: What is the tool's current action on the code below:

public class A { private static Object f; static { Object a = B.foo(); Object b = B.bar(); if (Objects.hashCode(a) > Objects.hashCode(b)) { f = a; } else { f = b; } } } public class B { @Nullable public static Object foo() {...} @Nullable public static Object bar() {...} }

Is it to add a suppression on the class level on A or is it to leave the error or is it something else?

Changed to Objects.hashCode(...) so there isn't a third-party call taking null in there...

@lazaroclapp I ran the example you provided, please find the output below:

public class A { @Nullable private static Object f; static { Object a = B.foo(); Object b = B.bar(); if (Objects.hashCode(a) > Objects.hashCode(b)) { f = a; } else { f = b; } } } class B { @Nullable public static Object foo() {return null;} @Nullable public static Object bar() {return null;} }

Leaving no remaining NullAway error.

Added a unit test 2218ea3

In summary, the current implementation does not produce inaccurate results and it will not leave an error which the proposed approach can resolve.

Ok, in this case, I am fine with going with this change, though we should track fixing this issue of invalid field regions everywhere (NullAway, scanner, annotator) as a follow-up.

lazaroclapp · 2022-11-18T20:22:25Z

core/src/main/java/edu/ucr/cs/riple/core/adapters/NullAwayVersionAdapter.java

+   *
+   * @param location Location of the targeted element.
+   * @param values Values in row of a TSV file.
+   * @return Corresponding Error instance with the passed values.


Error -> Fix

lazaroclapp · 2022-11-18T20:22:44Z

core/src/main/java/edu/ucr/cs/riple/core/adapters/NullAwayVersionAdapter.java

+   * corresponding {@link TrackerNode} instance.
+   *
+   * @param values Values in row of a TSV file.
+   * @return Corresponding Error instance with the passed values.


Error -> TrackerNode

Thank you for noticing this. 204ec6b

lazaroclapp · 2022-11-18T20:25:07Z

core/src/main/java/edu/ucr/cs/riple/core/adapters/NullAwayV0Adapter.java

+  }
+
+  @Override
+  public Fix deserializeFix(Location location, String[] values) {


Wouldn't it be a good idea to have an initial precondition here (and in all similar methods taking String[] values) checking for the number of elements in values? Presumably different serialization formats will have different number of fields?

Sure, sounds good. 94d4efe

lazaroclapp · 2022-11-18T21:07:22Z

core/src/main/java/edu/ucr/cs/riple/core/metadata/field/FieldInitializationAnalysis.java

-  public FieldInitializationAnalysis(Path path) {
-    super(path);
+  public FieldInitializationAnalysis(Config config) {
+    super(config, config.target.dir.resolve("field_init.tsv"));


Could we extract "field_init.tsv" into a global constant, like we do below for MethodDeclarationTree:

super(config, config.target.dir.resolve(Serializer.METHOD_INFO_FILE_NAME));

Actually "field_init.tsv" is not an output of TypeAnnotatorScanner and it might not be preferred to declare the file name in its Serializer. But we should definitely declare the file as a global constant. I extracted it into the user class. 2db1d9a

nimakarimipour · 2022-11-19T22:36:40Z

@lazaroclapp Thank you very much for the review, this is ready for another round.

lazaroclapp

All changes since my last review look good, and most comments addressed, however, this one still stands, see @msridhar's note.

lazaroclapp

Ok, given the discussion above and the results in the unit test, I think the chance of a case that will break due to the spurious local regions is sufficiently small. This LGTM! Thanks, Nima, and apologies for all the back and forth! 🙂

lazaroclapp · 2022-11-23T18:04:21Z

core/src/test/java/edu/ucr/cs/riple/core/CoreTest.java

+        .toDepth(5)
+        .addExpectedReports(new TReport(new OnField("A.java", "test.A", singleton("f")), -3))
+        .enableForceResolve()
+        .start();


nimakarimipour · 2022-11-23T22:54:59Z

@msridhar @lazaroclapp Thank you very much for the review. I will land it now.

nimakarimipour added 2 commits November 11, 2022 09:56

update

d820b07

add unit tests

6a5b075

nimakarimipour added the enhancement New feature or request label Nov 11, 2022

nimakarimipour requested review from msridhar and lazaroclapp November 11, 2022 16:55

nimakarimipour self-assigned this Nov 11, 2022

nimakarimipour added 3 commits November 11, 2022 14:04

more renames

38a79a6

update nullaway version

af9fd46

update field region

74b7b3b

nimakarimipour mentioned this pull request Nov 12, 2022

Add suppressWarnings on fields #95

Merged

msridhar reviewed Nov 15, 2022

View reviewed changes

nimakarimipour added 9 commits November 16, 2022 10:48

update region structure

440608d

cleanup

d218d29

update adapters in config

c4e9d89

update CI

55cd6ae

add adapters

7ee9a7b

add javadoc

28c52a1

remove unwanted test

898be2f

keep metadata untouched

605171e

fix bug

a0db707

msridhar reviewed Nov 17, 2022

View reviewed changes

nimakarimipour and others added 8 commits November 17, 2022 14:13

make adopter initialize lazily

adeaf9a

fix adapter

035053f

Merge branch 'master' into nimak/nullaway_update

6a2898e

resolve conflicts

eba2200

Update core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/Reg…

c818b9f

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

use temurin

21c8112

remove no deamon

08d9e2c

rename of adapters

b51475a

nimakarimipour requested a review from msridhar November 18, 2022 16:45

lazaroclapp reviewed Nov 18, 2022

View reviewed changes

nimakarimipour added 8 commits November 18, 2022 17:17

change NullAway test version to 0.10.5

ba70c68

change cmd line argument name

69e537b

change to calleeMember

851f8eb

change Error to Fix

2ae2b00

change Error to TrackerNode

204ec6b

add preconditions for deserializations

94d4efe

extract output file name into a constant

2db1d9a

rename to usedClass and usedMember

e86efcd

nimakarimipour requested a review from lazaroclapp November 19, 2022 22:36

nimakarimipour added 2 commits November 19, 2022 17:32

update header in unit tests

606de6e

rename to compareByMember

69e46e4

lazaroclapp reviewed Nov 21, 2022

View reviewed changes

nimakarimipour added 2 commits November 22, 2022 20:01

add static local var unit test

2218ea3

rename unit test

335ee94

msridhar approved these changes Nov 23, 2022

View reviewed changes

lazaroclapp approved these changes Nov 23, 2022

View reviewed changes

nimakarimipour merged commit e4b482e into master Nov 23, 2022

nimakarimipour deleted the nimak/nullaway_update branch November 23, 2022 22:55

Update region computation. #91

Update region computation. #91

Conversation

nimakarimipour commented Nov 11, 2022 • edited Loading

CI

msridhar left a comment

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

nimakarimipour commented Nov 18, 2022

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

nimakarimipour Nov 19, 2022 • 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

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 Nov 22, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Nov 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour Nov 19, 2022 • edited Loading

Choose a reason for hiding this comment

nimakarimipour commented Nov 19, 2022

lazaroclapp left a 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

nimakarimipour commented Nov 23, 2022

nimakarimipour commented Nov 11, 2022 •

edited

Loading

nimakarimipour Nov 18, 2022 •

edited

Loading

nimakarimipour Nov 18, 2022 •

edited

Loading

nimakarimipour Nov 18, 2022 •

edited

Loading

nimakarimipour Nov 18, 2022 •

edited

Loading

nimakarimipour Nov 18, 2022 •

edited

Loading

nimakarimipour Nov 19, 2022 •

edited

Loading

lazaroclapp Nov 22, 2022 •

edited

Loading

nimakarimipour Nov 19, 2022 •

edited

Loading

nimakarimipour Nov 19, 2022 •

edited

Loading