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

JSpecify: handle incorrect method parameter nullability for method reference #845

Merged
merged 7 commits into from
Oct 15, 2023
Merged

JSpecify: handle incorrect method parameter nullability for method reference #845

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0b49a42
changes for the test case: testForMethodReferenceInAnAssignment
akulk022 Oct 12, 2023
06d157f
updating the logic for the test case:testForMethodReferenceInAnAssign…
akulk022 Oct 13, 2023
77c7c4f
reverting accidental commit
akulk022 Oct 13, 2023
7c891d3
move null check
msridhar Oct 13, 2023
6f4505c
rewrite complex nested conditional expression and add comments
msridhar Oct 13, 2023
3f5c6cf
clarify comment
msridhar Oct 13, 2023
6092a56
more diagnostic
msridhar Oct 13, 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
11 changes: 8 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.ConditionalExpressionTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MemberReferenceTree;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
Expand Down Expand Up @@ -876,9 +877,13 @@ public static Nullness getGenericMethodParameterNullness(
int parameterIndex,
Symbol.MethodSymbol method,
Symbol enclosingSymbol,
@Nullable MemberReferenceTree memberReferenceTree,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool that this works! But it's a bit unclean to pass in a MemberReferenceTree directly. Instead, how about we have a parameter @Nullable Type enclosingType, and at the caller you pass in the type of the MemberReferenceTree if the tree is non-null. Then in this method, if enclosingType == null, we set it to be getTypeForSymbol(enclosingSymbol, state).

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will also need to update the Javadoc for this method to document the new parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I have a better idea. Just below this method (line 909), there is another overload of getGenericMethodParameterNullness() that takes a Type rather than a Symbol. How about we not modify this method, and instead at the call site just invoke the other method if we have a MemberReferenceTree? Right now we'll have to do a null check on the Type at the caller, but I am also open to making the parameter of the other method @Nullable and doing the check there.

Copy link
Collaborator Author

@akulk022 akulk022 Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if we have a MemberReferenceTree!=null, it's type will also not be null right? if I understand correctly is this what you're suggesting?, the place where we call the method from NullAway.java (line 734) to be something like:

if (isOverriddenMethodAnnotated) {
      for (int i = 0; i < superParamSymbols.size(); i++) {
        overriddenMethodArgNullnessMap[i] =
            Nullness.paramHasNullableAnnotation(overriddenMethod, i, config)
                ? Nullness.NULLABLE
                : (config.isJSpecifyMode()
                    ? (memberReferenceTree != null)
                        ? GenericsChecks.getGenericMethodParameterNullness(
                            i,
                            overriddenMethod,
                            ASTHelpers.getType(memberReferenceTree),
                            state,
                            config)
                        : GenericsChecks.getGenericMethodParameterNullness(
                            i,
                            overriddenMethod,
                            overridingParamSymbols.get(i).owner.owner,
                            state,
                            config)
                    : Nullness.NONNULL);
      }
    }

This does make that line very complicated but then we don't have to change any method signatures. Maybe I misunderstood and what you're suggesting is even simpler

VisitorState state,
Config config) {
Type enclosingType = getTypeForSymbol(enclosingSymbol, state);
Type enclosingType =
(memberReferenceTree != null)
? ASTHelpers.getType(memberReferenceTree)
: getTypeForSymbol(enclosingSymbol, state);
if (enclosingType == null) {
// we have no additional information from generics, so return NONNULL (presence of a @Nullable
// annotation should have been handled by the caller)
Expand All @@ -889,8 +894,8 @@ public static Nullness getGenericMethodParameterNullness(

/**
* Just like {@link #getGenericMethodParameterNullness(int, Symbol.MethodSymbol, Symbol,
* VisitorState, Config)}, but takes the enclosing {@code Type} rather than the enclosing {@code
* Symbol}.
* MemberReferenceTree, VisitorState, Config)}, but takes the enclosing {@code Type} rather than
* the enclosing {@code Symbol}.
*
* @param parameterIndex index of the parameter
* @param method the generic method
Expand Down
1 change: 1 addition & 0 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,7 @@ private Description checkParamOverriding(
i,
overriddenMethod,
overridingParamSymbols.get(i).owner.owner,
memberReferenceTree,
state,
config)
: Nullness.NONNULL);
Expand Down
3 changes: 1 addition & 2 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,8 +429,7 @@ public void testForMethodReferenceInAnAssignment() {
" return o.toString();",
" }",
" static void testPositive() {",
" // TODO: we should report an error here, since Test::foo cannot take",
" // a @Nullable parameter. we don't catch this yet",
" // BUG: Diagnostic contains: referenced method is @NonNull",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now, the full error message we emit here is:

warning: [NullAway] parameter o of referenced method is @NonNull, but parameter in functional interface method com.uber.Test.A.function(T1) is @Nullable

It'd be nice if we could find a way to print the functional interface method type so it refers to the annotated type arguments rather than the T1 type variable.

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 agree, I'll try and find a way to do that

Copy link
Collaborator

Choose a reason for hiding this comment

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

After further investigation getting better error messages needs more thought. I'll file a follow-up issue but let's not hold up this PR on this.

" A<@Nullable Object> p = Test::foo;",
" }",
" static void testNegative() {",
Expand Down