nvim-ts-autotag icon indicating copy to clipboard operation
nvim-ts-autotag copied to clipboard

Unable to rename children tags

Open catgoose opened this issue 1 year ago • 17 comments

To get enable_rename to work for vue filetype I have to use one or two setup opts:

local opts = {
  opts = {
    enable_close = true,
    enable_rename = true,
    enable_close_on_slash = true,
  },
  aliases = {
    ["vue"] = "html",
  },
}

or

local opts = {
  opts = {
    enable_close = true,
    enable_rename = true,
    enable_close_on_slash = true,
  },
  per_filetype = {
    ["vue"] = {
      enable_close = true,
      enable_rename = true,
      enable_close_on_slash = true,
    },
  },
}

catgoose avatar Jun 11 '24 13:06 catgoose

Huh, now that's interesting. Good to know the per user custom config extension allowed a work around though -- a key reason for that refactor 😄.

We're already defining vue as a an alias to html https://github.com/windwp/nvim-ts-autotag/blob/2692808eca8a4ac3311516a1c4a14bb97ecc6482/lua/nvim-ts-autotag/config/plugin.lua?plain=1#L136

When I test, it's working as expected for me, without modifying any defaults or aliases.

Can you share a file or recording of the rename not working for you?

PriceHiller avatar Jun 11 '24 22:06 PriceHiller

I’ll try with a minimal config and if I still have the bug I will reply here.

Thanks.

On Tue, Jun 11, 2024 at 5:16 PM Price Hiller @.***> wrote:

Huh, now that's interesting. Good to know the per user custom config extension allowed a work around though -- a key reason for that refactor 😄.

We're already defining vue as a an alias to html https://github.com/windwp/nvim-ts-autotag/blob/2692808eca8a4ac3311516a1c4a14bb97ecc6482/lua/nvim-ts-autotag/config/plugin.lua?plain=1#L136

When I test, it's working as expected for me.

Can you share a file or recording of the rename not working for you?

— Reply to this email directly, view it on GitHub https://github.com/windwp/nvim-ts-autotag/issues/196#issuecomment-2161694340, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFAJNGAV634SHWTLX6ZKHNTZG5ZNNAVCNFSM6AAAAABJEKW63OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRGY4TIMZUGA . You are receiving this because you authored the thread.Message ID: @.***>

catgoose avatar Jun 11 '24 22:06 catgoose

I’ll try with a minimal config and if I still have the bug I will reply here.

A good minimal config is in our tests/minimal_init.lua for validating behavior.

PriceHiller avatar Jun 11 '24 22:06 PriceHiller

Using tests/minimal_init.lua on test.html:

image

Renaming from outer to inner:

image

Renaming from inner to outer:

image

catgoose avatar Jun 12 '24 12:06 catgoose

Seems like https://github.com/windwp/nvim-ts-autotag/issues/190 is related?

catgoose avatar Jun 12 '24 12:06 catgoose

Seems like https://github.com/windwp/nvim-ts-autotag/issues/190 is related?

Yeah, now playing around with it with the sample you screen shotted, I think #190 is related.

When looking at the tree it appears the end_tag is ending up outside of its element parent.

This may end up becoming hell on earth to resolve with the way nvim-ts-autotag is layed out.

Will investigate further as time permits.

For now here's the sample html file I'm using (taken from above):

<div>
  <div>
    content
  </div>
</div>

Here's what it looks like when the tree misparses and explodes:

misparsed-tree.webm

I wonder if forcing a full reparse of the tree is to blame in combination with something else. Could be upstream on the html parser even -- though I doubt it.

One thing to note, sometimes I never have this issue -- very hit and miss. Seems the parser gets correctly parsed/loaded/whatever on some instances and in others doesn't correctly attach or something like that.

Going to have to investigate further.

Any help is much appreciated, this is likely going to be a right pain to hunt down and resolve.

Thanks for the initial report and follow up 🙂

PriceHiller avatar Jun 12 '24 13:06 PriceHiller

