nvim-surround is re-indenting lines inappropriately

[andrewferrier]

If this is a general question about the plugin or a point of confusion on a feature, please check the wiki before opening a new discussion post.

Checklist

• [X] Have you updated the plugin to the latest version on main branch?
• [X] Have you checked the Breaking Changes issue?
• [X] Have you read through :h nvim-surround to see if there might be any relevant information there?

Describe the bug It appears that a change has happened to nvim-surround which is causing it to re-indent lines when doing something unrelated to indenting (for example, deleting surrounds). I'm not 100% sure if this is intentional or not.

To Reproduce

Buffer, lua filetype, with treesitter enabled (including treesitter indent = true):

{
    abc = 123,
"def" = 456
}

Put cursor over def. Press ds".

Expected behavior

Buffer looks like:

{
    abc = 123,
def = 456
}

Actual behavior

Buffer looks like:

{
    abc = 123,
    def = 456
}

Additional context

I feel pretty strongly this should not be happening, at least by default. In my view, surround.nvim should only be adding, modifying or deleting surrounds. Here, what it appears to be doing is re-indenting the line also. In this case, that happens to be correct. But there are many reasons why I might have a line intentionally mis-aligned. I think the keybindings should be doing the minimum to achieve surround's goal only.

Not sure if this is a bug or by design?

[NoahTheDuke]

I think this is by design? I can't find the issue at the moment but i vaguely remember the discussion. It matches vim-surround at least.

Personally, I prefer it to update indentation.

[andrewferrier]

@NoahTheDuke hmm, well I just tested it and it does not seem to match https://github.com/tpope/vim-surround to me. I mis-indented a line, then deleted some parentheses from it, and the line does not move - just the parens disappear.

I'm not sure why this behaviour would be desirable to be honest. I get that having stuff correctly indented is useful, I just don't see why nvim-surround would be involved. With the behaviour built in nvim-surround becomes useless if I am trying to maintain particular indentations for some reason. @kylechui is this a side-effect of something else?

@kylechui would you be open to making this behaviour optional or is too complex to maintain both codepaths, do you think?

[kylechui]

A key smart_indentation can/will be added after the big rewrite/update that's going on involving pattern-based modifications. It is by design, and mostly hitches onto the = operator and indentexpr(), which can take advantage of Tree-sitter servers for "proper" indentation.

[andrewferrier]

@kylechui sure. I mean IMHO I still don't see the need (I'm kind of curious why this exists at all, personally, I just think it's rather odd!); but glad to hear you're planning on offering an option to turn it off! I'll live with it till then... thanks as always for all your hard work and for your input too @NoahTheDuke.

[kylechui]

The relevant issues are #58, #76; in particular I think it seems reasonable to make the argument either way that having a code formatter makes varying indentation styles (mostly) irrelevant, but I would still like to provide the end-user with as many choices as I can without compromising on code quality. For the record, vim-surround also does line re-indenting based on indentexpr and the = operator, but is much less consistent with it; my goal would be to have new surrounds always appear in the "proper" indentation level, or not re-indent at all if that's preferred. Like I said earlier though, pretty large refactoring is going on to accommodate for pattern-based modifications, so I'll think about this some more (hopefully) in a week or so. Thanks for opening the issue and please let me know of any particular use cases you might have regarding this/any further ideas you want to explore. Thanks!

[andrewferrier]

@kylechui yeah, that's fair. As I say, I just think that surrounds and indenting are orthogonal issues, so if I had my way, surround wouldn't focus on indenting at all, but I get it's useful for some people. Thanks for highlighting that you plan to allow it to be turned off though.

[andrewferrier]

For everyone's benefit, I have come up with a hacky monkey-patched way to disable this for now:

require("nvim-surround.buffer").format_lines = function(_, _)
    -- Do nothing
end

(you can put this after your call to require('nvim-surround') in your config)

Obviously, this is a bit fragile, but if anyone like me is desperate to turn this off I think this'll work until the re-work @kylechui mentions above.

[kylechui]

@andrewferrier Do you mind letting me know what particular use case you have where automatic indentation is undesired? I agree with you that they are orthogonal issues; however, I think that there are a large enough number of people that wanted it to be turned on (IMO probably the majority), such that not having the option would be a pain point for many that are using the plugin.

P.S. If your "hack" misses some indentations and can't easily be fixed in post, you can let me know and I can create a temporary branch that goes "into the code" and disables it for you

[andrewferrier]

@kylechui You ask good questions ;)

So some examples why you might not want this:

• You are manually lining up comments / lists / text so certain things line up. You are deliberately not formatting/indenting your file to avoid changing that.

• You are working in a language/filetype that has buggy or inaccurate indenting support via treesitter or conventional Vim indenting.

