Nested brackets detection is broken across newlines

[jovenlin0527]

OS (name + version; i.e. Linux Mint 20, Windows 10 2004, Macos Big Sur, etc.): Arch Linux
What vim? (+ version) Vim 8.2.2489

Autopairs config (if applicable):

let g:AutoPairsMultilineClose = 1

Describe the bug
Nested brackets detection is broken across newlines if you enable MultilineClose.

Steps to reproduce
| is cursor position.

void f() {
    if (true) {
    } else |
}

Typing { under insert mode does not insert }.
Not sure if it's intentional, This does not appear in 51a6038.
I cannot find the exact commit that leads to this behavior.

void f() {
    if (true) {
    } else {|
}

If you type }, the cursor jumps to the next } without inserting anything.
git bisect shows it was introduced in 64cf458 for those who enable MultilineClose.
Since 1ea8853 MultilineClose is enabled by default.

This bug also appears if you replace {} with ().
This bug does not appear when all nesting pairs are in the same line.

[LunarWatcher]

Added a hotpatch to re-disable it for the time being.

That being said, I've burned myself on that variable three times now, which is more than enough. There's definitely a balance here between an intentional feature (see the readme), and the overall annoyance this feature produces, and the annoyance heavily outweighs the features atm.

I won't close this for the time being, because I need to get another, proper look at this bit of the code. There's still a few chunks I don't understand (as well as a few variables, largely the third one returned by s:getline -- not important though). Even flymode has back insert, but there's no way to tell the system it's wrong in this case as it currently stands. Gonna need to look into options for making the feature bit substantially outweigh the absolute garbage (i.e. this) - if I can't, I'm probably gonna get rid of the entire feature for causing more problems than good.

Multiline balance checks are a major pain point, though. There's no optimal ways to get semantic auto-insert without going down the rabbit hole of basically parsing languages. This feature is largely annoying due to the lack of said good balance check. If balance checking worked, it might've been possible to distinguish between when multiline jumps make sense and when they don't. Some of the balance checks introduced by Krasjet have caused a few pain points as well, though (largely related to multiline blocks where parentheses span several lines.

Definitely overthinking at this point, though, and heavily overcomplicating. Gonna have to take another look when I'm not annoyed by how I forgot how annoying that variable is twice :')

[LunarWatcher]

As for 1, it works fine with the variable disabled. It's triggered by balance checks being extremely primitive. g:AutoPairsMultilineClose = 1 triggers this block:

https://github.com/LunarWatcher/auto-pairs/blob/master/autoload/autopairs.vim#L63-L75

