Update checkstyle and fix violations

[tlrx]

This PR updates the version of Checkstyle from current 5.9 to 7.1 and fix a bunch of new violations.

A more recent version would allow us to use some new checkstyle modules (like Javadocs).

RedundantModifier is improved and now detects more violations that are fixed by this PR. The changes fall into three categories (thanks @nik9000 for the nice summary):

• public methods on non-public classes don't make sense. Remove the modifier and let the method inherit the modifier.
• try-with-resources variables should not be declared final because they are implicitly final. final adds no extra information for the reader. I guess that makes them names rather than variables....
• enums are alway static so there is no need to declare them static.
• final doesn't do anything on anonymous classes because they can't be extended anyway. So we just remove the modifier.
• Classes declared inside of interfaces are always static so we shouldn't also declare them as static.

For the record, most of the changes were automated using this small Bash script:

#!/bin/bash
input="/tmp/violations.txt"

# File is composed of lines like:
# /tmp/elasticsearch/src/test/java/org/elasticsearch/action/Action.java:10:9:public
# 
while IFS=':' read -ra ADDR; do
    if [ "${ADDR[3]}" != "" ]; then
        echo "${ADDR[0]}" | xargs sed -i "${ADDR[1]}s/${ADDR[3]} //"
    else 
        echo "Skipping file ${ADDR[0]} line ${ADDR[1]} word ${ADDR[3]}"
    fi
done < "$input"

Checkstyle was not updated to the latest version 7.1.2 because it reports violations for final parameters in interface methods (checkstyle/checkstyle#3322) and we have a lot of these for good reason.

I'm really sorry for the boringness of this kind of pull request.

[nik9000]

I'll reviewing it. I like boring PRs while drinking coffee in the morning.

[nik9000]

we have a lot of these for good reason.

I think if we think our reasons are good we should pop over to the checkstyle project and file an issue asking to be able to disable that portion of the check. Is our reason that we want implementers to declare those parameters final and we like that IDEs do that by default if the parameters are final in the interfaces? Beyond that I can't think of any reason to declare parameters on interface methods final.

[nik9000]

I'd never really thought of this but it makes sense.

[tlrx]

Is our reason that we want implementers to declare those parameters final and we like that IDEs do that by default if the parameters are final in the interfaces?

Yes, this is the "good reason" I'm talking about. I didn't check every place but each time such a violation was reported I found a good reason to let the final modifier in place.

I think if we think our reasons are good we should pop over to the checkstyle project and file an issue asking to be able to disable that portion of the check.

Makes sense.

[nik9000]

All the code changes look good to me. They fall into three categories:

• public methods on non-public classes don't make sense. Remove the modifier and let the method inherit the modifier.
• try-with-resources variables should not be declared final because they are implicitly final. final adds no extra information for the reader. I guess that makes them names rather than variables....
• enums are alway static so there is no need to declare them static.
• final doesn't do anything on anonymous classes because they can't be extended anyway. So we just remove the modifier.
• Classes declared inside of interfaces are always static so we shouldn't also declare them as static.

[nik9000]

Yes, this is the "good reason" I'm talking about. I didn't check every place but each time such a violation was reported I found a good reason to let the final modifier in place.

Personally I'm not sure that "because implementers should declare them final" is really a good reason and I wouldn't be surprised it the checkstyle folks argued that way, but it is worth asking.

[javanna]

Question: why do we have all these violations to fix just because we are upgrading checkstyle? Did the set of applied rules change as well? This all makes sense (I was tired too of seeing all these warnings in my IDE) but it would be good (for next time) to do one category at a time maybe? Can we also update the description of the PR with the nice summary that Nik posted?

[nik9000]

Did the set of applied rules change as well?

The meaning of the RedundantModifier changed. There doesn't look like there is a way to disable each category while we upgrade either which is a pain.

[tlrx]

Question: why do we have all these violations to fix just because we are upgrading checkstyle? Did the set of applied rules change as well?

Not really the set of rules but rather the way the rule are working. RedundantModifier check is more "complete" in 7.1 than in 5.9 and that's why it reports new violations.

This all makes sense (I was tired too of seeing all these warnings in my IDE) but it would be good (for next time) to do one category at a time maybe?

Sadly we can't because we can't really configure the granularity of the checks.

Can we also update the description of the PR with the nice summary that Nik posted?

Sure, I should have done that.

[javanna]

Sadly we can't because we can't really configure the granularity of the checks.

Maybe we could have fixed the issues one category at a time before updating checkstyle for instance? Not sure though.

Anyways, thanks for fixing these!

[nik9000]

Maybe we could have fixed the issues one category at a time before updating checkstyle for instance? Not sure though.

Yeah, we certainly could have. If these changes hadn't been super mechanical stuff I could review while leisurely sipping coffee I'd have asked for that. And we should totally keep that as an option for next time!

[tlrx]

Closed in favor of #22960