Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two-colon keywords cause exceptions #21

Closed
weavejester opened this issue Jan 28, 2015 · 9 comments
Closed

Two-colon keywords cause exceptions #21

weavejester opened this issue Jan 28, 2015 · 9 comments
Labels

Comments

@weavejester
Copy link

Keywords like ::xyz/foo cause exceptions when read by rewrite-clj:

(p/parse-string-all "(require '[clojure.set :as xyz])\n(prn ::xyz/foo)")

Because xyz isn't a valid namespace alias in the namespace doing the parsing, rewrite-clj will throw an exception when it tries to read in the keyword ::xyz/foo.

You might need to add a specific keyword node, rather than relying on clojure.lang.Keyword.

@xsc
Copy link
Contributor

xsc commented Jan 28, 2015

There actually is a special keyword node but the issue here is that ::xyz/foo is not a valid keyword:

user=> ::xyz/foo
RuntimeException Invalid token: ::xyz/foo  clojure.lang.Util.runtimeException (Util.java:221)

The Clojure reader doesn't accept it:

user=> (read-string "::xyz/foo")
RuntimeException Invalid token: ::xyz/foo  clojure.lang.Util.runtimeException (Util.java:221)

user=> (clojure.tools.reader.edn/read-string "::xyz/foo")
ExceptionInfo Invalid token: ::xyz/foo  clojure.core/ex-info (core.clj:4403)

@weavejester
Copy link
Author

::xyz/foo is a valid keyword in Clojure, if there is a namespace aliased to xyz.

user=> (require '[clojure.set :as xyz])
nil
user=> ::xyz/foo
:clojure.set/foo
user=> (read-string "::xyz/foo")
:clojure.set/foo

@xsc
Copy link
Contributor

xsc commented Jan 28, 2015

I'll have to think about that. Allowing the parsing of these keywords is not a problem, but without keeping track of namespaces/aliases conversion of such a node to an s-expression would have to result in something like:

'(if-let [alias (get (ns-aliases *ns*) 'xyz)]
   (keyword (name (ns-name alias)) "foo")
   (throw ...))

Which is not really something I'd expect a :token node to produce...

@weavejester
Copy link
Author

I'm not sure what you mean...

You can't determine the keyword without evaluating the namespace, and conceivably the same keyword might turn out to have different namespaces. e.g.

(require (rand-nth '([clojure.set :as xyz] [clojure.zip :as xyz])))
(prn ::xyz/foo)

So the only way I can see around it, is to create some sort of custom structure for these unevaluated keywords, like:

#rewrite-clj.node.Keyword {:namespace ":xyz", :name "foo"}

Or vector AST style:

[:list [:symbol "prn"] [:keyword ":xyx" "foo"]]

@xsc
Copy link
Contributor

xsc commented Jan 28, 2015

As I said, parsing and storing as a node is not a problem and can be accomplished easily by adjusting this precondition.

But there is the function rewrite-clj.node.protocols/sexpr that every Node has to implement - and what would be the corresponding s-expression for ::xyz/foo? I see two possibilities:

  • It is expanded within the namespace that calls sexpr, throwing an exception if the alias does not exist.
  • It expands to something that can be passed to eval, yielding a result within a desired context.

I prefer the second solution but it would mean that:

(rewrite-clj.node/sexpr (rewrite-clj.parser/parse-string "::xyz/foo"))
;; => '(if-let [alias (get (ns-aliases *ns*) 'xyz)]
;;       (keyword (name (ns-name alias)) "foo")
;;       (throw ...))

@weavejester
Copy link
Author

Oh, I understand what you're saying now. I can see why you want that sexpr function, and also why it's so difficult in this case.

The really annoying part is that keywords like this can be quoted, so you could write something like:

(n/sexpr (p/parse-string "(foo '{:x ::xyz/foo})"))

@xsc
Copy link
Contributor

xsc commented Jan 29, 2015

I now think that the cleanest solution here is to cheat:

(n/sexpr (p/parse-string "::xyz/foo")) ;; => :xyz/foo

This should be fine in nearly all cases where sexpr is used as long as n/string still produces the original "::xyz/foo". Opinions?

xsc pushed a commit that referenced this issue Jan 29, 2015
Expansion to s-expression will return a namespaced keyword without
resolving the alias. This is in accordance with the expectation that a
`:token` node returns a token, not a nested structure.

(An alternative approach would have been to generate the form
`(read-string "::ns/x")`.)
@weavejester
Copy link
Author

I don't think I've ever actually needed the sexpr functionality before, as I tend to work directly with the AST, so I'm afraid I don't really have an opinion on this part. As long as rewrite-clj doesn't except, I'm happy :)

@xsc
Copy link
Contributor

xsc commented Jan 29, 2015

As long as rewrite-clj doesn't except, I'm happy :)

I thought so. :) Released in

[rewrite-clj "0.4.8"]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants