Add support for linters

[nedtwigg]

This is just a proposal, open to other UIs / designs / implementations

This PR adds a Lint pojo, as well as the new method FormatterFunc::lint with the following default implementation:

spotless/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java

Lines 41 to 44 in e8e5df5

	default List<Lint> lint(String content, File file) throws Exception {
	apply(content, file);
	return Collections.emptyList();
	}

We now split formatting into two parts: the composable String -> String part and the non-composable List<Lint> part. Before this PR, the only way to "lint" was to throw a single exception. In this PR, those exceptions are now caught and transformed into a lint 7a1e72a which has the following implications:

• apply always succeeds (it traps errors and continues, rather than bubbling them up to the user)
• check shows every exception to the user at once (with a best-guess line number from parsing the error message)
  • fixes Don't fail after first codestyle violation #287
• steps are free to be a pure linter or a hybrid formatter/linter
  • the value-add from Spotless is
    • git ratchet
    • maven/gradle/sbt and up-to-date/build-cache/config-cache
    • unified spotless:off/on across formatters and linters (take toggleOffOn into account for linters #719)
  • for now, the limitation from Spotless is that the linter must do source-only analysis
    • it's very possible to workaround this in the future
  • AndroidLint support #1089
  • Support for Kotlin Formatter Detekt #143
• we no longer have a FormatExceptionPolicy, just a LintSuppressionPolicy dd5165f

Gradle integration notes

There's an interesting subtlety here specific to the Gradle plugin, but it could also matter for maven if we go further with up-to-date checking. The spotlessJava task generates a folder of "clean" files for each file which is found to be unclean. Here (a25eded) we extend that to also have a folder of "lint" files for each file found to have non-zero lints . However, the lints will be different if the user calls ./gradlew spotlessCheck or instead ./gradlew spotlessApply spotlessCheck. The "clean" content will be the same, but the line numbers and content of the lints will depend on whether apply was called or not, spotlessJava can't know that ahead of time, so for now we just compute the lints twice (89339cd). That's not as expensive as it seems because we can skip the double-compute for clean files, which is almost-always almost-all files.

Open questions

There might be other things to think about too, but these are what is on my mind:

• should spotlessApply fail if there are any lints?
  • before this PR we failed on the first "lint" / exception, after this PR apply always succeeds
  • I think our goal of "think about formatting less" means that we should sweep lints under the rug until spotlessCheck (we could make applyFailsOnLint = true available as an option)
• should we support "info/warning/error" levels?
  • I think the answer is no, because that's the job of the step
  • It should be fooLint().level('info').confidence(70), so that each step can handle that
  • At the spotless level, it should just be "lint = failure", suppress with something like dd5165f

spotless {
    suppressLintForPath 'blah.txt'
    suppressLintForStep 'blah'
    suppressLintForCode 'SOME_CODE'
    suppressLintForMsgMatchesRegex 'blah'
    printSuppressedLintsToConsole = false

• should we export a "report" of some kind?
  • spotlessCheck could add comments and suggested fix to GitHub PR #655
  • for now we just print IDE-parseable stuff to console and make no guarantees it won't change
  • if we make a format, it should definitely be a reviewdog standard
  • SARIF is an option too
    • reviewdog plans to support it but its too complex https://github.com/reviewdog/reviewdog/tree/master/proto/rdf#the-static-analysis-results-interchange-format-sarif
    • github supports part of the standard out of the box https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning
    • this can be used freely in lib-extra https://github.com/Contrast-Security-OSS/java-sarif

Moving forward

It would be great to have some draft PRs based on this feat/lint branch that exercise the new lint method. I'm very confident that FormatterStep and FormatterFunc are stable. The uncertain part is what fields Lint will have, and that will be determined by a combination of:

• what do the linters have to give
• what do sarif and reviewdog want

This is a big change so it will happen slowly :)

[rougsig]

Can be. Or:

May be different tasks can be the Best solution? Like spotlessLint spotlessFormat?

[nedtwigg]

Degrees of freedom are cost (more surface to maintain, more ways for things to go wrong). It's only worth the cost if you get some value out of it.

The magic of spotless imo is that check says why your build failed, and apply fixes that as much as possible, and there's exactly nothing else to say. This simplicity is why I think there should not be any new tasks, and also why I think that Spotless should have no concept of "error/warning/info" - that should be configured per-linter.

[rougsig]

What is the plan for this feature? Waiting for me with a detekt PR?

[nedtwigg]

I'm waiting until we have at least two FormatterStep which use this feature, preferably three. Anyone can contribute a step by opening a PR against this feat/lint branch.

[nedtwigg]

A good one to add: https://github.com/openrewrite/rewrite

[nedtwigg]

I have been convinced that this is totally worth doing. There are three reasons:

1. Surprisingly, it simplifies the existing functionality by making our error handling simpler
2. It simplifies integration with hybrid fixers/linters like KtLint
3. It opens up a new class of tool like Spotbugs

I'm not sure if reason 3 is a good idea or not, but reasons 1 and 2 alone make it worthwhile.

In terms of disruption to our stakeholders:

• end users -> no disruption at all
• contributors to FormatterSteps -> no change, new abilities if they want, only real problem is merge conflicts from Breaking changes to Spotless' internal testing infrastructure testlib #1443 which needs to happen whether we do lint or not
• contributors to build plugins -> a little bit of change
• contributors to IDE plugins -> potentially a fair bit of change

spotless-lib is currently on version 2.x. I'm planning to roll this out in three stages:

• Breaking changes to Spotless' internal testing infrastructure testlib #1443
  • brings merge conflicts to basically every PR that touches any tests but needs to happen someday
  • only affects test, no change for end users or API
• Internal-only breaking changes #1096
  • spotless-lib 3.x causes zero change for end users
  • almost zero change for FormatterStep contributors
  • a tiny bit of chang to build plugins
• spotless-lib 4.x brings lint (this PR)

The existing PR's have all gone stale (a year old), but I'll be reusing them by force-pushing.

[jbduncan]

@nedtwigg Haha, I've only just seen your request for me to review this January last year, so sorry about that. Let me know when you're finished with this PR, and I'd be more than happy to review it. :)

[nedtwigg]

Thanks! It'll probably be a week or two before it's ready :)

