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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
Edward Langley
2026-09-22 21:20:07 -07:00
parent 0b3fb055a9
commit d9d064c882
4 changed files with 63 additions and 6 deletions
+3 -1
View File
@@ -258,7 +258,9 @@ expansions and `SetTransientPrefix` in `command/keymap.rs`. All nine are
`ViewOnly`, which is the half the `ViewBack` bug was in. `ViewOnly`, which is the half the `ViewBack` bug was in.
`improvise-command/src/ui/reduce/vocabulary_is_complete.rs` is that direction. `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) 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 or to appear in `NOT_ON_THE_WIRE` with a verdict. A verdict is one of four
kinds, and the check verifies each kind: kinds, and the check verifies each kind:
+15
View File
@@ -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 `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 improvise-c7mu's fault. With `code_only` made the identity, it failed with
`["SaveAs", "ExportCsv", "Real", "SetYanked", "AlsoReal"]`. `["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 `[]`.
@@ -178,10 +178,7 @@ fn effects_named(code: &str) -> Vec<String> {
let mut found = Vec::new(); let mut found = Vec::new();
for marker in ["impl Effect for ", "simple_effect!("] { for marker in ["impl Effect for ", "simple_effect!("] {
for (at, _) in code.match_indices(marker) { for (at, _) in code.match_indices(marker) {
let name: String = code[at + marker.len()..] let name = name_after(&code[at + marker.len()..]);
.chars()
.take_while(char::is_ascii_alphanumeric)
.collect();
if name.starts_with(|c: char| c.is_ascii_uppercase()) { if name.starts_with(|c: char| c.is_ascii_uppercase()) {
found.push(name); found.push(name);
} }
@@ -190,6 +187,49 @@ fn effects_named(code: &str) -> Vec<String> {
found 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] #[test]
fn an_effect_named_only_in_a_comment_or_string_is_not_an_effect() { fn an_effect_named_only_in_a_comment_or_string_is_not_an_effect() {
let text = concat!( let text = concat!(
+1 -1
View File
@@ -264,7 +264,7 @@ mod tests {
std::fs::write(src.join("p.rs"), "fn production() {}\n").unwrap(); 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.rs"), "fn test_only() {}\nmod deeper;\n").unwrap();
std::fs::write(src.join("t").join("deeper.rs"), "fn test_too() {}\n").unwrap(); std::fs::write(src.join("t").join("deeper.rs"), "fn test_too() {}\n").unwrap();
let mut found: Vec<String> = production_code(&[src.clone()]) let mut found: Vec<String> = production_code(std::slice::from_ref(&src))
.into_iter() .into_iter()
.map(|(file, _)| file.strip_prefix(&src).unwrap().display().to_string()) .map(|(file, _)| file.strip_prefix(&src).unwrap().display().to_string())
.collect(); .collect();