search

elm-tooling / tree-sitter-elm

Tree sitter implementation for elm
https://elm-tooling.github.io/tree-sitter-elm/
MIT License
74 stars 12 forks source link

Separate node names for module qualifiers #111

Closed jwoudenberg closed 2 years ago

jwoudenberg commented 2 years ago

Hey there! Thank you so much for your work on this grammar. It's been a pleasure to work with so far.

I'm using tree-sitter and tree-sitter-elm to build some development tooling that performs diffs of syntax trees to find what changed, basically trying to figure out which nodes got added and removed. One case that's giving me trouble is when in an Elm type like Dict a b the Dict is qualified, into Dict.Dict a b. My diff tool understands there's extra code here, but not whether the slice that was added is Dict. or .Dict. This only happens in cases where a type and module name match. If both Dict slices had different node kinds that would resolve the ambiguity. Currently they're both upper_case_identifiers.

I realize this is a super-specific need, and totally understand it's not one this grammar wants to support. I'd thought I'd bring it up though, in case a change like this would be useful in other places, for instance to highlight module qualifiers and types differently.

I've given implementing this change a shot, but I'm super new to writing tree-sitter parsers so good odds I'm not taking an optimal approach. If there's an interest I'd be happy to make a PR (this is the diff to grammar.js).

razzeee commented 2 years ago

Hey,

it seems sensible and from a quick glance an okay approach. However, I'm a bit afraid of the breakage it might cause in upstream projects. Maybe doing it via a field would be an alternative https://tree-sitter.github.io/tree-sitter/creating-parsers#using-fields

jwoudenberg commented 2 years ago

Yeah, that concern makes a ton of sense. Fields look like a great alternative approach, but I think they can only be used to point to a particular unique child, right? So in case of a type like Some.Module.MyType, I don't think Some and Module could have the same field name. Putting them together in a single qualifier node or something might work, but then the grammar would change again.

razzeee commented 2 years ago

Yeah, I kinda hoped, that it might return a list, but the docs seem to suggest, that's not the case.

jwoudenberg commented 2 years ago

Since this is a not a bug I'm happy to close this!