[jbduncan]

On the subject of OpenRewrite, I've actually used it a bit on a small project with some success. But IIRC it focuses purely on "solving" rather than "yelling", not to mention it already has a Gradle plugin, so it's not clear to me how Spotless linting would help. I don't suppose you can remember why you mentioned it now, can you?

But perhaps an advantage of incorporating OpenRewrite into Spotless would be the speed gained from Spotless's caching mechanism (at least on the Gradle side).

[blacelle]

for now, the limitation from Spotless is that the linter must do source-only analysis

I did not dig into the PR, but maybe this limitation is slightly stricter for now: 'the linter must do source-only analysis, one a per-file basis'. i.e. I suspect the first iteration will not be able to receive a whole Set of source-files in a single batch.

A good one to add: https://github.com/openrewrite/rewrite

OpenRewrite behave (much) better when called on a source-set, than on each individual files.

But IIRC it focuses purely on "solving" rather than "yelling",

I agree. (I integrated it as an underlying engine of CleanThat, which is focused on solving than yelling)

enum FailOnLint { CHECK, CHECK_AND_APPLY, NOFAIL }

About CHECK_AND_APPLY, I feel people are more interested in a APPLY_AND_CHECK (i.e. try fixing, and please yell is there is still outstanding issues).

[ztlr]

Is this PR still relevant for a future release although it was labeled as "pr-archive"?
We are highly interested in this since it will fix #287.

[nedtwigg]

This PR is absolutely still relevant to a future release, but it will be a while.

[WillHolbrook]

Hello I'd be interested in having this feature implemented and released. How would I be able to help contribute to getting this merged? I haven't contributed to open source before so sorry if this is a newbie Q

[nedtwigg]

@WillHolbrook which linter would you like to use? Merging this is blocked on me, but I could use help testing the API with specific steps that need the linter.