Fork me on GitHub
#clj-kondo
<
2021-12-04
>
amithgeorge13:12:27

I am thinking of a lint rule. Warn when there are unused inputs in some code paths of a function. Consider this function

(defn scan
  [price display input]
  (if-let [product-id (parse-product-id input)]
    (if-let [product-price (price product-id)]
      (display product-price)
      (display "Not found!"))
    (display "Invalid code!")))
If the parse function returns nil, the code execution moves to calling display with "Invalid code". In this code path, the price input remains unused. Is this something possible with a lint rule?

borkdude13:12:12

I suspect that would cause a lot of people to complain about this linter

amithgeorge13:12:15

I am not very familiar with cljkondo. If there is a way to store lint rules in another library and pass them to cljkondo via config, I would very much love to build/use such a lint rule.

amithgeorge13:12:39

Is such a rule even possible with current capabilities of clj-kondo?

borkdude13:12:50

It is possible to build rules for certain function calls

borkdude13:12:31

This is mostly used to macroexpand custom macros

borkdude13:12:43

But you can use it for purposes of linting bodies of function calls too

amithgeorge13:12:20

From an initial skim of the document, it sounds like I have to register my custom code for every function I wish it to be executed against. In my scenario scan was one function where the issue was demonstrated. I would ideally like to lint/check every (as many as needed) function.

borkdude13:12:02

@U0A5B1LJU In your case, I recommend to make a fork of clj-kondo to experiment and see how useful your linting rule becomes.

borkdude13:12:28

And then you could raise an issue and explain in more detail why you would want to contribute that back

borkdude13:12:09

In the case of your linting rule, I expect this would be "too much" for most people, it is very common to not use all arguments in all branches

☝️ 1
amithgeorge13:12:22

I noticed this section https://github.com/clj-kondo/clj-kondo/tree/master/analysis#analysis-data-and-tools ... is that a viable option? or forking is more viable?

borkdude13:12:50

in the analysis data you can access all locals

borkdude13:12:01

where they are introduced, their scope and where their usages occur

borkdude13:12:29

but you cannot see if they are in the left or right side of an if for example (I think)

amithgeorge13:12:11

> I expect this would be "too much" for most people, it is very common to not use all arguments in all branches True. Even I am not sure how useful it will be. It is an experiment for me.

borkdude13:12:18

I think what you can do with the analysis: You know the scope of each local You know the locations of each usage of the local You know the scope of each var usage (function call)

borkdude13:12:45

Then you can decide if there are function calls within the scope of a local that have no local usages

borkdude13:12:30

but this will also catch:

(let [x 1]
  (prn x)
  (prn :hello) ;; no usage)

borkdude13:12:54

so you kind of have to know if you are a direct child of a conditional

borkdude13:12:04

and then you would have sufficient data to calculate this

borkdude13:12:25

we could try to add that to the analysis data, if that turns out to be useful

🙏 1
borkdude13:12:03

clj-kondo actually already has a callstack like thing

borkdude13:12:14

which we could just include in the analysis

borkdude13:12:21

but for now, do it in a fork

👍 1
amithgeorge13:12:28

Would supporting the kind of linter we are theorizing , be a valid reason to augment the analysis data 🙏

borkdude13:12:29

for experimentation

👍 1
borkdude13:12:06

it's a chicken and egg problem: you don't know if it's worth adding until you know that what you added it for is useful enough

borkdude13:12:13

so a fork is best

👍 1
amithgeorge13:12:51

Thanks a lot. This is helpful. It will take me a while to understand the code enough to build this. It will be interesting for me to do though 🙂 Will ping in the channel when I need help. Thanks again.

👍 1
chrisn19:12:42

I moved on to working on tech.ml.dataset and I reduced the linting issues down to just a few things that are difficult to reduce further.

