Fork me on GitHub
#clojure-dev
<
2022-03-31
>
borkdude09:03:45

I'm working on a type hint linter (sponsored by Metabase, yay!) that detects if type hints on defn are "misplaced" and should be moved from a global position to the arg vec position. I'm finding several of those in clojure itself, e.g. in clojure/string.clj :

< clojure/string.clj:48:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:54:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:75:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:138:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:180:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:196:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:207:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:213:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:235:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:252:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:264:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:275:8: warning: Misplaced type hint, move to arg vector: String
< clojure/string.clj:301:8: warning: Misplaced type hint, move to arg vector: String
So the question is, is this linter a good idea or too strict as in, the global type hint is still widely accepted and encouraged? /cc @dpsutton @vemv The issue https://github.com/clj-kondo/clj-kondo/issues/1331

borkdude10:03:56

I found one edge case in which this linter would introduce more horrible cljc code since the global type hint is still used for CLJS (and only accepted in the global position):

(defn #?@(:clj  [^Boolean seqable?]
          :cljs [^boolean seqable?])
  [x] x)

vemv10:03:29

Shouldn't it be written like this?

(defn #?(:clj seqable?
         :cljs ^boolean seqable?)
  #?(:clj ^Boolean [x]
     :cljs [x])
  x)

borkdude10:03:27

yeah maybe so, but still it would complicate this example a bit - but given this head-scratcher you pointed out here, it may be worth it: https://clojurians.slack.com/archives/C03S1KBA2/p1648166150918889

borkdude10:03:45

@vemv I've already have this linter more or less ready to use in a branch. Maybe give it a test run if you like.

borkdude10:03:03

The SHA is 5b081ebd8a1e52018110038a5d10ff1b4404f656

borkdude10:03:11

You can invoke it with:

clojure -Sdeps '{:deps {clj-kondo/clj-kondo {:git/url "" :git/sha "5b081ebd8a1e52018110038a5d10ff1b4404f656"}}}' -M -m clj-kondo.main --lint src

vemv10:03:00

I trust that it would work. I also have been picky about fixing these in every codebase I encountered :) Can one easily disable the linter for a specific defn-like? Namely nedap.speced.def/defn

borkdude10:03:24

> I trust that it would work. I'm mostly concerned about false positives. One should be able to disable that using:

:config-in-call {nedap.speced.def/defn {:linters {:misplaced-defn-type-hint {:level :off}}}}

borkdude11:03:24

@vemv Seems to work :)

vemv11:03:30

