google / error-prone

Catch common Java mistakes as compile-time errors
https://errorprone.info
Apache License 2.0
6.87k stars 744 forks source link

Limitation of InlineMeInliner with lambda expressions #2347

Open lazaroclapp opened 3 years ago

lazaroclapp commented 3 years ago

Description of the problem / feature request:

I am seeing the following crash on InlineMeInliner for Error Prone 2.7.1.

This is on code edited after simply following the suggestions of InlineMeSuggester:

     error-prone version: 2.7.1
     BugPattern: InlineMeInliner
     Stack Trace:
     java.lang.ClassCastException: com.sun.tools.javac.code.Type$BottomType cannot be cast to com.sun.tools.javac.code.Type$ArrayType
        at com.sun.tools.javac.model.AnnotationProxyMaker$ValueVisitor.visitArray(AnnotationProxyMaker.java:186)
        at com.sun.tools.javac.code.Attribute$Array.accept(Attribute.java:327)
        at com.sun.tools.javac.model.AnnotationProxyMaker$ValueVisitor.getValue(AnnotationProxyMaker.java:167)
        at com.sun.tools.javac.model.AnnotationProxyMaker.generateValue(AnnotationProxyMaker.java:145)
        at com.sun.tools.javac.model.AnnotationProxyMaker.getAllReflectedValues(AnnotationProxyMaker.java:104)
        at com.sun.tools.javac.model.AnnotationProxyMaker.generateAnnotation(AnnotationProxyMaker.java:90)
        at com.sun.tools.javac.model.AnnotationProxyMaker.generateAnnotation(AnnotationProxyMaker.java:81)
        at com.sun.tools.javac.code.AnnoConstruct.getAnnotation(AnnoConstruct.java:185)
        at com.google.errorprone.bugpatterns.inlineme.Inliner.match(Inliner.java:148)
        at com.google.errorprone.bugpatterns.inlineme.Inliner.matchMethodInvocation(Inliner.java:126)
        at com.google.errorprone.scanner.ErrorProneScanner.processMatchers(ErrorProneScanner.java:450)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:747)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1644)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.visitMemberSelect(TreeScanner.java:680)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMemberSelect(ErrorProneScanner.java:729)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMemberSelect(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCFieldAccess.accept(JCTree.java:2112)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
        at com.sun.source.util.TreeScanner.visitMethodInvocation(TreeScanner.java:508)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:752)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1644)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.visitExpressionStatement(TreeScanner.java:433)
        at com.google.errorprone.scanner.ErrorProneScanner.visitExpressionStatement(ErrorProneScanner.java:634)
        at com.google.errorprone.scanner.ErrorProneScanner.visitExpressionStatement(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCExpressionStatement.accept(JCTree.java:1454)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at com.sun.source.util.TreeScanner.visitBlock(TreeScanner.java:248)
        at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:521)
        at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:1026)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
        at com.sun.source.util.TreeScanner.visitLambdaExpression(TreeScanner.java:559)
        at com.google.errorprone.scanner.ErrorProneScanner.visitLambdaExpression(ErrorProneScanner.java:703)
        at com.google.errorprone.scanner.ErrorProneScanner.visitLambdaExpression(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCLambda.accept(JCTree.java:1805)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
        at com.sun.source.util.TreeScanner.visitMethodInvocation(TreeScanner.java:509)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:752)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1644)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.visitReturn(TreeScanner.java:469)
        at com.google.errorprone.scanner.ErrorProneScanner.visitReturn(ErrorProneScanner.java:818)
        at com.google.errorprone.scanner.ErrorProneScanner.visitReturn(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCReturn.accept(JCTree.java:1548)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at com.sun.source.util.TreeScanner.visitBlock(TreeScanner.java:248)
        at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:521)
        at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:1026)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
        at com.sun.source.util.TreeScanner.visitMethod(TreeScanner.java:206)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:741)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:898)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
        at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
        at com.sun.source.util.TreeScanner.visitClass(TreeScanner.java:187)
        at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:549)
        at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:808)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
        at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
        at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
        at com.sun.source.util.TreeScanner.visitCompilationUnit(TreeScanner.java:144)
        at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:561)
        at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:151)
        at com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept(JCTree.java:591)
        at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:56)
        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:152)
        at com.sun.tools.javac.api.MultiTaskListener.finished(MultiTaskListener.java:120)
        at com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1404)
        at com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1353)
        at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:946)
        at com.sun.tools.javac.api.JavacTaskImpl.lambda$doCall$0(JavacTaskImpl.java:100)
        at com.sun.tools.javac.api.JavacTaskImpl.handleExceptions(JavacTaskImpl.java:142)
        at com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:96)
        at com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:90)
        at com.facebook.buck.jvm.java.plugin.adapter.JavacTaskWrapper.call(JavacTaskWrapper.java:100)
        at com.facebook.buck.jvm.java.plugin.adapter.JavacTaskWrapper.call(JavacTaskWrapper.java:100)
        at com.facebook.buck.jvm.java.plugin.adapter.BuckJavacTask.call(BuckJavacTask.java:52)
        at com.facebook.buck.jvm.java.plugin.adapter.BuckJavacTaskProxyImpl.call(BuckJavacTaskProxyImpl.java:134)
        at com.facebook.buck.jvm.java.Jsr199JavacInvocation$CompilerWorker.lambda$startCompiler$2(Jsr199JavacInvocation.java:424)
        at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:127)
        at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:57)
        at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:80)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)

