search

UserNobody14 / tree-sitter-dart

Attempt to make a tree-sitter grammar for dart
MIT License
59 stars 36 forks source link

Rationale behind identifier/selector #19

Open mizlan opened 3 years ago

mizlan commented 3 years ago

Hi, I was a bit confused by the tree generated by the following code:

void main() {
  f(a.b);
}

The arguments node for f contains the following, when dumped using Neovim's treesitter playground:

arguments [1, 3] - [1, 8]
  identifier [1, 4] - [1, 5]
  selector [1, 5] - [1, 7]
    assignable_selector [1, 5] - [1, 7]
      unconditional_assignable_selector [1, 5] - [1, 7]
        identifier [1, 6] - [1, 7]

As you can see, it looks like there are two arguments to this function. Indeed, the identifier-selector as sibling nodes without a containing parent node happens universally throughout Dart code. I would imagine that it would make sense for there to be a "wrapper" node that encloses the identifier and following selectors, but there isn't. Is there a plan to add that to the grammar?

mizlan commented 3 years ago

For reference, I have a plugin that allows for arbitrary children of an arguments node to be swapped (relevant thread here) but this identifier/selector behavior is interfering with the identification of function arguments.

TimWhiting commented 3 years ago

This is because a selector is not part of an identifier. This grammar tries to mimic the official dart spec as much as possible to make it easier to maintain, but I agree that it would make sense to group those nodes under another meta node. Feel free to take a look at the grammar and submit a pull request. I can't guarantee that I have time to look at it /change it right away, but I could probably spare the time to review a pull request.

akinsho commented 3 years ago

Hi @TimWhiting well since I basically started this whole train off with my PR to iswap.nvim I can look into contributing the necessary changes to the grammar. I'd really appreciate a bit of advice on how to proceed though since I've had a look through the grammar and am not really sure how to make the change?

Would this be scoped to all nodes that have a selector where the selector is treated as a sibling or just things like arguments and lists etc. since the same probably likely exists wherever a selector is used. Also do you know where they are being set as siblings that I can wrap?

TimWhiting commented 3 years ago

In general arguments will be some type of expression. Currently most expression types are hidden nodes, because they can get quite deeply nested. I've added an argument node underneath arguments. So now it can either be argument or named_argument underneath arguments, and the argument / named argument encapsulates the expression. I'm guessing there are other places in the grammar where the expression node is hidden and it could use some more aptly-named node to partition things. Take a look at the latest commit (https://github.com/UserNobody14/tree-sitter-dart/commit/7d7fa53d4a9cabf9fee2f8b5d32cb7a4caf82704) at the grammar.js file to see what I did, and you can do the same elsewhere in the grammar. You can ignore the stuff I changed with the 'assignable_selector'. I just cleaned up that node since it can only be an unconditional_assignable_selector or conditional_assignable_selector, so that node was just adding noise.

TimWhiting commented 3 years ago

I'll leave this issue open since I believe there are other places in the grammar that just use $._expression which is hidden, and could use more contextual information exposed in the tree. @akinsho, thanks for the offer, please feel free to improve the grammar and submit PRs.

akinsho commented 3 years ago

@TimWhiting thanks a bunch for adding that 👍🏾 I'll have a look to see if I encounter any other areas that require similar treatment and use the example of your commit if I find any.

mizlan commented 3 years ago

One other problem area may be in vector/array literals and other data structure literals. Haven't taken a look firsthand but that may potentially be an issue.

On Fri, Jun 4, 2021 at 9:31 AM Akin @.***> wrote:

@TimWhiting https://github.com/TimWhiting thanks a bunch for adding that 👍🏾 I'll have a look to see if I encounter any other areas that require similar treatment and use the example of your commit if I find any.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/UserNobody14/tree-sitter-dart/issues/19#issuecomment-854859546, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKSBU2KNQNADBUZBSMUHQXDTRD5UNANCNFSM46BURCTQ .

textzenith commented 1 year ago

Thanks, everyone, for your great work in creating a Dart tree-sitter grammar—seems like an arduous task!

Because I want to use tree-sitter to rearrange trees of Flutter widgets (i.e. identifer / selector node pairs), this behaviour is causing a lot of problems. I would like to offer to fix it, but I need to improve my TS-fu first. 😏

(As always, it seems like one of the hardest problems for a programmer is naming things—what's the parent node name that everyone would be able to agree on?)

TimWhiting commented 1 year ago

I took a closer look at this issue the other day, and was having a bit of trouble reorganizing. I'll give it a try again sometime soon now that I've added the dart 3.0 features, I'm moving on to cleaning up the grammar. If you want to start contributing though I'd be happy to review a PR. I would say even just making the _postfix_expression not hidden for selectors would be a good start.