Skip to content
Merged
Show file tree
Hide file tree
Changes from 64 commits
Commits
Show all changes
71 commits
Select commit Hold shift + click to select a range
44afe23
resolve #71
nimakarimipour Sep 17, 2022
b39d72c
resolve issue-79
nimakarimipour Oct 19, 2022
d820b07
update
nimakarimipour Nov 11, 2022
6a5b075
add unit tests
nimakarimipour Nov 11, 2022
919d2ee
Merge branch 'nimak/issue-79' into nimak/add-supresswarnings
nimakarimipour Nov 11, 2022
38a79a6
more renames
nimakarimipour Nov 11, 2022
b836928
Merge branch 'nimak/nullaway_update' into nimak/add-supresswarnings
nimakarimipour Nov 11, 2022
8af3ce6
merge with nimak/nullaway_update
nimakarimipour Nov 11, 2022
3f1009b
initial implementation
nimakarimipour Nov 12, 2022
39958a4
update
nimakarimipour Nov 12, 2022
af9fd46
update nullaway version
nimakarimipour Nov 12, 2022
74b7b3b
update field region
nimakarimipour Nov 12, 2022
690763d
merge with nimak/nullaway_update
nimakarimipour Nov 12, 2022
aa4ebc6
remove wrong check
nimakarimipour Nov 13, 2022
440608d
update region structure
nimakarimipour Nov 16, 2022
d218d29
cleanup
nimakarimipour Nov 16, 2022
c4e9d89
update adapters in config
nimakarimipour Nov 16, 2022
55cd6ae
update CI
nimakarimipour Nov 16, 2022
7ee9a7b
add adapters
nimakarimipour Nov 17, 2022
28c52a1
add javadoc
nimakarimipour Nov 17, 2022
898be2f
remove unwanted test
nimakarimipour Nov 17, 2022
605171e
keep metadata untouched
nimakarimipour Nov 17, 2022
a0db707
fix bug
nimakarimipour Nov 17, 2022
cb259a3
merge with nimak/nullaway_update
nimakarimipour Nov 17, 2022
b0b584a
Merge branch 'master' into nimak/add-supresswarnings
nimakarimipour Nov 17, 2022
aecff9f
fix conflict
nimakarimipour Nov 17, 2022
adeaf9a
make adopter initialize lazily
nimakarimipour Nov 17, 2022
035053f
fix adapter
nimakarimipour Nov 17, 2022
6a2898e
Merge branch 'master' into nimak/nullaway_update
nimakarimipour Nov 17, 2022
eba2200
resolve conflicts
nimakarimipour Nov 17, 2022
97dcdbb
add unit test
nimakarimipour Nov 17, 2022
6aa8bcb
Merge branch 'master' into nimak/add-supresswarnings
nimakarimipour Nov 17, 2022
e7d14c7
resolve conflict
nimakarimipour Nov 17, 2022
58d38d0
merge with latest version of master
nimakarimipour Nov 17, 2022
3b3a821
add unit tests
nimakarimipour Nov 17, 2022
8fdf83d
use temurin
nimakarimipour Nov 18, 2022
72ada16
remove no deamon
nimakarimipour Nov 18, 2022
aa68932
renaming of adapters
nimakarimipour Nov 18, 2022
c818b9f
Update core/src/main/java/edu/ucr/cs/riple/core/metadata/trackers/Reg…
nimakarimipour Nov 18, 2022
21c8112
use temurin
nimakarimipour Nov 18, 2022
08d9e2c
remove no deamon
nimakarimipour Nov 18, 2022
b51475a
rename of adapters
nimakarimipour Nov 18, 2022
10828a2
Merge branch 'nimak/nullaway_update' into nimak/add-supresswarnings
nimakarimipour Nov 18, 2022
ba70c68
change NullAway test version to 0.10.5
nimakarimipour Nov 18, 2022
69e537b
change cmd line argument name
nimakarimipour Nov 19, 2022
851f8eb
change to calleeMember
nimakarimipour Nov 19, 2022
2ae2b00
change Error to Fix
nimakarimipour Nov 19, 2022
204ec6b
change Error to TrackerNode
nimakarimipour Nov 19, 2022
c9c1ca1
Merge branch 'nimak/nullaway_update' into nimak/add-supresswarnings
nimakarimipour Nov 19, 2022
b21407e
add preconditions in deserializations
nimakarimipour Nov 19, 2022
caff1d3
extract output file name into a constant
nimakarimipour Nov 19, 2022
bc2ad4e
rename to usedClass and usedMember
nimakarimipour Nov 19, 2022
94d4efe
add preconditions for deserializations
nimakarimipour Nov 19, 2022
2db1d9a
extract output file name into a constant
nimakarimipour Nov 19, 2022
e86efcd
rename to usedClass and usedMember
nimakarimipour Nov 19, 2022
606de6e
update header in unit tests
nimakarimipour Nov 19, 2022
a89a281
erge branch 'nimak/nullaway_update' into nimak/add-supresswarnings
nimakarimipour Nov 19, 2022
287b50f
merge master
nimakarimipour Nov 23, 2022
308e7c8
undo unwanted change
nimakarimipour Nov 23, 2022
1e4f627
undo unwanted change
nimakarimipour Nov 23, 2022
33fe68f
undo unwanted change
nimakarimipour Nov 23, 2022
2c121cc
undo unwanted change
nimakarimipour Nov 23, 2022
41749d1
undo unwanted change
nimakarimipour Nov 23, 2022
4a4ada6
undo unwanted change
nimakarimipour Nov 23, 2022
789b608
updated suppresswarnings injections
nimakarimipour Nov 29, 2022
3301cda
rename variable
nimakarimipour Nov 29, 2022
c3b2464
remove redudant check
nimakarimipour Nov 29, 2022
e5868d9
make field declaration store all entities
nimakarimipour Nov 29, 2022
8fb982c
add unit test
nimakarimipour Nov 29, 2022
9463586
exlcude new test
nimakarimipour Nov 29, 2022
970a2ab
update unit test
nimakarimipour Nov 29, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -97,5 +97,14 @@ tasks.create("configureNullAwayVersion"){
println "NullAway Test version changed to: " + NULLAWAY_TEST

// exclude unsupported tests below...
switch (NULLAWAY_TEST){
case "0.10.4":
test {
filter {
excludeTest "edu.ucr.cs.riple.core.CoreTest", "errorInFieldDeclarationForceResolveTest"
}
}
break
}
}
tasks.test.dependsOn("configureNullAwayVersion")
45 changes: 27 additions & 18 deletions core/src/main/java/edu/ucr/cs/riple/core/Annotator.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
import edu.ucr.cs.riple.injector.changes.AddMarkerAnnotation;
import edu.ucr.cs.riple.injector.changes.AddSingleElementAnnotation;
import edu.ucr.cs.riple.injector.location.OnField;
import edu.ucr.cs.riple.injector.location.OnMethod;
import edu.ucr.cs.riple.injector.location.OnParameter;
import java.util.List;
import java.util.Objects;
import java.util.Set;
Expand Down Expand Up @@ -267,42 +267,49 @@ private void forceResolveRemainingErrors(
if (error.getRegion().isOnMethod()) {
return tree.findNode(error.encMember(), error.encClass());
}
if (error.nonnullTarget == null) {
// Just a sanity check.
return null;
}
// For methods invoked in an initialization region, where the error is that
// `@Nullable` is being passed as an argument, we add a `@NullUnmarked` annotation
// to the called method.
if (error.messageType.equals("PASS_NULLABLE")) {
OnMethod calledMethod = error.nonnullTarget.toMethod();
if (error.messageType.equals("PASS_NULLABLE")
&& error.nonnullTarget != null
&& error.nonnullTarget.isOnParameter()) {
OnParameter calledMethod = error.nonnullTarget.toParameter();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return tree.findNode(calledMethod.method, calledMethod.clazz);
}
return null;
})
// Filter null values from map above.
.filter(Objects::nonNull)
.map(node -> new AddMarkerAnnotation(node.location, config.nullUnMarkedAnnotation))
.collect(Collectors.toSet());
injector.injectAnnotations(nullUnMarkedAnnotations);

