Kibit breaks on namespace qualified keywords

[clj-commons]

Kibit breaks on namespace qualified keywords

[brabster]

I think a lot more people will run into this with the new clojure.spec feature depending heavily on namespaced keywords. I just did!

[arrdem]

Since people keep bumping this -

My $0.02 is that keeping kibit "static" is a waste of time. Clojure is quite deeply dynamic and IMO the onus should be on users generally to make sure that their code is loading/reloading safe. Relying on load time behavior for non-declarative operations is a huge antipattern.

There really isn't a good way to "fix" kibit to hack around this, since this behavior is determined by the state mapping in clojure.core/*ns*, namespaces being global mutable named objects 😛

I'll take a look at patching this later today.

[coreagile]

Me, too. :-)

[viebel]

This issue combined with the fact that kibit stops on first error #119 make it really painful

[ghoseb]

jonase - Indeed, ::f/some-kwd is valid if f is a valid ns/alias.

user> (ns foobar)
nil
foobar> (ns user)
nil
user> ::foobar/some-kwd
:foobar/some-kwd
user> ::fubar/some-kwd
; Evaluation aborted.
user> 

[jonase]

::f/some-kwd is not valid clojure syntax:

user=> ::f/some-kwd
java.lang.Exception: Invalid token: ::f/some-kwd

[jonase]

@ghoseb You're right -- it is valid syntax. I had never seen it before :)

It seems like we have to load the file prior to reading it. Some more discussion here.

[ghoseb]

@jonase I am following the discussion on the list. Indeed, :: is a dynamic feature and hence loading the file might be necessary in order to get this working.

Is loading the file and option for you?

[ohpauleez]

@ghoseb Currently, we can't do a work-around without eval'ing the whole file, which is strictly not "static". I have some weird hacks to try to get around this, but none are working out too well. Currently on master, there's a try/catch for this (which will be present in the next release that gets cut).

When we have a better solution, we'll put one in for sure. As always, we welcome any idea.

[alexander-yakushev]

Bump. I've just run into this as well. :)

[danielcompton]

I'll take a look at this soon!

[viebel]

@danielcompton Did you take a look at it?

[jimrthy]

+1

[danielcompton]

My $0.02 is that keeping kibit "static" is a waste of time. Clojure is quite deeply dynamic and IMO the onus should be on users generally to make sure that their code is loading/reloading safe.

Yeah I think I agree. While it would be nice to keep it static, short of Cursive open sourcing their parser for the rest of us 😁, this might be the best option. This would be a breaking change to communicate to people.

[christianromney]

Bump. :) Just checking to see if anyone's had the time to look into this since March. Cheers.

[danielcompton]

I haven't, and am unlikely to at least in the next month or so. As always, Patches Welcome ™ 😀.

[aredington]

Hi, I'm feeling the pain on this one along w/ @christianromney and would like to contribute a patch. I'd like to get some feedback on where to go before implementing.

The Problem

Reading is complected with the environment in which reads are occurring. The immutable token '::f/some-kwd' can either be valid Clojure syntax that resolves to a namespaced keyword, or invalid Clojure syntax that results in throwing an exception, depending on the current value of the *ns* var and the aliases defined therein.

Solution A: Bash in place

One way to resolve this is to analyze (ns ...) forms during read, and to modify the value bound to *ns* so that the appropriate aliases are interned during the reading of the file. A rigorous approach to this would require defining good boundaries for how long these aliases persist, and implementing stack discipline to restore ns to its value after all forms have been read.

One set of boundaries that may be sufficient is: Start a checkpoint when reading a file, read until an (ns) form is encountered. If encountered, push the checkpoint to the stack and derive a new ns value from it incorporating the aliases from the read ns form, and continue reading until a new ns form is read. On encountering a new ns, pop the stack, derive a new ns value from the head of the stack with new aliases, and continue reading. At EOF, pop the stack with no further actions.

