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

java.lang.RuntimeException: expected call to iterator(), instead saw null #866

Closed g-jiannan closed 10 months ago

g-jiannan commented 10 months ago

When we try to adopt NullAway (version 0.10.16), following exception is thrown unexpectedly sometimes:

        for (String key : sharedPreferences.getAll().keySet()) {
                                           ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:

     error-prone version: 2.23.0
     BugPattern: NullAway
     Stack Trace:
     com.google.common.util.concurrent.UncheckedExecutionException: java.lang.RuntimeException: expected call to iterator(), instead saw null
        at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2086)
        at com.google.common.cache.LocalCache.get(LocalCache.java:4012)
        at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4035)
        at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5011)
        at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5018)
        at com.uber.nullaway.dataflow.DataFlow.dataflow(DataFlow.java:157)
        at com.uber.nullaway.dataflow.DataFlow.resultFor(DataFlow.java:290)
        at com.uber.nullaway.dataflow.DataFlow.resultForExpr(DataFlow.java:266)
        at com.uber.nullaway.dataflow.DataFlow.expressionDataflow(DataFlow.java:208)
        at com.uber.nullaway.dataflow.AccessPathNullnessAnalysis.getNullness(AccessPathNullnessAnalysis.java:131)
        at com.uber.nullaway.NullAway.nullnessFromDataflow(NullAway.java:2415)
        at com.uber.nullaway.NullAway.mayBeNullExpr(NullAway.java:2393)
        at com.uber.nullaway.NullAway.matchDereference(NullAway.java:2442)
        at com.uber.nullaway.NullAway.matchMemberSelect(NullAway.java:548)
        at com.google.errorprone.scanner.ErrorProneScanner.processMatchers(ErrorProneScanner.java:449)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMemberSelect(ErrorProneScanner.java:726)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMemberSelect(ErrorProneScanner.java:150)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCFieldAccess.accept(JCTree.java:2458)
        at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:96)
        at jdk.compiler/com.sun.source.util.TreeScanner.visitMethodInvocation(TreeScanner.java:589)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:751)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:150)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1813)
        at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at jdk.compiler/com.sun.source.util.TreeScanner.visitMemberSelect(TreeScanner.java:820)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMemberSelect(ErrorProneScanner.java:728)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMemberSelect(ErrorProneScanner.java:150)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCFieldAccess.accept(JCTree.java:2458)
        at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:96)
        at jdk.compiler/com.sun.source.util.TreeScanner.visitMethodInvocation(TreeScanner.java:589)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:751)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:150)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1813)
        at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:96)
        at jdk.compiler/com.sun.source.util.TreeScanner.visitEnhancedForLoop(TreeScanner.java:337)
        at com.google.errorprone.scanner.ErrorProneScanner.visitEnhancedForLoop(ErrorProneScanner.java:620)
        at com.google.errorprone.scanner.ErrorProneScanner.visitEnhancedForLoop(ErrorProneScanner.java:150)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCEnhancedForLoop.accept(JCTree.java:1243)
        at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:96)
        at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:111)
        at jdk.compiler/com.sun.source.util.TreeScanner.visitBlock(TreeScanner.java:272)
        at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:520)
        at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:150)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:1103)
        at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:96)
        at jdk.compiler/com.sun.source.util.TreeScanner.visitMethod(TreeScanner.java:224)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:740)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:150)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:953)
        at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:96)
        at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:111)
        at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:119)
        at jdk.compiler/com.sun.source.util.TreeScanner.visitClass(TreeScanner.java:203)
        at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:548)
        at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:150)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:860)
        at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:86)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:111)
        at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:119)
        at jdk.compiler/com.sun.source.util.TreeScanner.visitCompilationUnit(TreeScanner.java:152)
        at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:560)
        at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:150)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept(JCTree.java:614)
        at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:60)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:58)
        at com.google.errorprone.scanner.ErrorProneScannerTransformer.apply(ErrorProneScannerTransformer.java:43)
        at com.google.errorprone.ErrorProneAnalyzer.finished(ErrorProneAnalyzer.java:156)
        at jdk.compiler/com.sun.tools.javac.api.MultiTaskListener.finished(MultiTaskListener.java:132)
        at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1394)
        at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1341)
        at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:933)
        at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:317)
        at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:176)
        at jdk.compiler/com.sun.tools.javac.Main.compile(Main.java:64)
        at jdk.compiler/com.sun.tools.javac.Main.main(Main.java:50)
  Caused by: java.lang.RuntimeException: expected call to iterator(), instead saw null
        at com.uber.nullaway.dataflow.AccessPathNullnessPropagation.handleEnhancedForOverKeySet(AccessPathNullnessPropagation.java:583)
        at com.uber.nullaway.dataflow.AccessPathNullnessPropagation.visitAssignment(AccessPathNullnessPropagation.java:521)
        at com.uber.nullaway.dataflow.AccessPathNullnessPropagation.visitAssignment(AccessPathNullnessPropagation.java:139)
        at org.checkerframework.nullaway.dataflow.cfg.node.AssignmentNode.accept(AssignmentNode.java:122)
        at org.checkerframework.nullaway.dataflow.analysis.AbstractAnalysis.callTransferFunction(AbstractAnalysis.java:349)
        at org.checkerframework.nullaway.dataflow.analysis.ForwardAnalysisImpl.callTransferFunction(ForwardAnalysisImpl.java:377)
        at org.checkerframework.nullaway.dataflow.analysis.ForwardAnalysisImpl.performAnalysisBlock(ForwardAnalysisImpl.java:128)
        at org.checkerframework.nullaway.dataflow.analysis.ForwardAnalysisImpl.performAnalysis(ForwardAnalysisImpl.java:105)
        at com.uber.nullaway.dataflow.DataFlow$1.load(DataFlow.java:93)
        at com.uber.nullaway.dataflow.DataFlow$1.load(DataFlow.java:85)
        at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3571)
        at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2313)
        at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2190)
        at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2080)
        ... 96 more

The reported code for (String key : sharedPreferences.getAll().keySet()) { looks trivial and the code pattern is already covered by the NullAwayKeySetIteratorTests. I'm not pretty sure the root cause but it turns out the problem is that AccessPathNullnessPropagation#isCallToMethod is not so robust: it expects Set.iterator() but the the receiver type is resolved as Collection unexpectedly sometimes.

A quick fix is leverage errorprone matchers and I'll send pull request soon.

msridhar commented 10 months ago

Thanks for the report @g-jiannan and for the candidate fix! Before we land any potential fix here, I'd like to try to create a test case that reproduces this issue. Can you share the return type of the sharedPreferences.getAll() method that is called in your code excerpt? Or if you could come up with a crashing test case that would be really useful.

I'm not pretty sure the root cause but it turns out the problem is that AccessPathNullnessPropagation#isCallToMethod is not so robust: it expects Set.iterator() but the the receiver type is resolved as Collection unexpectedly sometimes.

This is surprising to me. We have code that specifically looks for a call to keySet() on a java.util.Map, and that method is declared to return a Set. In that case, I don't see how the receiver type of the subsequent iterator() could be Collection. But we do have a crash so I must be missing something.

A quick fix is leverage errorprone matchers and I'll send pull request soon.

This code is performance critical, so my instinct is to not use Error Prone matchers here, at least not without careful understanding of the performance implications. If we can reproduce the crash, we may be able to find a more localized fix.

Thanks again!

g-jiannan commented 10 months ago

Can you share the return type of the sharedPreferences.getAll() method that is called in your code excerpt?

Here is the [API javadoc](http://go/android-dev/reference/android/content/SharedPreferences#getAll()) for SharedPreferences.getAll(), which returns a Map.

In that case, I don't see how the receiver type of the subsequent iterator() could be Collection. But we do have a crash so I must be missing something.

It also surprises me. I guess it may because Set extends Collection, and Collection interface has iterator() API? Another possible reason is compiler difference, we use OpenJDK.

This code is performance critical, so my instinct is to not use Error Prone matchers here, at least not without careful understanding of the performance implications.

FYI: In Google, Error Prone (MethodMatchers) is heavily used when compile Java code. Is there any way to profile the PR performance change for NullAway?

If we can reproduce the crash, we may be able to find a more localized fix.

The reported code is trivial and I tried to add a new test case to NullAwayKeySetIteratorTests but the issue cannot be reproduced unfortunately. Actually, I did not find a pattern that will cause the exception with NullAway repo, maybe it is compiler dependent.

msridhar commented 10 months ago

Hi @g-jiannan, I looked though some of the Error Prone code and came up with a narrower fix for this issue in #868. Could you test it out and see if it works for you? I think the potential issue here is that your Java compiler generates a call to Collection.iterator() for some enhanced for loops even when the type of the expression being iterated over is a Set, which is certainly legal. So in our case, what we can instead assert is that the receiver passed at this call site has a static type that is a subtype of java.util.Set.

Anyway, if you could test out the alternate fix in #868 I would appreciate it. I think even if we don't see performance differences in our benchmarks (which we do have, try running ./gradlew :nullaway:jmh), I'd prefer to stick to our current way of matching method calls and make a minimal change here rather than using MethodMatchers.

g-jiannan commented 10 months ago

I see, I'm OK with either solution as long as the issue gets fixed :)

Verified that #868 works, thanks!

msridhar commented 10 months ago

Great! I'll work on getting #868 reviewed and landed

msridhar commented 10 months ago

@g-jiannan the fix for this issue has landed. Would it be helpful to have a new NullAway release containing the fix? If so, we can look into it.

g-jiannan commented 10 months ago

That would be better, please help to create a new release, thanks!

g-jiannan commented 10 months ago

Hi @msridhar Any chance we can have a new release recently? My colleague is waiting for it. Thanks!

msridhar commented 10 months ago

@g-jiannan we want to do some internal testing before cutting the release. We'll try to have the release out by the end of the week. /cc @lazaroclapp

g-jiannan commented 10 months ago

Got it, thanks for the update!

lazaroclapp commented 10 months ago

@g-jiannan We have just cut a release of NullAway (0.10.18) with this fix. It will likely take a few hours to propagate through Maven Central, but should be ready later today 🙂

g-jiannan commented 10 months ago

Awesome, thanks for helping!