Which makes one of the variables (I forget which - the names aren't really descriptive) be equivalent to (at your cursor position) " }". When you insert {, the balance check thinks (in a false positive, triggered by a limited search scope) that your inserted { completes the } it found. It therefore doesn't insert a matching pair.

[kuntau]

I was about to report the same thing.. it drive me crazy this past few days 🤣

So the temporary solution is to set g:AutoPairsMultilineClose = 0 ? Am I right?

[LunarWatcher]

@kuntau yeah, or update auto-pairs. Redisabled it in beta6

[kuntau]

So this change is to revert my request #19, we're going full circle

[LunarWatcher]

That's part of why it's problematic to fix. It's either too aggressive (as seen here), or breaks another feature.

The feature isn't really meant for nested brackets, and I still have no idea if I can fix that. Might just be a problem with my limited vimscript skills though.

The best option I have atm is converting it from a default action to being explicitly triggered by a keybind. Been fairly busy for a few days and haven't gotten as far as looking at implementing it yet. I expect to have something this weekend

[LunarWatcher]

I've spent a couple hours looking at this, and I can't think of a way to save it as it currently stands. I see the practical applications of this, but without accurate balance detection systems (which I still don't think we can get accurately, among other things, due to language semantics, comments, and strings), there's no way to tell if the pair is balanced or not.

I've previously added measures to deal with bad balance checks that present the same balance issue as here (1 close, 0 open, the old system didn't detect the open as a new pair due to checking count and not position. Not that important though), but with a multiline search, it just gets more complex. There are ways to do it, but in a weigh-off between auto-pairs just working and having a few edge cases, vs. having fewer edge cases but potentially slowing down Vim in sufficiently big files, I'd personally take it just working.

Balance checking still needs a bit of work (once I figure out how I can do this generically, I plan to blatantly ignore all pairs in strings and comments, and potentially ignoring balance checks if typing in a comment or a string -- as options, though), but it'll still end up being scoped too narrowly for multiline close to work.

My current plan is to eventually add a keybind alternative for multiline close - that way, it's possible to use the feature without it constantly being a royal pain in the ass. Also tested enabling back insert, but back insert isn't really good when there's a write involved. (the bracket ends up in the wrong place in some cases, like the example in the readme mentioned in issue 19 - still works fine in others). I don't have a time estimate on when it'll be ready though, largely because I'll have to clean up a decent chunk of code before I can implement this.

A direct implementation cannot be done right now; autopairs#AutoPairsInsert contains the bulk of the logic for it, and I have to separate out some move logic before I can make a keybind. The problem there is that it's quite the confusing code base and I'm not entirely sure what I can separate out as blocks (and much less what does what; there's relatively few comments or other helpful indicators). In the meanwhile, it should at least not be a problem as long as the option is off. Means there's one less working feature, but arguably better than having an annoying feature.

[jovenlin0527]

Thanks for your time looking into this ☺️.

I do believe multipleline close feature belongs to another keybind. Insertion mode is for inserting text, not for moving your cursor.

The only scenario when this feature makes sense in insertion mode (that I can come up with) is when the closing bracket belongs to the text you are currently inserting. (Hope it doesn't sound confusing, English is not my first language.) But I can image it being pretty hard to implement.

[LunarWatcher]

Depends on what you mean by "the text you are currently inserting". I have no idea how that could be recognized in code, not gonna lie. And don't worry about your English, it's fine ^^ It's not my first language either - you're definitely not alone with that ^^"

[kuntau]

Maybe a quick fix is to create shortcut to toggle auto-pairs on/off so we come to edge case like this we just toggle off auto-pairs for a while.

Anyway I think auto-pairs should always insert pair without checking if the closing bracket is in the next line. It is easy enough to delete matching pair rather than having weird situation like this.

[LunarWatcher]

@kuntau already exists :)

Also present in the upstream repo, though it wouldn't surprise me if it's a lesser-known feature. It's per-buffer though -- I could add a global disable as well (... assuming anyone has a use for it)

[LunarWatcher]

I'll close this in favor of #23 for the time being. You'd think it was easy to make this change, but there's so much legacy code and redundant variables that even removing this may have consequences I'm not aware of. More deets in the notes section of #23.

Got a mild headache atm from looking at the unnecessary complexity of some of this code. I can't actually disable this sensibly. I've disabled the block in s:getline (without testing much, may be entirely broken; that's why I recommend tags :') ), but I can't actually get rid of it without cleaning up the code. That's why I'm deferring this to #23 (and why I'm leaving issue 19 closed); I need to keep track of the rest of the mess as well.

Gotta be careful with what I remove to make sure the entire plugin doesn't collapse in the process (read: it's not something I should be doing at 00:44 :') Git is always there to revert to, but still). This is relatively high on the list, so whenever I get through the cleanup and documentation phase, this should be easy to do.

[LunarWatcher]

g:AutoPairsShortcutMultilineClose is gonna be a thing in 3.0.0-beta8.

[caoshenghui]

g:AutoPairsShortcutMultilineClose is gonna be a thing in 3.0.0-beta8.

hey, I think this opt inconvenient to use,especially I used to press eg '{' to jump closed pairs.Is there a better solution?

[LunarWatcher]

Depends on what you mean by inconvenient.

If you mean the entire system... There might be, but I don't have one. Between terminal input problems, conflicting maps, keyboard layouts, and just the number of keys to map, I can't think of options for manual mapping that doesn't cause problems.

If that's not what you mean, you'll have to clarify.

I'm open for ideas though - it's not an absolute implementation. The next couple releases are going to be primarily cleanup anyway. The beta9 hotpatch was not pretty :ablobsweats: