Generalize attribute syntax

[ninevra]

I attempted to fix #92. I didn't find a perfect solution; here's the resulting code, in case maintainers are interested.

I considered several approaches:

1. Parse attributes as meta-items iff they match the meta-item syntax, and as general attrs otherwise. This preserves backwards compatibility, but creates a highly ambiguous grammar, since any meta-item is also a valid attr. I played around with prec(), conflicts, and prec.dynamic() but wasn't able to resolve the ambiguity correctly. Someone more familiar with LR/GLR parsing might be able to? In-progress code at https://github.com/ninevra/tree-sitter-rust/tree/ambiguitiy-and-precedence.
2. Parse built-in attributes as meta-items, and all others as attrs. This breaks backwards compatibility in that some custom attributes previously successfully parsed as meta-items and will now parse as attrs. It's arguably more correct than (1), since only built-in attributes are actually restricted to the meta-item syntax (see Stabilize unrestricted_attribute_tokens rust-lang/rust#57367, Tracking issue for future-incompatibility lint ill_formed_attribute_input rust-lang/rust#57571). This is the approach taken in this PR.
3. Stop using the meta-item grammar at all and just use attr. This is fairly trivial, and I'd be happy to implement it if anyone wants. Breaks backwards compatibility for all attributes.

Are any of these options worth pursuing further?

[resolritter]

Stop using the meta-item grammar at all and just use attr. This is fairly trivial, and I'd be happy to implement it if anyone wants. Breaks backwards compatibility for all attributes.

I think backwards compatibility is not a concern since Rust's grammar is still evolving and this project doesn't promise to be stable (still in the 0.x releases). For highlighting, selection and editing purposes, it shouldn't make a difference if those items are considered a meta-item or something else, as long as it's consumable. People's queries might need some slight update, but that's to be expected given the reasons I mentioned at the beginning.

I didn't find a perfect solution

Can you go over why this isn't a "perfect solution"? What would be one?

I am thinking you implied this work is "imperfect" because you couldn't decide which way to go, but I would personally lean towards interpreting the word as referring to something "incomplete". Is that the case? Is the implementation not fully done for what you wanted to do, or what's the meaning of "imperfect" here?

EDIT: Forgot to comment

since any meta-item is also a valid attr

I think solution 3 is the best if this is right.

[ninevra]

Can you go over why [option 3, changing the attribute grammar entirely] isn't a "perfect solution"? What would be one?

I was hoping for a solution that wouldn't change any existing successful parse results. If breaking changes are allowable, then option 3 seems fine to me. It does produce more consistent & accurate-to-spec results.

Its only other disadvantage is that the resulting parse (which consists of just nested tokentrees) isn't as useful to consumers as the meta-item grammar, which has more structure; consumers who did make use of the meta-item grammar would have to re-parse the attr's arguments.

(A similar workflow is used in syn: Attributes are initially parsed with the general grammar, and then the attr's arguments can be re-parsed either as Meta with Attribute::parse_meta or as a user-selected Parse type with Attribute::parse_args.) (I'm not familiar with tree-sitter's end-user interface; is re-parsing a syntax tree node as some other type simple to do?)

[resolritter]

tree-sitter has Injections (https://tree-sitter.github.io/tree-sitter/syntax-highlighting#language-injection). One could "re-inject" the Rust parser inside of arbitrary nodes in order to parse them further. It's something I use, in practice, for highlighting interpolated code inside of JS strings. It's supported by the API already, but I don't know about how common of a practice this is.

Re-reading the OP

2. [...] It's arguably more correct than (1), since only built-in attributes are actually restricted to the meta-item syntax

I think 2 is a suitable solution as well, given this reasoning. If nothing's missing then you could move it out of Draft (AFAIK it's generally for unfinished work).

[ninevra]

Personally, I'd prefer option 3 if practical, since re-parsing attr arguments as the desired type is a cleaner interface than having to branch on whether the arguments are meta-item or token-tree.

Regardless, there are still design decisions to make: should the type of non-meta attr arguments be token_tree, or should it be some new non-terminal? (Currently, this PR uses delim_token_tree for the contents and attr_item for the wrapping type; I think it should be just token_tree, but I'd rather wait for input from a maintainer before putting more work into this.)

This PR is also currently blocked by #96, which introduces some of the same grammar productions (general token trees) and should be a much simpler/less controversial change.

[resolritter]

I'm not familiar with tree-sitter's end-user interface; is re-parsing a syntax tree node as some other type simple to do?)

@ninevra found this comment about reparsing on another issue

#71 (comment)

https://github.com/atom/atom/blob/0b34be796312a94331acbc4602944cfe3666c76a/packages/language-rust-bundled/lib/main.js#L2-L13

The injections are also showcased in the queries here. Yet another situation is what I've described in #93 (comment).

[aryx]

@ninevra @resolritter this looks like a great PR. Would you mind rebasing on the latest to avoid the conflicts?
Also why is this still in a Draft state? This looks good to me.

[ninevra]

@aryx I've rebased this and updated it to reflect (afaict) the current spec, adding support for extended_key_value_attributes. Should be ready for review.

[aryx]

Would it be possible to add a link to a section of the rust reference manual that lists those builtin attributes?

[ninevra]

Sure? I'm not sure how best to do that given this PR has already been merged - should I create a second PR for that and any other changes that might be requested on review?

The reference section is https://doc.rust-lang.org/reference/attributes.html#built-in-attributes-index.

Keeping this updated is a little more complicated than checking that list; according to the current reference, only "most" built-in attributes follow the meta item syntax (although I haven't yet found one that doesn't), and additionally there may be other well-known attributes (e.g. tool attributes) that do use the meta item syntax.

[aryx]

You can make a second PR, or maybe because it's a small thing, you can fix it when you make another PR for fixing another problem.