Code samples below (identifier names changed, but AST structure is similar).

This gets reported on the call-site for:

Client client = Client.client(() -> \"text\");",

Where that version of client(...) has been @Deprecated and marked with @InlineMe as follows:

  @InlineMe(replacement = "Client.client(supplier.text())", imports={"com.example.Client"})
  public static Client client(TextSupplier supplier) {
    return client(supplier.text());
  }
  public static Client client(String text) {
    return new Client(text);
  }

I can confirm, stepping through the build with a debugger, that when this line is hit when processing imports={"com.example.Client"}, a.type happens to be BottomType/<nulltype> (even though the value does show an array containing the string "com.example.Client"). We are using the EP javac on a JDK 8 JVM.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

I tried to make a repro case as a test case for Error Prone here, but while there is indeed a problem in that the rewriting doesn't happen (and thus the test fails), it doesn't crash with the same exception. I'll be working on a stand alone project gradle that attempts to show the issue without our build system in the way and update this issue if that works.

What version of Error Prone are you using?

2.7.1

Have you found anything relevant by searching the web?

Nothing that quite matches, either as an Error Prone or a JDK bug.

cushon commented 3 years ago

The unit test doesn't get refactored because the check uses compiledWithFix to filter suggested fixes, and the test doesn't compile after the refactoring is applied:

Caller.java:4: error: cannot find symbol
    Client client = Client.client(() -> "text".text());
                                              ^

I think this might be a known limitation of the handling of lambdas.

The original issue might be separate. Was the latest version of the error-prone-annotations artifact on the classpath for that compilation?

cushon commented 3 years ago

The crash should be fixed by dc7252bda054c3de2c349b173aca0133b2afb283.

I'll keep this open to track https://github.com/google/error-prone/issues/2347#issuecomment-844562624.

lazaroclapp commented 3 years ago

Few updates:

  1. Agree that those are two distinct issues 🙂
  2. I tested the current master with the patch listed above, it does solve the problem within our original build! Thanks so much! 😃 (I am happy with this resolution and will enable this checker in our build when an EP release incorporating this fix is available)
  3. I wasn't entirely able to repro the exception with a simple standalone gradle project, so there might be something from our build that is part of the issue. From seeing the fix, I suspect maybe the fact that in the real build our Client class and the class using it are in two different build targets? (See below for some info on the classpath of both targets) Not sure, though.

Was the latest version of the error-prone-annotations artifact on the classpath for that compilation?

It's not in -classpath, but it is in -processorpath for the failing javac invocation. (It would be in -classpath for the compilation of the target that actually includes the annotated method)

Note that we fork error-prone-annotations because of #2122, but our artifact is identical to the 2.7.1 error-prone-annotations except for removing usages of the @IncompatibleModifiers meta-annotation. In particular, @InlineMe should be unchanged there.