Give line numbers for uninitialized fields when reporting error on an initializer #380

Lycheeicy · 2020-01-09T01:09:35Z

This adds the line numbers for uninitialized fields when reporting error on an initializer. As a response to issue #95.

Summary of changes:

-Edit function errMsgForInitializer() in ErrorBuilder.
-Create a new function GetLineforField() which gives back a source line number.
-Adjust the expected error messages in the test cases.

claassistantio · 2020-01-09T01:10:21Z

All committers have signed the CLA.

lazaroclapp

Much better PR!

I'd honestly not be opposed to accepting it as is. But I did have a few minor comments worth at least considering, plus I want to give @msridhar a chance to look over this too.

lazaroclapp · 2020-01-09T21:39:53Z

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

-  static String errMsgForInitializer(Set<Element> uninitFields) {
+  static int GetLineforField(Element uninitField, VisitorState state) {
+    Tree fieldTree = getTreesInstance(state).getTree(uninitField);
+    DiagnosticPosition position = (DiagnosticPosition) fieldTree;


I recommend adding a comment over this line saying that we expect Tree to be JCTree and thus implement DiagnosticPosition for statements in Java code.

Also, in case that assumption ever breaks, do you guys think we should crash with a cast exception or just return -1 or some other dummy value? My thoughts on this are: a) if we are just printing out the value, then returning -1 makes this code more robust, but b) if we ever use this method for something else where we are acting on the line number info, then we could just be hiding the issue. I think I vote on leaving the code itself as is (with the comment), and just having it crash early if it ever sees a Tree it can't cast to DiagnosticPosition, but I thought it was worth to bring up the issue explicitly and get your thoughts on it @Lycheeicy and @msridhar :)

I think we should just fail fast, which is what the current code will do.

Another possible failure mode is getTreesInstance(state).getTree(uninitField) returning null, e.g., if uninitField comes from bytecode. We should add a null check and throw a RuntimeException with some appropriate message if that happens.

Thanks for advice, I will add the comment as well as a null check later.

lazaroclapp · 2020-01-09T21:41:35Z

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

+    return source.getLineNumber(position.getStartPosition());
+  }
+
+  static String errMsgForInitializer(Set<Element> uninitFields, VisitorState state) {


Technically not needed for non-public methods, but a Javadoc comment makes sense to me here (not needed for GetLineforField, since the name is fairly self-documenting, although see comment above).

Yes, makes sense. I think this method existed before this PR, but good to add some docs now.

Sure, that was our issue, then, not a new one, but it's still worth fixing/improving :)

Sounds great! I will add a javadoc comment for errMsgForInitializer!

lazaroclapp · 2020-01-09T21:42:30Z

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

@@ -302,12 +307,39 @@ boolean symbolHasSuppressWarningsAnnotation(Symbol symbol, String suppression) {
    return false;
  }

-  static String errMsgForInitializer(Set<Element> uninitFields) {
+  static int GetLineforField(Element uninitField, VisitorState state) {


nit: getLineNumForField would be even more self-documenting as the method name, and proper camel case.

Yes we need to use camel case here (getLineNumForField). Also, I believe this will work for any Element coming from source code, not just fields, right? In which case, we can name the method getLineNumForElement instead.

Yes, I will change the method name to getLineNumForElement, that make great sense!

lazaroclapp · 2020-01-09T21:44:01Z

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

+    return source.getLineNumber(position.getStartPosition());
+  }
+
+  static String errMsgForInitializer(Set<Element> uninitFields, VisitorState state) {
    String message = "initializer method does not guarantee @NonNull ";


Not sure if this matters for modern Java compilers but... given the amount of concatenations we are doing here, is there any advantage in using a StringBuilder?

Yeah I think using StringBuilder is a bit cleaner, rather than all this string concatenation.

Good idea, I will use the StringBuilder instead.

lazaroclapp · 2020-01-09T21:48:35Z

nullaway/src/test/resources/com/uber/nullaway/testdata/CheckFieldInitPositiveCases.java

@@ -32,7 +32,7 @@

    Object f;

-    // BUG: Diagnostic contains: initializer method does not guarantee @NonNull field f is
+    // BUG: Diagnostic contains: initializer method does not guarantee @NonNull field f(Line:33) is


This is going to make adding test cases to these files a bit harder. But: 1) we do need to test the new line number printing code too, so using existing test cases for it makes sense, 2) I am actually fine with discouraging this kind of test cases going forward vs proper unit tests via .addLines(...), etc :)

p.s. nothing to change here, just a note.

Yeah, I agree. This will discourage us from messing with this older, worse way of writing tests in the future 😄

Yes, thank you for pointing that out.

msridhar

Looks great overall! Just a few things to fix before we can land this

msridhar · 2020-01-09T23:00:45Z

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

@@ -302,12 +307,39 @@ boolean symbolHasSuppressWarningsAnnotation(Symbol symbol, String suppression) {
    return false;
  }

-  static String errMsgForInitializer(Set<Element> uninitFields) {
+  static int GetLineforField(Element uninitField, VisitorState state) {


Yes we need to use camel case here (getLineNumForField). Also, I believe this will work for any Element coming from source code, not just fields, right? In which case, we can name the method getLineNumForElement instead.

msridhar · 2020-01-09T23:02:45Z

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

-  static String errMsgForInitializer(Set<Element> uninitFields) {
+  static int GetLineforField(Element uninitField, VisitorState state) {
+    Tree fieldTree = getTreesInstance(state).getTree(uninitField);
+    DiagnosticPosition position = (DiagnosticPosition) fieldTree;


I think we should just fail fast, which is what the current code will do.

Another possible failure mode is getTreesInstance(state).getTree(uninitField) returning null, e.g., if uninitField comes from bytecode. We should add a null check and throw a RuntimeException with some appropriate message if that happens.

msridhar · 2020-01-09T23:03:21Z

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

+    return source.getLineNumber(position.getStartPosition());
+  }
+
+  static String errMsgForInitializer(Set<Element> uninitFields, VisitorState state) {


Yes, makes sense. I think this method existed before this PR, but good to add some docs now.

msridhar · 2020-01-09T23:04:23Z

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

+    return source.getLineNumber(position.getStartPosition());
+  }
+
+  static String errMsgForInitializer(Set<Element> uninitFields, VisitorState state) {
    String message = "initializer method does not guarantee @NonNull ";


Yeah I think using StringBuilder is a bit cleaner, rather than all this string concatenation.

msridhar · 2020-01-09T23:05:03Z

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

+      message +=
+          "field "
+              + uninitField.toString()
+              + "(Line:"


Right now the line number gets formatted as (Line:XX). I think that (line XX) would look better and be more consistent. @lazaroclapp?

I agree with that, I will do this kind of change later.

msridhar · 2020-01-09T23:05:38Z

nullaway/src/test/resources/com/uber/nullaway/testdata/CheckFieldInitPositiveCases.java

@@ -32,7 +32,7 @@

    Object f;

-    // BUG: Diagnostic contains: initializer method does not guarantee @NonNull field f is
+    // BUG: Diagnostic contains: initializer method does not guarantee @NonNull field f(Line:33) is


Yeah, I agree. This will discourage us from messing with this older, worse way of writing tests in the future 😄

lazaroclapp · 2020-01-10T04:15:03Z

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

-    DiagnosticPosition position = (DiagnosticPosition) fieldTree;
+    if (fieldTree == null)
+      throw new RuntimeException(
+          "When getting the line number for uninitialized field, can't get the fieldTree from the element.");


Given that this should work for arbitrary elements now, I'd change both this message and the variable name from fieldTree to just tree.

Yes, that make sense.

lazaroclapp · 2020-01-10T04:17:56Z

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

    TreePath path = state.getPath();
    JCCompilationUnit compilation = (JCCompilationUnit) path.getCompilationUnit();
    JavaFileObject file = compilation.getSourceFile();
    DiagnosticSource source = new DiagnosticSource(file, null);
    return source.getLineNumber(position.getStartPosition());
  }

+  // Generate the message for uninitialized fields, including the line number for fields.


Not really in JavaDoc format (see https://www.baeldung.com/javadoc ), but honestly it's enough by me.

Got it, I will change it to be better.

msridhar

Just a couple more small things

msridhar · 2020-01-10T05:12:49Z

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

-    DiagnosticPosition position = (DiagnosticPosition) fieldTree;
+    if (fieldTree == null)
+      throw new RuntimeException(
+          "When getting the line number for uninitialized field, can't get the fieldTree from the element.");


msridhar · 2020-01-10T05:14:02Z

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

+      uninitField = uninitFields.iterator().next();
+      message.append("field ");
+      message.append(uninitField.toString());
+      message.append("(Line ");


Sorry to be picky, but can we have a space before the ( and a lowercase l? So it will read, e.g., field f (line 33)

It's good to be picky as it makes the code better! I will adjust the message!

msridhar

🎉

msridhar · 2020-01-12T17:25:38Z

Thanks for the first contribution @Lycheeicy!

Lycheeicy · 2020-01-12T17:42:28Z

Thanks for the first contribution @Lycheeicy!

My pleasure!

Lycheeicy added 4 commits January 6, 2020 22:50

Adding the location for uninitialized fields in error report

2d6197a

fix some bugs and add some head file

24b5dcc

Update the expected error messages in the test cases.

72a41d9

Fix the bug in the test cases.

5522f02

lazaroclapp reviewed Jan 9, 2020

View reviewed changes

lazaroclapp requested a review from msridhar January 9, 2020 21:50

msridhar requested changes Jan 9, 2020

View reviewed changes

Adjusted the codes according to the advice

a336818

lazaroclapp reviewed Jan 10, 2020

View reviewed changes

msridhar requested changes Jan 10, 2020

View reviewed changes

Lycheeicy added 2 commits January 10, 2020 11:47

Adjust the comment to the javadoc format and some little changes

57dce66

Fixed the mismatch about the expected messages

8c8b2d7

msridhar approved these changes Jan 12, 2020

View reviewed changes

msridhar merged commit 71b7ec8 into uber:master Jan 12, 2020

Give line numbers for uninitialized fields when reporting error on an initializer #380

Give line numbers for uninitialized fields when reporting error on an initializer #380

Conversation

Lycheeicy commented Jan 9, 2020 • edited

claassistantio commented Jan 9, 2020 • edited

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

lazaroclapp Jan 9, 2020 • edited

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

msridhar 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

msridhar 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

msridhar left a comment

Choose a reason for hiding this comment

msridhar commented Jan 12, 2020

Lycheeicy commented Jan 12, 2020

Lycheeicy commented Jan 9, 2020 •

edited

claassistantio commented Jan 9, 2020 •

edited

lazaroclapp Jan 9, 2020 •

edited