Warn about let shadowing vars

[scramjet]

A friend of mine just spent an hour wondering why filter was not working properly. He just realised that he had code like (let [filter {a: 1}] … (filter pred some-list)). This kind of shadowing has bitten me before too when I was learning.

It would be great if clj-kondo could flag this as a warning, since it's almost never what you want.

[borkdude]

Feedback: https://twitter.com/borkdude/status/1289925943025229825

[royalaid]

I believe it would be insightful for when shadowing the "wrong" thing, with the above case being a function, especially from clojure.core, to a value. The problem is that I can see instances where something should or can be shadowed but is also a function from core; name, key, etc.

[borkdude]

Maybe a configurable list of fully qualified var names like [clojure.core/name, clojure.core/key, clojure.core/val] could be useful, which is empty by default. Not sure.

[borkdude]

https://twitter.com/souenzzo/status/1289967423039270912

This user suggests warning only when the local binding is used in a function call:

(let [[key val] x]
  (prn key))
;; OK
(let [[key val] x]
  (key ..args))
;; WARN

[borkdude]

https://twitter.com/glenathan/status/1289973899837321216

How about warning when you call a shadowed var as a function when it isn’t?

This would just be an enhancement to the type checking.

[borkdude]

https://github.com/dakrone/lein-bikeshed has an option to warn about function args shadowing core functions

[borkdude]

From Alex Miller:

https://twitter.com/puredanger/status/1290015991477366784

probably the worst case is shadowing to an invokable data structure, then attempting to invoke the original function. so, legal, but sometimes (!) wrong - that's why its tricky. a warning in that narrow case would be worth it

[scramjet]

I like the idea of a warning by default when calling a shadowed symbol. My real-world example above would have been caught by that, as well as Alex Miller's example (which I've also encountered in real life!)

An optional warning when let shadows anything from namespace scope would be great too. I would always have that on since I never want to mislead the future reader of my code. In fact I really can't think of an example where it makes sense to shadow a global, except perhaps in the area of auto-generated code.

[rschmukler]

I sometimes write partially applied shadow variants in let bindings, so this would be something that I'd likely turn off. Just offering some commentary on the "using shadowed local bindings as function calls."

[borkdude]

It will be turned off by default, because else this would "break" a lot of valid code. I think an :include and :exclude list will work. If you use the :include list then the linter will ONLY warn for this symbols. If you use an :exclude list then the linter will warn for all symbols but those.

[borkdude]

I think a :suggest map is appropriate here, so the user doesn't have to think too hard, e.g. {name nom}, maybe also for consistent style.

"Name is forever nom."

https://twitter.com/fogus/status/1290342186006044673

[borkdude]

I think this linter should also warn about other vars than clojure.core. I was bitten by overriding a var in the current namespace with a local more than once (in fact, it just happened to me a few seconds ago).

Proposed config:

{:shadowed-var {:include [clojure.core my.ns/foo] ;; only warn for these, unqualified symbol = namespace, qualified symbol = var
                :exclude [my.ns/foo] ;; don't warn about these vars
                ;; when include and exclude aren't set, warn on every shadowed var
                :level :off ;; default, set to :warning to enable linter
                :suggest {clojure.core/name nom}
                }}

[borkdude]

First result from the shadow-binding-646 branch.

I wonder if the :suggestions map should just take unqualified names, since the namespace you are shadowing doesn't matter much perhaps. And maybe a present :suggestions key should imply that it will always warn for that symbol.

[borkdude]

New simplified version. I think I'll only support the :exclude option and everything works with unqualified symbols:

(ns foo
  {:clj-kondo/config
   '{:linters {:shadowed-var
               {:level :warning
                :exclude [ns]
                :suggest {name nom}}}}})

(defn foo [ns name] ;; warning
  [ns name])

(let [name 1] ;; warning
  name)

(defn bar [ns #_:clj-kondo/ignore name] ;; no warning
  [ns name])

[serioga]

Without :suggest the message will be just “Shadowing var: clojure.core/name”?

[borkdude]

Correct.

[borkdude]

You can download binaries by going to the CircleCI artifacts of the commits of this branch: https://github.com/borkdude/clj-kondo/tree/shadow-binding-646 if you want to try this out.

[borkdude]

Merged to master: https://github.com/borkdude/clj-kondo/blob/master/doc/config.md#shadowed-var
Binaries can be obtained by navigating from Github to the CircleCI builds.