Tokenize symbols and keywords into namespace and name parts

[dannyfreeman]

Attempts to address issue #21 and issue #28.

A brief overview of the changes:
kwd_lit and sym_lit will now contain up to 3 new nodes:

(kwd_lit
  ns: (kwd_ns)
  delimiter: "/"
  name: (kwd_name))

(sym_lit
  ns: (sym_ns)
  delimiter: "/"
  name: (sym_name))

Only the (kwd_name) and (sym_name) are required.
sym_lit nodes can still contain metadata nodes (this is unchanged).

[NoahTheDuke]

I can give this a test-run.

[sogaiu]

One thing that I think likely won't work all of the time is this bit.

Here some examples in the wild:

• https://github.com/clojure/clojure/blob/527b330045ef35b47a968d80ed3dc4999cfa2623/src/clj/clojure/zip.clj#L295
• https://github.com/clojens/clj-nstools/blob/635f408a297723db3cc929275cc7cb5a6f5632f9/src/nstools/generic_math.clj#L8)

where parsing doesn't succeed.

[dannyfreeman]

@sogaiu I had a feeling that $._ws was going to cause an issue but wasn't sure what the issue would be.

There is an issue of precedence that comes up with the / character when trying to use it for so many things. The $._ws token there was the first (incorrect) way I found to resolve one of them.

The introduction of keyword names and namespaces further complicates things. I believe I can address it using precedence rules. I need to take some time to understand them then apply them correctly in this situation.

[dannyfreeman]

I've removed the $._ws, but it comes with some trade offs, see the commit message for 6880025

Edit: The only way I can think of to really solve some of the false positives like /asdf would be with a lookahead mechanism, which could express something like,

match the single character / as a symbol, if it is followed by whitespace or a closing bracket, but do not capture the following character.

But tree-sitter is not capable of that. Instead it relies on the branched parsing and precedence rules. I still plan to play with those a little more today.

[sogaiu]

@dannyfreeman

I tried parsing with 6880025 and 1183880 (essentially the same, right?) over the collection of Clojure source from clojars I've fetched locally.

They didn't produce any more easily detectable parse errors than the master branch (8c23e0e) does. This seems like an improvement over 8154310, which yielded > 120 files that weren't handled [1].

Regarding:

(/////) ;; 1 token per /

/asdf ;; 2 tokens: / and asdf

/asdf/hjkl ;; 2 token: / and asfd/hjkl

My memory is a bit fuzzy, but I think I didn't put much effort into handling incorrect code:

tree-sitter-clojure/grammar.js

Lines 1 to 4 in 8c23e0e

	// one aim is to try to parse what is correct (in the sense of
	// officially supported), but also be looser in parsing additional
	// things. this is more or less in line with advice from tree-sitter
	// folks.

I don't recall precisely where I got this impression, but I think maxbrunsfeld may have mentioned it some number of times. I think I found it hard enough just trying to handle what's correct (and even determining what's correct can be tricky for a spec-less language like Clojure) with what tree-sitter is capable of. Though perhaps it depends on the specifics a bit.

---

[1] I didn't go through all of the files that weren't handled but I think I looked at over half of them. Almost all of them were of the type with / being followed by a closing delimiter. There were two other cases I noticed. One is the file that the master branch fails on (it contains a zero byte, which tree-sitter cannot (yet?) handle) and the other is a template file which isn't really legitimate Clojure code anyway as it has sequences of characters like {{name}}.

[dannyfreeman]

I tried parsing with 6880025 and 1183880 (essentially the same, right?)

The later is just a comment change. I already posted a link to 6880025 so I didn't want to squash the two.

over the collection of Clojure source from clojars I've fetched locally.

I need to figure out how to do this :)

I don't recall precisely where I got this impression, but I think maxbrunsfeld may have mentioned it some number of times. I think I found it hard enough just trying to handle what's correct (and even determining what's correct can be tricky for a spec-less language like Clojure) with what tree-sitter is capable of. Though perhaps it depends on the specifics a bit.

I have that impression of tree-sitter as well: parse correct code but not necessarily worry about incorrect code. The outcome of this change, if left as is, would be that extra tokens are generated for invalid symbols beginning with a / character. I think that is rare and not something to be found in published code, linters like clj-kondo will flag them as invalid and clojure will throw errors. From a syntax-highlighting perspective, the extra tokens don't matter. They will just be highlighted as symbols.

If using treesitter for code navigation it might make things act funny on invalid code like ///. Imagine a move forward over expression command, such that | is the cursor. Executing it once in this scenario, |/// would move the cursor over one character instead of the group: /|//. But that code is is invalid so I wouldn't necessarily expect it to work perfectly anyways.

[dannyfreeman]

I have figured out the precedence conflicts, the latest commit 19eb4e7 should now split keywords into ns/name parts as well.

Something I'm not sure about: I've set : and :: to both be identified by the name kwd_marker. Should they be identified separately? Something like : is kwd_marker and :: is kwd_auto_resolve_marker or kwd_aliased_marker? I would still place them under the same marker: field in kwd_lit nodes.

[sogaiu]

@dannyfreeman More later, but regarding testing over clojars code, please check your matrix.org account.

[sogaiu]

@dannyfreeman I tried testing 19eb4e7. This time the checking revealed 36 unexpected files that didn't quite parse.

One issue seems to be not handling :/ appropriately. That does happen in the wild and it is accepted at the repl as a keyword AFAICT.

Another thing I noticed is stuff like :/usr/bin or :/path/from/request/to/a/file/or/folder not parsing:

• https://github.com/jacobmbr/jepsen/blob/f92354820bf788bad48ced24874cf9b9cb7a9b8b/jepsen/src/jepsen/os/centos.clj#L129
• https://github.com/cyverse-de/common-swagger-api/blob/01f20b2b58f1343eb6ebf806c9b0031008242a76/src/common_swagger_api/schema/stats.clj#L112

These types of things are also accepted by my local clojure repl.

Don't have a whole lot to say yet regarding the : and :: point. Will try to think more on it.

[sogaiu]

parse correct code but not necessarily worry about incorrect code. The outcome of this change, if left as is, would be that extra tokens are generated for invalid symbols beginning with a / character. I think that is rare and not something to be found in published code, linters like clj-kondo will flag them as invalid and clojure will throw errors. From a syntax-highlighting perspective, the extra tokens don't matter. They will just be highlighted as symbols.

Yeah, although I do take hints from highlighting to notice whether I've made some kinds of mistakes, I tend to rely more on clj-kondo via things like flycheck-clj-kondo. I think I had things set up so that the mode line changed color when clj-kondo had something to report. I think that resulted in noticing problems with the code sooner than otherwise so it was much easier to figure out what I'd done wrong as the amount of "change" since the last valid state would tend to be on the smaller side (and it was more likely that I could recall what changes I might have made). Very much appreciate clj-kondo being as fast as it is.

If using treesitter for code navigation it might make things act funny on invalid code like ///. Imagine a move forward over expression command, such that | is the cursor. Executing it once in this scenario, |/// would move the cursor over one character instead of the group: /|//. But that code is is invalid so I wouldn't necessarily expect it to work perfectly anyways.

Yeah, although as a user I would prefer if things worked well even in the face of invalid code, having tried my hand at providing that sort of experience and not having succeeded that well, my current sense is that learning to pay close attention to when code becomes invalid (at least from the parsing perspective) and fixing things as soon as possible is currently more practical. May be some day things will be different though :)

[dannyfreeman]

One issue seems to be not handling :/ appropriately. That does happen in the wild and it is accepted at the repl as a keyword AFAICT.

Another thing I noticed is stuff like :/usr/bin or :/path/from/request/to/a/file/or/folder not parsing

This makes sense, I think I have an idea of how I can handle this now too.

Keywords I think are one of the more difficult things to deal with in Clojure, because just about any string can be made into one, but not all of them can be represented in source. One of my favorites: (name (keyword " "))

[dannyfreeman]

Now that I have figured out how to perform the bulk testing, 7cfbef6 seems like a good state for the parser. I'll update the test files soon to conform to the new grammar in this branch, and be sure that some of my discovered edge cases are included.

[dannyfreeman]

I think this is pretty close to ready, outside of the differences in tree-sitter versions that needs to be fixed.

I've added prettier to format the grammar.js file automatically.

Something else I've added in that is more in line with the rest of the grammar:

Keyword delimiters (: and ::) do not have named nodes, just fields.
The same for the / delimiters in namespaced keywords and symbols. The can be referred to by position or field name (marker and delimiter respectively).

These fields don't show up in the web-ui or in parsing output, but they are picked up by queries. Emacs also displays them weird in treesit-explor-moode, but that does not prevent them from being correctly picked up in queries there as well.