Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure castToNonNull insertion/removal suggested fixes do not remove comments #815

Merged
merged 5 commits into from
Aug 25, 2023
Merged

Ensure castToNonNull insertion/removal suggested fixes do not remove comments #815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e894a84
WIP
msridhar Aug 22, 2023
e16779a
Merge branch 'master' into manu/casttononnull-fix-no-comment-delete
msridhar Aug 25, 2023
b3e3056
fix
msridhar Aug 25, 2023
b8cd9ac
Merge branch 'master' into manu/casttononnull-fix-no-comment-delete
msridhar Aug 25, 2023
74228e6
more tests
msridhar Aug 25, 2023
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: 5 additions & 4 deletions nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -356,15 +356,16 @@ 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");
}
// 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)
Expand All @@ -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);
}

Expand Down
79 changes: 79 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayAutoSuggestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens in the nonNullF = nullVal; // some comment case? Just nonNullF = castToNonNull(nullVal); // some comment, I assume?

Is there any case you can imagine where the comment ends up inside the castToNonNull(...) call like here, but without a newline between the comment and )?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add a couple more tests. We could also have castToNonNull(f./* comment */getObj())

" .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(
Expand Down