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

bytecode contains unexpected annotations #7331

Closed
2 tasks done
GGGTTTTT opened this issue Jul 24, 2024 · 7 comments · Fixed by #7333
Closed
2 tasks done

bytecode contains unexpected annotations #7331

GGGTTTTT opened this issue Jul 24, 2024 · 7 comments · Fixed by #7333
Assignees
Labels
P2 package=general type=defect Bug, not working as expected

Comments

@GGGTTTTT
Copy link

Guava Version

33.2.1-jre

Description

In the latest version of Guava, the bytecode for the method com.google.common.base.Joiner#iterable contains unexpected annotations.
image

Example

You can simply use the jclasslib ByteCode Viewer to inspect the class files.

Expected Behavior

image

Actual Behavior

image

Packages

com.google.common.base

Platforms

Java 8

Checklist

  • I agree to follow the code of conduct.

  • I can reproduce the bug with the latest version of Guava available.

@GGGTTTTT GGGTTTTT added the type=defect Bug, not working as expected label Jul 24, 2024
@cpovirk
Copy link
Member

cpovirk commented Jul 24, 2024

Thanks, that looks like a javac bug from when I switched from using a javac from a Google-built JDK11 to one from a Debian stock JDK11. How much trouble does the problem annotation cause you?

$ /usr/local/buildtools/java/jdk11/bin/java -version                                                                                                                                                             
openjdk version "11.0.20.1" 2024-04-11
OpenJDK Runtime Environment (build 11.0.20.1+1-google-release-623923767)
OpenJDK 64-Bit Server VM (build 11.0.20.1+1-google-release-623923767, mixed mode, sharing)

$ JAVA_HOME=/usr/local/buildtools/java/jdk11 mvn clean package -f guava -Dmaven.javadoc.skip -Dmaven.source.skip && javap -private -v -cp guava/target/guava-33.2.1-jre.jar com.google.common.base.Joiner | perl -ne 'print if /iterable[(]/ ... /^\s*$/' | perl -ne 'print if (/RuntimeVisibleTypeAnnotations/ ... /RuntimeVisibleParameterAnnotations/) && !/RuntimeVisibleParameterAnnotations/ && !/RuntimeVisibleTypeAnnotations/'
...
      0: #74(): METHOD_RETURN, location=[TYPE_ARGUMENT(0)]
        org.checkerframework.checker.nullness.qual.Nullable
      1: #74(): METHOD_FORMAL_PARAMETER, param_index=2, location=[ARRAY]
        org.checkerframework.checker.nullness.qual.Nullable
$ /usr/lib/jvm/java-11-openjdk-amd64/bin/java -version
openjdk version "11.0.22" 2024-01-16
OpenJDK Runtime Environment (build 11.0.22+7-post-Debian-1)
OpenJDK 64-Bit Server VM (build 11.0.22+7-post-Debian-1, mixed mode, sharing)

$ JAVA_HOME=/usr/lib/jvm/java-11-openjdk-amd64 mvn clean package -f guava -Dmaven.javadoc.skip -Dmaven.source.skip && javap -private -v -cp guava/target/guava-33.2.1-jre.jar com.google.common.base.Joiner | perl -ne 'print if /iterable[(]/ ... /^\s*$/' | perl -ne 'print if (/RuntimeVisibleTypeAnnotations/ ... /RuntimeVisibleParameterAnnotations/) && !/RuntimeVisibleParameterAnnotations/ && !/RuntimeVisibleTypeAnnotations/'
...
      0: #74(): METHOD_RETURN, location=[TYPE_ARGUMENT(0)]
        org.checkerframework.checker.nullness.qual.Nullable
      1: #74(): METHOD_FORMAL_PARAMETER, param_index=2, location=[ARRAY]
        org.checkerframework.checker.nullness.qual.Nullable
      2: #74(): CLASS_EXTENDS, type_index=65535, location=[TYPE_ARGUMENT(0)]
        org.checkerframework.checker.nullness.qual.Nullable

The Google version is nominally a lower version number, so either the bug was introduced after that point, the bug was patched in the Google version, or there was some other difference in where Debian gets its version from. Hmm, a openjdk-11.0.16 that I have lying around also shows the bug. That sounds like further reason to believe that Google patched the bug. And it looks like the bug is JDK-8198945.

As the bug report suggests, the problem goes away if I use /usr/lib/jvm/java-17-openjdk-amd64. The problem with that is that it breaks JDiff (which we should probably migrate away from). But the right thing to do would be to automatically download and use the newest javac that we can, presumably by using Maven toolchains.

I might just be able to perform future releases with jdk-12.0.2, which has the fix but doesn't yet break JDiff :) I'll test it out now and report back if there are any problems.

@cpovirk
Copy link
Member

cpovirk commented Jul 24, 2024

