[BUG] ErrorProne compiler reports "MissingSummary" on @SuperBuilder w/ 1.18.18

[mmoayyed]

Describe the bug

ErrorProne compiler 2.5.1 complains about MissingSummary when @SuperBuilder is used in 1.18.18.

To Reproduce

Stacktrace follows:

> Task :api:cas-server-core-api-authentication:compileJava
/Users/Misagh/Workspace/GitWorkspace/cas-server/api/cas-server-core-api-authentication/src/main/java/org/apereo/cas/authentication/AuthenticationPolicyExecutionResult.java:18: error: An unhandled exception was thrown by the Error Prone static analysis plugin.
@SuperBuilder
^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.5.1
     BugPattern: MissingSummary
     Stack Trace:
     java.lang.IllegalArgumentException: Start [-1] should not be less than zero
        at com.google.common.base.Preconditions.checkArgument(Preconditions.java:190)
        at com.google.errorprone.fixes.IndexedPosition.<init>(IndexedPosition.java:32)
        at com.google.errorprone.fixes.SuggestedFix$Builder.replace(SuggestedFix.java:213)
        at com.google.errorprone.bugpatterns.javadoc.MissingSummary.generateReturnFix(MissingSummary.java:139)
        at com.google.errorprone.bugpatterns.javadoc.MissingSummary.handle(MissingSummary.java:109)
        at com.google.errorprone.bugpatterns.javadoc.MissingSummary.matchMethod(MissingSummary.java:80)
        at com.google.errorprone.scanner.ErrorProneScanner.processMatchers(ErrorProneScanner.java:450)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:740)
        at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:151)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:898)
        at jdk.compiler/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 jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
        at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
        at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
        at jdk.compiler/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 jdk.compiler/com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:808)
        at jdk.compiler/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 jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
        at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)

Expected behavior

Passing build.

Version info (please complete the following information):

• Lombok 1.18.18
• JDK 11.0.10
• Gradle 6.8.1
• ErrorProne 2.5.1

Additional context

• Disabling the MissingSummary with ErrorProne bypasses the issue.
• Works OK with Lombok 1.18.16

[Rawi01]

Can confirm that this does not work.
This happens because lombok does not add start and end positons for generated Javadoc comments, most likely caused by the changes in #2684.

[rspilker]

@mmoayyed Can you please test the edge release and report if that resolves the exception?

[mmoayyed]

Yes, the edge release certainly does remove the exception, but it does not fix the problem. After switching and instead of the stacktrace, I now see the following:

warning: [MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.
@SuperBuilder
^
    (see https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment)
  Did you mean 'Returns {@code this}.'?
error: warnings found and -Werror specified

This is what the class structure looks like:

@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS)
@Getter
@SuperBuilder
@ToString
public class AuthenticationPolicyExecutionResult implements Serializable {
    private final boolean success;
}

I suspect the javadoc that is generated for the success() method is missing the appropriate summary. If I can help further, please let me know.

[sergeykad]

I started getting these errors for the regular @Builder after switching from Lombok 1.18.16 to 1.18.20

Yes, the edge release certainly does remove the exception, but it does not fix the problem. After switching and instead of the stacktrace, I now see the following:

warning: [MissingSummary] A summary fragment is required; consider using the value of the @return block as a summary fragment instead.
@SuperBuilder
^
    (see https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment)
  Did you mean 'Returns {@code this}.'?
error: warnings found and -Werror specified

[hakanai]

This looks like the current issue we're getting as well.

In our case it's Lombok v1.18.24 + errorprone v2.13.1, and we're only using @Builder, not @SuperBuilder, but it's flagging the same MissingSummary check. I'm currently desperately trying to find any relatively new combination of these two which work together.

[rzwitserloot]

This sounds like a linting tool that requires javadoc on some node we generate. A few notes on this:

• It's a silly rule that you shouldn't be using. Forcing javadoc will result in folks writing pointless javadoc, which is a DRY violation. In a place that you can't unit test, even. I'm not in a position to tell you that this is the only resolution available, but it should perhaps indicate that we rate such issues as extremely low priority.
• We stick @Generated on lots of places, evidently the linter tool ignores those annotations. Unfortunately, linters don't make it easy for us. Are you aware of the various lombok.config options, and did you check your linter tool for any configuration in the vein of 'do not lint-check anything that is annotated with these annotations'? Is this a problem where these settings do not work, or is this a problem where it's not clear that they need to be set up?
• This doesn't sound related to the actual issue at all. A new issue should be filed with this 'MissingSummary' issue.

NB: When I mean 'silly' rule, this is what I mean. Imagine a codebase with this method:

/**
 * Counts the number of days.
 * @return int The number of days.
 * @throws IllegalStateException If the object is in a state where counting the number of days doesn't make sense or is not possible.
 */
public int countDays() throws IllegalStateException {
  ...
}

