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

Dagger codegen uses ImmutableMap.builderWithExpectedSize which is marked as @Beta #1569

Open
perezd opened this issue Jul 22, 2019 · 5 comments

Comments

@perezd
Copy link

perezd commented Jul 22, 2019

Using latest released version of dagger, I've noticed that Map multibindings is making use of ImmutableMap.builderWithExpectedSize. When compiling code w/ Bazel + errorprone enabled its throwing a warning about this and halting my builds.

This goes against Google philosophy: https://github.com/google/guava/wiki/PhilosophyExplained#beta-apis

What is the best way to suppress this for my users (I am building a library on top of Dagger/Bazel). Perhaps the generated code could @SuppressWarnings("BetaApi") since we consider this safe?

@michaelxinsun
Copy link

Hello @perezd :

Thank you for reaching out! I was the engineer that added this change of generating ImmutableMap.builderWithExpectedSize, which is, as of the current Guava latest version (28.0-jre as of this writing), still a @Beta API:
https://guava.dev/releases/28.0-jre/api/docs/com/google/common/collect/ImmutableMap.html#builderWithExpectedSize-int-

I was coding at Google and I was doing 20% work for the Dagger team for a while. I intend to keep myself involved in Dagger and Guava and keep helping, on and off, as soon as I have bandwidth.

I'm aware of the Google Guava philosophy. I guess you mean the current situation is at odds with this, right?
"If your code is a library itself (i.e. it is used on the CLASSPATH of users outside your own control), you should not use beta APIs, unless you repackage them (e.g. using ProGuard)"

Anyways, could you paste us the bazel + errorprone compilation that halts your build, please? What's the error message you're seeing? I'd like to learn more. Thank you @perezd !

@perezd
Copy link
Author

perezd commented Jul 24, 2019

Thanks for the reply!

You are correct, it seems that dagger generated code shouldn't be relying on @beta annotated fields according to the spirit of that annotation, and Bazel + Guava beta checker basically enforces that rule such that it becomes a fatal error unless you disable errorprone which is non-ideal.

This is the error message Bazel provides:

ERROR .../BUILD:6:1: Building component.jar (1 source jar) and running annotation processors (ComponentProcessor) failed (Exit 1)
bazel-out/k8-fastbuild/bin/.../generated/DaggerComponent.java:330: error: [BetaApi] @Beta APIs should not be used in library code as they are subject to change
        return ImmutableMap.<Class<? extends Foo>, Bar<? extends Baz>>builderWithExpectedSize(30).put(...).build();}
                           ^
    (see https://github.com/google/guava/wiki/PhilosophyExplained#beta-apis)
Target failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 160.108s, Critical Path: 71.45s
INFO: 118 processes: 58 linux-sandbox, 60 worker.
FAILED: Build did NOT complete successfully
FAILED: Build did NOT complete successfully

This kicks in just after the codegen plugin produces a source code file and just before bazel goes to compile that into a classfile jar I suspect.

@perezd
Copy link
Author

perezd commented Aug 6, 2019

Any thoughts on how to resolve this for Bazel/Errorprone users?

michaelxinsun added a commit to michaelxinsun/guava that referenced this issue Sep 2, 2019
Previously, I modified Dagger so that its generated code is using
`builderWithExpectedSize` for immutable collections whenever it's
available, i.e., Guava 23.1 or later:
google/dagger#1094

Since `builderWithExpectedSize` is a @beta API, if a Bazel client
project depends on Dagger, and if the depended Dagger is using
Guava 23.1 or later, Dagger generates code that uses this Guava
@beta API, which fails ErrorProne, thus failing Bazel build:
google/dagger#1569

The same issue could also happen if a Maven client project has a
`maven-compiler-plugin` that also uses ErrorProne.

If the Guava team feel confident about those
`builderWithExpectedSize` static factory methods, maybe it's time
to remove `@Beta` for them altogether.

Happy to discuss.  Thank you!

Reference:
https://github.com/google/guava/wiki/PhilosophyExplained#beta-apis
@michaelxinsun
Copy link

Hey @perezd ! My current idea is to remove @Beta for those builderWithExpectedSize methods, so when you depend on a newer version of Dagger that depends on a newer version of Guava, you'll no longer have this problem:
google/guava#3591

Meanwhile, for now, you might want to consider adding the following flag inside your .bazelrc, so that you don't have to disable ErrorProne entirely:

# TODO(perezd):  Remove this line when Dagger no longer depends on Guava `builderWithExpectedSize`
--javacopt="-Xep: BetaApi:WARN"

Tell me whether that works for your case. Let me know if you have any questions. Thank you for using Dagger!

@perezd
Copy link
Author

perezd commented Sep 3, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants