Use kondo api

[thumbnail]

Brief

Removes vendorized clj-kondo source in favor of public api.

discuss compiler and disabled linter-options in kondo

QA plan

cache invalidation:

run this snippet:

(formatting-stack.protocols.processor/process! 
  (formatting-stack.processors.kondo/new)
  (formatting-stack.strategies/all-files))

notice folder creation:

$ ls -lah .clj-kondo/*/clj/formatting-stack.util.transit.json

Run the snippet again; notice new modified date.

Author checklist

• I have QAed the functionality
• The PR has a reasonably reviewable size and a meaningful commit history
• I have run the branch formatter and observed no new/significative warnings
• The build passes
• I have self-reviewed the PR prior to assignment
• Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
  • Correctness
  • Robustness (red paths, failure handling etc)
  • Modular design
  • Test coverage
  • Spec coverage
  • Documentation
  • Security
  • Performance
  • Breaking API changes
  • Cross-compatibility (Clojure/ClojureScript)

Reviewer checklist

• I have checked out this branch and reviewed it locally, running it
• I have QAed the functionality
• I have reviewed the PR
• Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
  • Correctness
  • Robustness (red paths, failure handling etc)
  • Modular design
  • Test coverage
  • Spec coverage
  • Documentation
  • Security
  • Performance
  • Breaking API changes
  • Cross-compatibility (Clojure/ClojureScript)

[vemv]

Great work!

I'd have exactly one PR for all things Kondo, fixing all rough edges at once.

• An essential feature is cache invalidation (changed code should invalidate cached kondo analyses)
  • Should be part of the QA plan
• Another essential feature is selective choice of linters, depending on the type of consumer project
  • This is hinted in maybe it's ideal to enable arity checking only for a specific case: if at least 1 .cljs file is present in the project (Use Kondo's new Clojure API + project cache #91)
  • Might need some careful thinking about the notion of strategies. Discuss first
• The most meaningful QA plan is ensuring that there are false positives in cljs (especially wrt arity) with f-s 1.0.1, and that they are gone with this PR
  • Another usual suspect is our test-runner ns pattern. It always complains about unused requires

Given we are changing things like strategies, protocols etc, it's likely that this PR is best left open for some time.

I would not merge #102 as-is because it would not add a tangible benefit (internals change, not without risk as Kondo itself may have changed)

[vemv]

Make sure that all new linters added since the last version are excluded from JVM clojure, if they are redundant with Eastwood, or if they can possibly give false positives

[thumbnail]

#{:datalog-syntax
  :type-mismatch
  :misplaced-docstring
  :consistent-alias
  :if
  :use
  :unused-referred-var
  :not-empty?
  :file
  :refer-all
  :unresolved-symbol
  :cond-else
  :missing-docstring
  :unused-import
  :deprecated-var
  :unbound-destructuring-default
  :syntax
  :not-a-function
  :duplicate-require
  :unused-private-var
  :unresolved-namespace
  :unreachable-code
  :redefined-var
  :unused-binding}

These are the new linters since 2019.05.19-alpha. The following are redundant i think;

• :consistent-alias: duped by how-to-ns
• :duplicate-require: duped by clean-ns
• :unused-import: duped by clean-ns
• :unused-namespace: duped by clean-ns
• :unused-referred-var: duped by clean-ns
• :deprecated-var: duped by Eastwood
• :redefined-var: duped by Eastwood
• :refer-all: can't exclude clojure.test

But I'm not confident this is all. clj-kondo has less extensive documentation on linters and their behavior (compared to eastwood) so it's hard to determine the exact result.

[vemv]

Alright, thanks for this one!

This is my initial judgement:

:cond-else ;; NOK (some of use prefer `true`)
:consistent-alias ;; f-s
:datalog-syntax ;; ?
:deprecated-var ;; eastwood
:duplicate-require ;; clean-ns
:file ;; ?
:if ;; ?
:misplaced-docstring ;; eastwood
:missing-docstring ;; NOK (docstrings at our discretion)
:not-a-function ;; ?
:not-empty? ;; ?
:redefined-var ;; Eastwood
:refer-all ;; OK (I'll be happy to improve our clojure.test usage. It's not cljs-friendly anyway)
:syntax ;; ?
:type-mismatch ;; speced.def
:unbound-destructuring-default ;; ?
:unreachable-code ;; OK
:unresolved-namespace ;; clean-ns
:unresolved-symbol ;; NOK (kondo works statically, cannot know every corner case)
:unused-binding ;; OK
:unused-import ;; clean-ns
:unused-private-var ;; NOK (kondo works statically, cannot know every corner case)
:unused-referred-var ;; clean-ns
:use ;; OK

The ?s can be researched at some point. I don't know what they do and/or whether they're 100% reliable

[thumbnail]

applied your suggestions, also gathered examples for the unknown linters

:datalog-syntax
;; reports faulty queries e.g. unknown vars; see https://git.io/Jejbj

:file 
;; reports when file's can't be read or do not exist. different than broken syntax of a file (see :syntax)
;; https://github.com/borkdude/clj-kondo/blob/ca94d2b749447e7441f5d25961f765286413ba97/src/clj_kondo/impl/core.clj#L182-L200

:if
;; reports on missing else branch
;; https://github.com/borkdude/clj-kondo/blob/de4931f9f39d6eba92b15b3323d5b6a92437a78b/src/clj_kondo/impl/analyzer.clj#L967

:not-a-function
;; warns when type is a known type which isn't a ifn, e.g:
(defn t [] ("wat"))

:not-empty?
;; warns on (not (empty? ...)) in favor of (seq)

:syntax
;; triggers on reader errors, e.g:
(a 

:unbound-destructuring-default
;; reports on 'extra' keys in :or. (`j` in this example)
(let [{:keys [:i] :or {i 2 j 3}} {}] i)

:type-mismatch seems to do something different than speced.def; see https://github.com/borkdude/clj-kondo/blob/de4931f9f39d6eba92b15b3323d5b6a92437a78b/test/clj_kondo/types_test.clj. it warns on known-bad usage of clj.core usage.

[vemv]

Some stuffs

Happy to have the current clj<->cljs dispatching thing as a POC, thanks for that one!

[thumbnail]

b4059b1 prevents processing the classpath more than once;

(let [processor (formatting-stack.processors.kondo/new)]
  (time (formatting-stack.protocols.processor/process! processor
                                                       (strategies/all-files)))
  (time (formatting-stack.protocols.processor/process! processor
                                                       (strategies/all-files))))
"Elapsed time: 21294.871522 msecs"
"Elapsed time: 396.488685 msecs"

This enables us to enable processing in the normal stack.

An essential feature is cache invalidation (changed code should invalidate cached kondo analyses)

Updated QA and found that the cache is recreated with each run. As long as changed files are provided to the processor, it'll be fine.

The most meaningful QA plan is ensuring that there are false positives in cljs

I'm not entirely sure how to test this, not really familiar with cljs. Got anything in mind?

[vemv]

b4059b1 prevents processing the classpath more than once;

Not sure:

• what about the case of repeatedly invoking f-s.core/format!? i.e. no Component involved
• even if using Component, systems are subject to be re-created from scractch.

That might call for placing the state in the worker :source-path.

I'm not entirely sure how to test this, not really familiar with cljs. Got anything in mind?

I'll try to prepare a couple reprodudible scenarios

[vemv]

Continued in #109