I have this exact same problem in svelte files. I took a look at the grammar.js files for both the html and svelte parsers. I'm not entirely sure, but it seems like the svelte grammar used the html grammar as it's base to build from.

The gif you showed is precisely the issue I've been running into and I believe is the root cause of these inconsistencies.

I started working a possible solution for this in my own config by creating a subset of the AST in a lua table.

It's a filetype autocmd that recursively walks the tree from the root node and looks for any instance of an element node. It gets the start and end nodes for each element node found. Then it uses the start/end nodes to get the text, position start, and position end for both start and end tags. When text in the buffer changes, it rebuilds another instance of that table and compares it with the original. It should be possible to detect the discrepancies and react accordingly based on the information contained in both tables.

I haven't completed my implementation yet, but I do have it built out enough to generate the initial table and create a comparison table when text changes. Admittedly, this implementation is fundamentally different from the strategy used in this plugin, but, if you're interested in this approach I can give you what I have so far.

The hard part about this issue is that at any point in time the parser maintainers can modify the grammar and it'll completely bork the plugin.

Thanks for maintaining this plugin, it's a life saver for us front-end guys.

roycrippen4 avatar Jun 16 '24 01:06 roycrippen4

Thanks for reaching out. Imo, I think the approach we're taking in nvim-ts-autotag makes things simple from the configuration side, but not robust.

Imo, the best way to do this is the way you're approaching it from. Do a recursive AST search and then apply the pairs that way. Doing that would even let nvim-ts-autotag do more than just html/xml/whatever tags, even auto closing pairs for functions in any language, any sort of pair, and renames etc.

I genuinely don't have much time currently to really dig in on some things for nvim-ts-autotag, thought I would, but I don't.

If you want to shoot me what you have it may serve as a base for a new approach later on, because I think it's probably better in the long run (thanks for offering that by the way :)).

PriceHiller avatar Jun 17 '24 17:06 PriceHiller

Should I just comment the snippets here or fork it and pr with the partial implementation in a new file? What's your preferred way to receive the code? It's ~150 lines for context.

I'm at work right now, but I can get it to you later today when I'm on my personal machine.

roycrippen4 avatar Jun 17 '24 18:06 roycrippen4

Go with whatever is easiest for you, if you have something working based on nvim-ts-autotag then a PR is preferred of course (just mark it as a draft), but if you'd have to do additional work for that then feel free to paste snippets -- I can largely figure it out from there.

It may be a while before I actually get around to integrating the ideas here into nvim-ts-autotag, so there's no real rush from my side. All of this is as we have time, nobody is payed to work on this plugin. Work, school, life, etc. come first always :).

On 6/17/24 13:13, roycrippen4 wrote:

Should I just comment the snippets here or fork it and pr with the partial implementation in a new file? What's your preferred way to receive the code? It's ~150 lines for context.

I'm at work right now, but I can get it to you later today when I'm on my personal machine.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

PriceHiller avatar Jun 17 '24 18:06 PriceHiller

Should I just comment the snippets here or fork it and pr with the partial implementation in a new file? What's your preferred way to receive the code? It's ~150 lines for context.

I'm at work right now, but I can get it to you later today when I'm on my personal machine.

If you fork and create a branch and post it here, I would like to take a look!

catgoose avatar Jun 17 '24 23:06 catgoose

Link to the fork

I'm willing to spend some time attempting to integrate this if the interest is there and ya'll think the approach is promising.

