Add suppressWarnings on fields #95

nimakarimipour · 2022-11-12T15:54:18Z

~~This PR is built upon #91 and #74~~ (#91 and #74 is landed)

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 adds @SuppressWarnings("NullAway") on fields that cannot get annotated as @Nullable due to their impacts and a @Nullable value is used as the initializer.

nimakarimipour · 2022-11-28T17:03:02Z

@msridhar @lazaroclapp FYI, This PR is rebased and ready for review. Thank you 🙂.

msridhar

There seem to be some minor unrelated changes here but I am still fine with landing.

msridhar · 2022-11-28T19:27:44Z

core/src/test/resources/fielderrorregion/Foo.java

+    this.f2 = new Bar();
+  }
+
+  public void run1() {


Why do we need both run1 and run2?

I believe that's so that more new errors are added than the one that is resolved by marking each field @Nullable. But that should be explained in a comment on this test file...

See these reports on the test case:

new TReport(new OnField("Foo.java", "test.Foo", Set.of("f1")), 2), new TReport(new OnField("Foo.java", "test.Foo", Set.of("f2", "f3")), 2), new TReport(new OnField("Foo.java", "test.Foo", Set.of("f4")), 1))

I found the one for f4 a bit confusing, though. For each of f1, f3, and f4 I see two new errors created by making them @Nullable (the 2 derefs on the run methods) and one error resolved (initialization error for f1 and f3, direct @Nullable assignment for f4). So no idea why the effect is 1 for f4.

Yes exactly that was needed to produce more errors to make sure @Nullable is not inferred.

Regarding f4, I am not sure if I fully understood the question, making f4 will resolve one error which is assigning @Nullable and triggers 2 new errors which is deref of f4 therefore the actual effect should be 1. I checked it manually the effect is 1 and correct. Please let me know if I am missing something.

Ok, then, but, by that logic: shouldn't it be the same number for f1 and f3? There is also one error being resolved (the missing initialization error) and two being added. But the corresponding TReports have 2 as the effect. Either way, I don't understand the discrepancy between the number for f4 and the other two.

Actually the difference is that making fields f1 or f3 individually will not resolve the initialization error. The error still remains after the injection. It needs both annotations to be inserted to resolve the error. It only creates two new errors and does not resolve any error, hence the effect is +2.

What is the initialization error in question here?

Is it one error for f1 not being initialized and one error for f3 not being initialized, or is the generic error at Foo() saying some fields are not initialized by the constructor?

I thought we were serializing individual initialization errors. If we aren't, then wouldn't any class with 2 or more constructors and at least one field which is a) not initialized, b) treated as non-null in more than one place, then result in all non-initialized fields being marked @SuppressWarnings(...)? Even when many could otherwise be marked @Nullable? It seems like the auto-annotator should be able to consider each uninitialized field as a separate error...

To test this, could we have an example analogous to f1 but without the explicit dereferences and make sure that is actually determined to be @Nullable?

Or am I missing a different reason why adding @Nullable to f1 or f3 doesn't remove the initialization error for that field?

@lazaroclapp Actually we only serialize one error for initialization errors per constructor which can consist of multiple fields not being initialized. The main purpose of Annotator is to minimize the number of errors the user sees from NullAway output, therefore, we do not split an error coming from NullAway into multiple errors for each uninitialized field. However, the scenario which is mentioned does not happen as we serialize a fix object for each uninitialized field and nullability of each one is investigated individually. I added a unit test for that, please see 8fb982c

In this unit test,

f1, f2 and f3 are not initialized by constructors and are candidates for @Nullable.

Marking f1, f2 or f3 as @Nullable individually will not resolve the one initialization error complaining about the other two fields.

f3 has both conditions, not initialized and treated as @Nonnull in more than one place.

f1 and f2 are determined to be @Nullable.

f3 is determined to be @Nonnull and instead is annotated with SuppressWarnings("NullAway.Init")

f4 was not involved in initialization errors (just assign nullable error) and cannot be @Nullable, therefore it is marked with @SuppressWarnings("NullAway").

