Fix possible misplaced cursor after :map expr (Issue #12707)

[JaySandhu]

My attempt at tackling issue: #12707.

Fix corner case that occurs when both:

1. Evaluating a mapped expression causes the cursor to move.
2. The evaluated result does not resolve to a character.

When a mapping does not result in a character, the UI is flushed, and we go back to waiting for user input. When the cursor has moved, the UI is flushed in an incorrect state, this can be resolved with a simple call to setcursor().

[justinmk]

setcursor() is a bit drastic, specially in vgetorpeek() which is a hot path. But (1) this is a rare branch in that function so it probably is ok, and (2) there is precedent elsewhere in the function. Actually :map <expr> is pretty common. I think this might work just by accident, could probably plop setcursor() anywhere in a common part of vgetorpeek().

• Looks like a couple tests are failing, but easy to fix those. The result seems "better"? I didn't look at the steps in those tests though.
• Can you think of a test case that will cover this behavior specifically? Those failing tests provide a good starting point, or maybe just use the steps from cursor stays in the command line after input() if the buffer doesn't change #12707 .
  • You can run the test like this:

    TEST_FILE=test/functional/example_spec.lua make functionaltest
    

[JaySandhu]

Yh I agree, calling setcursor there was probably a bit hasty. Most mapped expressions do result in a character, and we shouldn't be pessimizing the common case. But it should be noted that eval_map_expr is a pretty heavy function, it might overwhelm the effect of the setcursor call.

• Looks like a couple tests are failing, but easy to fix those. The result seems "better"?

The way the tests are failing is not good. The cursor is updated but the screen is not. So, for example, the screen is not unscrolled after displaying messages. The way to fix it would be to do something like:

if (must_redraw) {
  update_screen(0);
}

setcursor();

Which is obviously a heavy handed approach.

There already exists logic in vgetorpeek that attempts to update the screen before doing a blocking wait:

neovim/src/nvim/getchar.c

Lines 2235 to 2246 in 1e14dac

	/* In insert mode a screen update is skipped when characters
	* are still available. But when those available characters
	* are part of a mapping, and we are going to do a blocking
	* wait here. Need to update the screen to display the
	* changed text so far. Also for when 'lazyredraw' is set and
	* redrawing was postponed because there was something in the
	* input buffer (e.g., termresponse). */
	if (((State & INSERT) != 0 || p_lz) && (State & CMDLINE) == 0
	&& advance && must_redraw != 0 && !need_wait_return) {
	update_screen(0);
	setcursor(); /* put cursor back where it belongs */
	}

The screen update is skipped when must_redraw is 0, sensible enough. The problem is, the cursor can be misplaced even when must_redraw == 0. Also, the screen update only happens in insert mode, or if lazyredraw is enabled, but the cursor can be misplaced in other modes with lazyredraw off.

I don't know the best way to tackle this, two options I came up with:

1. Handle the screen update immediately after eval_map_expr.
2. Add a local did_eval_expr flag and do something like:

if (did_eval_expr) {
  if (must_redraw) {
    update_screen(0);
  }

  setcursor();
  did_eval_expr = FALSE;
} else if (((State & INSERT) != 0 || p_lz) && (State & CMDLINE) == 0
           && advance && must_redraw != 0 && !need_wait_return) {
  update_screen(0);
  setcursor();
}

In the first case we pay for a setcursor and a potential screen update after every :map expr. In the second we add an additional branch to a very hot code path.

[JaySandhu]

The tests are still failing, but now the results really are better.

[x-yuri]

In case this might give you an idea for a test, I can describe what led to this issue ("X" in the "XY problem"). eskk.vim makes it possible to type in Japanese. To give you some idea of what it might look like (animated gif):

For that it imap <expr>s alphabetical letters and some other keys to a function (eskk#filter()) that decides what to send to the buffer next. Sometimes it's just transliteration (romaji -> hiragana/katakana), sometimes it's search in a system dictionary (hiragana -> kanji). If a word is not found in the dictionary, user is prompted for a kanji version of a word (input), to be able to add an entry (hiragan/kanji pair) to a user dictionary. And user can cancel that (Esc, Ctrl-C, Ctrl-G, which are as well mapped to eskk#filter()). As a result, eskk#filter() returns '', but before that it does redraw | echo ''.

At some point I tried to compile nvim from this branch, and I can confirm that it behaved identically to vim in this regard. And as such the same went for eskk.vim. But considering you seem to haven't yet decided on the solution, for now it probably makes no sense trying.

On a side note, can you possibly comment on this redraw | echo '' thing here or in the issue? It might have served some additional purpose in the past. But at the moment it looks like a hack. If you want to empty the cmdline mode line, you need only echo ''. But instead you can do just redraw, which will redisplay the --INSERT-- thing. And it seems more in line with the vim's usual behavior. Especially considering that when eskk.vim is enabled/activated, it indicates that it's going to convert your input (--INSERT (lang)--). What do you think?

[JaySandhu]

can you possibly comment on this redraw | echo '' thing

I'm not an expert, but I think you have it right. I think having just a redraw would be best, that way --INSERT-- will be redrawn, which is nice. But if you're really after an empty command line, then echo '' works, but I don't see why you would want both a redraw and an echo ''.

[JaySandhu]

I uncovered a few more drawing bugs related to expression mappings, they're documented in the original issue (#12707). This PR addresses those too. After commit e30e054 I believe them all to be fixed.

I went digging around the Vim code base to see how they're dealing with this. It turns out Vim does something analogous to setcursor and ui_flush, so we're on good footing.

Just to further put everyone's mind at ease, I did some profiling:
Timings done on an Intel Core i7-4790K CPU @ 4.00GHz.

function! F1()
    return ""
endfunction

imap <expr> a ""
imap <expr> b F1()

Mapping	eval_map_expr Time
a	2.10 ms
b	5.50 ms

The timings really start to explode with any moderately complex expression. I refactored the screen updating changes into a function for testing purposes:

void expr_map_redraw() {
  if (State & CMDLINE) {
    redrawcmdline();
  } else {
    if (must_redraw) {
      update_screen(0);
    }

    setcursor();
  }
}

The function executed within 200.00 µs even when the must_redraw branch is taken. In the common case, where it only has to do a setcursor, it finishes in under 100.00 µs. I suspect the actual number is much lower, but we're hitting up against Xcode's profiler's resolution limits.

[justinmk]

I went digging around the Vim code base to see how they're dealing with this. It turns out Vim does something analogous to setcursor and ui_flush, so we're on good footing.

Ah nice, thanks for investigating. So we can mark Vim patch 8.1.2336 vim/vim@4ebe0e6 as merged after this PR.

• include the changes to src/testdir/test_mapping.vim , even though we don't use Vim's screendump tests this helps make merges easier.
• ignore the .dump file
• the vim test would be easy to port to a Nvim Lua screen test. See test/functional/example_spec.lua for example. You can run the test like this:

   TEST_FILE=test/functional/example_spec.lua make functionaltest
  

Just to further put everyone's mind at ease, I did some profiling:

Perfect, thank you!

[justinmk]

any reason not to use the same approach as in vim/vim@4ebe0e6 ? I.e.

int was_screen_col = screen_cur_col;
int was_screen_row = screen_cur_row;

...

ui_cursor_goto(was_screen_row, was_screen_col);
ui_flush();

[JaySandhu]

The update_screen is needed to clear messages. Super contrived example:

function! F1()
    echoerr "test"
    echoerr "test"
    echoerr "test"
    call input("> ")
    return ""
endfunction

map <expr> a F1()

Without a screen_update, the cursor is restored, but the screen isn't. We could maybe test for msg_did_scroll rather than must_redraw, but there could some other obscure corner case that requires a screen update. The ui is flushed the next time we block, or when a something has changed, figured it was fine to wait for that flush.

The cursor strategy does seem better. One thing though, I'm not sure how ui_cursor_goto and multigrid interact, I did notice that ui_cursor_goto always uses the default grid, would that produce the correct result when multigrid is active? Maybe it takes something like:

ui_grid_cursor_goto(curwin->w_grid.handle, was_screen_row, was_screen_col);

[justinmk]

ui_grid_cursor_goto is low-level, ui_cursor_goto will (must) do the right thing.

[JaySandhu]

Great, thanks. I'll make the changes.

I forgot to mention, the redrawcmdline call is their to address the bugs I added to the original issue.

[JaySandhu]

Switched to ui_cursor_goto and added tests. A couple points:

• The Vim and Neovim versions of src/testdir/test_mapping.vim are quite out of sync, do I update it all or only the changes in vim/vim@4ebe0e6? Also, they use Vim specific terminal APIs, so not sure what to do there.

• The test case used in vim/vim@4ebe0e6 already passes in Neovim. I used a different one, but it's getting at the same bug.

• Vim patch 8.1.1591 (vim/vim@7d491c4) is also needed, it addresses the memory corruption issues in Race condition in GC may free arguments twice #12013 and Use after free caused by early garbage collection #12846. It's a couple of lines and it's in the same location as the changes in this PR, should I just lump it in here, or is it best to open a new PR?

[justinmk]

note: the linter is complaining about the length of

src/nvim/getchar.c:2397:  Small and focused functions are preferred: vgetorpeek() has 502 non-comment lines (error triggered by exceeding 500 lines).  [readability/fn_size] [1]

We obviously can't rewrite vgetorpeek() here, so let's add a NOLINT line to the end of it like this :

neovim/src/nvim/ex_cmds.c

Line 4221 in d8e6a03

} // NOLINT(readability/fn_size)

• The Vim and Neovim versions of src/testdir/test_mapping.vim are quite out of sync, do I update it all or only the changes in vim/vim@4ebe0e6? Also, they use Vim specific terminal APIs, so not sure what to do there.

only changes from the given patch. Otherwise it's confusing when merging the other patches. Usually we try to merge in-order though, to avoid confusion.

• The test case used in vim/vim@4ebe0e6 already passes in Neovim. I used a different one, but it's getting at the same bug.

👍

• Vim patch 8.1.1591 (vim/vim@7d491c4) is also needed, it addresses the memory corruption issues in Race condition in GC may free arguments twice #12013 and Use after free caused by early garbage collection #12846. It's a couple of lines and it's in the same location as the changes in this PR, should I just lump it in here, or is it best to open a new PR?

small patch is fine to put here, thanks!

[JaySandhu]

Fixed the linter errors and merged the vim patches. I think that should do it. The CI build failure seems unrelated.

[JaySandhu]

Hmm, seems as though my lua also has linting issues. I will take a look, but I'm not a lua expert.

A quick note on the GC patch, the Vim version addresses a bug in Vim's timer code. We use a different timer implementation, and as far as I can tell, our version is not susceptible to the same flaw. Specifically, the GC is correctly handled in nv_event in normal.c.

[justinmk]

We use a different timer implementation, and as far as I can tell, our version is not susceptible to the same flaw. Specifically, the GC is correctly handled in nv_event in normal.c.

Yes, correct.

Should I clean up the commit history, or leave it as is?

Since we are just trying to merge now (review is done), it's a good time to rebase and fixup commits. We don't want meaningless commits like "fix single lint error" in the history.

[JaySandhu]

CI is happy yay! Commits can be squashed down further if that's desired. Other than that, I think it's ready.

Closes #12707, #12846, and #12013.

[x-yuri]

I'm not sure about your workflow, but shouldn't this PR be added to the 0.6 milestone? Considering there's already a pre-release. It'd be nice to have this fixed in the next release.

[clason]

Don't pay too much attention to milestones; they don't mean much unless close to a release. If/when it's merged, it will be automatically included in the next (non-bugfix) release, which is 0.6. (And "pre-release" is just another name for nightlies, it doesn't imply anything about the imminence of a release.)

For this to be merged, it first has to be rebased on master and the conflicts fixed. @JaySandhu are you still available for that?

[x-yuri]

I've rebased the changes:

Restore cursor position after expr mapping
vim-patch 8.1.1591: on error garbage collection may free memory in use
vim-patch 8.1.2336: when an expr mapping moves the cursor it is not restored

onto master (or rather cherry-picked the commits):

Restore cursor position after expr mapping
vim-patch 8.1.2336: when an expr mapping moves the cursor it is not restored

8.1.1591 was applied here.

The first commit had a conflict in src/nvim/getchar.c because a function handle_mapping() was extracted from vgetorpeek(). As such NOLINT is supposedly no longer needed.

The second commit had a conflict in the same file, and in src/nvim/testdir/test_mapping.c. I resolved the second conflict by putting the test to the place where it is in the vim repository, for what it's worth.

It's worth noting that they put the cursor back before restoring vgetc_busy, and may_garbage_collect.

Other than that, it resolves the issue, and the tests pass.

@JaySandhu, can you update the PR? Or should I create a new one?

[JaySandhu]

I updated the PR. Thank you @x-yuri for resolving the conflicts.

[x-yuri]

Okay, now, how do we make it merged? :) I see that it needs a maintainer approval. Is there anything I can do to help you? @justinmk? @clason?

[zeertzjq]

Bram's comment about the redrawing problem: vim/vim@74a0a5b#commitcomment-66443154:

Nope, in case you intentionally show a message you have to cleanup your own mess.
It's only for an (accidental) error that I made it work better. In the help is mentioned:
If something changed that requires Vim to
go through the main loop (e.g. to update the display), return <Ignore>.

I think it makes sense.

[JaySandhu]

Interesting. That language wasn't in the help when this PR was originally authored. I believe it was added later: b16c7c5.

In my opinion, there's not a lot of harm done by the update_screen call, it executes reasonably fast, and I think it could catch some remaining edge cases.

In any case, if we were to remove it, we would have to add something similar to that vim patch to handle accidental error messages. At the moment, the call to update_screen is clearing both intentional and accidental messages.

[zeertzjq]

I merged #17432, which includes some code from this PR and fixes #12707. The screen is still not redrawn, but as the comment in the code said:

The mapping may do anything, but we expect it to take care of redrawing.