Just clear your mind of the style you've been (forced to / used to) write for a moment. Imagine you have programmed in some other language with different conventions for a second.

Now that you've done that, look at that code again.

It is a RIDICULOUS pile of DRY violations. It's staggering amount of pointless boilerplate. The entire comment adds absolutely nothing whatsoever. The javadoc tool doesn't skip javadoc-less public members, in fact, even the docs get needlessly polluted with this stuff if you do add the javadoc like the way its written above.

In fact, it gets even worse: Code readability and API usability falls in 3 tiers and there's a big gap between all 3 of them:

1. Self-evident code – you can look at it and understand how it works, and comments are neither required nor desired. For APIs: The type name, hierarchy, and combination of method name + argument types on their own are enough to fully understand what it is for.
2. Documented code – the code requires reading the comment / the API requires reading the docs; without it, you are either likely not to understand what it does, or worse, you get misled and first impressions lead to wrong conclusions.
3. The same code (would need comments / API docs, as first impressions either leave you befuddled or worse, lead to wrong conclusions), but it lacks comments/documentation.

The problem with forcing javadoc on all nodes is that you 'excuse' type 2 code. Normally the benefit of type 1 code is that it's fairly obvious you wrote type 1 code: You never get the itch to add those docs and even if you think explicitly on what it should read, you can't think of anything that the method name and params themselves are already conveying. However, force javadoc and you lose this.

You'd think that at least forcing javadoc prevents type 3 code, but, no it does not. See my example: The javadoc note about IllegalStateException is entirely useless and adds nothing new, but the linter can't know that. It's the one thing where API docs in this hypothetical methods could have provided some benefit, by explaining which state(s) are considered invalid and perhaps under which conditions you'd end up there. The docs don't include any of that, and the linter tool is never going to flag this; that's hard AI.

[hakanai]

Yes, ErrorProne is essentially a linting tool. It just plugs into the compiler so that it can run during the compilation process to save some build time.

And yes, it does ignore code with the @Generated annotation, if you configure it to do so, which we have.

But what's interesting with this one is that we only get the issue when we upgrade Lombok to the current version. The original reporter filed this against 1.18.18; we see it on 1.18.20. If we remain on 1.18.12, we don't get the issue. Actually, I was only upgrading to the newer version to see if it had fixed some other issue with Lombok's generated code.

Edit: Additional edit just to say that it's very possible this issue is actually an issue in ErrorProne, because it really looks like it's ignoring the annotation in this case for some reason. If I roll ErrorProne back to an earlier version - that too fixes the issue. So it apparently only occurs when the latest version of both are in use.

[hakanai]

Test repo: https://github.com/hakanai/errorprone-lombok-missingsummary

This build deliberately elevates MissingSummary to ERROR to make it easier to see when it fails.

If I edit Lombok version to 1.18.16 the build passes. Any later version fails.

If I comment out the elevation of MissingSummary to ERROR, the build passes (as it should) and yet when I inspect what IDEA sees when it decompiles the classfile, the code looks exactly the same as the earlier version, including all the annotations.

Edit: Also on this project, for some reason, rolling ErrorProne back to 2.4.0 doesn't make it work. So it's behaving subtly differently to our real project somehow.

[rzwitserloot]

So it apparently only occurs when the latest version of both are in use.

Thank you for figuring that one out. Sounds like that took some time!

And a test repo to boot. Much appreciated.

[rzwitserloot]

low-prio: While it's probably something we can fix (given the test repo, and that it seems to have worked before), it remains catering to idiotic linting rules. If I could detect that rule was on I'd 'fix' this by emitting a warning that your linting tools are badly configured :)

[hakanai]

What MissingSummary usually flags is where you have put @return in the Javadoc, but then not put in any summary line.

In these cases, it's better to put that description as the summary, because that's what it shows in the summary list at the top of the Javadoc page.

https://github.com/google/error-prone/blob/master/docs/bugpattern/javadoc/MissingSummary.md

I kind of agree with this particular linter rule. If you're going to put in any description at all, it might as well be written in such a way that it appears in the summary.

Also, all previous commentary about it being a DRY violation is a fallacy, because we're talking about what a tool generates, and DRY only applies to what you are manually writing. :)

[rzwitserloot]

Also, all previous commentary about it being a DRY violation is a fallacy

No, unless you somehow turned that rule on to apply only to generated code, which you obviously didn't.

[rzwitserloot]

To be clear, my opinion about linting tools is what it is, I mention it to clarify what you can expect us to work on / agree to. Not for you to hold a pointless debate about, hence such commentary will be deleted. If you'd like to hold the discussion, forum, not issue tracker.

[crutcher]

Ran into this again in 2023; so it's clearly not fixed at head.
The issue isn't which ones right; it's that they're both popular.
Lombok is upstream of Errorprone; and the fix could/should go here.

[Rawi01]

You can add -XepDisableWarningsInGeneratedCode to ignore the generated code.