lazaroclapp · 2022-11-28T19:46:35Z

core/src/test/resources/fielderrorregion/Foo.java

+    this.f2 = new Bar();
+  }
+
+  public void run1() {


I believe that's so that more new errors are added than the one that is resolved by marking each field @Nullable. But that should be explained in a comment on this test file...

See these reports on the test case:

new TReport(new OnField("Foo.java", "test.Foo", Set.of("f1")), 2), new TReport(new OnField("Foo.java", "test.Foo", Set.of("f2", "f3")), 2), new TReport(new OnField("Foo.java", "test.Foo", Set.of("f4")), 1))

I found the one for f4 a bit confusing, though. For each of f1, f3, and f4 I see two new errors created by making them @Nullable (the 2 derefs on the run methods) and one error resolved (initialization error for f1 and f3, direct @Nullable assignment for f4). So no idea why the effect is 1 for f4.

lazaroclapp · 2022-11-28T19:53:41Z

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

+                "NullAway",
+                false),
+            new AddSingleElementAnnotation(
+                new OnField(srcRoot.resolve("Foo.java").toString(), "test.Foo", Set.of("f0")),


Why? Shouldn't Foo.f0 simply be marked @Nullable? Where is it being used as non-null?

Thank you very much for catching this bug. It was both annotated as @Nullable and @SuppressWarnings. The @SuppressWarnings was added because of the passing null error but that error is resolved with @NullUnmarked injection but was selected again later in SuppressWarnings selection logic. This is resolved now and f0 is just annotated as @Nullable. 789b608

lazaroclapp · 2022-11-28T19:57:19Z

core/src/main/java/edu/ucr/cs/riple/core/Annotator.java

+                  if (error.messageType.equals("PASS_NULLABLE")
+                      && error.nonnullTarget != null
+                      && error.nonnullTarget.isOnParameter()) {
+                    OnParameter calledMethod = error.nonnullTarget.toParameter();


Why is this parameter "called method"? Not sure I understand what this change is doing...

Thank you for noticing this 3301cda. It is marking a method @NullUnmarked that received a @Nullbale for parameter which annotater rejected its parameter to be @Nullable. Renamed it.

nimakarimipour · 2022-11-29T01:25:40Z

@msridhar @lazaroclapp Thank you for your comments and review. This is ready for another round. Thank you.

lazaroclapp

The last couple of changes make sense, but I still have some questions about how we are counting the effect / errors resolved, when dealing with field initialization...

lazaroclapp · 2022-11-29T03:02:08Z

core/src/test/resources/fielderrorregion/Foo.java

+    this.f2 = new Bar();
+  }
+
+  public void run1() {


What is the initialization error in question here?

Is it one error for f1 not being initialized and one error for f3 not being initialized, or is the generic error at Foo() saying some fields are not initialized by the constructor?

I thought we were serializing individual initialization errors. If we aren't, then wouldn't any class with 2 or more constructors and at least one field which is a) not initialized, b) treated as non-null in more than one place, then result in all non-initialized fields being marked @SuppressWarnings(...)? Even when many could otherwise be marked @Nullable? It seems like the auto-annotator should be able to consider each uninitialized field as a separate error...

To test this, could we have an example analogous to f1 but without the explicit dereferences and make sure that is actually determined to be @Nullable?

Or am I missing a different reason why adding @Nullable to f1 or f3 doesn't remove the initialization error for that field?

lazaroclapp · 2022-11-29T03:07:44Z

core/src/main/java/edu/ucr/cs/riple/core/Annotator.java

                  if (!error.getRegion().isOnField()) {
                    return false;
                  }
+                  if (error.messageType.equals("PASS_NULLABLE") && error.getRegion().isOnField()) {


Isn't the second condition already guaranteed by the check right above? (i.e. at this point error.getRegion().isOnField() is known to be true or we would have hit the early return on line 295)

Yes, that makes sense, removed it. Thank you c3b2464

lazaroclapp

One small test case request and then this looks ready to me!

lazaroclapp · 2022-11-29T17:47:15Z

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

+                false));
+    Assert.assertEquals(
+        expectedAnnotations, Set.copyOf(coreTestHelper.getConfig().log.getInjectedAnnotations()));
+  }