// Update log.
config.log.updateInjectedAnnotations(nullUnMarkedAnnotations);

// Collect explicit Nullable initialization to fields
Set<AddAnnotation> suppressWarningsAnnotations =
remainingFixes.stream()
// Collect suppress warnings, errors on field declaration regions.
Set<OnField> fieldsWithSuppressWarnings =
remainingErrors.stream()
.filter(
fix -> {
if (!(fix.isOnField() && fix.reasons.contains("ASSIGN_FIELD_NULLABLE"))) {
error -> {
if (!error.getRegion().isOnField()) {
return false;
}
OnField onField = fix.toField();
return onField.clazz.equals(fix.encClass()) && !fix.getRegion().isOnMethod();
// We can silence them by SuppressWarnings("NullAway.Init")
return !error.messageType.equals("METHOD_NO_INIT")
&& !error.messageType.equals("FIELD_NO_INIT");
})
.map(
fix ->
new AddSingleElementAnnotation(
fix.toField(), "SuppressWarnings", "NullAway", false))
error ->
fieldDeclarationAnalysis.getLocationOnField(
error.getRegion().clazz, error.getRegion().member))
.filter(Objects::nonNull)
.collect(Collectors.toSet());

Set<AddAnnotation> suppressWarningsAnnotations =
fieldsWithSuppressWarnings.stream()
.map(
onField ->
new AddSingleElementAnnotation(onField, "SuppressWarnings", "NullAway", false))
.collect(Collectors.toSet());
injector.injectAnnotations(suppressWarningsAnnotations);
// Update log.
Expand All @@ -316,6 +323,8 @@ private void forceResolveRemainingErrors(
fix.isOnField()
&& (fix.reasons.contains("METHOD_NO_INIT")
|| fix.reasons.contains("FIELD_NO_INIT")))
// Filter nodes annotated with SuppressWarnings("NullAway")
.filter(fix -> !fieldsWithSuppressWarnings.contains(fix.toField()))
.map(
fix ->
new AddSingleElementAnnotation(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import edu.ucr.cs.riple.core.metadata.MetaData;
import edu.ucr.cs.riple.injector.Helper;
import edu.ucr.cs.riple.injector.exceptions.TargetClassNotFound;
import edu.ucr.cs.riple.injector.location.OnField;
import edu.ucr.cs.riple.scanner.TypeAnnotatorScanner;
import java.io.File;
import java.io.FileNotFoundException;
Expand Down Expand Up @@ -77,7 +78,7 @@ protected FieldDeclarationInfo addNodeByLine(String[] values) {
System.err.println(notFound.getMessage());
return null;
}
FieldDeclarationInfo info = new FieldDeclarationInfo(clazz);
FieldDeclarationInfo info = new FieldDeclarationInfo(path, clazz);
members.forEach(
bodyDeclaration ->
bodyDeclaration.ifFieldDeclaration(
Expand Down Expand Up @@ -116,4 +117,20 @@ public ImmutableSet<String> getInLineMultipleFieldDeclarationsOnField(
candidate.fields.stream().filter(group -> !Collections.disjoint(group, fields)).findFirst();
return inLineGroupFieldDeclaration.orElse(ImmutableSet.copyOf(fields));
}

/**
* Creates a {@link OnField} instance targeting the passed field and class.
*
* @param clazz Enclosing class of the field.
* @param field Field name.
* @return {@link OnField} instance targeting the passed field and class.
*/
public OnField getLocationOnField(String clazz, String field) {
FieldDeclarationInfo candidate =
findNodeWithHashHint(node -> node.clazz.equals(clazz), FieldDeclarationInfo.hash(clazz));
if (candidate == null) {
return null;
}
return new OnField(candidate.pathToSourceFile, candidate.clazz, Collections.singleton(field));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ public class FieldDeclarationInfo implements Hashable {
public final Set<ImmutableSet<String>> fields;
/** Flat name of the containing class. */
public final String clazz;
/** Path to source file containing this class. */
public final String pathToSourceFile;

public FieldDeclarationInfo(String clazz) {
public FieldDeclarationInfo(String path, String clazz) {
this.clazz = clazz;
this.pathToSourceFile = path;
this.fields = new HashSet<>();
}

Expand All @@ -26,8 +29,8 @@ public boolean equals(Object o) {
if (!(o instanceof FieldDeclarationInfo)) {
return false;
}
FieldDeclarationInfo info = (FieldDeclarationInfo) o;
return clazz.equals(info.clazz);
FieldDeclarationInfo other = (FieldDeclarationInfo) o;
return clazz.equals(other.clazz);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ public String encMember() {
/**
* Getter for region.
*
* @return region.
* @return region instance.
*/
public Region getRegion() {
return region;
return this.region;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,15 @@ public boolean isOnMethod() {
return type.equals(Type.METHOD);
}

/**
* Checks if region targets a field declaration.
*
* @return true, if region is targeting a field declaration.
*/
public boolean isOnField() {
return type.equals(Type.FIELD);
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down
65 changes: 65 additions & 0 deletions core/src/test/java/edu/ucr/cs/riple/core/CoreTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,16 @@

import com.google.common.base.Preconditions;
import edu.ucr.cs.riple.core.tools.TReport;
import edu.ucr.cs.riple.injector.changes.AddAnnotation;
import edu.ucr.cs.riple.injector.changes.AddMarkerAnnotation;
import edu.ucr.cs.riple.injector.changes.AddSingleElementAnnotation;
import edu.ucr.cs.riple.injector.location.OnField;
import edu.ucr.cs.riple.injector.location.OnMethod;
import edu.ucr.cs.riple.injector.location.OnParameter;
import java.nio.file.Path;
import java.util.List;
import java.util.Set;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -297,6 +303,65 @@ public void deactivateInferenceTest() {
.equals("org.jspecify.nullness.NullUnmarked"));
}

@Test
public void errorInFieldDeclarationForceResolveTest() {
coreTestHelper
.addInputDirectory("test", "fielderrorregion")
.addExpectedReports(
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("f0")), -1),
new TReport(new OnParameter("Bar.java", "test.Bar", "process(java.lang.Object)", 0), 1),
new TReport(new OnField("Foo.java", "test.Foo", Set.of("f4")), 1))
.toDepth(1)
.enableForceResolve()
.start();
Path srcRoot =
coreTestHelper
.getConfig()
.globalDir
.resolve("unittest")
.resolve("src")
.resolve("main")
.resolve("java")
.resolve("test");
Set<AddAnnotation> expectedAnnotations =
Set.of(
new AddMarkerAnnotation(
new OnField(srcRoot.resolve("Foo.java").toString(), "test.Foo", Set.of("f0")),
"javax.annotation.Nullable"),
new AddMarkerAnnotation(
new OnMethod(
srcRoot.resolve("Bar.java").toString(),
"test.Bar",
"process(java.lang.Object)"),
"org.jspecify.nullness.NullUnmarked"),
new AddSingleElementAnnotation(
new OnField(srcRoot.resolve("Foo.java").toString(), "test.Foo", Set.of("f4")),
"SuppressWarnings",
"NullAway",
false),
new AddSingleElementAnnotation(
new OnField(srcRoot.resolve("Foo.java").toString(), "test.Foo", Set.of("f0")),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@nimakarimipour nimakarimipour Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"SuppressWarnings",
"NullAway",
false),
new AddSingleElementAnnotation(
new OnField(srcRoot.resolve("Foo.java").toString(), "test.Foo", Set.of("f2", "f3")),
"SuppressWarnings",
"NullAway.Init",
false),
new AddSingleElementAnnotation(
new OnField(srcRoot.resolve("Foo.java").toString(), "test.Foo", Set.of("f1")),
"SuppressWarnings",
"NullAway.Init",
false));
// todo: change test infrastructure to do the expected added annotations comparison internally
// in the upcoming refactoring cycle.
Assert.assertEquals(
expectedAnnotations, Set.copyOf(coreTestHelper.getConfig().log.getInjectedAnnotations()));
}

@Test
public void staticBlockLocalVariableInitializationTest() {
coreTestHelper
Expand Down
22 changes: 22 additions & 0 deletions core/src/test/resources/fielderrorregion/Bar.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package test;

import javax.annotation.Nullable;

public class Bar {

Object foo = new Object();

public void deref() {}

@Nullable
public Bar process(Object foo) {
this.foo = foo;
receiveNonnull(foo);
return null;
}

public Object receiveNonnull(Object o) {
// just to keep Bar#process:foo @Nonnull
return o;
}
}
29 changes: 29 additions & 0 deletions core/src/test/resources/fielderrorregion/Foo.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package test;

import javax.annotation.Nullable;

public class Foo {
Bar f0 = new Bar().process(null);
Bar f1;
Bar f2, f3;
@Nullable Bar nullableBar;
Bar f4 = nullableBar.process(new Object());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, please find it 970a2ab


Foo() {
this.f2 = new Bar();
}

public void run1() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need both run1 and run2?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nimakarimipour nimakarimipour Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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").

// to prevent annotator making f1, f3 and f4 @Nullable.
f1.deref();
f3.deref();
f4.deref();
}

public void run2() {
// to prevent annotator making f1, f3 and f4 @Nullable.
f1.deref();
f3.deref();
f4.deref();
}
}