With JDK12, I see failures in Javadoc:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.5.0:jar (attach-docs) on project guava: MavenReportException: Error while generating Javadoc: 
[ERROR] Exit code: 1
[ERROR] javadoc: error - The code being documented uses modules but the packages defined in https://errorprone.info/api/latest/ are in the unnamed module.
[ERROR] javadoc: error - The code being documented uses modules but the packages defined in https://static.javadoc.io/com.google.j2objc/j2objc-annotations/1.1/ are in the unnamed module.
[ERROR] javadoc: warning - URL https://docs.oracle.com/javase/9/docs/api/element-list was redirected to https://docs.oracle.com/en/java/javase/22/ -- Update the command-line options to suppress this warning.
[ERROR] javadoc: error - The code being documented uses modules but the packages defined in https://checkerframework.org/api/ are in the unnamed module.

Updating to maven-javadoc-plugin:3.8.0 doesn't help (though I should probably do that anyway). There may be a way to work around the problem, but I should probably make a quick attempt at changing our maven-compiler-plugin setup to use a toolchain instead.

copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
I'm not sure that any of these end up being _necessary_ to what I'm doing in #7331 (comment) / #3990 (comment). But the upgrade to `maven-surefire-plugin` changes that plugin's toolchain behavior, so I particularly want to use the new version there in advance of starting to use toolchains.

RELNOTES=n/a
PiperOrigin-RevId: 655556207
@cpovirk cpovirk self-assigned this Jul 24, 2024
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
I'm not sure that any of these end up being _necessary_ to what I'm doing in #7331 (comment) / #3990 (comment). But the upgrade to `maven-surefire-plugin` changes that plugin's toolchain behavior, so I particularly want to use the new version there in advance of starting to use toolchains.

