uber / NullAway

A tool to help eliminate NullPointerExceptions (NPEs) in your Java code with low build-time overhead
MIT License
3.63k stars 293 forks source link

Improve documentation for LibraryModels usage with overloaded methods #1016

Open moaxcp opened 2 months ago

moaxcp commented 2 months ago

I am running into an issue where a LibraryModels is not detecting a null parameter in an overloaded method.

I have created a complete example project for this issue here.

This is an issue with version 0.11.1.

These are the overloaded methods from the external-module project.

public class Check {
    public String method(String p) {
        return null;
    }

    public String method(String p, String p1) {
        return null;
    }
}

The model returns both methods.

    private MethodRef method1 = MethodRef.methodRef("com.github.moaxcp.nullaway.external.Check", "method(java.lang.String)");
    private MethodRef method2 = MethodRef.methodRef("com.github.moaxcp.nullaway.external.Check", "method(java.lang.String, java.lang.String)");

    @Override
    public ImmutableSetMultimap<MethodRef, Integer> nonNullParameters() {
        return ImmutableSetMultimap.<MethodRef, Integer>builder()
                .put(method1, 0)
                .put(method2, 0)
                .build();
    }

The methods are called in the example-project.

public class Failure {
    public static void main(String... args) {
        Check check = new Check();

        check.method(null, null);
        check.method(null);
    }
}

This results in a compile error for method(String) only, but I expect an error for method(String, String) as well.

> Task :example-project:compileJava FAILED
/home/john/projects/nullaway-example/example-project/src/main/java/com/github/moaxcp/nullaway/project/Failure.java:10: error: [NullAway] passing @Nullable parameter 'null' where @NonNull is required
        check.method(null);
                     ^
    (see http://t.uber.com/nullaway )
1 error

When I fix the error the class compiles but it shouldn't.

check.method("value");
> Task :external-module:compileJava UP-TO-DATE
> Task :nullaway-library-models:compileJava UP-TO-DATE
> Task :nullaway-library-models:processResources NO-SOURCE
> Task :nullaway-library-models:classes UP-TO-DATE
> Task :nullaway-library-models:jar UP-TO-DATE
> Task :example-project:compileJava
> Task :example-project:processResources NO-SOURCE
> Task :example-project:classes

BUILD SUCCESSFUL in 753ms
4 actionable tasks: 1 executed, 3 up-to-date
4:40:50 PM: Execution finished 'classes'.
moaxcp commented 2 months ago

I figured out the issue. The newer javadoc says "signature is a method name plus argument types, with no spaces between the argument types".

msridhar commented 2 months ago

@moaxcp glad you figured this out. Is there a place where you think we could document this such that it would be easier to find?

moaxcp commented 2 months ago

@msridhar Yes in the sample it would be good to have more methods. On the wiki it would help to explain MethodRef and why it has this format. If there is an easy way to generate the correct text for a method that would help too.

I think the library models feature is really awesome and helpful for applications with troublesome dependencies like JsonObject.

msridhar commented 1 month ago

Thanks for the feedback @moaxcp. I've re-opened this issue to track the task of improving the documentation and sample as you suggest.