Skip to content

Commit c050c96

Browse files
committed
refactor(keymap): one typed spine taxonomy, fixing inline-vector bindings
The shapes a keymap element can take -- `(KEY . BINDING)` cons, inline vector, char-table, composed submap, parent, prompt string -- were re-decoded ad-hoc at four separate sites, and the copies had drifted. Two iterators in keymap.rs disagreed: `list_keymap_for_each_binding_recursive` handled inline vectors while `list_keymap_for_each_binding` (the one the `where-is-internal` reverse scan uses) did not, so a command bound through a vector was unreachable by `where-is-internal` even though `lookup-key` found it: (let* ((v (make-vector 128 nil)) (m (list 'keymap v))) (aset v ?a 'cmd) (list (lookup-key m "a") ; => cmd both (where-is-internal 'cmd (list m) t))) ; => nil neomacs ; [97] GNU That same drift is what made the #164 dashboard leader hints vanish (three separate sites each missing a different shape). So decode the union once: KeymapElement::{ Binding{key,value} | Submap | IndirectTail | Prompt } `for_each_keymap_element` mirrors GNU `map_keymap_internal` exactly -- including the inline-vector arm and `map_keymap_item`'s `t`-value -> nil normalization (a `t` binding is an explicit unbinding, not a binding of the symbol `t`). It yields `Submap` instead of descending, so each consumer keeps its own descent policy; GNU `map_keymap` treats composed submaps and the parent identically, hence one variant for both. Rebuilt on it: `list_keymap_for_each_binding`, its recursive variant, and `keymap_prompt_scan` (a fourth hand-rolled copy). Now `match` exhaustiveness makes a missed shape a compile error rather than a silent nil. `IndirectTail` is a typed known gap: GNU resolves a symbol spine tail via get_keymap, neomacs resolves it nowhere, so lookup-key and where-is-internal both return nil where GNU finds the binding. Fixing it means plumbing an &Obarray through the forward lookup chain; doing only the reverse side would make the two disagree, which is worse than today's consistent miss. Typed so it is visible instead of silently skipped. Verified against GNU 31 across every element kind (vector, char-table, `(t . DEF)` default, nested vector, composed submap): identical results. 184 keymap / menu / where-is / TTY-menu tests pass across both crates; 20 keymap oracle cases pass, including a new inline-vector parity case.
1 parent 713a9ce commit c050c96

5 files changed

Lines changed: 296 additions & 82 deletions

File tree

‎neovm-core/src/emacs_core/builtins/symbols.rs‎

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6358,29 +6358,32 @@ pub(crate) fn builtin_keymap_prompt(args: Vec<Value>) -> EvalResult {
63586358
}
63596359

63606360
fn keymap_prompt_scan(map: Value) -> Value {
6361-
let mut cursor = map;
6362-
let mut seen = 0usize;
6363-
while cursor.is_cons() {
6364-
seen += 1;
6365-
if seen > 10_000 {
6366-
return Value::NIL;
6367-
}
6368-
6369-
let item = cursor.cons_car();
6370-
if item.is_string() {
6371-
return item;
6372-
}
6373-
if item.is_cons()
6374-
&& crate::emacs_core::keymap::KeymapMarker::Keymap.is_value(item.cons_car())
6375-
{
6376-
let prompt = keymap_prompt_scan(item);
6377-
if !prompt.is_nil() {
6378-
return prompt;
6361+
keymap_prompt_scan_at_depth(map, 0)
6362+
}
6363+
6364+
/// The first prompt string found in MAP's spine, descending into composed
6365+
/// submaps and the parent. Built on the shared keymap-spine taxonomy so this
6366+
/// scan cannot drift from the other spine walkers (see
6367+
/// `keymap::for_each_keymap_element`).
6368+
fn keymap_prompt_scan_at_depth(map: Value, depth: usize) -> Value {
6369+
use crate::emacs_core::keymap::KeymapElement;
6370+
if depth > 64 {
6371+
return Value::NIL;
6372+
}
6373+
let mut found = Value::NIL;
6374+
crate::emacs_core::keymap::for_each_keymap_element(&map, |element| {
6375+
if !found.is_nil() {
6376+
return; // the first prompt in spine order wins
6377+
}
6378+
match element {
6379+
KeymapElement::Prompt(prompt) => found = prompt,
6380+
KeymapElement::Submap(submap) => {
6381+
found = keymap_prompt_scan_at_depth(submap, depth + 1);
63796382
}
6383+
KeymapElement::Binding { .. } | KeymapElement::IndirectTail(_) => {}
63806384
}
6381-
cursor = cursor.cons_cdr();
6382-
}
6383-
Value::NIL
6385+
});
6386+
found
63846387
}
63856388

63866389
pub(crate) fn plan_kill_emacs_request(

‎neovm-core/src/emacs_core/interactive_test.rs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4734,6 +4734,27 @@ fn where_is_internal_reduces_string_menu_item_under_symbol_prefix_like_doom_lead
47344734
assert_eq!(result, "OK t");
47354735
}
47364736

4737+
#[test]
4738+
fn where_is_internal_finds_bindings_stored_in_an_inline_vector() {
4739+
// A keymap element may be an inline vector indexing bindings by char code
4740+
// (GNU `map_keymap_internal`). The reverse where-is scan lacked that arm, so
4741+
// such a command was unreachable by `where-is-internal` although
4742+
// `lookup-key` found it -- found by probing the element taxonomy, not by a
4743+
// bug report.
4744+
crate::test_utils::init_test_tracing();
4745+
let result = eval_one(
4746+
r#"(let* ((v (make-vector 128 nil))
4747+
(sub (list 'keymap v))
4748+
(m (make-sparse-keymap)))
4749+
(aset v ?a 'vec-cmd)
4750+
(define-key m "p" sub)
4751+
(list (eq (lookup-key m "pa") 'vec-cmd)
4752+
(equal (where-is-internal 'vec-cmd (list sub) t) [?a])
4753+
(equal (where-is-internal 'vec-cmd (list m) t) [?p ?a])))"#,
4754+
);
4755+
assert_eq!(result, "OK (t t t)");
4756+
}
4757+
47374758
#[test]
47384759
fn where_is_internal_descends_into_composed_keymaps() {
47394760
// evil/general build active state keymaps with `make-composed-keymap`. The

‎neovm-core/src/emacs_core/keymap.rs‎

Lines changed: 140 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -3446,37 +3446,149 @@ fn keymap_value_eq(a: &Value, b: &Value) -> bool {
34463446
}
34473447
}
34483448

3449-
/// Iterate over all bindings in a keymap (not following parent).
3450-
/// Calls `f(event, def)` for each binding.
3451-
pub fn list_keymap_for_each_binding<F>(keymap: &Value, mut f: F)
3449+
// ---------------------------------------------------------------------------
3450+
// Keymap spine taxonomy
3451+
// ---------------------------------------------------------------------------
3452+
3453+
/// Guard against a circular keymap spine (a `setcdr`-built cycle).
3454+
const MAX_KEYMAP_SPINE_STEPS: usize = 100_000;
3455+
/// Guard against mutually-nested keymaps when a consumer recurses into submaps.
3456+
const MAX_KEYMAP_WALK_DEPTH: usize = 64;
3457+
3458+
/// One element of a keymap's own spine, classified exactly as GNU
3459+
/// `map_keymap_internal` / `access_keymap_1` (keymap.c) classify it.
3460+
///
3461+
/// A keymap is `(keymap ELEMENT... . TAIL)`, and "element" is an untyped union in
3462+
/// Lisp. Re-decoding that union ad-hoc at each call site is how the shapes
3463+
/// drift: this scan silently lacked the inline-vector arm that
3464+
/// [`list_keymap_for_each_binding_recursive`] had, so a command bound through a
3465+
/// vector was invisible to `where-is-internal` even though `lookup-key` found
3466+
/// it. Decode the union once, here, and let `match` exhaustiveness oblige every
3467+
/// consumer to face every shape.
3468+
pub(crate) enum KeymapElement {
3469+
/// A key -> binding pair. Sources: a `(KEY . BINDING)` cons, one slot of an
3470+
/// inline vector (key = slot index), or one entry of a char-table (key = a
3471+
/// character, or a `(FROM . TO)` range).
3472+
///
3473+
/// `value` is normalized like GNU `map_keymap_item`: a `t` value is an
3474+
/// explicit unbinding and is reported as nil.
3475+
Binding { key: Value, value: Value },
3476+
/// A keymap embedded in the spine: a composed submap
3477+
/// (`make-composed-keymap`) or the parent. GNU `map_keymap` treats the two
3478+
/// identically -- recurse into it, then continue with the rest of the spine
3479+
/// -- and both share the enclosing keymap's prefix.
3480+
Submap(Value),
3481+
/// A spine tail that is not a cons: typically a symbol whose function cell is
3482+
/// a keymap (GNU `map_keymap`: `if (!CONSP (map)) map = get_keymap (map,
3483+
/// ...)`). Resolving it needs an obarray, which this structural walk does not
3484+
/// take.
3485+
///
3486+
/// KNOWN GAP, deliberately typed rather than silently skipped: neomacs
3487+
/// resolves this nowhere yet, so both `lookup-key` and `where-is-internal`
3488+
/// return nil where GNU finds the binding (reproduce by `setcdr`-ing a
3489+
/// keymap's tail to a symbol whose function cell is a keymap --
3490+
/// `set-keymap-parent` itself rejects non-keymaps, so this only arises from
3491+
/// hand-built spines). Fixing it means plumbing an `&Obarray` through the
3492+
/// whole forward lookup chain; doing it on only one side would make forward
3493+
/// and reverse lookup disagree, which is worse than today's consistent miss.
3494+
IndirectTail(#[expect(dead_code, reason = "known gap; see variant docs")] Value),
3495+
/// The keymap's prompt string (`make-sparse-keymap` PROMPT).
3496+
Prompt(Value),
3497+
}
3498+
3499+
/// GNU `map_keymap_item`: a `t` binding shadows lower-precedence keymaps exactly
3500+
/// like an explicit nil binding, so it is reported as nil.
3501+
fn normalized_binding_value(value: Value) -> Value {
3502+
if matches!(value.kind(), ValueKind::T) {
3503+
Value::NIL
3504+
} else {
3505+
value
3506+
}
3507+
}
3508+
3509+
/// Visit the elements of ONE keymap's spine, mirroring GNU
3510+
/// `map_keymap_internal`: every binding at this level, in spine order.
3511+
///
3512+
/// Descent is deliberately *not* performed here -- embedded keymaps are yielded
3513+
/// as [`KeymapElement::Submap`] so each consumer keeps its own policy. A
3514+
/// single-level scan ignores them; a `map_keymap`-style walk recurses into them
3515+
/// at the same prefix (they share this keymap's prefix). Elements that match none
3516+
/// of GNU's cases are skipped, as GNU skips them.
3517+
pub(crate) fn for_each_keymap_element<F>(keymap: &Value, mut f: F)
34523518
where
3453-
F: FnMut(Value, Value),
3519+
F: FnMut(KeymapElement),
34543520
{
34553521
let Some(mut cursor) = keymap_binding_spine(keymap) else {
34563522
return;
34573523
};
3524+
let mut steps = 0usize;
34583525
while cursor.is_cons() {
3526+
steps += 1;
3527+
if steps > MAX_KEYMAP_SPINE_STEPS {
3528+
return;
3529+
}
3530+
// The spine tail is itself a keymap (the classic parent). Everything
3531+
// remaining lives inside it, so hand it over and stop walking this level.
34593532
if is_list_keymap(&cursor) {
3460-
break;
3533+
f(KeymapElement::Submap(cursor));
3534+
return;
34613535
}
3462-
let entry_car = cursor.cons_car();
3463-
let entry_cdr = cursor.cons_cdr();
34643536

3465-
if super::chartable::is_char_table(&entry_car) {
3466-
super::chartable::for_each_non_nil_char_table_run(&entry_car, &mut f);
3467-
}
3537+
let element = cursor.cons_car();
3538+
let rest = cursor.cons_cdr();
34683539

3469-
if entry_car.is_cons() {
3470-
let binding_car = entry_car.cons_car();
3471-
let binding_cdr = entry_car.cons_cdr();
3472-
f(binding_car, binding_cdr);
3540+
if is_list_keymap(&element) {
3541+
// A composed submap. GNU `map_keymap` recurses into it and then
3542+
// continues with the rest of the spine, so do not stop here.
3543+
f(KeymapElement::Submap(element));
3544+
} else if super::chartable::is_char_table(&element) {
3545+
super::chartable::for_each_non_nil_char_table_run(&element, |key, value| {
3546+
f(KeymapElement::Binding {
3547+
key,
3548+
value: normalized_binding_value(value),
3549+
});
3550+
});
3551+
} else if element.is_vector() {
3552+
// An inline vector indexes bindings by character code. GNU reports
3553+
// every slot, empty ones included.
3554+
if let Some(items) = element.as_vector_data() {
3555+
for (index, binding) in items.iter().enumerate() {
3556+
f(KeymapElement::Binding {
3557+
key: Value::fixnum(index as i64),
3558+
value: normalized_binding_value(*binding),
3559+
});
3560+
}
3561+
}
3562+
} else if element.is_cons() {
3563+
f(KeymapElement::Binding {
3564+
key: element.cons_car(),
3565+
value: normalized_binding_value(element.cons_cdr()),
3566+
});
3567+
} else if element.is_string() {
3568+
f(KeymapElement::Prompt(element));
34733569
}
34743570

3475-
if is_list_keymap(&entry_cdr) {
3476-
break;
3477-
}
3478-
cursor = entry_cdr;
3571+
cursor = rest;
34793572
}
3573+
3574+
// A non-nil, non-cons tail: a symbol that may name a keymap.
3575+
if !cursor.is_nil() {
3576+
f(KeymapElement::IndirectTail(cursor));
3577+
}
3578+
}
3579+
3580+
/// Iterate over all bindings in a keymap (not following parent or submaps).
3581+
/// Calls `f(event, def)` for each binding.
3582+
pub fn list_keymap_for_each_binding<F>(keymap: &Value, mut f: F)
3583+
where
3584+
F: FnMut(Value, Value),
3585+
{
3586+
for_each_keymap_element(keymap, |element| match element {
3587+
KeymapElement::Binding { key, value } => f(key, value),
3588+
// Single-level by contract: descent, and the prefix bookkeeping it
3589+
// needs, belong to the caller.
3590+
KeymapElement::Submap(_) | KeymapElement::IndirectTail(_) | KeymapElement::Prompt(_) => {}
3591+
});
34803592
}
34813593

34823594
/// Iterate over all bindings in a keymap and its embedded/parent keymaps.
@@ -3494,51 +3606,18 @@ where
34943606
where
34953607
F: FnMut(Value, Value),
34963608
{
3497-
if depth > 64 {
3609+
if depth > MAX_KEYMAP_WALK_DEPTH {
34983610
return;
34993611
}
3500-
3501-
let Some(mut cursor) = keymap_binding_spine(keymap) else {
3502-
return;
3503-
};
3504-
3505-
let mut steps = 0usize;
3506-
while cursor.is_cons() {
3507-
steps += 1;
3508-
if steps > 100_000 {
3509-
break;
3510-
}
3511-
3512-
if is_list_keymap(&cursor) {
3513-
walk(&cursor, f, depth + 1);
3514-
break;
3515-
}
3516-
3517-
let entry_car = cursor.cons_car();
3518-
let entry_cdr = cursor.cons_cdr();
3519-
3520-
if is_list_keymap(&entry_car) {
3521-
walk(&entry_car, f, depth + 1);
3522-
} else if super::chartable::is_char_table(&entry_car) {
3523-
super::chartable::for_each_non_nil_char_table_run(&entry_car, &mut *f);
3524-
} else if entry_car.is_vector() {
3525-
if let Some(items) = entry_car.as_vector_data() {
3526-
for (idx, binding) in items.iter().enumerate() {
3527-
f(Value::fixnum(idx as i64), *binding);
3528-
}
3529-
}
3530-
} else if entry_car.is_cons() {
3531-
let binding_car = entry_car.cons_car();
3532-
let binding_cdr = entry_car.cons_cdr();
3533-
f(binding_car, binding_cdr);
3534-
}
3535-
3536-
if is_list_keymap(&entry_cdr) {
3537-
walk(&entry_cdr, f, depth + 1);
3538-
break;
3539-
}
3540-
cursor = entry_cdr;
3541-
}
3612+
for_each_keymap_element(keymap, |element| match element {
3613+
KeymapElement::Binding { key, value } => f(key, value),
3614+
// Composed submaps and the parent share this keymap's prefix, so
3615+
// their bindings belong to this traversal (GNU `map_keymap`).
3616+
KeymapElement::Submap(submap) => walk(&submap, f, depth + 1),
3617+
// Resolving a symbol tail to its keymap needs an obarray, which this
3618+
// signature does not take; callers that have one resolve it instead.
3619+
KeymapElement::IndirectTail(_) | KeymapElement::Prompt(_) => {}
3620+
});
35423621
}
35433622

35443623
walk(keymap, &mut f, 0);

0 commit comments

Comments
 (0)