Solution B: Migrate to tools.reader and use clojure.tools.reader/*alias-map*

Similar to A, we would have to do static analysis of NS forms to generate an alias map. Dissimilar, we do not have to pollute the *ns* value for the ns reading the forms, and can instead use clojure.tools.reader's *alias-map* feature:

user=> (reader/read-string "::foo/bar")

ExceptionInfo Invalid token: ::foo/bar  clojure.core/ex-info (core.clj:4725)

vs

user=> (binding [reader/*alias-map* {'foo 'foo.man.chu}] (reader/read-string "::foo/bar"))
:foo.man.chu/bar

We'll still need to intelligently manage the value of clojure.tools.reader/*alias-map* as ns forms are encountered.

Solution C: Pre-process streams to fully qualify all aliased keywords

The token :long.ns.name/keyword is always valid Clojure syntax, regardless of the value of the *ns* var at read time. If we transformed input streams to first fully qualify all namespace aliased keywords before the Clojure reader sees them, we can ensure that it only ever sees fully qualified keywords and therefore cannot fail on reading an aliased keyword.

Of the options I've identified, I like B best but it requires taking a dependency on clojure.tools.reader (but at least doesn't require any upstream adjustments). C seems like it could resolve the problem without having to add a new lib, but seems marginally error prone as it has kibit analyze derivatives of source files instead of source files. A feels really gross and I think it is the worst option.

[danielcompton]

Hi @aredington thanks for looking at this. Reading your post, I came to the same conclusions as you about the different options. I've got no objections to taking on a tools.reader dependency. I suspect we may need to use something like mranderson to vendor it so that we avoid conflicting dependencies from other projects, but we can save that for later. AFAICT Leiningen doesn't have a dependency on tools.reader which would be the main one to avoid.

Choosing B also has the advantage that if there are future aliasing enhancements (e.g. CLJ-2123), hopefully tools.reader can handle them for us too.

I'm happy to take a patch on this with whichever option you feel is best, but I have the same order of preferences as you (B, C, A).

[arrdem]

Some thoughts.

1. Namespace-per-file is not a safe assumption at all. It's considered Bad Style not to do so for sure, but core does it pervasively and I've seen other codebases do this too for various not entirely bad reasons. (in-ns ...) is the real side-effect and there are plenty of ways to play tricks with it.
2. That the (ns) form (when there is one) comes first and is complete is not a safe assumption. In order to achieve cyclic module dependencies some people use inline require and import calls. Again it's Bad Style to do so, but sometimes you have to.

There isn't really a good way to get spooky action at a distance since you can only really side-effect *ns* without doing really evil things with resolve and Java interop, but you could imagine remotely manipulating another namespace's bindings.

In order to work around a very related issue CLJ-1579 the tactic was to simply establish the appropriate *ns* binding to the loaded code while reading source. Here, as stated previously, I think the real solution is to simply load up the user's code, bind *ns* and then start linting using the existing reader. There are too many evil tricks you could play and too many ways that faking out *ns* and its bindings could go wrong (Eg. monkeypatch and try again) to in my mind defend any other tactic.

To be completely precise, you need to sequentially read the next top level form (the first one must be readable in no context or whatever the continuing context is), lint it, evaluate it and continue because otherwise you're linting in a different environment than the one in which evaluation occurs. Getting that really right would require patching clojure.core/load to instead be a lint-and-load so that (as in clojure.core) files which "inherit" their name from imperative use of (load) in other files get analyzed with the appropriate context. Monkeypatching clojure.core/load for stuff like this has prior art, but there may be trouble due to static linking in clojure.core ignoring changes to the binding of #'clojure.core/load.

[danielcompton]

@arrdem what you says makes sense, and namespace per file, (or only switching namespace within a file via ns calls) is not valid in all cases. However we could consider if we're willing to accept that in those unusual cases the keywords may be read successfully but incorrectly.

If we were ok with that, then the end result is that people doing funky namespace stuff, and using namespace-aliased keywords and had code that Kibit made a suggestion for, would get a bad suggestion for that code.

Given that currently Kibit sometimes makes bad suggestions (like replacing [...] with (...)), I don't feel like this is the worst tradeoff to make. OTOH, I'm not sure how much harder it is to do this "properly", so I guess that would be better if it was feasible.

[achikin]

Could you at least provide a flag to suppress it?