• You are modifying existing source code that's poorly indented, but you want to maintain that poor indenting for diffing reasons.

• It's visually confusing, you've pasted some text in and are cleaning it up, but you aren't ready to format it all yet. Deleting a surround suddenly causes the text to jump.

Basically, my argument is this: nvim-surround doesn't (or shouldn't) have anything to do with indenting. It could organize my Python imports for me too, but it shouldn't, as that's the job of another plugin ;) daw doesn't reindent. ct! doesn't reindent. Neither should ysiw" ;)

But I get it, others feel differently. I hope that the easy of me enabling the hack above means it should be relatively easy to continue having a true/false switch for it in future. I may be in the minority and defaulting to reindenting is OK with me!

[andrewferrier]

And the hack seems fine for now :) Thanks.

[kylechui]

Thanks for the insight, I hadn't considered those cases before. I would think hopefully that your first case is less of an issue (if Tree-sitter doesn't mess with comments), but the other reasons all seem valid. When this gets added as an option, another benefit is that you can leave indentation on for the vast majority of files, and use buffer_setup to "disable" this for .diff files, for instance. Or you could do it "on-the-fly" as needed, by calling buffer_setup from the command line.

[Invertisment]

@andrewferrier Do you mind letting me know what particular use case you have where automatic indentation is undesired? I agree with you that they are orthogonal issues; however, I think that there are a large enough number of people that wanted it to be turned on (IMO probably the majority), such that not having the option would be a pain point for many that are using the plugin.

P.S. If your "hack" misses some indentations and can't easily be fixed in post, you can let me know and I can create a temporary branch that goes "into the code" and disables it for you

I use LSP to format my file when I save the buffer and == doesn't format the same way as LSP server does so I don't use == for formatting. I.e. I press :w and then buffer formats itself.

I adopted the hack from https://github.com/kylechui/nvim-surround/issues/109#issuecomment-1194034971 and it works as I expect (now I can paste some code, delete the delimiters and save(+auto-format) the file).

This removed formatting works well for Clojure editing but it's possible that you probably would need to reformat for Python (but then you would also be reindenting by columns manually because it will preserve columns so... then it's manual).

Also if you want to indent your code after the surround changes the parens.. then why don't you provide a lua binding (your plugin has name nvim so lua is fully allowed) to provide my own formatting callback that you would call? For instance like this:

require("nvim-surround").setup({
    format_lines = function(from_line, to_line)
        -- I can call whatever I want here
    end

})

This way you could hook this plugin into whatever you want in your config. Also you don't need to care what kind of formatting your users will use. Because some will use something like LSP, other will user Treesitter, ==, some others will use zprint or gofmt... Maybe they want to set these up themselves. Who knows. You can't know that in advance. At least I think that you can't.

And that would also mean that you could extract your formatting into a fully separate plug-in which people would use separately even if they don't use this one.

[kylechui]

Oh wow, that's actually a really good idea, and I suppose people can just use false to disable formatting altogether as well... Thanks a lot for the suggestion! I'm still going to be waiting another day or two to help people get used to the new release, but after that I'll start some work on this.

CC: @andrewferrier What are your thoughts on this? I think this matches a lot of the current code philosophy of this project, which seems to be "make everything a function and let the user decide how to handle it" :laughing:

[Invertisment]

Also one more thing with formatting -- when formatter changes the content using nvim_buf_set_lines then they reset marks and other things. And that's a very rough thing to handle because you need to stash away your marks, then calculate a diff and then reapply the marks once something calls nvim_buf_set_lines. So this is not something that you can handle by coding for a couple of hours. So you pretty much don't want to call nvim_buf_set_lines from your plug-in when you change a lot of lines (I think if a line moves then marks should disappear). Or you can call it but it may reset your marks (I don't know how it works but people in formatting plugin issues complained that this is a hard thing to do).

The formatting plugin https://github.com/lukas-reineke/lsp-format.nvim has this issue that even if it's centered around formatting it still doesn't retain marks (the mm and '`m' things). And this plugin does some kind of formatting and it retains marks but then they don't use LSP for it: https://github.com/sbdchd/neoformat/issues/400 So it's not at all easy to implement a formatting plugin that has no problems.

Why didn't they decide to implement mark saving in one plugin and formatting in the other so that all plugins could save marks... weird...

Anyway. So far I like your plug-in very much.

[kylechui]

@Invertisment Indeed; I've tried mitigating this issue by using nvim_buf_set_text, which doesn't remove extmarks, but like you said I don't think there's a way around deleting/removing regular marks. One thing I could do is to save all marks prior to setting the text, and restore them after, which I think could be fine(?), since regular marks aren't affected by gravity anyways. I would just need to delete or "snap" marks that are in invalid locations.

P.S. Thanks for the kind words, I'm trying my best to get as much work done now before school starts up again :)

[andrewferrier]

Oh wow, that's actually a really good idea, and I suppose people can just use false to disable formatting altogether as well... Thanks a lot for the suggestion! I'm still going to be waiting another day or two to help people get used to the new release, but after that I'll start some work on this.

CC: @andrewferrier What are your thoughts on this? I think this matches a lot of the current code philosophy of this project, which seems to be "make everything a function and let the user decide how to handle it" 😆

I think that's a good idea. As I've said... before... ;)... I don't think nvim-surround should be involved, really, so for me it would then be easy to override this with a simple null function that does nothing. Good solution!

