From c42b0f789bbea95c0174c404fc2f5f9360f82cb2 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 25 Aug 2023 13:33:44 -0700 Subject: [PATCH] Ensure castToNonNull insertion/removal suggested fixes do not remove comments (#815) We now preserve the original source code of the AST node for which we are adding or removing a cast, ensuring that comments within the node are preserved. --- .../java/com/uber/nullaway/ErrorBuilder.java | 9 ++- .../nullaway/NullAwayAutoSuggestTest.java | 79 +++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index 5188f823b1..fb7aee6f70 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -201,7 +201,7 @@ private Description.Builder addSuggestedSuppression( case ASSIGN_FIELD_NULLABLE: case SWITCH_EXPRESSION_NULLABLE: if (config.getCastToNonNullMethod() != null && canBeCastToNonNull(suggestTree)) { - builder = addCastToNonNullFix(suggestTree, builder); + builder = addCastToNonNullFix(suggestTree, builder, state); } else { // When there is a castToNonNull method, suggestTree is set to the expression to be // casted, which is not suppressible. For simplicity, we just always recompute the @@ -356,7 +356,8 @@ private boolean canBeCastToNonNull(Tree tree) { } } - private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Builder builder) { + private Description.Builder addCastToNonNullFix( + Tree suggestTree, Description.Builder builder, VisitorState state) { final String fullMethodName = config.getCastToNonNullMethod(); if (fullMethodName == null) { throw new IllegalStateException("cast-to-non-null method not set"); @@ -364,7 +365,7 @@ private Description.Builder addCastToNonNullFix(Tree suggestTree, Description.Bu // Add a call to castToNonNull around suggestTree: final String[] parts = fullMethodName.split("\\."); final String shortMethodName = parts[parts.length - 1]; - final String replacement = shortMethodName + "(" + suggestTree.toString() + ")"; + final String replacement = shortMethodName + "(" + state.getSourceForNode(suggestTree) + ")"; final SuggestedFix fix = SuggestedFix.builder() .replace(suggestTree, replacement) @@ -390,7 +391,7 @@ private Description.Builder removeCastToNonNullFix( invTree, suggestTree)); // Remove the call to castToNonNull: final SuggestedFix fix = - SuggestedFix.builder().replace(invTree, suggestTree.toString()).build(); + SuggestedFix.builder().replace(invTree, state.getSourceForNode(suggestTree)).build(); return builder.addFix(fix); } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java index d025fa9a7c..03ae7e35a3 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java @@ -22,6 +22,8 @@ package com.uber.nullaway; +import static com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH; + import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.ErrorProneFlags; import com.sun.source.tree.Tree; @@ -215,6 +217,38 @@ public void removeUnnecessaryCastToNonNullFromLibraryModel() throws IOException .doTest(); } + @Test + public void removeUnnecessaryCastToNonNullMultiLine() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "class Test {", + " static class Foo { Object getObj() { return new Object(); } }", + " Object test1(Foo f) {", + " return castToNonNull(f", + " // comment that should not be deleted", + " .getObj());", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "class Test {", + " static class Foo { Object getObj() { return new Object(); } }", + " Object test1(Foo f) {", + " return f", + " // comment that should not be deleted", + " .getObj();", + " }", + "}") + .doTest(TEXT_MATCH); + } + @Test public void suggestSuppressionOnMethodRef() throws IOException { makeTestHelper() @@ -258,6 +292,51 @@ public void suggestSuppressionOnMethodRef() throws IOException { } @Test + public void suggestCastToNonNullPreserveComments() throws IOException { + makeTestHelper() + .addInputLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " Object x = new Object();", + " static class Foo { @Nullable Object getObj() { return null; } }", + " Object test1(Foo f) {", + " return f", + " // comment that should not be deleted", + " .getObj();", + " }", + " void test2(Foo f) {", + " x = f.getObj(); // comment that should not be deleted", + " }", + " Object test3(Foo f) {", + " return f./* keep this comment */getObj();", + " }", + "}") + .addOutputLines( + "out/Test.java", + "package com.uber;", + "import static com.uber.nullaway.testdata.Util.castToNonNull;", + "", + "import javax.annotation.Nullable;", + "class Test {", + " Object x = new Object();", + " static class Foo { @Nullable Object getObj() { return null; } }", + " Object test1(Foo f) {", + " return castToNonNull(f", + " // comment that should not be deleted", + " .getObj());", + " }", + " void test2(Foo f) {", + " x = castToNonNull(f.getObj()); // comment that should not be deleted", + " }", + " Object test3(Foo f) {", + " return castToNonNull(f./* keep this comment */getObj());", + " }", + "}") + .doTest(TEXT_MATCH); + } + public void suggestInitSuppressionOnConstructor() throws IOException { makeTestHelper() .addInputLines(