Static methods can be accessed from within subclasses, but NullAway doesn't know this

[lazaroclapp]

The following is (annoyingly) valid Java code for calling a static method:

class Foo {
    public static String foo(@Nullable Object ignored) { return "Nonsense java black magic!"; };
}

class Bar extends Foo { }

class Main {
    public static void main(String[] args) {
        System.out.println(Bar.foo(null));
    }
}

However, NullAway will fail to read the annotations of Foo.foo(...) when inspecting the static call to Bar.foo(...) and thus give an error on Bar.foo(null).

[lazaroclapp]

Note: Making this issue as a reminder for myself, will take a look when possible. We have a workaround, which is to use the class that declares the static method, which is what I would have expected to be needed for static methods 😅

[msridhar]

I'm surprised there isn't already an Error Prone check to flag the call. But even if there was, NullAway should do the right thing.

[lazaroclapp]

Actually, this seems to be working with NullAway as written. Trying to fish the original example I saw internally to see if we can repro this...

[lazaroclapp]

Interesting: In the following example, only the last call produces the error...

            .addSourceLines(
                    "Test.java",
                    "package com.uber;",
                    "import static com.uber.Subclass.foo;",
                    "class Test {",
                    "    static void m() {",
                    "        // Calling foo() the obvious way: safe because @Nullable arg",
                    "        System.out.println(Superclass.foo(null));",
                    "        // Calling foo() through Subclass: also safe",
                    "        System.out.println(Subclass.foo(null));",
                    "        // Static import from Subclass: also safe but produces error currently",
                    "        System.out.println(foo(null));",
                    "    }",
                    "}")

It turns out that both Superclass.foo(...) and Subclass.foo(...) give a methodSymbol with .owner == Superclass here, but the foo(...) that was statically imported shows .owner == Subclass. No idea if that's because of some shallower resolution of Subclass in that case... possibly a javac bug?

Edit: The only fix on our end I can think of is to double-check, when dealing with a method invocation from a static import, that methodSymbol.owner.members() actually contains the method and if not manually traverse the class hierarchy. But that sounds expensive for what should be a very rare edge case...

[msridhar]

I think this Error Prone check would catch the bad import?

https://errorprone.info/bugpattern/NonCanonicalStaticMemberImport

It's experimental so not enabled by default.

I agree adding support within NullAway for this case may not be worth the cost.

[msridhar]

@lazaroclapp should we label this as low priority? Will running the Error Prone check above work well enough for avoiding this issue?

[cpovirk]

I just wanted to chime in to say that this is quite interesting and good for us to know, too (for nullness checks and other Error Prone checks), so thanks!

We haven't enabled NonCanonicalStaticMemberImport inside Google except as a review-time comment on modified lines. That's mainly because there are a lot of violations and relatively few true "bugs" that the check catches. (It's more of a style thing.) But this bug shows that such imports have undesirable side effects for static analysis. I'm still not expecting us to do anything about it anytime soon, but I suspect you've saved us a day of mystery somewhere down the line.

If we do need to tackle this, I wonder if we could scan the imports before processing each file, record the names of the !isCanonical() ones, and look into using your workaround only for APIs that match ones of those names. That might help some with the performance concerns.

[cpovirk]

I don't have a self-contained repro for you, but you might also be interested in knowing that I think I saw another case in which owner is misleading:

<T extends Serializable> void m(T t) {
  t.equals("");
}

I think I was seeing equals with an owner of java.io.Serializable, rather than the java.lang.Object I would have expected (since Serializable doesn't override equals).

[cpovirk]

Depending on which specific Error Prone APIs you use, this might be fixed when you pick up the next Error Prone release. Or maybe it was even fixed already on the NullAway side by #898?

(I said slightly more about this in #897 (comment).)

[msridhar]

@cpovirk you're right that an extension of the fix for #898 also fixes this issue. I have a WIP version here:

master...msridhar:NullAway:issue-764

One thing I'm not sure about though. We need this fix to work across recent Error Prone versions, including the upcoming release that includes your fix. Do you have a suggested way of coding that? I'm concerned that on the latest EP version my fix will lead to baseSymbol() being called twice. The best option I can think of right now is to copy-paste the fixed ASTHelpers.getSymbol() overload into NullAway, but I'm wondering if there's a better way.

[cpovirk]

Hmm. My largely unconscious assumption had been that calling baseSymbol() twice was a no-op. But I have little to base that on.

I do see that the main implementations of Symbol.baseSymbol() is return this. But of course the whole reason we're in this mess is that there are other implementations :)

So the worry would be: Maybe there's one Symbol for which baseSymbol() returns another Symbol whose baseSymbol() returns yet something else. That would seem kind of weird to me, but I can't easily rule it out.

[msridhar]

Thanks! Probably for now, we can go with my WIP fix. I think you're right that calling baseSymbol() twice is likely to be a no-op. We can test further once the next Error Prone version is released.