clj-commons / rewrite-clj

Rewrite Clojure code and edn
https://cljdoc.org/d/rewrite-clj
MIT License
590 stars 55 forks source link

`^MyType foo` is not recognized as reader sugar #280

Closed lread closed 3 months ago

lread commented 3 months ago

Version 1.1.47 (and probably all prior versions)

Platform All

Background In Clojure, ^SomeSymbol foo is syntactic shorthand for ^{:tag SomeSymbol} foo:

(require '[clojure.tools.reader.edn :as edn])

(meta (edn/read-string "^SomeSymbol foo"))
;; => {:tag SomeSymbol}

Symptom The code ^SomeSymbol foo after parsed and run through sepxr, returns {SomeSymbol true}.

Reproduction

(require '[rewrite-clj.node :as node])
(require '[rewrite-clj.parser :as p])

(meta (node/sexpr (p/parse-string "^SomeSymbol foo")))

Actual behavior

;; => {SomeSymbol true}

Expected behavior

;; => {:tag SomeSymbol}

Clarification This is related to sexpr behaviour only. Rewrite-clj should continue to store and rewrite ^SomeSymbol foo and {:tag SomeSymbol} foo as originally expressed by the author in their source code.

Action I'll follow up.

lread commented 3 months ago

Our current sexpr logic for metadata x is if x is a map use as is, else use {x true}.

This correctly covers:

We'll add in explicit checks for m as a keyword (for existing working conversions) and m as a symbol (to support our fix).

In the spirit of parsing even if code is slightly broken, we'll continue to allow the parsing and writing of technically invalid metadata.

So ^(a list is not valid) baz will continue to be parseable and writeable.

Decision1: An sexpr on a node with invalid metadata should:

option 1) throw indicating metadata is invalid

PRO: might be helpful? CON: might not be helpful?

option 2) continue to not throw

Currently behaviour is:

user=> (require '[rewrite-clj.node :as n] '[rewrite-clj.parser :as p])
user=> (meta (n/sexpr (p/parse-string "^(a list is not valid) baz)")))
{(a list is not valid) true}

CON: nonsensical PRO: preserves existing nonsensical behaviour

Not sure here, I think option 1, but if you have other another opinion lemme know.

lread commented 3 months ago

Oh right: there is also the string variant ^"MyTpe" foo" which should expand to ^{:tag "MyType"} foo.