Checkstyle Improvements

[RussellSpitzer]

Ok so I've spent possibly to much time looking into this today, the reason some the checks don't work is basically just bugs in earlier versions of checkstyle. Since we get Checkstyle through Palantir's baseline-java we can upgrade basline-java and in doing so upgrade our Checkstyle. Doing this requires disabling a large number of warnings and checks that we don't currently care about as well as handling some deprecated configurations. I ran through a prototype of this and it's a bit annoying, but the upgraded checkStyle though does fix a bunch of our missing rules and false negatives.

Iceberg currently has Checkstyle 8.13 (Current is 8.34) via Baseline 0.55.0 (Current is 3.36.1)

For example

1. x=5 : WhitespaceAround, noting that there isn't whitespace around the assignment. This is fixed in 8.14
2. foo {\n\n\n} : MultipleBlankLines, MultilineRegexp has some weird behavior, I can't figure out why it triggers sometimes and doesn't other times, Seems fixed in 8.25
3. //fooo : Space after comment is just a fast rule we can add, we can fix this without a change in the style module

Not sure if anyone has strong feelings about doing some version bumps
or
We can fix at least the whitespace issues by adding some additional rules to our Checkstyle.xml

Note if you are using Checkstyle-IDEA you can bump up the Checkstyle version up to a bit to get some of these fixes, but they won't show up in Gradle.

[kbendick]

Forgive my ignorance, but this is for a separate linter that we use vs. Error Prone? I noticed we used the Palantir version of both.

I'm currently on a crusade to fix all of the Error Prone warnings, and potentially bring it up on the dev@ list to add a blocking check.

Is there a way to autofix these things very easily in the later version?

[kbendick]

Oh and while we're on the subject, here's my open PR on the ByteBuffers class of warnings from Error Prone. #1334

There's one area, writing ByteBuffers to Orc files, that I can't tell if I can be sure if the buffer was allocated on or off heap. The working example I have is a spark application writing to orc-files that has off-heap enabled memory.

