Clojurians Log v2

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)

Shouldn't it be written like this?

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

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

@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.

The SHA is 5b081ebd8a1e52018110038a5d10ff1b4404f656

You can invoke it with:

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

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

> 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}}}}

@vemv Seems to work :)

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?)

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

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

(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 😅.

> 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.

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

I found an edge case in clojure.core:

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

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)

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?

Or split the linter into two different ones?

I don't understand the tradeoffs of those choices enough

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

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

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

There are some functions in the compiler you can grep for

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

got it

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

'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]

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

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

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

@alexmiller Why is there a double type hint here? https://clojurians.slack.com/archives/C06E3HYPR/p1648729416345399?thread_ts=1648720725.216029&cid=C06E3HYPR

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)

@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

@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

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

but only because of backward compatibility maybe?

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.

There is only one way documented it seemed.

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

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?

no, even then it's fine

depending on how you type hint it

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

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

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

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

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

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

it is

Yes, it was, ok then

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

correct

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

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

in the else branch, the type hint is not correct

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

> 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 [])

yes, it's a known issue

too late to reconsider how this works

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"

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

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

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

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

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

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

or :global-defn-return-type-hint

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

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)

@camsaul 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.

The issue being: https://github.com/clj-kondo/clj-kondo/issues/1331

Done

@camsaul Much appreciated!

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

(ns another-ns
  (:require my-ns))

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

@camsaul 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

Here's the final docs: https://github.com/clj-kondo/clj-kondo/blob/master/doc/linters.md#non-arg-vec-return-type-hint