Skip to content

Commit 3948d4a

Browse files
committed
fix(keymap): accessible-keymaps follows parent keymaps (breadth-first)
`accessible-keymaps` used a depth-first walk that deliberately stopped at a keymap's parent ("parent keymap, don't descend"), so prefix maps reachable only through the parent were omitted -- e.g. a child and parent both binding C-c to different prefix maps listed one [3] entry instead of GNU's two. Rewrite it to mirror GNU `Faccessible_keymaps` (keymap.c): a breadth-first walk over a growing (prefix, map) queue, scanning each map's bindings via a new map_keymap-style walker that follows the parent chain (reporting embedded prefix maps as bindings without descending). Enqueue a map again under a longer/unrelated prefix but skip it when an already-queued strict-prefix path reaches the same map -- GNU's cycle rule, which reproduces GNU's duplicate listings and terminates on self-referential parents. Also picks up char-table bindings the old DFS missed. Update the guard unit test (which asserted the non-GNU no-descend behavior) to expect the parent's prefix map. Verified against GNU: combo case now ([] [3] [3]); BFS ordering ([] [3] [24] [3 4]) matches. Fixes keymap_parent_accessible_keymaps_combo.
1 parent c9e9bc7 commit 3948d4a

3 files changed

Lines changed: 88 additions & 53 deletions

File tree

neovm-core/src/emacs_core/builtins/keymaps.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -210,9 +210,7 @@ pub(crate) fn builtin_accessible_keymaps_impl(obarray: &Obarray, args: &[Value])
210210

211211
// Collect all accessible keymaps
212212
let mut all_out = Vec::new();
213-
let mut prefix = Vec::new();
214-
let mut seen = Vec::new();
215-
list_keymap_accessible(&keymap, &mut prefix, &mut all_out, &mut seen);
213+
list_keymap_accessible(&keymap, &mut all_out);
216214

217215
// If prefix argument is provided, filter results
218216
if let Some(prefix_arg) = args.get(1) {

neovm-core/src/emacs_core/keymap.rs

Lines changed: 75 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -3281,59 +3281,90 @@ fn copy_char_table_for_keymap(ct: &Value, depth: usize) -> Value {
32813281
copied
32823282
}
32833283

3284-
/// Collect all accessible sub-keymaps with their key prefixes.
3285-
pub fn list_keymap_accessible(
3286-
keymap: &Value,
3287-
prefix: &mut Vec<Value>,
3288-
out: &mut Vec<Value>,
3289-
seen: &mut Vec<Value>,
3290-
) {
3291-
// Detect cycles: check if we've seen this exact keymap object
3292-
for s in seen.iter() {
3293-
if keymap_value_eq(s, keymap) {
3294-
return;
3284+
/// Walk KEYMAP's own bindings and those of its parent chain, mirroring GNU
3285+
/// `map_keymap`: it reports embedded/prefix keymaps as ordinary bindings
3286+
/// (without descending into them) and then FOLLOWS the parent keymap. Bounded
3287+
/// against self-referential parents by walking each map object at most once.
3288+
fn map_keymap_following_parents<F>(keymap: &Value, mut f: F)
3289+
where
3290+
F: FnMut(Value, Value),
3291+
{
3292+
let mut map = *keymap;
3293+
let mut seen_maps: Vec<Value> = Vec::new();
3294+
while is_list_keymap(&map) {
3295+
if seen_maps.iter().any(|m| keymap_value_eq(m, &map)) {
3296+
break;
32953297
}
3296-
}
3297-
seen.push(*keymap);
3298-
3299-
// Add current keymap
3300-
out.push(Value::cons(Value::vector(prefix.clone()), *keymap));
3301-
3302-
if !keymap.is_cons() {
3303-
return;
3304-
};
3305-
let pair_car = keymap.cons_car();
3306-
let pair_cdr = keymap.cons_cdr();
3307-
if !KeymapMarker::Keymap.is_value(pair_car) {
3308-
return;
3309-
}
3298+
seen_maps.push(map);
33103299

3311-
// Scan alist entries for prefix keymaps
3312-
let mut cursor = pair_cdr;
3313-
while cursor.is_cons() {
3314-
if is_list_keymap(&cursor) {
3300+
let Some(mut cursor) = keymap_binding_spine(&map) else {
33153301
break;
3316-
}
3317-
let entry_car = cursor.cons_car();
3318-
let entry_cdr = cursor.cons_cdr();
3302+
};
3303+
while cursor.is_cons() {
3304+
if is_list_keymap(&cursor) {
3305+
break; // reached the parent keymap
3306+
}
3307+
let entry_car = cursor.cons_car();
3308+
let entry_cdr = cursor.cons_cdr();
33193309

3320-
if entry_car.is_cons() {
3321-
let binding_car = entry_car.cons_car();
3322-
let binding_cdr = entry_car.cons_cdr();
3323-
if is_list_keymap(&binding_cdr) {
3324-
prefix.push(binding_car);
3325-
list_keymap_accessible(&binding_cdr, prefix, out, seen);
3326-
prefix.pop();
3310+
if super::chartable::is_char_table(&entry_car) {
3311+
super::chartable::for_each_non_nil_char_table_run(&entry_car, &mut f);
3312+
}
3313+
if entry_car.is_cons() {
3314+
f(entry_car.cons_car(), entry_car.cons_cdr());
33273315
}
3328-
}
33293316

3330-
if is_list_keymap(&entry_cdr) {
3331-
break; // parent keymap, don't descend
3317+
if is_list_keymap(&entry_cdr) {
3318+
break; // the parent follows; handled by the outer loop
3319+
}
3320+
cursor = entry_cdr;
3321+
}
3322+
map = get_keymap_tail_parent(&map);
3323+
}
3324+
}
3325+
3326+
/// Collect all accessible sub-keymaps with their key prefixes, mirroring GNU
3327+
/// `Faccessible_keymaps` (keymap.c). This is a breadth-first walk over a growing
3328+
/// queue of `(prefix, map)` pairs: for each queued map we scan its bindings
3329+
/// (following parents, per GNU `map_keymap`) and enqueue every prefix sub-map.
3330+
/// A map is enqueued again under a longer/unrelated prefix, but skipped when an
3331+
/// already-queued strict-prefix path reaches the same map (GNU's cycle rule),
3332+
/// which both matches GNU's duplicate listings and terminates on cycles.
3333+
pub fn list_keymap_accessible(keymap: &Value, out: &mut Vec<Value>) {
3334+
let mut maps: Vec<(Vec<Value>, Value)> = vec![(Vec::new(), *keymap)];
3335+
let mut i = 0;
3336+
while i < maps.len() {
3337+
let thisseq = maps[i].0.clone();
3338+
let thismap = maps[i].1;
3339+
i += 1;
3340+
3341+
let mut found: Vec<(Vec<Value>, Value)> = Vec::new();
3342+
map_keymap_following_parents(&thismap, |event, def| {
3343+
if is_list_keymap(&def) {
3344+
let mut newseq = thisseq.clone();
3345+
newseq.push(event);
3346+
found.push((newseq, def));
3347+
}
3348+
});
3349+
3350+
for (newseq, submap) in found {
3351+
let is_cycle = maps.iter().any(|(prefix, map)| {
3352+
keymap_value_eq(map, &submap)
3353+
&& prefix.len() < newseq.len()
3354+
&& prefix
3355+
.iter()
3356+
.zip(newseq.iter())
3357+
.all(|(a, b)| a.bits() == b.bits())
3358+
});
3359+
if !is_cycle {
3360+
maps.push((newseq, submap));
3361+
}
33323362
}
3333-
cursor = entry_cdr;
33343363
}
33353364

3336-
seen.pop();
3365+
for (prefix, map) in &maps {
3366+
out.push(Value::cons(Value::vector(prefix.clone()), *map));
3367+
}
33373368
}
33383369

33393370
/// Check if two keymap values are the same object (by cons cell identity).

neovm-core/src/emacs_core/keymap_test.rs

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -590,7 +590,7 @@ fn list_keymap_for_each_binding_stops_before_direct_sparse_parent() {
590590
}
591591

592592
#[test]
593-
fn list_keymap_accessible_does_not_descend_into_direct_sparse_parent() {
593+
fn list_keymap_accessible_descends_into_direct_sparse_parent() {
594594
crate::test_utils::init_test_tracing();
595595
let parent = make_sparse_list_keymap();
596596
let prefix_map = make_sparse_list_keymap();
@@ -599,12 +599,18 @@ fn list_keymap_accessible_does_not_descend_into_direct_sparse_parent() {
599599
list_keymap_define(parent, Value::fixnum('a' as i64), prefix_map);
600600
list_keymap_set_parent(child, parent);
601601

602-
let mut prefix = Vec::new();
603602
let mut out = Vec::new();
604-
let mut seen = Vec::new();
605-
list_keymap_accessible(&child, &mut prefix, &mut out, &mut seen);
606-
607-
assert_eq!(out.len(), 1);
603+
list_keymap_accessible(&child, &mut out);
604+
605+
// GNU `accessible-keymaps` follows the parent (via map_keymap), so the
606+
// parent's `a` prefix map is listed under [?a]. GNU prints ([] [97]).
607+
assert_eq!(out.len(), 2);
608+
assert_eq!(out[0].cons_car().as_vector_data().unwrap().len(), 0);
609+
let second_prefix = out[1].cons_car();
610+
let second_prefix = second_prefix.as_vector_data().unwrap();
611+
assert_eq!(second_prefix.len(), 1);
612+
assert_eq!(second_prefix[0], Value::fixnum('a' as i64));
613+
assert!(keymap_value_eq(&out[1].cons_cdr(), &prefix_map));
608614
}
609615

610616
#[test]

0 commit comments

Comments
 (0)