If you want to quickly try out the code and verify it works you can copy the content of the linked file and paste it into your config (that's the way I was initially developing it). I also use my own plugin for logging, so I'm unsure if the print(vim.inspect(...)) stuff works correctly. I just quickly replaced my log statements since I'm at work.

roycrippen4 avatar Jun 19 '24 17:06 roycrippen4

I haven't tested anything, but I think SFC frameworks like vue/svelte all use treesitter HTML grammar so this should work for everyone.

Btw I think you can just use vim.print(...) instead of print(vim.inspect(...))

catgoose avatar Jun 20 '24 00:06 catgoose

Link to the fork

I'm willing to spend some time attempting to integrate this if the interest is there and ya'll think the approach is promising.

If you want to quickly try out the code and verify it works you can copy the content of the linked file and paste it into your config (that's the way I was initially developing it). I also use my own plugin for logging, so I'm unsure if the print(vim.inspect(...)) stuff works correctly. I just quickly replaced my log statements since I'm at work.

Once this weekend picks up I'll have time to really look at it. Just by a brief look I don't mind what I'm seeing at all.

I have some concerns around getting all the tag positions -- a issue that happens with treesitter that causes bugs over here is that the parsers, for short periods of time, reach invalid states and show errors in their trees before correctly reparsing. If you look at the video I uploaded in this issue previously, you'll see what I mean. That behavior with the misparsed tree is not consistent at all and creates all sorts of headaches.

If we grab all the tag positions and then for whatever reason a tree ends up in a bad state, we may fall out of synchronization with the tree.

Again, I haven't pulled it out to play around with it so I can't be 100% certain if that's even a valid concern. I'll add another reply tomorrow evening with some more thoughts when I really have time to look closer at this. (In fact, I have a few plans this weekend relating to nvim-ts-autotag, like some other issues that have been waiting a bit too long for me to get to.)

PriceHiller avatar Jun 21 '24 18:06 PriceHiller

Seems like #190 is related?

Yeah, now playing around with it with the sample you screen shotted, I think #190 is related.

When looking at the tree it appears the end_tag is ending up outside of its element parent.

This may end up becoming hell on earth to resolve with the way nvim-ts-autotag is layed out.

Will investigate further as time permits.

For now here's the sample html file I'm using (taken from above):

<div>
  <div>
    content
  </div>
</div>

Here's what it looks like when the tree misparses and explodes:

misparsed-tree.webm I wonder if forcing a full reparse of the tree is to blame in combination with something else. Could be upstream on the html parser even -- though I doubt it.

One thing to note, sometimes I never have this issue -- very hit and miss. Seems the parser gets correctly parsed/loaded/whatever on some instances and in others doesn't correctly attach or something like that.

Going to have to investigate further.

Any help is much appreciated, this is likely going to be a right pain to hunt down and resolve.

Thanks for the initial report and follow up 🙂

This problem doesn't occur in tsx files. Treesitter is always correct and the nesting levels are good. I tried #201 and it solves all the problems in tsx files

niba avatar Jul 14 '24 09:07 niba

Seems like #190 is related?

Yeah, now playing around with it with the sample you screen shotted, I think #190 is related.

When looking at the tree it appears the end_tag is ending up outside of its element parent.

This may end up becoming hell on earth to resolve with the way nvim-ts-autotag is layed out.

Will investigate further as time permits.

For now here's the sample html file I'm using (taken from above):

content

Here's what it looks like when the tree misparses and explodes:

misparsed-tree.webm

I wonder if forcing a full reparse of the tree is to blame in combination with something else. Could be upstream on the html parser even -- though I doubt it.

One thing to note, sometimes I never have this issue -- very hit and miss. Seems the parser gets correctly parsed/loaded/whatever on some instances and in others doesn't correctly attach or something like that.

Going to have to investigate further.

Any help is much appreciated, this is likely going to be a right pain to hunt down and resolve.

Thanks for the initial report and follow up 🙂

This problem doesn't occur in tsx files. Treesitter is always correct and the nesting levels are good. I tried #201 and it solves all the problems in tsx files

If #201 gets merged then I'll close this out.

Sorry for my lack of response lately, I really do need to look at the newer batch of issues that have been opened in the next couple of days among other tasks.

PriceHiller avatar Jul 14 '24 14:07 PriceHiller

I'll pull down #201, try it in svelte, and report back how it does. If it fixes the problem there as well then it should work for any html/xml based template language.

roycrippen4 avatar Jul 14 '24 15:07 roycrippen4

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Sep 13 '24 02:09 stale[bot]