Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Error: cannot unmap key that is currently executing #60

Closed
greenfork opened this issue Jun 2, 2023 · 6 comments
Closed

Error: cannot unmap key that is currently executing #60

greenfork opened this issue Jun 2, 2023 · 6 comments

Comments

@greenfork
Copy link

greenfork commented Jun 2, 2023 •
edited

On the latest kakoune master this key sequence from the new line (<enter>s throws these errors

error running hook WinSetOption(inserted_pairs=0)/auto-pairs: 3:7: 'unmap': cannot unmap key that is currently executing
error running hook InsertChar(s)/auto-pairs: 2:7: 'unmap': cannot unmap key that is currently executing

I checked it with clear kak -n only sourcing this plugin.

Any workaround that comes to mind?
@greenfork
Copy link
Author

Looks like this commit mawww/kakoune@e49c0fb is responsible for this error. The version just before this commit works fine.

@altunyurt altunyurt mentioned this issue Jul 4, 2023
Closed
@krobelus
Copy link

krobelus commented Jul 5, 2023

typing the opening ( adds this mapping

map -docstring 'insert a new indented line in pair' window insert <ret> '<a-;>: insert-new-line-in-pair<ret>'

which unmaps itself.
(insert-new-line-in-pair triggers a hook that unmaps <ret>).
We could allow the unmap to proceed. I don't see anything obviously wrong with the script.

@greenfork
Copy link
Author

I suppose it will be fixed in the kakoune then. Just in case, still the same error on the latest master.

@krobelus
Copy link

krobelus commented Jul 5, 2023

if it's easy to rewrite the script to not rely on this behavior, we might want to keep the error.

This patch fixes your reproducer although there is a remaining issue when typing ())

diff --git a/rc/auto-pairs.kak b/rc/auto-pairs.kak
index 8583a56..fa932ac 100644
--- a/rc/auto-pairs.kak
+++ b/rc/auto-pairs.kak
@@ -103,20 +103,16 @@ define-command -override -hidden handle-inserted-opening-pair -params 2 %{
     map -docstring 'insert a new indented line in pair' window insert <ret> '<a-;>: insert-new-line-in-pair<ret>'
     map -docstring 'prompt a count for new indented lines in pair' window insert <c-ret> '<a-;>: prompt-insert-new-line-in-pair<ret>'
 
+    hook -group auto-pairs -once window InsertChar %exp{\Q%arg{2}} %exp{
+      unmap window insert %arg{2} # TODO quote
+    }
+
     # Enter is only available on next key.
     hook -group auto-pairs -once window InsertChar '.*' %{
       unmap window insert <ret>
       unmap window insert <c-ret>
     }
 
-    # Clean insert mappings and remove hooks
-    hook -group auto-pairs -once window WinSetOption 'inserted_pairs=0' "
-      unmap window insert %%🐈%arg{2}🐈
-      unmap window insert <ret>
-      unmap window insert <c-ret>
-      remove-hooks window auto-pairs
-    "
-
     # Clean state when moving or leaving insert mode
     hook -group auto-pairs -once window InsertMove '.*' %{
       reset-inserted-pairs-count

krobelus added a commit to krobelus/kakoune that referenced this issue Jul 6, 2023
@krobelus
Allow map/unmap during mapping execution
999dba5 
Commits e49c0fb (unmap: fail if the mapping is currently executing,
2023-05-14) 42be005 (map: fail if key is currently executing,
2023-06-24) fixed potential use-after-free issues. By doing so,
it broke configurations that in practice have not triggered any
crashes[1][2].

For example with,

	set -remove global autocomplete insert
	hook global InsertCompletionShow .* %{
	    map window insert <esc> <c-o>
	}
	hook global InsertCompletionHide .* %{
	    unmap window insert <esc> <c-o>
	}

The execution of the <esc> mapping triggers InsertCompletionHide fails
at unmapping. This seems legit and I don't see an obvious alternative
way to write it (InsertIdle would not be correct though it would work
in practice).

Fix the regression by allowing map and unmap again while keeping the
mappings alive until garbage collection. This is similar to how we
treat hooks and others.
To-do: we can probably clean up the KeymapManager a bit.

[1]: <kakoune-lsp/kakoune-lsp#689>
[2]: <alexherbo2/auto-pairs.kak#60>
krobelus added a commit to krobelus/kakoune that referenced this issue Jul 6, 2023
@krobelus
Allow map/unmap during mapping execution
89cca38 
Commits e49c0fb (unmap: fail if the mapping is currently executing,
2023-05-14) 42be005 (map: fail if key is currently executing,
2023-06-24) fixed potential use-after-free issues. By doing so,
it broke configurations that in practice have not triggered any
crashes[1][2].

For example with,

	set -remove global autocomplete insert
	hook global InsertCompletionShow .* %{
	    map window insert <esc> <c-o>
	}
	hook global InsertCompletionHide .* %{
	    unmap window insert <esc> <c-o>
	}

The execution of the <esc> mapping triggers InsertCompletionHide fails
at unmapping. This seems legit and I don't see an obvious alternative
way to write it (InsertIdle would not be correct though it would work
in practice).

Fix the regression by allowing map and unmap again while keeping the
mappings alive until garbage collection. This is similar to how we
treat hooks and others.

Applying map/unmap immediately seems like the most obvious semantics.
Alternatively, we could apply them in between key presses.

To-do: we can probably clean up the KeymapManager a bit.

[1]: <kakoune-lsp/kakoune-lsp#689>
[2]: <alexherbo2/auto-pairs.kak#60>
@krobelus krobelus mentioned this issue Jul 6, 2023
Merged
krobelus added a commit to krobelus/kakoune that referenced this issue Jul 8, 2023
@krobelus
Allow map/unmap during mapping execution
98b5b70 
Commits e49c0fb (unmap: fail if the mapping is currently executing,
2023-05-14) 42be005 (map: fail if key is currently executing,
2023-06-24) fixed potential use-after-free issues. By doing so,
it broke configurations that in practice have not triggered any
crashes[1][2].

For example with,

	set -remove global autocomplete insert
	hook global InsertCompletionShow .* %{
	    map window insert <esc> <c-o>
	}
	hook global InsertCompletionHide .* %{
	    unmap window insert <esc> <c-o>
	}

The execution of the <esc> mapping triggers InsertCompletionHide fails
at unmapping. This seems legit and I don't see an obvious alternative
way to write it (InsertIdle would not be correct though it would work
in practice).

Fix the regression by allowing map and unmap again while keeping the
mappings alive until garbage collection. This is similar to how we
treat hooks and others.

Applying map/unmap immediately seems like the most obvious semantics.
Alternatively, we could apply them in between key presses.

To-do: we can probably clean up the KeymapManager a bit.

[1]: <kakoune-lsp/kakoune-lsp#689>
[2]: <alexherbo2/auto-pairs.kak#60>
krobelus added a commit to krobelus/kakoune that referenced this issue Jul 9, 2023
@krobelus
Allow map/unmap during mapping execution
b3146d6 
Commits e49c0fb (unmap: fail if the mapping is currently executing,
2023-05-14) 42be005 (map: fail if key is currently executing,
2023-06-24) fixed potential use-after-free issues. By doing so,
it broke configurations that in practice have not triggered any
crashes[1][2].

For example with,

	set -remove global autocomplete insert
	hook global InsertCompletionShow .* %{
	    map window insert <esc> <c-o>
	}
	hook global InsertCompletionHide .* %{
	    unmap window insert <esc> <c-o>
	}

The execution of the <esc> mapping triggers InsertCompletionHide fails
at unmapping. This seems legit and I don't see an obvious alternative
way to write it (InsertIdle would not be correct though it would work
in practice).

Fix the regression by allowing map and unmap again while keeping the
mappings alive until garbage collection. This is similar to how we
treat hooks and others.

Applying map/unmap immediately seems like the most obvious semantics.
Alternatively, we could apply them in between key presses.

To-do: we can probably clean up the KeymapManager a bit.

[1]: <kakoune-lsp/kakoune-lsp#689>
[2]: <alexherbo2/auto-pairs.kak#60>
krobelus added a commit to krobelus/kakoune that referenced this issue Jul 20, 2023
@krobelus
Allow map/unmap during mapping execution
1231041 
Commits e49c0fb (unmap: fail if the mapping is currently executing,
2023-05-14) 42be005 (map: fail if key is currently executing,
2023-06-24) fixed potential use-after-free issues. By doing so,
it broke configurations that in practice have not triggered any
crashes [1] [2].

For example with,

	set -remove global autocomplete insert
	hook global InsertCompletionShow .* %{
	    map window insert <esc> <c-o>
	}
	hook global InsertCompletionHide .* %{
	    unmap window insert <esc> <c-o>
	}

The execution of the <esc> mapping triggers InsertCompletionHide fails
at unmapping. This seems legit and I don't see an obvious alternative
way to write it (InsertIdle would not be correct though it would work
in practice).

Fix the regression by allowing map and unmap again while keeping the
mappings alive until they have finished executing.

Applying map/unmap immediately seems like the most obvious semantics.
Alternatively, we could apply them in between key presses.

[1]: <kakoune-lsp/kakoune-lsp#689>
[2]: <alexherbo2/auto-pairs.kak#60>
@krobelus
Copy link

this is fixed latest Kakoune master

@greenfork
Copy link
Author

I can confirm, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@krobelus @greenfork