(graal-native/when-not-defined-graal-native
 (require '[clojure.pprint :as pprint])
 (defmethod pprint/simple-dispatch
   tech.v3.dataset.impl.dataset.Dataset [f] (pr f))) 
I use that to keep pprint out of graal native. That simple-dispatch is required for some editors to correctly print a dataset. It produces the warning:
src/tech/v3/dataset/impl/dataset.clj:452:13: warning: Unresolved namespace pprint. Are you missing a require?
Next -
(definterface PParser
  (addValue [^long idx value])
  (finalize [^long rowcount]))
This produces a warning -
src/tech/v3/dataset/io/column_parsers.clj:132:20: warning: unused binding rowcount
Next I have a file with a complex macro system so I can efficiently bind the many and varied arrow https://github.com/techascent/tech.ml.dataset/blob/master/src/tech/v3/libs/arrow/datatype.clj. I would like to exclude this file entirely from processing.

borkdude19:12:52

@chris441 I think this would work:

{:lint-as {graal-native/when-not-defined-graal-native clojure.core/comment}}

borkdude19:12:12

(you need to fully qualify the namespace on the left hand side as well)

chrisn19:12:40

clj-kondo lints my comments.

borkdude19:12:00

yes, then you're good, since it will see that require as a top level thing

borkdude19:12:24

or probably clojure.core/do is better

borkdude19:12:32

since some people turn off linting for comment

borkdude19:12:17

You can exclude a file with {:output {:exclude-files ...}}

borkdude19:12:32

it will still process them though, just not show lint warnings about them

borkdude19:12:44

this should be better supported

chrisn19:12:48

no dice, it still caused the error.

borkdude19:12:19

can you post a github issue with a repro about each thing?

borkdude19:12:45

or just a repository, issue not needed yet

borkdude19:12:51

then I can test it locally

borkdude19:12:55

and provide better advice

borkdude19:12:11

if you can tell me how to lint that locally

chrisn19:12:06

I added a config to turn comments off. This combined with the graal-macro-as-comment pathway worked for two of them.

chrisn19:12:31

Down to 2, from I think about 500.

chrisn20:12:17

Again, I would be interested in being able to exclude files either in config.edn or in a bit of namespace metadata from linting altogether. I don't think supporting complex macro systems that are used purely to compress implementation is worth it.

borkdude20:12:53

I get zero errors on that master branch

borkdude20:12:14

> linting took 979ms, errors: 0, warnings: 232

borkdude20:12:59

@chris441 can you provide some more specific things that you're not satisfied with?

chrisn20:12:33

Sorry, I am right now checking into master a cleanup. One sec.

chrisn20:12:17

(base) chrisn@chrisn-lt3:~/dev/tech.all/tech.ml.dataset$ gpush To http://github.com:techascent/tech.ml.dataset b6f9f4e..e3fca12 master -> master (base) chrisn@chrisn-lt3:~/dev/tech.all/tech.ml.dataset$ scripts/clj-kondo src/tech/v3/dataset/io/column_parsers.clj:132:20: warning: unused binding rowcount src/tech/v3/libs/arrow/datatype.clj:15:13: warning: Unused import TimeStampMilliTZVector linting took 969ms, errors: 0, warnings: 2

chrisn20:12:33

lol, -- not asking you to look into hundreds of warnings!

borkdude20:12:20

I'm now getting: > linting took 1106ms, errors: 0, warnings: 232

borkdude20:12:33

on branch master

borkdude20:12:31

btw, this config is not correct:

:config-path ["cnuernber/dtype-next"]

borkdude20:12:43

it should be: :config-paths

chrisn20:12:01

Done. Now I get a few more warnings but not many.

borkdude20:12:47

Also I recommend checking in the config for cneurber/dtype-next into the .clj-kondo dir

borkdude20:12:04

so everyone who clones the repo can use the config straight away

chrisn20:12:30

makes sense.

chrisn20:12:59

btw- I am super happy with where this ended up. I get a bit more robustness, linting, and discoverability across every clojure editor as a result of the whole process. Thanks for the help so far - will definitely shout it out.

borkdude20:12:27

cool :) I lost track of what the remaining issue is

chrisn20:12:10

definterface produces spurious rowcount warning.

chrisn20:12:31

That would be one. The second is being able to skip linting for a particular file via config.

borkdude20:12:39

Have you tried

{:output {:exclude-files ["regex/for/file.clj"]}}

borkdude20:12:58

About the definterface one: I think that's a bug in clj-kondo, please file an issue

👍 1
borkdude20:12:24

you can temporarily put a #_:clj-kondo/ignore before the form to ignore the warning