From d9d064c8829f8f27d6701ee7b9f948a43da36728 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:20:07 -0700 Subject: [PATCH] Forward guard reads an effect name past attributes and doc comments (improvise-94x2); clippy slice fix Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 4 +- context/test-evidence/command.md | 15 ++++++ .../src/ui/reduce/vocabulary_is_complete.rs | 48 +++++++++++++++++-- crates/improvise-protocol/src/source_scan.rs | 2 +- 4 files changed, 63 insertions(+), 6 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index d76a58d2..ca4e3e82 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -258,7 +258,9 @@ expansions and `SetTransientPrefix` in `command/keymap.rs`. All nine are `ViewOnly`, which is the half the `ViewBack` bug was in. `improvise-command/src/ui/reduce/vocabulary_is_complete.rs` is that direction. -It scans the crate's `src/` for both ways an effect is declared, and requires +It scans the crate's `src/` for both ways an effect is declared, reading +each name past any attributes and doc comments before it. The macro accepts +those, and missing them hid effects (improvise-94x2). It requires each name to be a `Command` or `ModelCommand` variant (read off serde's derive) or to appear in `NOT_ON_THE_WIRE` with a verdict. A verdict is one of four kinds, and the check verifies each kind: diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index bfbaec6e..2a810e55 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4927,3 +4927,18 @@ broken walk therefore fails that test instead of shrinking the set. `an_effect_named_only_in_a_comment_or_string_is_not_an_effect` shares improvise-c7mu's fault. With `code_only` made the identity, it failed with `["SaveAs", "ExportCsv", "Real", "SetYanked", "AlsoReal"]`. + +**improvise-94x2, found by the PR #172 review.** The reader took the +identifier *immediately* after `simple_effect!(`. The macro's own matcher is +`$(#[$meta:meta])* $name:ident(…)`, so a doc comment before the name (blanked +to whitespace by `code_only`), an attribute, or a line break after `(` left +nothing to read, and the effect was invisible. The reviewer injected a +documented `simple_effect!(\n /// Rival.\n SetBogusRival(bool) …)` into +`effect.rs`, and all five tests passed. Re-injected here, it also passed all +five before the fix. After the fix +`every_effect_is_on_the_wire_or_has_a_verdict` fails naming `SetBogusRival`. +The rival was removed afterwards. `name_after` now skips whitespace and every +bracket-matched `#[…]` before reading the name. +`an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture: +a doc comment, two attributes (one with nested brackets), and a wrapped call. +With the whitespace skip removed it fails with `[]`. diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs index fb973742..b91ed80d 100644 --- a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -178,10 +178,7 @@ fn effects_named(code: &str) -> Vec { let mut found = Vec::new(); for marker in ["impl Effect for ", "simple_effect!("] { for (at, _) in code.match_indices(marker) { - let name: String = code[at + marker.len()..] - .chars() - .take_while(char::is_ascii_alphanumeric) - .collect(); + let name = name_after(&code[at + marker.len()..]); if name.starts_with(|c: char| c.is_ascii_uppercase()) { found.push(name); } @@ -190,6 +187,49 @@ fn effects_named(code: &str) -> Vec { found } +/// The identifier a marker introduces. `simple_effect!` takes `$(#[$meta])*` +/// before the name, and a doc comment is one: skip them (improvise-94x2). +fn name_after(rest: &str) -> String { + let mut rest = rest.trim_start(); + while let Some(attr) = rest.strip_prefix("#[") { + rest = attr[closing_bracket(attr) + 1..].trim_start(); + } + rest.chars() + .take_while(|c| c.is_ascii_alphanumeric() || *c == '_') + .collect() +} + +/// Where the `]` that closes an attribute's `#[` falls in `inside`, the text +/// after it. Nested brackets count; an unclosed one runs to the end. +fn closing_bracket(inside: &str) -> usize { + let mut depth = 1usize; + for (i, c) in inside.char_indices() { + match c { + '[' => depth += 1, + ']' if depth == 1 => return i, + ']' => depth -= 1, + _ => {} + } + } + inside.len().saturating_sub(1) +} + +/// The spelling the macro itself documents, a doc comment and an attribute +/// before the name, and a line break after `(`. Each hid an effect. +#[test] +fn an_attributed_or_wrapped_effect_is_still_an_effect() { + let text = concat!( + "simple_effect!(\n", + " /// Documented, as the macro's own matcher allows.\n", + " #[allow(dead_code)]\n", + " #[cfg_attr(all(), doc = \"x\")]\n", + " Documented(bool) => |app| x, K\n", + ");\n", + "simple_effect!(\n Wrapped(bool) => |app| y, K);\n", + ); + assert_eq!(effects_in(text), vec!["Documented", "Wrapped"]); +} + #[test] fn an_effect_named_only_in_a_comment_or_string_is_not_an_effect() { let text = concat!( diff --git a/crates/improvise-protocol/src/source_scan.rs b/crates/improvise-protocol/src/source_scan.rs index 0b35c845..419705e8 100644 --- a/crates/improvise-protocol/src/source_scan.rs +++ b/crates/improvise-protocol/src/source_scan.rs @@ -264,7 +264,7 @@ mod tests { std::fs::write(src.join("p.rs"), "fn production() {}\n").unwrap(); std::fs::write(src.join("t.rs"), "fn test_only() {}\nmod deeper;\n").unwrap(); std::fs::write(src.join("t").join("deeper.rs"), "fn test_too() {}\n").unwrap(); - let mut found: Vec = production_code(&[src.clone()]) + let mut found: Vec = production_code(std::slice::from_ref(&src)) .into_iter() .map(|(file, _)| file.strip_prefix(&src).unwrap().display().to_string()) .collect();