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.

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

4.0.0: StackOverflowError in error-prone #12926

Closed
dmivankov opened this issue Jan 29, 2021 · 5 comments · Fixed by google/turbine#100
Closed

4.0.0: StackOverflowError in error-prone #12926

dmivankov opened this issue Jan 29, 2021 · 5 comments · Fixed by google/turbine#100
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-Java Issues for Java rules type: support / not a bug (process)

Comments

@dmivankov
Copy link
Contributor

dmivankov commented Jan 29, 2021

Description of the problem / feature request:

Trying out bazel 4.0.0, with --javabase=@bazel_tools//tools/jdk:remote_jdk11 --java_toolchain=@bazel_tools//tools/jdk:toolchain_java11 and hit a bug in error-prone 2.4.0 (bazel 3.3.1 has 2.3.2 I think, and it works)

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

Haven't yet reproduced in a small setup but likely is triggered by code generated by https://github.com/immutables/immutables 2.8.8

Any other information, logs, or outputs that you want to share?

Most recent error_prone is 2.5.1, but haven't tested if it'd fix the issue https://github.com/google/error-prone/releases

some_class.java:192: error: An unhandled exception was thrown by the Error Prone static analysis plugin.
                    SomeClass.builder().from(someInstance).build())
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.4.0
     BugPattern: ArrayToString
     Stack Trace:
     java.lang.StackOverflowError
  	at jdk.compiler/com.sun.tools.javac.code.Types$UnaryVisitor.visit(Types.java:4937)
  	at jdk.compiler/com.sun.tools.javac.code.Types.supertype(Types.java:2462)
  	at jdk.compiler/com.sun.tools.javac.code.Types$12.visitClassType(Types.java:2128)
  	at jdk.compiler/com.sun.tools.javac.code.Types$12.visitClassType(Types.java:2117)
  	at jdk.compiler/com.sun.tools.javac.code.Type$ClassType.accept(Type.java:993)
  	at jdk.compiler/com.sun.tools.javac.code.Types$DefaultTypeVisitor.visit(Types.java:4857)
  	at jdk.compiler/com.sun.tools.javac.code.Types.asSuper(Types.java:2114)
  	at jdk.compiler/com.sun.tools.javac.code.Types$12.visitClassType(Types.java:2130)
  	at jdk.compiler/com.sun.tools.javac.code.Types$12.visitClassType(Types.java:2117)
  	at jdk.compiler/com.sun.tools.javac.code.Type$ClassType.accept(Type.java:993)
  	at jdk.compiler/com.sun.tools.javac.code.Types$DefaultTypeVisitor.visit(Types.java:4857)
  	at jdk.compiler/com.sun.tools.javac.code.Types.asSuper(Types.java:2114)
  	at jdk.compiler/com.sun.tools.javac.code.Types$12.visitClassType(Types.java:2130)
  	at jdk.compiler/com.sun.tools.javac.code.Types$12.visitClassType(Types.java:2117)
  	at jdk.compiler/com.sun.tools.javac.code.Type$ClassType.accept(Type.java:993)
  	at jdk.compiler/com.sun.tools.javac.code.Types$DefaultTypeVisitor.visit(Types.java:4857)
...

Not sure if there's an easy way to upgrade error_prone version without the need to rebuild bazel itself

Bump of error_prone commit 239b2aa
Some issues with earlies attempts of upgrade 2.3.2->2.3.3 #9286

@cushon
Copy link
Contributor

cushon commented Feb 1, 2021

Do you have a repro?

It looks like it's just running out of stack. Does setting a higher stack size make a difference, e.g.:

load(
  "@bazel_tools//tools/jdk:default_java_toolchain.bzl",
  "default_java_toolchain", "JDK9_JVM_OPTS",
)

default_java_toolchain(
  name = "toolchain",
  jvm_opts = JDK9_JVM_OPTS + ["-Xss7m"],
)
bazel build --java_toolchain=:toolchain ...

dmivankov added a commit to dmivankov/bug_reports that referenced this issue Feb 1, 2021
@dmivankov
Copy link
Contributor Author

