Augment error serializarion info #643

nimakarimipour · 2022-08-26T00:24:16Z

This PR augments error information serialized by NullAway. At current state, if SerializeFixMetada is active, all reporting errors will be serialized to errors.tsv output file where each row has the format below:

MESSAGE_TYPE    MESSAGE   CLASS   METHOD

The above information does not contain any information regarding the element which involved in a pseudo-assignment of a @Nullable expression into a @NonNull. This information is crucial to detect origins of the error and other computable features.

With this PR, the format of the errors.tsv is changed to below to serialize all the required information to denote the involved element in the error:

message_type    message    enc_class    enc_method    kind    class   method     param    index    uri

This PR involves other small refactoring (mostly renaming) as it was necessary to reuse the existing methods to produce the desired outputs.

lazaroclapp

Generally looks good to me, though I think the testing infra has a couple bugs (see comments below).

One other thing I wonder is about the headers of the TSV. "class" and "method" are very generic, and they aren't the class or method where the error is happening, but the class and method referenced at the point the error is happening... maybe "target_class", "target_method", etc? Not sure if there is a better name than that, since it's sometimes the method being called and sometimes the method being overridden, etc. (cc: @msridhar )

lazaroclapp · 2022-08-26T17:21:31Z

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

@@ -50,16 +77,19 @@ public boolean equals(Object o) {
    }
    ErrorDisplay that = (ErrorDisplay) o;
    return type.equals(that.type)
-        // To increase readability, a shorter version of the actual message might be present in the
-        // expected output of tests.


Why are we removing this comment?

Thank you for the notice, added it back
e9182db

lazaroclapp · 2022-08-26T17:24:05Z

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

  }

  @Override
  public int hashCode() {
-    return Objects.hash(type, message, method, clazz);
+    return Objects.hash(type, message, encMethod, encClass);


Now that this has a lot more fields, shouldn't those be part of its hashCode()? This was all fields before, no?

This was all fields before, but according to the usage of these classes which are mostly used in test cases and are compared to at most 3/4 other instances, I did not see it necessary to include all fields. But you are right, it is better to be more precise about the hash.

04b3664

lazaroclapp · 2022-08-26T17:25:53Z

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

+                "test(java.lang.Object)",
+                "METHOD",
+                "com.uber.Super",
+                "test(java.lang.Object",


Isn't there a missing ')' here? And shouldn't the test fail because of it?

yes it should fail, updated the test.

lazaroclapp · 2022-08-26T17:35:34Z

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

+        && encMethod.equals(that.encMethod)
+        && encClass.equals(that.encClass)
+        && kind.equals(that.kind)
+        && clazz.equals(that.clazz)


This is missing comparing this.method, which I believe is why that test case with the missing parenthesis succeeds...

Thank you very much for noticing this, I just used the intellij generated code. updated it: 258a341

lazaroclapp · 2022-08-26T17:36:23Z

nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java

@@ -25,23 +25,23 @@
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.util.TreePath;
 import com.uber.nullaway.ErrorMessage;
-import com.uber.nullaway.fixserialization.location.FixLocation;
+import com.uber.nullaway.fixserialization.location.SymbolLocation;
 import java.util.Objects;

 /** Stores information suggesting adding @Nullable on an element in source code. */
 public class SuggestedNullableFixInfo {

  /** FixLocation of the target element in source code. */


This comment is outdated given the renaming.

ddb2afa

Also ran git grep FixLocation, there is no usage of FixLocation anymore.

lazaroclapp · 2022-08-26T17:40:17Z

nullaway/src/main/java/com/uber/nullaway/fixserialization/location/SymbolLocation.java

-        + '\t'
-        + "index"
-        + '\t'
+    return "kind" + '\t' + "class" + '\t' + "method" + '\t' + "param" + '\t' + "index" + '\t'


Was this auto-formatted? I think it looks better in the previous/existing code (unless GJF recommends this style)

Since Java 1.8 there's String#join(CharSequence delimiter, CharSequence... elements), so another option would be:

String.join("\t", "kind", "class", "method", "param", "index", "uri")

Right. @nimakarimipour, probably a good idea to do this in all the places we are performing these tab-separated string concatenations.

a8fdd11

@Stephan202 thank you for the suggestion :)
@lazaroclapp replaced all multiple calls with single join call.

