zyedidia / micro

A modern and intuitive terminal-based text editor
https://micro-editor.github.io
MIT License
24.4k stars 1.16k forks source link

Crash when attempting to bind two key sequences C-x C-c Quit and C-x C-s Save #3194

Closed khaytsus closed 2 months ago

khaytsus commented 3 months ago

Description of the problem or steps to reproduce

I've used an editor for many, many years called microEmacs and I'm binding a few common keys I am used to use there into Micro to make it a little easier to transition to it. I was able to bind Control-x Control-s to Save just fine,

bind <Ctrl-x><Ctrl-s> "Save"

And it works. But I was trying to bind Control-X Control-c to quit and Micro crashes as soon as I hit enter.

bind <Ctrl-x><Ctrl-c> "Quit"

Micro encountered an error: runtime.errorString runtime error: comparing uncomparable type action.KeySequenceEvent /usr/lib/golang/src/runtime/alg.go:266 (0x55e57f146d1a) ifaceeq: if eq == nil { /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/internal/action/bindings.go:269 (0x55e57f603514) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/internal/action/command.go:663 (0x55e57f60c8c6) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/internal/action/command.go:983 (0x55e57f60f1e4) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/internal/action/actions.go:1470 (0x55e57f5ff892) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/internal/info/infobuffer.go:147 (0x55e57f5d258e) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/internal/action/infopane.go:190 (0x55e57f61310c) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/internal/action/infopane.go:54 (0x55e57f61255d) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/internal/action/infopane.go:125 (0x55e57f612a59) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/internal/action/infopane.go:93 (0x55e57f6127d0) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/cmd/micro/micro.go:443 (0x55e57f642c1c) /builddir/build/BUILD/micro-2.0.11/_build/src/github.com/zyedidia/micro/cmd/micro/micro.go:382 (0x55e57f64268a) /usr/lib/golang/src/runtime/internal/atomic/types.go:194 (0x55e57f17bf72) (*Uint32).Load: return Load(&u.value) /usr/lib/golang/src/runtime/asm_amd64.s:1598 (0x55e57f1ac481) goexit: DATA shifts<>+0x00(SB)/8, $0x0000000000000000

After experimenting some more, it appears that I can't bind this sort of thing twice, unrelated to the command or control characters. If I remove the bindings.json I can bind whatever, but if I set one, then set another one, it crashes every time. If I remove bindings.json I can create Ctrl-x Ctrl-c and set it to Quit, works, but now I can't create Ctrl-x Ctrl-s for Save.

bindings.json after I've created a "Quit" command:

{ "\u003cCtrl-x\u003e\u003cCtrl-c\u003e": "Quit", "Alt-/": "lua:comment.comment", "CtrlUnderscore": "lua:comment.comment" }

If I manually make a binding.json of the following, these two bindings do function in the editor:

{ "\u003cCtrl-x\u003e\u003cCtrl-c\u003e": "Quit", "\u003cCtrl-x\u003e\u003cCtrl-s\u003e": "Save", }

And I can even add simple bindings later, like bind Ctrl-s "Find". 
It's just when I try to do another <Ctrl-x><Ctrl-s> type binding it fails.

Basic reproduction steps:

1. Run micro
2. Hit control-e, type bind <Ctrl-x><Ctrl-c> "Quit"
3. Hit control-e, type bind <Ctrl-x><Ctrl-s> "Save"

Specifications

Commit hash: 225927b OS: Fedora 38 Terminal: xfce-terminal

I have also tried on the latest Release version from the github, 68d88b57

Sorry for the weird formatting, github markdown (?) was eating some of the sequences.

dustdfg commented 3 months ago

I confirm it leads to crash on current master c64add289b396b35f790c5b20b9db9c750fb1706 with

Micro encountered an error: runtime.errorString runtime error: comparing uncomparable type action.KeySequenceEvent
runtime/alg.go:266 (0x40519a)
github.com/zyedidia/micro/v2/internal/action/bindings.go:282 (0x8ac385)
github.com/zyedidia/micro/v2/internal/action/command.go:673 (0x8b67df)
github.com/zyedidia/micro/v2/internal/action/command.go:989 (0x8b9144)
github.com/zyedidia/micro/v2/internal/action/actions.go:1545 (0x8a8092)
github.com/zyedidia/micro/v2/internal/info/infobuffer.go:152 (0x87d494)
github.com/zyedidia/micro/v2/internal/action/infopane.go:206 (0x8bd68c)
github.com/zyedidia/micro/v2/internal/action/infopane.go:54 (0x8bc656)
github.com/zyedidia/micro/v2/internal/action/infopane.go:129 (0x8bcea2)
github.com/zyedidia/micro/v2/internal/action/infopane.go:93 (0x8bcb5e)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:476 (0x8f0287)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:395 (0x8efc9e)
runtime/proc.go:250 (0x43afd2)
runtime/asm_amd64.s:1594 (0x46a9a1)

Manually setting values int the bindings.json file works

khaytsus commented 3 months ago

Out of curiosity I tried to see if this was a regression and it seems like it maybe has always been this way, 2.0.7 was the first version that seemed to support this syntax and it crashes similarly.

dustdfg commented 3 months ago

By adding small print in the code before error, I understood why it reports uncomparable types

screen-1710855638

screen-1710855662

function findEvent internally use two functions: findSingleEvent for single events and findEvents for frequencies. First returns one event while another returns an array of such events wrapped to an object. So you can't compare. These two types are uncomparable but findEvent just a facade that returns one of the result of those two functions.

https://github.com/zyedidia/micro/blob/c64add289b396b35f790c5b20b9db9c750fb1706/internal/action/bindings.go#L111-L136

https://github.com/zyedidia/micro/blob/c64add289b396b35f790c5b20b9db9c750fb1706/internal/action/bindings.go#L138

But I am curios why does type system doesn't report it as a problem? I findEvent returns Event as like findSingleEvent but findEvents returns KeySequenceEvent how it can fit there?

https://github.com/zyedidia/micro/blob/c64add289b396b35f790c5b20b9db9c750fb1706/internal/action/bindings.go#L237-L252

JoeKar commented 3 months ago

Interesting, before the comparison they should...at least from my understanding...type checked and in case of KeySequenceEvent iterated once again and within that iteration compared against the entry, which is a KeyEvent.

dustdfg commented 3 months ago

You iterate through all the bindings in the bindings.json And then compare them with event passed to TryBindKey. If we will represent it like array of events in the bindings.json we will get something like [Event,Event,KeySequenceEvent,Event,...]. When you call the TryBindKey you iterate trough this array and will check all the elemnts one by one if it is equal to the event you passed to TryBindKey so it leads to bug

dustdfg commented 3 months ago

Or maybe I am wrong. I am almost always wrong ¯\_(ツ)_/¯. I've just deleted from my bindings all the events in order to prevent check between Event and KeySequenceEvent and the problem still persist 😅

dmaluka commented 3 months ago

https://go.dev/ref/spec#Comparison_operators

Interface types that are not type parameters are comparable. Two interface values are equal if they have identical dynamic types and equal dynamic values or if both have value nil.

Struct types are comparable if all their field types are comparable.

A comparison of two interface values with identical dynamic types causes a run-time panic if that type is not comparable.

KeyEvent is a comparable type, KeySequenceEvent is not.

So comparing KeyEvent with KeyEvent is legal, comparing KeyEvent with KeySequenceEvent is also legal (and always returns false), but comparing KeySequenceEvent with KeySequenceEvent triggers a panic.