This includes a workaround for a bug in the JDK 8 javac. (I don't know why the bug is appearing only after these upgrades.)

```
Error:  /home/runner/work/guava/guava/guava/src/com/google/common/hash/BloomFilter.java:[78,29] error: cannot find symbol
  symbol:   class Serializable
  location: class BloomFilter<T>
  where T is a type-variable:
    T declared in class BloomFilter
```
RELNOTES=n/a
PiperOrigin-RevId: 655556207
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to remove the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592.

(See also [these notes](#5457 (comment)).)

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to remove the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592. I assume that we'll now get the default from the JDK whose Javadoc binary we're using, which (again) will now be 11. That seems fine. We could consider setting it to 8 to match our normal build (which I thought I had remembered was the `maven-javadoc-plugin` default, but I don't think I'm seeing that, at least not under our current versions), but I don't see much downside to 11—or even to newer versions that we might someday use for Maven Javadoc generation, given that we keep the code compiling under new versions already.

Some other thing I'm wondering:

- I wonder if we should activate(?) some of the plugins, including the new toolchain plugins, in the `<plugins>` (not just `<pluginManagement>`) section of the parent `pom.xml`. Might that save us from having to do so in each separate `pom.xml`? (We might actually mostly get away with activating(?) them only in the main `guava` build: That _downloads and registers_ the toolchains, and then at least the other projects' _per-plugin_ toolchain configuration probably finds them? But for the more general configuration to work, I think we at least need to activate(?) `maven-toolchains-plugin` in each? I haven't experimented a ton with this.)

(See also [these notes](#5457 (comment)).)

So

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
@ben-manes
Copy link
Contributor

[ERROR] javadoc: error - The code being documented uses modules but the packages defined in https://errorprone.info/api/latest/ are in the unnamed module.

I believe that is the -link-modularity-mismatch warning that can be downgraded to info in jdk18. It shouldn't be an error unless you fail on warnings (e.g. -Werror). When multi-release jars add the module metadata, they don't usually generate modular javadoc which causes this mismatch warning. It is harmless though.

copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to remove the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592. I assume that we'll now get the default from the JDK whose Javadoc binary we're using, which (again) will now be 11. That seems fine. We could consider setting it to 8 to match our normal build (which I thought I had remembered was the `maven-javadoc-plugin` default, but I don't think I'm seeing that, at least not under our current versions), but I don't see much downside to 11—or even to newer versions that we might someday use for Maven Javadoc generation, given that we keep the code compiling under new versions already.

Some other thing I'm wondering:

- I wonder if we should activate(?) some of the plugins, including the new toolchain plugins, in the `<plugins>` (not just `<pluginManagement>`) section of the parent `pom.xml`. Might that save us from having to do so in each separate `pom.xml`? (We might actually mostly get away with activating(?) them only in the main `guava` build: That _downloads and registers_ the toolchains, and then at least the other projects' _per-plugin_ toolchain configuration probably finds them? But for the more general configuration to work, I think we at least need to activate(?) `maven-toolchains-plugin` in each? I haven't experimented a ton with this.)

(See also [these notes](#5457 (comment)).)

So

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to change the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592. I first tried going with the default from the JDK whose Javadoc binary we're using, which (again) will now be 11. But that led to a problem in `org.codehaus.plexus.languages.java.jpms.CmdModuleNameExtractor`, which apparently tries to look up the module name for the `-source 11` run but uses the Maven run's JDK instead of the Javadoc toolchain or Maven toolchain. So now I've set it to 8 to match what we use for `maven-compiler-plugin`. (I _thought_ I had remembered that `maven-javadoc-plugin` defaulted to matching `maven-compiler-plugin`, even though that's weird. Maybe the two actually just read from the same Maven property or something, or maybe the behavior changed.)

Some other thing I'm wondering:

- I wonder if we should activate(?) some of the plugins, including the new toolchain plugins, in the `<plugins>` (not just `<pluginManagement>`) section of the parent `pom.xml`. Might that save us from having to do so in each separate `pom.xml`? (We might actually mostly get away with activating(?) them only in the main `guava` build: That _downloads and registers_ the toolchains, and then at least the other projects' _per-plugin_ toolchain configuration probably finds them? But for the more general configuration to work, I think we at least need to activate(?) `maven-toolchains-plugin` in each? I haven't experimented a ton with this.)
- I forgot the other thing while I was typing :(

(See also [these notes](#5457 (comment)).)

So

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
I'm not sure that any of these end up being _necessary_ to what I'm doing in #7331 (comment) / #3990 (comment). But the upgrade to `maven-surefire-plugin` changes that plugin's toolchain behavior, so I particularly want to use the new version there in advance of starting to use toolchains.

This includes a workaround for a bug in the JDK 8 javac. (I don't know why the bug is appearing only after these upgrades.)

```
Error:  /home/runner/work/guava/guava/guava/src/com/google/common/hash/BloomFilter.java:[78,29] error: cannot find symbol
  symbol:   class Serializable
  location: class BloomFilter<T>
  where T is a type-variable:
    T declared in class BloomFilter
```
RELNOTES=n/a
PiperOrigin-RevId: 655556207
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
I'm not sure that any of these end up being _necessary_ to what I'm doing in #7331 (comment) / #3990 (comment). But the upgrade to `maven-surefire-plugin` changes that plugin's toolchain behavior, so I particularly want to use the new version there in advance of starting to use toolchains.

This includes a workaround for a bug in the JDK 8 javac. (I don't know why the bug is appearing only after these upgrades.)

```
Error:  /home/runner/work/guava/guava/guava/src/com/google/common/hash/BloomFilter.java:[78,29] error: cannot find symbol
  symbol:   class Serializable
  location: class BloomFilter<T>
  where T is a type-variable:
    T declared in class BloomFilter
```
RELNOTES=n/a
PiperOrigin-RevId: 655637260
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation (also, for any other [affected plugins](https://maven.apache.org/guides/mini/guide-using-toolchains.html#prerequisites)) to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet. There are probably other simplifications that we could perform, as well, such as `maven-javadoc-plugin.additionalJOptions`.
   - Originally, I'd set up this CL to explicitly set only the toolchain of `maven-compiler-plugin` to 22. I had it using 11 for any other plugins (just Animal Sniffer, maybe?), I think from when I was trying to get toolchains to take effect at all. I've since changed this CL to set the _default_ toolchain to 22 while still including overrides for `maven-javadoc-plugin` and `maven-surefire-plugin`.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to change the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592. I first tried going with the default from the JDK whose Javadoc binary we're using, which (again) will now be 11. But that led to a problem in `org.codehaus.plexus.languages.java.jpms.CmdModuleNameExtractor`, which apparently tries to look up the module name for the `-source 11` run but uses the Maven run's JDK instead of the Javadoc toolchain or Maven toolchain. So now I've set it to 8 to match what we use for `maven-compiler-plugin`. (I _thought_ I had remembered that `maven-javadoc-plugin` defaulted to matching `maven-compiler-plugin`, even though that's weird. Maybe the two actually just read from the same Maven property or something, or maybe the behavior changed.)

Some other thing I'm wondering:

- I wonder if we should activate(?) some of the plugins, including the new toolchain plugins, in the `<plugins>` (not just `<pluginManagement>`) section of the parent `pom.xml`. Might that save us from having to do so in each separate `pom.xml`? (We might actually mostly get away with activating(?) them only in the main `guava` build: That _downloads and registers_ the toolchains, and then at least the other projects' _per-plugin_ toolchain configuration probably finds them? But for the more general configuration to work, I think we at least need to activate(?) `maven-toolchains-plugin` in each? I haven't experimented a ton with this.)
- I forgot the other thing while I was typing :(

(See also [these notes](#5457 (comment)).)

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
@cpovirk
Copy link
Member

cpovirk commented Jul 24, 2024

Reopening temporarily to get this into our internal tracking system.

@cpovirk cpovirk reopened this Jul 24, 2024
@GGGTTTTT
Copy link
Author

Thank you very much for your response. Due to usage of agent developed based on byte-buddy, it caused unexpected errors, but it hasn't caused significant problems.

@cpovirk
Copy link
Member

cpovirk commented Jul 26, 2024

Thanks. I'm not sure yet when our next release will be, but this will be in it.

@cpovirk cpovirk closed this as completed Jul 26, 2024
@cpovirk
Copy link
Member

cpovirk commented Aug 17, 2024

The fix is now released in https://github.com/google/guava/releases/tag/v33.3.0. Please let us know if you see any similar trouble in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 package=general type=defect Bug, not working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants