fix(format): Don't leak bufnr into opts tables

[AlexanderArvidsson]

Since Util.merge performs an in-place merge, bufnr is being leaked into the opts tables before being sent to the formatter.
This causes, in rare cases, the bufnr to be replaced by the wrong bufnr (i.e, not the current buffer), causing the formatting to apply to the wrong file.

This bug caused random text to be copied and inserted into unrelated files on save, which eventually snuck into our QA environment. Luckily, it got caught and I started investigating the issue. I found that the issue happened after (afca0b7), and found the root cause.

To demonstrate why this is a problem, do as follows:

• Open an arbitrary file and save it, triggering conform
• Note which bufnr this has via :echo bufnr('%')
• Open a different file (do not save)
• Check that bufnr is now a different bufnr as expected via :echo bufnr('%')
• Run the following :lua vim.print(require("lazyvim.util").opts("conform.nvim").format or {})
• Observe that bufnr is present in the conform.nvim options table, and that it points to the old buffer

Because of a recent change (afca0b7), the conform.nvim opts.format table is applied into the opts table before formatting, overwriting the proper bufnr value in the opts sent to the formatter, because conform.nvim would now contain it's own, stale bufnr.

This made me look at other uses of Util.merge and see that this indeed happens in several places.

The fix is to make sure we make a clone of the opts table before we trigger format, avoiding leaking the bufnr value into the plugin opts.

After this fix, observe that :lua vim.print(require("lazyvim.util").opts("conform.nvim").format or {}) does not contain bufnr in the conform.nvim table.

[dpetka2001]

Hi @AlexanderArvidsson

I'm the one who made the commit you're referring to.

You're right that bufnr is present in opts.format table and with your change it's not present any more. However, I checked with LazyVim 10.12.1 by binding the LazyVim to commit = "e3bfcb9" in its spec (my commit was included in release 10.13) and it had the same behavior, that is bufnr is still present in opts.format table (following exactly the same steps that you described).

My commit however does introduce this line and I can't tell with certainty that it doesn't cause the problem you're observing. But the steps you described are reproducible even in 10.12.1 before my commit, that is bufnr is present in conform's opts.format table.

Please keep in mind that I'm a beginner in programming and just trying to understand the underlying issue. Not trying to divert any responsibilities. Just to make sure that you don't misunderstand me.

[folke]

Looks good to me. Thanks!

[dpetka2001]

@folke Would you mind explaining if my change introduced this and why it didn't happen before my change (for example in 10.12.1) where bufnr was still leaked to opts.format table?

I'd appreciate it if you could take the time to answer my question, as I really want to understand.

Thanks in advance.

[briandipalma]

I imagine it's because opts is a shared object used in different buffers/windows so if it gets changed in one to point to its buffer it will also affect other buffers. Which is why it's best to create a different object for each merge in this case?

[dpetka2001]

So, if I understand correctly what you're saying, bufnr was also present in conform's opts.format table prior to this change, it's just that it wasn't introduced in the lsp.format.opts before so there wasn't a problem. Is that correct @briandipalma ?

[folke]

I'm not sure your change actually introduced the bug. It might have been happening before.
(I didn't have this issue at all, even with your changes)

The reason is indeed that deep_copy (and Util.merge) will merge into the first table, so definitely better to merge in a new empty table instead.

[dpetka2001]

Tnx @folke for replying. I did test with 10.12.1 and bufnr was even then present in conform's opts.format table, but the difference is that it maybe wasn't introduced in lsp.format.opts before.

I understand the logic for merging into a new table and why bufnr is not present in conform's opts.format table any more. Appreciate your time explaining.

[AlexanderArvidsson]

@dpetka2001 You're correct, the bug as described by my PR was present before your change, but the practical effects of the bug was only surfaced after the change. Just to make things clear; I did not link the commit you made to put blame, I believe your change is great because it allows me to override conform formatting settings! So good job identifying that! ❤️

Even to me it was hard to create a minimal reproducible environment (I did try!) as the "formatting applied to wrong file" problem was only reliably reproduced in our company repository, so I decided to just describe what the problem was which caused the end-problem. I believe it could be caused by race conditions, as our repo is huge and formatting is generally a lot slower, so a minimal environment would not trigger the practical bug.

When I was researching the bug, I found the effects of the bug did not happen before your commit, but that's because the conform opts table was not merged into the formatting before the change. In short, before your change, the conform object wasn't used in the formatting, so bufnr would not overwrite the bufnr of the passed opts. The changes this PR makes fixes the underlying bug (i.e. leaking bufnr into the shared conform opts table) which wasn't a problem practically before (I believe the underlying bug was introduced over 5 months ago, but at the time it may not have been a concern and as such there's nothing wrong with that other than correctness).
Again, I'm glad I was able to find the cause and the fix is out now, great job to all of you and especially @folke for this great setup!

Thanks for the merge, now I can continue using the main branch! 😄

[dpetka2001]

@AlexanderArvidsson No problem. I was just genuinely trying to understand the reason why this happened even though the bufnr was leaked before my commit as well. I never encountered this problem, but that may be because i usually use a specific formatter via conform and don't rely on LSP formatting.

I'm still new at programming and still don't really understand so much about what's going under the hood, so my main concern in my previous reply was to try and understand why it happened and I never encountered it. That's all.

Thanks for taking time explaining.