Now your only decision is what the default behaviour of that function should be!

[kylechui]

I think that the default should be as-is to maintain parity with vim-surround, and also (to me) it seems that most people would like the same style of indentation as they would otherwise have via Tree-sitter. Other users that prefer LSP-style or no indentation overall can reset it themselves.

[kylechui]

Ok sorry for the (long) delay; I'm now going to work on this via the custom-indentation branch :sweat_smile:

[kylechui]

I just tested it with

require("nvim-surround").setup({
    format_lines = false,
})

and it seems to work as intended :)

[kylechui]

@Invertisment Do you mind trying it out with the LSP formatter that you said you used?

[Invertisment]

It still formats the code when I press dsb to delete (). It doesn't format code when I press ys$) though :thinking:

My version is 328f20c.

This is my whole config:

lua << EOF
require("nvim-surround").setup({
  format_lines = false,
  -- Configuration here, or leave empty to use defaults
  highlight = { -- Disables highlights
      duration = false,
  },

  surrounds = {
    --["f"] = { }, -- probably interferes with vim-sexp's `f`=form
    ["#"] = {
      add = { "#{", "}" },
      find = function() -- TODO
        -- return require("nvim-surround.config").get_selection({ textobject = "{" })
        return { {"#{"}, {"}"} }
      end,
      find = "#{[^}]+}", -- TODO
      delete = "^(. ?)().-( ?.)()$", -- TODO
      change = {
        target = "^(. ?)().-( ?.)()$", -- TODO
      },
    },
  },
})
-- https://github.com/kylechui/nvim-surround/issues/109#issuecomment-1194034971
--require("nvim-surround.buffer").format_lines = function(_, _)
--    -- Do nothing
--end
EOF

[kylechui]

I only added it relatively recently, as of 237dc43. I would still recommend that you update to the most recent commit, i.e. c977bcc.

P.S. A few notes about your config:

• It should be the case that you can just do ["f"] = false to disable the f surround
• Your find function can be set to find = "#{.-}"

[Invertisment]

I used the master branch, not the v2.0.0 branch. I updated using Plug and I didn't think that you have a different branch. So do you intend to merge it into master or keep it in 2.0.0?

Which branch should I use? There are three branches now. Should I switch to custom-indentation for testing? What is it for?

[kylechui]

Sorry for the confusion. Use custom-indentation, as that is the branch I've delegated to this particular issue. v2.0.0 is a "beta" branch of sorts that I'll be asking people to test prior to a full release. main is the "stable" branch for now. Thinks are kind of a mess right now since I messed up the versioning

[kylechui]

After this issue gets resolved, custom-indentation will get merged into v2.0.0; at some point further in the future v2.0.0 will get merged into main

[Invertisment]

I tried and I think custom-indentation works as expected. Great! (tbh it was ok to have that ugly callback that we overriden as well. But as you decided to add a boolean then it's nicer)

[kylechui]

While it was fine for the time being, I think long-term it makes less sense that way since it makes you dependent on the mechanisms, not the policies for nvim-surround. In any case, glad to know it works well for you; I'll see if I can get a few more things done before asking people to beta test the next release

[andrewferrier]

Just tested the custom-indentation branch and this works perfectly for me if I see format_lines to false! Thanks @kylechui .

This is a super-niggly point, but is format_lines the correct name here? Technically in NeoVim I think what you are doing is indenting - the = operator - not formatting - the gq or gw operators. I'm splitting hairs I know, but since you haven't released it yet, I guess now is the time to mention it :)

[kylechui]

Splitting hairs is good! I would much rather realize this now rather than after I release v2. Hopefully fingers crossed I will be able to make it to a v2.1.0 before v3 haha. In any case, what do you think about indent_lines vs. reindent_lines? The former seems "simpler" in a sense while the latter perhaps a bit more accurate? Of course if you have any other suggestions they are all welcome.

[andrewferrier]

I'm happy with either I think, it was format that didn't sit right.

[kylechui]

I've renamed everything to indent_lines as of ab6fe96; I might merge this into v2.0.0 soon and close this issue if there are no further suggestions :)

[kylechui]

Merged into v2.0.0; the branch has been deleted.