A couple of fixes in MoreAnnotations

[msridhar]

I discovered these issues while using this code in NullAway; see uber/NullAway#1055. (We support older Error Prone versions so can't directly call this API and had to adapt the code instead.) The previous code didn't handle explicit annotations on lambda parameters (e.g., (@Nullable Object x) -> { ... }) and annotations on enum constants. Not sure the best way to add tests for this but happy to add them if given a suggestion. Actually the enum constant case was weird; I was unable to repro with bytecodes output by javac and I only observed the case with an ijar from Bazel.

FYI @cpovirk @cushon

[cushon]

Thanks for the fix!

Not sure the best way to add tests for this but happy to add them if given a suggestion.

So the tests for this util package are in limbo because they create a cyclic dependency in the maven build (but not the internal blaze build), and they never got fully open sourced. I have a change to export the files, but exclude them from the maven build until that's fixed: #4621

If you have a self-contained example that could be used as a test like the ones in this file, I can help add it: https://github.com/google/error-prone/pull/4621/files#diff-e6d404ec63bd19b1baf132220462ccf5fba126f25dc6c6f1773b83c71fc12b20

Actually the enum constant case was weird; I was unable to repro with bytecodes output by javac and I only observed the case with an ijar from Bazel.

Interesting. Is that a public jar you could attach to the bug, ideally both the ijar and the non-ijar jar for the same library? I'd be interested in taking a look at it, if it's a ijar bug I can try to get that routed.

[msridhar]

I'll try to look at tests later. For the enum constant case, this was the jar:

https://repo1.maven.org/maven2/org/apache/beam/beam-runners-google-cloud-dataflow-java/2.52.0/beam-runners-google-cloud-dataflow-java-2.52.0.jar

I don't know if I can share the ijar. The reference in the client code was to DataflowWorkerLoggingOptions.Level.INFO and that's where the ENUM_CONSTANT case was popping up. (Also strange as I couldn't find the annotations in the source code.) Maybe that's enough to repro?

[cushon]

The bytecode for DataflowWorkerLoggingOptions.Level.INFO is the following. (I also compared the ijar output, ijar doesn't appear to do anything interesting to that field.) The source code doesn't have annotations, so I think their release builds are using the Checker Framework which is doing its magic to add the annotations:

  public static final org.apache.beam.runners.dataflow.options.DataflowWorkerLoggingOptions$Level INFO;
    descriptor: Lorg/apache/beam/runners/dataflow/options/DataflowWorkerLoggingOptions$Level;
    flags: (0x4019) ACC_PUBLIC, ACC_STATIC, ACC_FINAL, ACC_ENUM
    RuntimeVisibleTypeAnnotations:
      0: #8(): FIELD
        org.checkerframework.checker.nullness.qual.UnknownKeyFor
      1: #9(): FIELD
        org.checkerframework.checker.nullness.qual.NonNull
      2: #10(): FIELD
        org.checkerframework.checker.initialization.qual.Initialized

This isn't bytecode that you can get from javac, there's no way to write type annotations on the type of a desugared enum constant field. Annotating enum constants just results in declaration annotations, not type annotations:

import org.checkerframework.checker.initialization.qual.Initialized;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.UnknownKeyFor;

enum E {
  @UnknownKeyFor
  @NonNull
  @Initialized
  ONE
}

  public static final E ONE;
    descriptor: LE;
    flags: (0x4019) ACC_PUBLIC, ACC_STATIC, ACC_FINAL, ACC_ENUM
    RuntimeVisibleAnnotations:
      0: #33()
        org.checkerframework.checker.nullness.qual.UnknownKeyFor
      1: #34()
        org.checkerframework.checker.nullness.qual.NonNull
      2: #35()
        org.checkerframework.checker.initialization.qual.Initialized

So this is kind of weird, but I guess it's a field in bytecode, and if there are type annotations there it's probably fine to just interpret them like we would for normal fields.

[msridhar]

Thanks for digging in, @cushon! And sorry for the ijar red herring. Somehow I couldn't repro this in a NullAway unit test even using that exact jar, but probably I messed something else up, as the annotations are clearly there in the bytecodes.

[msridhar]

@cushon I've attempted to add a test for the explicitly-annotated lambda case but I'm not sure if it will pass. It's possible the MoreAnnotationsTester checker needs to be changed to also match lambda trees.

[msridhar]

As a follow up i re-tested NullAway with the linked Apache Beam jar and confirmed I could reproduce the previous crash, and that the line case ENUM_CONSTANT: from this PR fixes it. I must have messed up with my testing before.

[cushon]

Sounds good, thanks for confirming. The test passes as-is, thanks for adding it.