Serialization of Type Change Suggestions for Type Violations. #517

nimakarimipour · 2021-12-15T23:47:34Z

Overview

This PR enables NullAway to serialize information on code locations to resolve the reporting errors if they can be resolved via a type change. This information can be used by other tools such as AutoFixer to have an understanding of the source code, code points where violations occurred, and possible solutions. In the example below, NullAway will report the error:
assigning @Nullable null to @Nonnull bar.

class Foo {
     Object bar = new Object();
     
     void baz(){
           this.bar = null;
     }
}

This PR enables NullAway to suggest bar type to be @Nullable to resolve the error and creates two output files. errors.tsv where all reporting errors are stored and fixes.tsv where all the type change suggestions are stored.

errors.tsv: Every row represents a reporting error with the format: MESSAGE_TYPE, MESSAGE, ENCLOSING CLASS, ENCLOSING METHOD
For instance: ASSIGN_FIELD_NULLABLE, assigning nullable null to field bar, Foo, baz
fixes.tsv: Every row represents a type change suggestion with the format: LOCATION, MESSAGE_TYPE, ANNOT
For instance: Field bar of Foo, ASSIGN_FIELD_NULLABLE, @Nullable

To activate the feature above the following flags must be passed to NullAway via ErrorProneFlags:

"-XepOpt:NullAway:SerializeFixMetadata=true"
"-XepOpt:NullAway:FixSerializationConfigPath=configpath"

When -XepOpt:NullAway:SerializeFixMetadata=true is observed in the flags, NullAway will look for a file at FixSerializationConfigPath to activate/deactivate serialization features.

Below is the FixMetadataConfigPath format:

<serialization>
    <suggest activation="true" enclosing="true"/>
    <output>
           "outputdir"
    </output>
    <annotation>
        <nullable>javax.annotation.Nullable</nullable>
        <nonnull>javax.annotation.Nonnull</nonnull>
    </annotation>
</serialization>

If -XepOpt:NullAway:SerializeFixMetadata=true is observed, NullAway will serialize all reporting errors to errors.tsv regardless of flag values in FixSerializationConfig.

Finding enclosing method/class for suggested fixes is costly and not always required, they can be controlled using enclosing attribute in the config file above. If any of the keys in the template config above is not found, the default value will be considered.

Please note, at the places where explicit @Nonnull is observed, NullAway will acknowledge that annotation and will not suggest any type change.

Implementation

All implemented classes reside in the fixserialization package. The goal of this PR is to create SuggestedFixInfo objects which are type change suggestions of elements in the source code. Each of these objects contains a single Location instance and an Annotation . Location instances target an element in the source code. Throughout this PR the only suggested annotation in SuggestedFixInfo instances is @Nullable.

The creation of SuggestedFixInfo objects is implemented through ErrorBuilder. The createErrorDescription signature is changed and now it accepts a fifth argument called nonNullTarget. If this parameter is non-null, the error involved a pseudo-assignment of a @Nullable expression into a @NonNull target, and this parameter is the Symbol for that target. Whenever ErrorBuilder is asked to create an ErrorDescription object, it also creates a SuggestedFixInfo targeting the parameter nonNullTarget to be @Nullable.

…_metadata

nimakarimipour · 2022-01-28T00:42:31Z

Hello @msridhar and @lazaroclapp,

I ran the new version on my benchmarks to make sure everything is working and the tool crashed, I fixed the bug and added a test case: e86e2a3. It seems methods and constructs have different values for ElementKind.

Lazaro, there are a lot of comments on this PR and the GitHub UI is not working really well, for your convenience I rewrote my responses to some of your comments, please find them below:

Side Note on TSV: You mentioned that file names can have tabs inside but this is fine for now, please let me know of you prefer to address it now.
Keeping the config file: In our last meeting we decided to keep the config file, therefore, I did not do anything for this comment.
Make Nullable: You mentioned to make a field (outputdir at FixSerializationConfig) @Nullable for a reason which I did not understand, would you please give me more instructions what I have to do ?

Also you asked why we need to have a control flag on fix serialization, since fix serialization comes with a cost on performance, and in our diagnose phase where we rebuild the project per fix, we do not need to know the list of new fixes at every single build unless we are performing a deep search.

I believe this is ready for another review, sorry for the delay, I did not expect it to take this long :)

lazaroclapp · 2022-01-31T20:08:11Z

Doing another pass over the PR right now, but went over the responses to comments first, and they generally look good to me.

Regarding the three questions in the summary:

Side Note on TSV: You mentioned that file names can have tabs inside but this is fine for now, please let me know of you prefer to address it now.

Fine to leave for later. Maybe worth a comment in the code noting the potential issue, but filenames with [TAB] should be rare.

Keeping the config file: In our last meeting we decided to keep the config file, therefore, I did not do anything for this comment.

Agreed!

Make Nullable: You mentioned to make a field (outputdir at FixSerializationConfig) @Nullable for a reason which I did not understand, would you please give me more instructions what I have to do ?

Responded inline. But, basically outputdir is not initialized in the constructor for the builder, so it should be an @Nullable field, and presumably checked to be non-null at build, throwing some sort of exception related to incorrect object construction if it is or just a Preconditions.checkNotNull(...) if this will only ever be used internally within NullAway.

Also you asked why we need to have a control flag on fix serialization, since fix serialization comes with a cost on performance, and in our diagnose phase where we rebuild the project per fix, we do not need to know the list of new fixes at every single build unless we are performing a deep search.

Makes sense to me!

lazaroclapp

This is basically ready. One issue with the output from tests that I would really want fixed and the rest are a couple nits and questions. Hopefully, this should be a much easier review to address than the last one :)

nullaway/src/main/java/com/uber/nullaway/Config.java

lazaroclapp · 2022-01-31T20:12:13Z

nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java

-import com.uber.nullaway.fixserialization.location.FixLocation;
-import com.uber.nullaway.fixserialization.out.ErrorInfo;
-import com.uber.nullaway.fixserialization.out.SuggestedFixInfo;
+import com.uber.nullaway.fixserialization.SerializationHandler;


Again, probably a different name, but love how much cleaner this is in terms of changes to core NullAway! :)

Renamed to SerializationService, please let me know if you still think I should rename it to something else.

lazaroclapp · 2022-01-31T20:14:33Z

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

+                      .map(
+                          element -> ASTHelpers.getSymbol(getTreesInstance(state).getTree(element)))
+                      .collect(Collectors.toList()))
+              .build();


I think I said this before, but ImmutableList also has a collector, so no need to generate the intermediate mutable list and copy the elements. See my response comment to the original change here.

nullaway/src/main/java/com/uber/nullaway/fixserialization/XMLUtil.java

nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java

lazaroclapp · 2022-01-31T20:43:47Z

nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java

+                "-XepOpt:NullAway:SerializeFixMetadata=true",
+                "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath))
+        .addSourceLines(
+            "com/uber/android/Super.java",


Why Super.java?

lazaroclapp · 2022-01-31T20:45:40Z

nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java

+            "     return h;",
+            "   }",
+            "   Object test_param(@Nullable String o) {",
+            "   // BUG: Diagnostic contains: passing @Nullable",


Match indentation of the following line.

lazaroclapp · 2022-01-31T20:45:54Z

nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java

+            "package com.uber;",
+            "import java.util.ArrayList;",
+            "public class Child extends Super<String>{",
+            "   public void newSideEffect(ArrayList<String> op) {",
            "   // BUG: Diagnostic contains: passing @Nullable",


Match indentation of the following line.

…ggest

nimakarimipour · 2022-02-08T20:58:27Z

Hello @lazaroclapp, thank you very much for your comments. This is ready for another review, with best regards.

lazaroclapp

LGTM. Consider fixing the one typo below before landing, but otherwise this looks ready to me! 🎉 🚀

lazaroclapp · 2022-02-08T23:28:14Z

nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java

+    if (output.size() != 0) {
+      errorMessage
+          .append(output.size())
+          .append(" unexpected fix(s) suggestions were found:")


Doesn't need the (s), "fix suggestions" is already plural.

nimakarimipour · 2022-02-09T03:07:14Z

@msridhar and @lazaroclapp Thank you very much for your comments, this was super educating for me.

Following GH-517 If `serializeFixMetadata` is active, `NullAway` will serialize suggested type changes (currently just adding `@Nullable`). In the `FixSerializationConfig` file we can set a custom annotation that `NullAway` will use in the output serializations. However, this process can be simplified by just asking `NullAway` to serialize type change locations with a default annotation name, any tool processing this output can use their own preferred annotation. After this PR the following entity will be removed from the config `.xml` file: ```xml <annotation> <nonnull> ... </nonnull> <nullable> ... </nullable> </annotation ``` And a sample out below: ``` Field bar of Foo, ASSIGN_FIELD_NULLABLE, @nullable ``` Will be changed to simply: ``` Field bar of Foo, ASSIGN_FIELD_NULLABLE, nullable ```

nimakarimipour added 30 commits June 20, 2021 12:20

added updated config version

a55d27f

Merge branch 'master' of https://github.com/uber/NullAway into method…

a3a45e2

…_metadata

updated to lastest upstream main

a63fe2a

updated config

572725a

updated config

64c9daa

update

8dbc0d7

changed structure

eeb94f3

update

b7e395a

update

ba20acd

update

d487942

update

d8d320b

update

53d3722

update

79dcea5

update

1a01457

update

047ebdf

update

c8875ab

update

dac44ae

update

b9343d2

update

61b6b13

update

0577229

update

956d3c5

update

337cf92

update

2a15052

refactored

53d4873

refactored

1e949b6

update

cf63081

update

4c354a1

update

f83e92d

update

d8a9c72

update

4136794

nimakarimipour added 9 commits January 27, 2022 14:31

Chgangd to private

9dfa553

Moved to static method in interface

e398a94

Moved to static method in interface

275c31e

Changd from public to private

7271a3e

Changd Name to Cursor

89b2f5d

removed explicit toString()

99061e7

Fixed a critical bug.

e86e2a3

Removed unnecessary replaceAll

3e8071c

Renamd flag variable

e649145

nimakarimipour requested a review from lazaroclapp January 28, 2022 00:42

lazaroclapp reviewed Jan 31, 2022

View reviewed changes

nimakarimipour added 7 commits February 8, 2022 11:44

Merge branch 'master' of https://github.com/uber/NullAway into fix_su…

f69c5cf

…ggest

Merged with upstream and minor javadoc fixes

ccb5dfc

Fixed error message

5ef177a

Updated javadoc

798b7aa

Fixed exception stack frame

168b88a

Resolved test codes comments

b144824

Renamed to SerializationService

9499ce3

nimakarimipour requested a review from lazaroclapp February 8, 2022 20:58

lazaroclapp approved these changes Feb 8, 2022

View reviewed changes

Fixed comments plural

0602a68

msridhar merged commit 9dbdd65 into uber:master Feb 9, 2022

nimakarimipour mentioned this pull request Feb 23, 2022

Enhanced Serialization Test Infrastructure #579

Merged

nimakarimipour deleted the fix_suggest branch March 21, 2022 23:56

nimakarimipour mentioned this pull request Jul 10, 2022

Remove nullable annotation configuration in fix serialization. #621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialization of Type Change Suggestions for Type Violations. #517

Serialization of Type Change Suggestions for Type Violations. #517

nimakarimipour commented Dec 15, 2021 •

edited by lazaroclapp

Loading

nimakarimipour commented Jan 28, 2022 •

edited

Loading

lazaroclapp commented Jan 31, 2022

lazaroclapp left a comment

lazaroclapp Jan 31, 2022

nimakarimipour Feb 8, 2022

lazaroclapp Jan 31, 2022

lazaroclapp Jan 31, 2022

nimakarimipour Feb 8, 2022

lazaroclapp Jan 31, 2022

nimakarimipour Feb 8, 2022

lazaroclapp Jan 31, 2022

nimakarimipour Feb 8, 2022

nimakarimipour commented Feb 8, 2022

lazaroclapp left a comment

lazaroclapp Feb 8, 2022

nimakarimipour commented Feb 9, 2022

Serialization of Type Change Suggestions for Type Violations. #517

Serialization of Type Change Suggestions for Type Violations. #517

Conversation

nimakarimipour commented Dec 15, 2021 • edited by lazaroclapp Loading

Overview

Implementation

nimakarimipour commented Jan 28, 2022 • edited Loading

lazaroclapp commented Jan 31, 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 commented Feb 8, 2022

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour commented Feb 9, 2022

nimakarimipour commented Dec 15, 2021 •

edited by lazaroclapp

Loading

nimakarimipour commented Jan 28, 2022 •

edited

Loading