Xss7m didn't help, stacktrace is quite repetitive, so could be an infinite recursion.
Small repro https://github.com/dmivankov/bug_reports/tree/bazel_12926

@cushon
Copy link
Contributor

cushon commented Feb 2, 2021

Thanks, taking a close look I see that immutables is generating a class that completes a cyclic type hierarchy:

$ javap -cp bazel-bin/liba-hjar.jar A\$Builder ImmutableA\$BuilderImpl
public class A$Builder extends ImmutableA$BuilderImpl {
  public A$Builder();
}
public final class ImmutableA$BuilderImpl extends A$Builder {
  public final ImmutableA$BuilderImpl from(A);
  public ImmutableA build();
}

This is an interaction between immutables and https://github.com/google/turbine's implementation of annotation processing.

This logic is relying on the toString() representation of ImmutableA.BuilderImpl in class Builder extends ImmutableA.BuilderImpl { before the code has been generated yet, while ImmutableA.BuilderImpl is still being modelled as an error type: https://github.com/immutables/immutables/blob/dd249064688af65ee3ff00fa5819e9deeb1bcf67/generator/src/org/immutables/generator/SourceExtraction.java#L396-L406

@dmivankov
Copy link
Contributor Author

As a workaround

import org.immutables.value.Value;

@Value.Style(
    typeBuilder = "ABuilderImpl",
    builderVisibility = Value.Style.BuilderVisibility.PUBLIC,
    implementationNestedInBuilder = true
)
@Value.Immutable
interface A {
  static Builder builder() {
    return new Builder();
  }

  class Builder extends ABuilderImpl {
  }
}

seems to work on the repro case, class hierarchy is different, but may work well.

For the original setup, what looks like a correct place to implement a fix

  • error_prone (something changed there?)
  • bazel annotation processing (something changed there?)
  • immutables (is such hierarchy really valid? or maybe there's a chance to have small modification of codegen to remove the cycle?)

@cushon
Copy link
Contributor

cushon commented Feb 2, 2021

bazel annotation processing (something changed there?)

This is the thing that actually changed in Bazel--as of 10ffddb Bazel uses a different annotation processing implementation by default (https://github.com/google/turbine).

The way errors are modeled in the annotation processing API isn't very well specified, but I think improving turbine here is a good resolution, I don't see anything in particular immutables should be doing differently.

Ideally javac would detect the cycle and not crash, but there are a lot of ways that invalid class files can cause problems for javac, so I'm not sure it's worth trying to handle that better.

The mention of Error Prone in the stack trace is a red herring, it just catches top-level exceptions and tries to log additional information.

copybara-service bot pushed a commit to google/turbine that referenced this issue Feb 2, 2021
When creating error types to model invalid elements during annotation processing,
include all simple names present in source for qualified names, instead of
just the last one.

This fixes bazelbuild/bazel#12926

PiperOrigin-RevId: 355065427
copybara-service bot pushed a commit to google/turbine that referenced this issue Feb 2, 2021
When creating error types to model invalid elements during annotation processing,
include all simple names present in source for qualified names, instead of
just the last one.

This fixes bazelbuild/bazel#12926

PiperOrigin-RevId: 355249091
cushon added a commit to cushon/bazel that referenced this issue Feb 2, 2021
@cushon cushon mentioned this issue Feb 2, 2021
@oquenchil oquenchil added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-Java Issues for Java rules type: support / not a bug (process) labels Feb 4, 2021
philwo pushed a commit that referenced this issue Mar 15, 2021
Built at
google/turbine@1bbb136

Fixes #12926

Partial commit for third_party/*, see #12950.

Signed-off-by: Philipp Wollermann <philwo@google.com>
philwo pushed a commit that referenced this issue Mar 15, 2021
Built at
google/turbine@1bbb136

Fixes #12926

Partial commit for third_party/*, see #12950.

Signed-off-by: Philipp Wollermann <philwo@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-Java Issues for Java rules type: support / not a bug (process)
Projects
None yet
3 participants