lazaroclapp · 2022-11-29T17:49:35Z

core/src/test/resources/fielderrorregion/Foo.java

+  Bar f1;
+  Bar f2, f3;
+  @Nullable Bar nullableBar;
+  Bar f4 = nullableBar.process(new Object());


New test case looks great to me! Could we still add Bar f5; without initialization or dereference to this single constructor test case and make sure it's marked as @Nullable. Just for the sake of testing all cases. If I understood the explanation correctly, then I fully expect it to work and then we can land this 🙂

Sure, please find it 970a2ab

lazaroclapp

Looks good to land if/when CI passes! 🚀

nimakarimipour added 10 commits September 16, 2022 21:05

resolve #71

44afe23

resolve issue-79

b39d72c

update

d820b07

add unit tests

6a5b075

Merge branch 'nimak/issue-79' into nimak/add-supresswarnings

919d2ee

more renames

38a79a6

Merge branch 'nimak/nullaway_update' into nimak/add-supresswarnings

b836928

merge with nimak/nullaway_update

8af3ce6

initial implementation

3f1009b

update

39958a4

nimakarimipour requested review from lazaroclapp and msridhar November 12, 2022 15:56

nimakarimipour self-assigned this Nov 12, 2022

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

nimakarimipour added 16 commits November 12, 2022 14:36

update nullaway version

af9fd46

update field region

74b7b3b

merge with nimak/nullaway_update

690763d

remove wrong check

aa4ebc6

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

merge with nimak/nullaway_update

cb259a3

Merge branch 'master' into nimak/add-supresswarnings

b0b584a

fix conflict

aecff9f

nimakarimipour added 4 commits November 19, 2022 16:33

extract output file name into a constant

2db1d9a

rename to usedClass and usedMember

e86efcd

update header in unit tests

606de6e

erge branch 'nimak/nullaway_update' into nimak/add-supresswarnings

a89a281

nimakarimipour mentioned this pull request Nov 20, 2022

Add informative comment on @NullUnmarked injections #99

Closed

nimakarimipour added 7 commits November 23, 2022 17:01

merge master

287b50f

undo unwanted change

308e7c8

undo unwanted change

1e4f627

undo unwanted change

33fe68f

undo unwanted change

2c121cc

undo unwanted change

41749d1

undo unwanted change

4a4ada6

msridhar approved these changes Nov 28, 2022

View reviewed changes

lazaroclapp reviewed Nov 28, 2022

View reviewed changes

nimakarimipour added 2 commits November 28, 2022 19:07

updated suppresswarnings injections

789b608

rename variable

3301cda

nimakarimipour requested a review from lazaroclapp November 29, 2022 01:45

lazaroclapp reviewed Nov 29, 2022

View reviewed changes

nimakarimipour added 4 commits November 28, 2022 22:47

remove redudant check

c3b2464

make field declaration store all entities

e5868d9

add unit test

8fb982c

exlcude new test

9463586

lazaroclapp reviewed Nov 29, 2022

View reviewed changes

update unit test

970a2ab

nimakarimipour requested a review from lazaroclapp November 29, 2022 18:01

lazaroclapp approved these changes Nov 29, 2022

View reviewed changes

nimakarimipour merged commit 0307283 into master Nov 29, 2022

nimakarimipour deleted the nimak/add-supresswarnings branch November 29, 2022 18:20

Add suppressWarnings on fields #95

Add suppressWarnings on fields #95

Uh oh!

Conversation

nimakarimipour commented Nov 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI

Uh oh!

nimakarimipour commented Nov 28, 2022

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nimakarimipour Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nimakarimipour Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nimakarimipour commented Nov 29, 2022

Uh oh!

lazaroclapp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lazaroclapp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lazaroclapp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nimakarimipour commented Nov 12, 2022 •

edited

Loading

nimakarimipour Nov 29, 2022 •

edited

Loading

nimakarimipour Nov 29, 2022 •

edited

Loading