I would love your input on this small PR if you have time (of if you could ping people more versed in Orc than myself. I'm more comfortable with Parquet).

[RussellSpitzer]

Yep we use this runtime called baseline

iceberg/build.gradle

Line 31 in a6a511a

classpath 'com.palantir.baseline:gradle-baseline-java:0.55.0'

Which has a ton of different checkstyle/findbug/linter things inside of it. They are only enabled on particular modules

iceberg/baseline.gradle

Lines 34 to 43 in a6a511a

	apply plugin: 'org.inferred.processors'
	if (!project.hasProperty('quick')) {
	apply plugin: 'com.palantir.baseline-checkstyle'
	apply plugin: 'com.palantir.baseline-error-prone'
	}
	apply plugin: 'com.palantir.baseline-class-uniqueness'
	apply plugin: 'com.palantir.baseline-reproducibility'
	apply plugin: 'com.palantir.baseline-exact-dependencies'
	apply plugin: 'com.palantir.baseline-release-compatibility'

Then we disable a bunch of the checks

iceberg/baseline.gradle

Lines 62 to 79 in a6a511a

	pluginManager.withPlugin('com.palantir.baseline-error-prone') {
	tasks.withType(JavaCompile).configureEach {
	options.errorprone.errorproneArgs += [
	// error-prone is slow, don't run on tests
	'-XepExcludedPaths:.*/test/.*',
	// specific to Palantir
	'-Xep:PreferSafeLoggingPreconditions:OFF',
	'-Xep:PreferSafeLoggableExceptions:OFF',
	'-Xep:Slf4jLogsafeArgs:OFF',
	// subclasses are not equal
	'-Xep:EqualsGetClass:OFF',
	// patterns that are allowed
	'-Xep:SwitchStatementDefaultCase:OFF',
	'-Xep:MissingCasesInEnumSwitch:OFF',
	'-Xep:TypeParameterShadowing:OFF',
	'-Xep:TypeParameterUnusedInFormals:OFF',
	]
	}

---

The plugin here automatically pulls in versions of other style / static analysis tools. They are hardwired into the plugin but I believe we can override them in the build if we really want.

See for example

https://github.com/palantir/gradle-baseline/blob/6dea8d29efaf1b0a5b9bcc7d7ebb51fe6b388b44/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineCheckstyle.java#L32

--

The Palantir package also does include some auto fix code which we are currently not using, or at least I haven't used them yet.

https://github.com/palantir/gradle-baseline#programmatic-application

I'll take a look at your PR ASAP too. Anything to cleanup the project is good imho!

[kbendick]

Thank you so much @RussellSpitzer. This is super informative for me and others I'm sure. I really appreciate it. I don't want to threadjack you so I'll keep my commentary and questions over on my PR if you don't mind, but I wanted to +1 the idea of your version bumps. And also possibly discuss getting the ability to use a github_token in legitimate github actions (which I'll keep the discussion on my PR so as not to threadjack this discussion).

I currently depend on just ./gradlew and not the IDEA plugin, though that will be changing. I think all of the rules you mentioned becoming usable are a win. I also personally believe that linting and dependencies and its whole lifecycle are something that needs to be kept up to date or it will might one day fall really far behind and become a blocker to something else critical and it can then become pretty painful.

If you have a PR with the updated dependencies, I'd approve it (not that would help you much but I'd download it and review it at least). If you're just looking to discuss, then I'm definitely a +1.

[RussellSpitzer]

Thanks for your support, I'll try doing that. I should also probably add a doc for new developers with IDEA setup and basic build setup. I'm pretty new to this as well and I found it kind of difficult to track some of this down

[RussellSpitzer]

This is an incomplete upgrade but enough to compile the new code and expose the new warnings.

Version Bumps
baseline 0.55 -> 3.36.2
git-version 0.9 -> 0.12.3

Checkstyle Changes Require for Compile

• LineLength no longer is a Treewalker so it needs to be moved
• MissingJavaDocMethod is now it's own module, allowMissingThrows is now validate throws
• MinLineCount was owned by MissingJavaDocMethod but we only set this to a huge number

FindBug Suppression Required for Compile

• StrictUnusedVariables must be disabled because we have several hundred cases of method parameters not being used in their bodies

New Checkstyle Violations

• RightCurly - We use Same but this triggers on private constructors with same line braces

New Baseline/Findbug Warnings

• Final Class - Class not declared final but all constructors are private
• Javadoc Missing Summary - No Summary mostly in our case this is where we have methods with only a @return
• RawTypes - Using a parameterized class without a parameterized type
• ReferenceEquality - We have a few instances of == instead of Equals
• LambdaMethodReference - Using s -> s.foo instead of s::foo
• LoggerEnclosingClass - using getLogger for super in child
• UncessaryAnonymousClass - Extend Function instead of using a lambda
• CollectionUndefinedEquality - Using equality or contains on Collection without it's own equals or badly behaving equality like (UTF8 strings)
• JDKObsolete : Collection of many things
• • Using Date (we use Date internally in some places we don't need to be using it)
• • Using linked list ( Warning argues that it is not performed compared to ArrayList and we should us that unless we need nulls or want to benchmark)
• • SortedMap is deprecated use NavigableMap instead
• Public constructor for Abstract Class
• CollectionStreamForeach : Collection.stream.foreach should be Collection.foreach
• MissingOverrides

[RussellSpitzer]

@rdblue ^ I'll have a PR available without fixing any of this for other's reproduction in a bit

[RussellSpitzer]

#1350 An example if anyone wants to see the new slew of warnings and such. I'll try to figure out the RightCurly Checkstyle thing as well since front he docs It doesn't make sense to me why it's failing ...

[rdblue]

Thanks for fixing this, @RussellSpitzer!