feat(Rust): Support reordering declarations with attributes #428

I wonder why it puts bar below execute even though it was defined above it at the right side... Arguably not that big of a problem because of, well, commutativity, but still

Good question, I haven't looked into it at all…

It comes from the choices I made in the grammar tweaks: the new execute() method with the attribute can't be matched as a whole to the old one, because it having an attribute makes it wrapped into a declaration_with_attribute node (which contains the function_item node as a child). So in the commutative merging logic, this node appears as a new one, added from the left-hand side. Then, it makes sense to have all nodes added on the left-hand side appear before the ones on the right-hand side.

I see, thanks for the explanation!

because it having an attribute makes it wrapped into a declaration_with_attribute node

Would it be feasible to maybe put the attribute as an optional first child of function_item instead? I think that would solve the problem at hand, and also... make sense in general?

Like if we add the same logic for comments and docstrings in the future, will we sometimes end up with triply nested declarations? 😅

declaration_with_comment
|-comment
|-declaration_with_docstring
  |-docstring
  |-declaration_with_attribute
    |-attribute
    |-function_item

It's an option, but it would be more invasive in the grammar, as a lot of different node types can have annotations. For docstrings, it won't introduce the structure you describe, because docstrings are attributes themselves and can be interleaved freely, such as:

/// first docstring
#[first_attr]
/// second docstring
#[second_attr]
fn my_function() { }

I'm not opposed to introducing attributes as an optional field in front of all those node types, but I don't think it would be strictly necessary, as the behaviour we have here looks fine to me.

as a lot of different node types can have annotations

But with this approach we'd still need to add a declaration_with_attribute type for each those attribute+grammar_name pairs, no? Or would you have only one declaration_with_attribute which accepts any declaration? That would work I guess...

the behaviour we have here looks fine to me

while it looks innocuous in this case, what if bar and execute were some rather big functions in the middle of an impl block? Not too rarely does one make an intentional choice on how to order the functions, and this resolution would do the opposite of that... Again, maybe I'm exaggerating the importance of this – it would be nice to hear outside opinions

But with this approach we'd still need to add a declaration_with_attribute type for each those attribute+grammar_name pairs, no? Or would you have only one declaration_with_attribute which accepts any declaration? That would work I guess...

Yes that's how it's currently defined in the grammar, see grammar-orchard/tree-sitter-rust-orchard#1/files

unless there is something more you want to add, I'd say let's consider this approved

Thanks! I've made a note in the grammar repo about the interest in removing the declaration_with_attribute node:
grammar-orchard/tree-sitter-rust-orchard#13

My original motivation with keeping the change not too invasive was motivated by the fact that I wanted it to be easier to review to integrate it in tree-sitter/tree-sitter-rust, but now that I've given up on this, it's less of a constraint…