nimakarimipour · 2022-08-26T20:58:15Z

@lazaroclapp Resolved all comments and changed headers to use (target_kind, target_class, target_method).
ab046f9
This is ready for another review.

Regarding changing null\tnull\t... to \t\t..., that requires escaping to be picked by split as individual columns and at this moment I think it may be simpler to just go with null\tnull\t..., please let me know if you prefer the other approach and escaping. Thanks.

lazaroclapp

LGTM 🚀 !

@nonnull

#### ~~This PR is Built upon GH-51~~ GH-51 is landed. ----- This PR resolves #47 by updating `Error` instance creation according to the latest changes. In the new release of NullAway (Specifically [PR643](uber/NullAway#643)), information of the non-null target (the element involved in the pseudo-assignment of a @nullable expression into a @nonnull and causing the error) is serialized along the reporting errors in the `errors.tsv`. Header of `errors.tsv` files is changed from below: ``` MESSAGE_TYPE MESSAGE CLASS METHOD ``` to: ``` message_type message enc_class enc_method target_kind target_class target_method param index uri ``` This requires updates on how `Error` instances are created due to change in `errors.tsv` files contents. ----- ~~Note: tests for `core` module are temporarily excluded in this PR for CI as they are only compatible with NullAway verion `0.9.10`.~~ _All_ tests are included since `NullAway 0.9.10` is released.

nimakarimipour added 2 commits August 25, 2022 16:41

add nonnull target info to error serialization

d9d1726

Merge branch 'master' into nimak/augment-error-serializarion

c469e7d

nimakarimipour added the enhancement label Aug 26, 2022

nimakarimipour requested a review from lazaroclapp August 26, 2022 00:24

nimakarimipour self-assigned this Aug 26, 2022

lazaroclapp reviewed Aug 26, 2022

View reviewed changes

nimakarimipour added 6 commits August 26, 2022 13:23

undo comment deletion

e9182db

update hash function to include all fields

04b3664

fix bug in test infra / update equals defitinition for ErrorDisplay

258a341

update comment, replace FixLocation with SymbolLocation

ddb2afa

replace all multiple \t concatenation with single String.join call

a8fdd11

update headers with target_ prefix

ab046f9

nimakarimipour requested a review from lazaroclapp August 26, 2022 20:58

lazaroclapp approved these changes Aug 26, 2022

View reviewed changes

nimakarimipour merged commit 658f4c3 into uber:master Aug 26, 2022

nimakarimipour deleted the nimak/augment-error-serializarion branch August 26, 2022 22:10

This was referenced Aug 29, 2022

Update to upcoming new release of NullAway 0.9.10 ucr-riple/NullAwayAnnotator#47

Closed

Prepare for upcoming version of NullAway - 0.9.10 ucr-riple/NullAwayAnnotator#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Augment error serializarion info #643

Augment error serializarion info #643

nimakarimipour commented Aug 26, 2022

lazaroclapp left a comment

lazaroclapp Aug 26, 2022

nimakarimipour Aug 26, 2022

lazaroclapp Aug 26, 2022

nimakarimipour Aug 26, 2022

lazaroclapp Aug 26, 2022

nimakarimipour Aug 26, 2022

lazaroclapp Aug 26, 2022

nimakarimipour Aug 26, 2022

lazaroclapp Aug 26, 2022

nimakarimipour Aug 26, 2022

lazaroclapp Aug 26, 2022

Stephan202 Aug 26, 2022

lazaroclapp Aug 26, 2022

nimakarimipour Aug 26, 2022

nimakarimipour commented Aug 26, 2022

lazaroclapp left a comment

Augment error serializarion info #643

Augment error serializarion info #643

Conversation

nimakarimipour commented Aug 26, 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

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 Aug 26, 2022

lazaroclapp left a comment

Choose a reason for hiding this comment