Add missing Override annotations

[zbynek]

Used ErrorProne's patch functionality to add override annotations where possible. Used regular expressions to make sure overrides are on a separate line.

This should make it easier to show errorprone warnings during compilation without going through much spam -- this is the most common errorprone as pointed out in #10322 (comment).

[niloc132]

I'm a little hesitant to fix these, like a lot of the other checkstyle issues

• it isn't indicative of a problem, just not the style that we want
• gwt was originally built for old enough java that override didn't exist (so the style wasn't even appropriate)
• there isn't an actual bug being fixed here, just a missing annotation. This could be wrong in two cases - if one of these fails the build with the annotation (but it doesn't), or if the author meant to override a method but wasnt able to or didn't at the time and got the signature wrong (but we can't know what they meant).

From this I tend to say this is better to add in diffs that change related code, to ensure that the author of a given diff did what they expected.

I wonder if we can collect warnings that only apply to the current diff like we do with checkstyle and ensure we fix when we change things, rather than bulk changes like this? From reviewdog/reviewdog#1087 it looks like this should be possible in theory (includes an example with errorprone), but it apparently doesn't work. (My quick attempt to match errorprone+ant, untested via command line, but seems to capture enough for our purposes)

---

The flip side of course is that this is an obvious change (...after one reviews all 2126 changed lines), and if it compiles it must be correct.

If we want this to be required, the build should also be changed to promote this to a compile failure.

[jnehlmeier]

If we want this to be required, the build should also be changed to promote this to a compile failure.

At work we configure errorprone checks which default to warning as error if we prefer the general intent of that check. And yes we do bulk updates to fix these checks (especially when updating errorprone and new checks have been added). We thought about doing it incremental as we came across it but given the code base is large it would probably never come to an end. So these checks would need to default to warning forever.

Fixing them and configure them as error improves code style consistency, makes code more readable, produces less noise during reviews and developers learn something if they run into such compile errors. If the build output is too verbose then warnings are not always noticed and they end up in reviews as well. At least that is my experience.

So in general I would be fine with that bulk change here.

[niloc132]

Agreed, I'm not against it, just want to caution against bulk "I fixed every typo in the project" changes (or worse a deluge of small "fix: Removed final from a private method" PRs from autonomous agents that gotta get those stats up). Ant output is pretty gross already too - though we could turn it up specifically for CI to capture related warnings.

Override is sort of a special case too, since there are so many missing, more than 10x the next most common errorprone warning, about about 2x every other warning combined. If we do fix it and turn up warnings overall, we're adding 2k or so more warnings in CI builds... though at least a few hundred of the next most common types are just a facet of GWT/JS and not going away any time soon.

But if it's important enough to fix, we should ratchet up warn to error, and at least start capturing/alerting other warnings so we can fix/ratchet or downgrade.

[tbroyer]

If we do fix it and turn up warnings overall, we're adding 2k or so more warnings in CI builds... though at least a few hundred of the next most common types are just a facet of GWT/JS and not going away any time soon.

Then those probably should be disabled?

[niloc132]

They all are disabled - -nowarn has been passed to javac via the ant build since the initial svn commit, nearly 20 years ago (though the line has moved a bit according to git, but the value has always been the same):

gwt/common.ant.xml

Line 58 in 4e6ae10

<property name="javac.nowarn" value="true"/>

The greater context is that if you turn that back on (and ask javac to print more than a handful of warnings), you get all these EP issues. #10322 (comment)

Maybe you're saying "if we do this change and if we turn on those other warnings and if we somehow don't make any other changes to isolate/permit accepted issues" then we should disable those checks? But the premise is "let's enable appropriate checks" so I think that's a given?

[jnehlmeier]

Agreed, I'm not against it, just want to caution against bulk "I fixed every typo in the project" changes (or worse a deluge of small "fix: Removed final from a private method" PRs from autonomous agents that gotta get those stats up).

Yes, but we could also settle on some contribution guidelines saying that such bulk updates are generally only accepted from core members with the goal to make better use of errorprone.

If we do fix it and turn up warnings overal

At work we have javac warnings turned on (with some javac warnings excluded) and we disabled ALL errorprone checks which default to warning and showed up when we introduced errorprone or when we update errorprone so we do not get overwhelmed. Then we turn them on in errorprone one by one, fix them and switch them to error.

So yes we should not just remove -nowarn. Errorprone need to be configured first (I don't know out of my head if there is any ep configuration in the ant script with regard to checks).

[tbroyer]

I was specifically talking about those “at least a few hundred of the next most common types [that] are just a facet of GWT/JS”: if we enable warnings (as a whole), then disable those "next most common types" that we don't care about (we've ignored them for all these years, so even if there are "real positives" there, it might not be worth getting the spam of the ones we consider "false positives").

And BTW, if we otherwise don't care at all about warnings (any warning, like today), then we should probably use -epDisableAllWarnings in addition to -nowarn to avoid the work of detecting those warnings and then ignore them anyway (technically, there could be "mandatory warnings" but Error Prone don't produce any of them).

We could also -XepDisableAllWarnings -Xep:MissingOverride:WARN or some other combination (e.g. bumping to error).

[zbynek]

With the last commit the most common patterns are suppressed + reviewdog should report the less common ones as comments (we can use separate invocation of reviewdog since we only care about annotations and no merged summary).

Also MissingOverride is bumped to trigger an error from now on.

[niloc132]

Could you introduce a deliberate EP warning so we can validate that this is working?

[niloc132]

Teeeeeechnically protobuf uses the javax.annotation.Generated annotation, though now they also use @com.google.protobuf.Generated

protocolbuffers/protobuf@31e57fc

This isn't worth changing, since our version of protobuf is so old (and can't change, since Google never open sourced the build for the dev mode plugins...), just pointing it out.

[niloc132]

It seems that this is selectively disabling some options, but by doing so in errorprone.args, the suggestion seems to be that we want to re-enable them for certain projects. Instead however, only user/build.xml overrides this, by disabling all warnings.

Wouldn't it make more sense to move these to the compilerarg tags where we want them disabled globally? Otherwise, if we were to, for example, attempt to selectively disable one more check in dev/build.xml, we would end up re-enabling all of the rest? Or, something like only put -XepDisableWarningsInGeneratedCode -xEp:MissingOverridde:ERROR here in a compiler arg, then leave user disabling all warnings (except that), and leave the rest of these as warnings until we fix them as we interact with the code?

No firm thoughts here except these are all potential bugs.

[zbynek]

I didn't really consider using different settings for different modules. Maybe we can look into that once we need it?

The only override is in the no-servlet compilation (user/build.xml, as you pointed out) that prevents all issues from user module to be reported twice (technically there could be some issues specific to Jakarta, but IMHO not worth the overhead).

The selection of things to ignore is based on number of reports which is not a great metric, but should avoid having too many annotations in PRs. Follow-up tickets can be made to reduce this list.

[zbynek]

Reported warnings with the current filter:

     1 AmbiguousMethodReference
      1 ExtendsObject
      1 IncorrectMainMethod
      1 InputStreamSlowMultibyteRead
      1 MockNotUsedInProduction
      1 MutablePublicArray
      1 StaticGuardedByInstance
      2 AlmostJavadoc
      2 AssertEqualsArgumentOrderChecker
      2 EmptyTopLevelDeclaration
      2 Finally
      2 InlineTrivialConstant
      2 LogicalAssignment
      2 MissingImplementsComparable
      2 MultipleUnaryOperatorsInMethodCall
      2 NarrowingCompoundAssignment
      2 StaticAssignmentOfThrowable
      2 ThreadPriorityCheck
      2 TypeNameShadowing
      2 WaitNotInLoop
      3 UnnecessaryStringBuilder
      4 DateFormatConstant
      4 IncrementInForLoopAndHeader
      4 LabelledBreakTarget
      4 ModifyCollectionInEnhancedForLoop
      4 NonCanonicalType
      4 ToStringReturnsNull
      5 EqualsIncompatibleType
      5 NegativeCharLiteral
      5 TypeEquals
      5 UnusedTypeParameter
      6 ArgumentSelectionDefectChecker
      6 EscapedEntity
      6 JavaLangClash
      6 LoopOverCharArray
      6 ReturnFromVoid
      6 SuperEqualsIsObjectEquals
      7 HidingField
      7 InvalidThrows
      7 ShortCircuitBoolean
      7 TimeUnitConversionChecker
      8 EqualsUnsafeCast
      8 NarrowCalculation
      8 URLEqualsHashCode
      9 ComplexBooleanConstant
      9 OverrideThrowableToString
      9 TypeParameterShadowing
     10 OrphanedFormatString
     11 BigDecimalEquals
     11 InconsistentCapitalization
     11 IntLongMath
     11 StringCaseLocaleUsage
     12 ModifiedButNotUsed
     13 DefaultPackage
     13 DoNotCallSuggester
     13 InlineFormatString
     13 LongDoubleConversion
     13 removal
     14 OutlineNone
     15 UnusedNestedClass
     16 FutureReturnValueIgnored
     16 InlineMeInliner
     17 UnicodeEscape
     18 SynchronizeOnNonFinalField
     20 BadInstanceof
     20 MixedMutabilityReturnType
     21 UnsynchronizedOverridesSynchronized
     23 FloatingPointLiteralPrecision
     23 ReturnAtTheEndOfVoidFunction
     24 MissingFail
     25 AssertionFailureIgnored
     25 CatchAndPrintStackTrace
     31 CollectionUndefinedEquality
     31 EqualsGetClass
     31 ImmutableEnumChecker
     32 MissingCasesInEnumSwitch
     33 StaticAssignmentInConstructor
     35 CatchFail
     35 StringCharset
     43 ClassNewInstance
     43 InvalidParam
     44 FallThrough
     45 UndefinedEquals
     61 UnnecessaryAsync
     84 UnusedMethod
     94 EmptyCatch
    136 ClassCanBeStatic

[niloc132]

Looking at #10348, it doesnt look like they are being reported in the PR still?

[zbynek]

it doesnt look like they are being reported in the PR still?

@niloc132 now it worked (I forgot reviewdog only considers the changed parts of the file), but it reports it twice

[niloc132]

Sorry it took me a while, but i've read it all now. Just a few comments.