Cheers 🍻 do you have a policy on whether to ship config for some libs within kondo itself? Would be sweet not to trigger warns for existing projects (Other than that I reckon that I could ship a config thingy, which however isn't retroactive?)

borkdude11:03:11

I'm not shipping config for external libs, those should be added to the external libs

πŸ‘ 1
borkdude11:03:11

but since nedap/defn supports both, I guess it shouldn't hurt if people abided by the recommended way

thumbnail11:03:36

(just noticed the thread) speced.defn also ships clj-kondo config 😬 : https://github.com/nedap/speced.def/blob/main/resources/clj-kondo.exports/nedap/speced.def/config.edn so it can probably be expanded. Good efforts on the linter btw πŸŽ‰, I recently discovered the issue by hlship and didn't realize this was even a thing πŸ˜….

vemv11:03:31

> but since nedap/defn supports both, I guess it shouldn't hurt if people abided by the recommended way it's a feature, it precisely avoids those ugly reader conditionals. speced.def takes the hints, however you wrote them, and relocates them.

vemv11:03:06

> Good efforts on the linter btw +1! Will be happy for more codebases out there to become a touch saner.

borkdude12:03:36

I found an edge case in clojure.core:

(defn ^:static ^clojure.lang.ChunkBuffer chunk-buffer ^clojure.lang.ChunkBuffer [capacity]
  (clojure.lang.ChunkBuffer. capacity))

Alex Miller (Clojure team)12:03:37

Re the clojure.string cases, those are fine of course. There's really two levels here - I think it's preferential style-wise to put it on the arg vector. For type hints that are special symbol type hints though, it's an error to put it on the var (where it will get eval’ed to the overlapping function). So maybe it would be good to separate these into two things (for cases where you want the latter but might ignore the former)

borkdude12:03:20

So when the tag is not a class, but a special (lower cased) symbol, it's an error. In case of a class it's fine, but should just be a warning?

borkdude12:03:28

Or split the linter into two different ones?

Alex Miller (Clojure team)12:03:03

I don't understand the tradeoffs of those choices enough

borkdude12:03:15

splitting the linter into two different ones means you will have more control, e.g. turn the stylistic one off completely while keeping the error for the other one

borkdude12:03:05

And one could be on by default, while the other is more a stylistic choice and should then probably be off by default

Alex Miller (Clojure team)12:03:41

The set of special symbols to look for is small and finite - all of the byte etc primitives, and the plural array forms like bytes, plus objects and probably one or two more

Alex Miller (Clojure team)12:03:21

There are some functions in the compiler you can grep for

borkdude12:03:24

I think clj-kondo already knows about these (somewhere) since others trigger an unresolved symbol, but it couldn't hurt to have a complete list of this somewhere so I can verify

Alex Miller (Clojure team)12:03:11

Just making the point that it shouldn't be upper/lower case, but special symbol or not

πŸ‘ 1
vemv12:03:37

'Stylistic' might be an understatement though

user> (defn ^String a [])
#'user/a
user> (defn b ^String [])
#'user/b
user> [(-> #'a meta :tag), (-> #'b meta :tag)]
[java.lang.String nil]
old-style hints leave an incorrect tag which hinders introspection for users and some tools alike. Both a and b are fns here - not strings

borkdude12:03:54

A compromise then: one linter that just recommends to always use the arg vec type hint (in CLJ only) which is off by default.

borkdude12:03:22

(that's what I have now, just have to tweak the default setting)

borkdude12:03:39

If everyone just stuck by that style, there would be no errors and code would be more consistent

vemv12:03:37

What is gained by leaving it off? The snippet seems pretty clear that the other style is incorrect / has an objective reason not to use it (if you ask me, I created #1331 precisely for making other tools guaranteedly more precise. If the linter is off by default, I'm in almost the same situation as before)

borkdude13:03:59

@vemv Can you post that problem also in the issue? When I'm getting questions about this, I'd like to refer to that so people know why it's good to use this linter

πŸ‘ 1
Alex Miller (Clojure team)13:03:59

@vemv re "old-style hints leave an incorrect tag" - that's not incorrect and is used by the compiler as the return type if invoked as a function

Alex Miller (Clojure team)13:03:22

@U04V15CAJ re 'Why is there a double type hint here?" - don't know

borkdude13:03:37

but only because of backward compatibility maybe?

Alex Miller (Clojure team)13:03:30

this is not considered a deprecated feature or anything. there are two ways to do things. I think stylistically putting it on the arglist is preferred.

borkdude13:03:03

There is only one way documented it seemed.

borkdude13:03:15

I think the first added type hint might have been an oversight, but I would be surprised if Rich made any oversights :) https://github.com/clojure/clojure/commit/e354b01133e7cff8dc0d0eb9e90cde894c12e127

bronsa13:03:53

So when the tag is not a class, but a special (lower cased) symbol, it's an error. In case of a class it's fine, but should just be a warning?

bronsa13:03:55

no, even then it's fine

bronsa13:03:03

depending on how you type hint it

bronsa13:03:22

it's not a question of "some type hints" are valid in the argvec only

bronsa13:03:39

the difference is that metadata on the var is evaluated, while on the arvec it isn't

bronsa13:03:33

so (defn ^ints x [] ..) is incorrect, but (defn ^{:tag 'ints} x [] ..) is of course fine (note the explicit quoting)

bronsa13:03:02

I would say, type hinting on the argvec is preferred because it is less prone to those sort of evaluation errors

bronsa13:03:34

another thing you may run into, is that the type hint needn't necessarily be true

borkdude13:03:41

Let me check if https://clojurians.slack.com/archives/C03S1KBA2/p1648166150918889 was also one of the "gets evaluated" errors

borkdude13:03:14

Yes, it was, ok then

borkdude13:03:35

So tl;dr, it's just less headaches for everyone if we moved to arg-vec style type hints, but it's not (always) an error

bronsa13:03:46

re: my last point, see the definition of sigs in clojure.core

bronsa13:03:04

(let [m (meta argvec)
                              ^clojure.lang.Symbol tag (:tag m)]
                          (if (instance? clojure.lang.Symbol tag)
                              ...
                              

bronsa13:03:12

in the else branch, the type hint is not correct

bronsa13:03:21

but for clojure that is fine, since no reflection is done in that branch

vemv13:03:13

> that's not incorrect and is used by the compiler as the return type if invoked as a function I don't question that it's used (since it was the behavior before the newer syntax was introduced), however it makes things less introspectable/homogeneous Given:

(def ^String a "a")

(defn ^String b [])
...these two values (-> (map (comp :tag meta) [#'a #'b])) will refer to wildly different things (type of value itself, type of invoking the value). Why would different semantics would be accessible under an identical API? Seems a clear oversight in the original Clojure's design, later amended by the new syntax

bronsa13:03:49

yes, it's a known issue

πŸ‘ 1
bronsa13:03:58

too late to reconsider how this works

vemv13:03:04

But one can argue, linters can leave things in a better place than they were before. That's part of the point right? :) Else we'd be stuck in "it compiles / tests pass"

bronsa13:03:35

if the messaging is "it's bad style in modern clojure", then I agree, as long as it's not reported as an error

πŸ‘ 1
bronsa13:03:59

adding type hints to argvecs didn't come without its own issues until the last few releases of clojure but AFAIK things should be better now

borkdude13:03:03

yeah, many linting things fall into the category of "just not do that, it's better for everyone"

πŸ™‚ 1
bronsa13:03:36

(the argvec issues being caused by.. the lack of evaluation, leading to classnames not being properly resolved )

borkdude13:03:38

so for a name. :misplaced-defn-return-type-hint is obviously too strong, any suggestions?

borkdude13:03:20

:non-arg-vec-return-type-hint ?

πŸ‘ 1
borkdude13:03:32

or :global-defn-return-type-hint

vemv13:03:43

:style/type-hint-position (seems an unlikely choice, but I like how it reads)

Cam Saul18:03:40

IMO this is not a style thing at all, it is totally busted to put type hints on the function name instead of the arglist(s). The reason being is if you pass the function as a value and the function itself has the tag then the compiler will incorrectly treat the function itself as being of the tagged type. Here's an example:

(set! *warn-on-reflection* true)

(defn fn-with-tag-in-correct-spot ^java.sql.Connection [])

(defn ^java.sql.Connection fn-with-tag-in-wrong-spot [])

;; fn with correctly placed tag, function call = ok
(defn a []
  (with-open [_ (fn-with-tag-in-correct-spot)]))

;; fn with correctly placed tag, passing as value = reflection warning
(defn b []
  (with-open [_ fn-with-tag-in-correct-spot]))
;; => Reflection warning, reference to field close can't be resolved.

;; fn with incorrectly placed tag, function call = ok
(defn c []
  (with-open [_ (fn-with-tag-in-wrong-spot)]))

;; fn with incorrectly placed tag, function call = no warning
(defn d []
  (with-open [_ fn-with-tag-in-wrong-spot]))
;; (No warning, but this is wrong)
In d the compiler is treating fn-with-tag-in-wrong-spot itself as a java.sql.Connection even tho it's not. If you put the tag on the arglists then only the function invocation is treated as returning that type

borkdude18:03:30

@U42REFCKA Good example! Like @vemv could you maybe post this in the issue? Then I can post a link to the issue in the docs so people know why this linter exists and is enabled by default.

πŸ‘ 1
borkdude18:03:24

@U42REFCKA Much appreciated!

Cam Saul18:03:32

Here's another thing you might want to lint. :tag metadata has to be one of: β€’ a class β€’ a symbol, either class name or one of the special symbols like bytes . Class name should be fully qualified (normally compiler does this automatically so this AFAIK only applies to macros) β€’ a string class name You might want to add a check to make sure :tag is one of those things and not something wacky like an Integer. Another idea: It would be nice to be able to somehow lint that classnames (either symbols or strings) are fully qualified, but since this only seems to be an issue in macros where you add the metadata itself I'm not sure how you'd be able to lint this in clj-kondo Example: If I tag something with the symbol Connection it works totally fine if I import java.sql.Connection in that namespace:

(ns my-ns
  (:import java.sql.Connection))

(defn tag-is-imported-symbol [])
(alter-meta! #'tag-is-imported-symbol assoc :arglists (list (with-meta [] {:tag 'Connection})))

(println "TAG =>" (-> #'tag-is-imported-symbol meta :arglists first meta :tag pr-str))
;; TAG => Connection

(defn a []
  (with-open [_ (tag-is-imported-symbol)]))
;; Totally fine
But if I use that in another namespace that doesn't import java.sql.Connection, it doesn't work:
(ns another-ns
  (:require my-ns))

(defn a []
  (with-open [_ (my-ns/tag-is-imported-symbol)]))
;; Unable to resolve classname: Connection

borkdude18:03:24

@U42REFCKA All good ideas, but let's focus on the original issue here. Welcome to discuss ideas elsewhere, e.g. in #clj-kondo or Github Discussions on the clj-kondo repo

πŸ‘ 1