Merge pull request #195 from fiddlerwoaroof/wire2/59q8

Source scanner: bound test-only items by brackets, evaluate cfg predicates (59q8)
This commit is contained in:
Ed Langley
2026-09-22 22:15:24 -07:00
committed by GitHub
3 changed files with 492 additions and 80 deletions
+27 -5
View File
@@ -230,11 +230,33 @@ guard below (improvise-c7mu). It removes three things a scan must not see.
composers, so no verdict was wrong yet. It also had a mirror: a comment
explaining that a `NOT_COMPOSED` variant is held back would have raised a
false "stale entry" alarm.
- **Test items inside a file.** A line scan that relies on rustfmt putting an
item's closing brace at the attribute's own indentation. It treats
`cfg(any(test, …))` as test code and `cfg(not(test))` as production.
Over-stripping **fails** the test rather than passing it, which is the safe
direction.
- **Test items inside a file.** Each test-only attribute's item is bounded by
**balanced brackets on the blanked code** (improvise-59q8). It ends at a `;`
at depth zero, or at the `}` closing its body unless `;`, `.`, `?` or `else`
follows. The first version was a line scan that relied on rustfmt putting an
item's closing brace at the attribute's own indentation. The #172 review
found three shapes it never bounded:
- an item ending in `];` or `);`;
- a `#[cfg(test)]` statement spanning lines;
- a one-line `#[cfg(test)] use x;`.
In each case the scan read on to the end of the file and **hid every line of
production code after it**, which is the silent direction for the forward
guard. An item the scan cannot bound is now a **panic**, never a scan to EOF.
**Which `cfg` is test-only is evaluated, not substring-matched.** The
predicate is read off the file's *text*, because a `feature` value is a
literal and blanked in the code. It is evaluated in three-valued logic for a
build without `test`: `test` is false, and so is `feature = "test-support"`,
which this repo never ships (improvise-26dm). Everything else a scan cannot
know, such as a target or an ordinary feature, is unknown. An item is
test-only when its predicate is **false**. So
`cfg(all(test, not(target_arch = "wasm32")))` is test code, which the old
"any line containing `not(`" rule got wrong. `cfg(any(test, feature =
"serde"))` and `cfg(not(test))` are production. An inner `#![cfg(test)]`
heading a file makes the whole file test code. One appearing anywhere else
is a panic, because this scan cannot bound an inline module's inner
attribute.
- **Test-only files.** A `#[cfg(test)] mod x;` makes `x.rs` and everything under
`x/` test code, even though nothing inside the file says so. Before
improvise-c7mu, this checker's own file and `improvise-io`'s
+48
View File
@@ -1050,3 +1050,51 @@ Faults injected, one per half, each compiling only `improvise-protocol`:
A field added to an existing variant is caught too, but not in isolation,
because every constructor of that variant also fails. Reverted with
`git restore`.
### source_scan::tests::a_test_only_item_hides_nothing_after_it
improvise-59q8, found by the PR #172 review. The scanner's line-based
`strip_test_items` looked for `}` at the attribute's indentation, or for an item
line ending in `;`. So three shapes were never bounded, and it dropped
**everything to the end of the file**:
- a test-only `const … = &[\n … \n];`;
- a `#[cfg(test)] let x = foo(\n 1,\n);` statement;
- a one-line `#[cfg(test)] use foo::bar;`, where the attribute's own line is
also the item line.
In `vocabulary_is_complete` that silently hides every effect after such an
item. Each shape is a case here, asserting that the production code after it
(`Hidden1`, `Visible`) survives. Against the line scan all three failed; the
first printed `"impl X {\n"` and nothing else. They now pass because items are
bounded by balanced brackets on the blanked code.
`an_unbounded_test_only_item_is_refused_not_swallowed` pins the other half of
the acceptance criteria: an item that cannot be bounded panics instead of
consuming the rest of the file. Against the line scan it failed, because
nothing panicked.
### source_scan::tests::a_test_only_cfg_is_recognised_whatever_its_spelling
improvise-59q8, the other direction: test code counted as production, which
makes `vocabulary_is_composed` weaker. `cfg(all(test, not(target_arch =
"wasm32")))` was kept because the old rule rejected any line containing
`not(`. A leading `#![cfg(test)]` was kept because only `#[cfg(` was
recognised. An attribute split across lines was not seen at all. All three
failed against the old scanner. The predicate is now parsed and evaluated in
three-valued logic (`context/code/protocol.md` §2). The controls
(`cfg(not(test))`, `cfg(any(test, feature = "serde"))` and `cfg(target_arch =
…)`) must stay production, so a scanner that dropped every `cfg` item could
not pass.
A consequence to know about: `cfg(any(test, feature = "x"))` for an ordinary
feature is now production. Only `feature = "test-support"`, the one test
feature this repo has, counts as a test build.
`test_only_items_and_modules_are_not_production` was updated to that spelling.
### source_scan::tests::a_c_string_literal_is_blanked
improvise-59q8's minor case. In `cr#"x" Command::InRawC(2) "y"#` the `r` has a
word character before it, so the literal was lexed as two ordinary strings,
and the text between the inner quotes leaked. It failed as
`InRawC survived`. `c"…"` and `cr#"…"#` are now literals. A first version of
this test used a C string with no inner quotes and passed against the bug,
because the ordinary lexing happened to blank it too.
+417 -75
View File
@@ -11,28 +11,28 @@ pub fn production_code(roots: &[PathBuf]) -> Vec<(PathBuf, String)> {
for root in roots {
collect_rust(root, &mut files);
}
let code: Vec<(PathBuf, String)> = files
let read: Vec<(PathBuf, Source)> = files
.into_iter()
.map(|file| {
let text = std::fs::read_to_string(&file).expect("a listed source reads");
let code = code_only(&text);
(file, code)
let source = Source::of(&text, &file);
(file, source)
})
.collect();
let test_modules: BTreeSet<PathBuf> = code
let test_modules: BTreeSet<PathBuf> = read
.iter()
.flat_map(|(file, code)| test_module_paths(file, code))
.flat_map(|(file, source)| test_module_paths(file, source))
.collect();
code.into_iter()
read.into_iter()
.filter(|(file, _)| !test_modules.iter().any(|m| is_module_file(m, file)))
.map(|(file, code)| (file, strip_test_items(&code)))
.map(|(file, source)| (file, source.production()))
.collect()
}
/// One file's text as [`production_code`] cleans it. Which *files* are test
/// modules needs their declaring file too, so only `production_code` knows.
pub fn production_text(text: &str) -> String {
strip_test_items(&code_only(text))
Source::of(text, Path::new("<text>")).production()
}
fn collect_rust(dir: &Path, into: &mut Vec<PathBuf>) {
@@ -55,83 +55,319 @@ fn is_module_file(m: &Path, file: &Path) -> bool {
file == m.with_extension("rs") || file.starts_with(m)
}
/// One file, read once. Its `cfg` predicates are read off the TEXT, whose
/// `feature` values are literals; every bracket off the code (improvise-59q8).
struct Source {
code: Vec<char>,
/// Test-only items, as `(attribute start, item start, item end)`.
items: Vec<(usize, usize, usize)>,
/// A leading `#![cfg(test)]`: the whole file is test code.
whole_file: bool,
}
impl Source {
fn of(text: &str, file: &Path) -> Self {
let code: Vec<char> = code_only(text).chars().collect();
let text: Vec<char> = text.chars().collect();
let (items, whole_file) = test_only_items(&text, &code, file);
Self {
code,
items,
whole_file,
}
}
/// The code with every test-only item removed.
fn production(&self) -> String {
if self.whole_file {
return String::new();
}
let mut kept = String::with_capacity(self.code.len());
let mut at = 0;
for &(start, _, end) in &self.items {
kept.extend(&self.code[at..start]);
at = end;
}
kept.extend(&self.code[at..]);
kept
}
fn item_code(&self, start: usize, end: usize) -> String {
self.code[start..end].iter().collect()
}
}
/// The out-of-line modules `file` declares under a test-only `cfg`, as paths
/// without extension (`#[cfg(test)] mod x;` is `<dir>/x`).
fn test_module_paths(file: &Path, code: &str) -> Vec<PathBuf> {
fn test_module_paths(file: &Path, source: &Source) -> Vec<PathBuf> {
let dir = match file.file_stem().and_then(|s| s.to_str()) {
Some("lib" | "main" | "mod") => file.parent().map(Path::to_path_buf),
Some(stem) => file.parent().map(|p| p.join(stem)),
None => None,
}
.expect("a source file has a directory and a stem");
let lines: Vec<&str> = code.lines().map(str::trim).collect();
let mut found = Vec::new();
for (i, line) in lines.iter().enumerate() {
if !is_test_cfg(line) {
continue;
}
let item = lines[i + 1..]
.iter()
.find(|l| !l.starts_with("#[") && !l.is_empty());
if let Some(name) = item.and_then(|l| out_of_line_mod(l)) {
found.push(dir.join(name));
}
}
found
source
.items
.iter()
.filter_map(|&(_, start, end)| out_of_line_mod(source.item_code(start, end).trim()))
.map(|name| dir.join(name))
.collect()
}
/// `mod x;`, `pub mod x;`, `pub(crate) mod x;` → `x`.
fn out_of_line_mod(line: &str) -> Option<&str> {
let rest = line.split_once("mod ")?.1.strip_suffix(';')?;
let prefix = line.split_once("mod ")?.0.trim();
(prefix.is_empty() || prefix.starts_with("pub")).then_some(rest.trim())
fn out_of_line_mod(item: &str) -> Option<String> {
let (prefix, rest) = item.split_once("mod ")?;
let name = rest.strip_suffix(';')?.trim();
let prefix = prefix.trim();
(prefix.is_empty() || prefix.starts_with("pub")).then(|| name.to_string())
}
/// A `cfg` attribute that compiles its item only under `test`. `cfg(not(test))`
/// is production, and `"test-support"` is a literal, already blanked.
fn is_test_cfg(line: &str) -> bool {
let line = line.trim();
line.starts_with("#[cfg(")
&& !line.contains("not(")
&& line
.split(|c: char| !c.is_alphanumeric() && c != '_')
.any(|word| word == "test")
/// Every test-only item in a file, bounded by balanced brackets on the code
/// rather than by indentation. An item it cannot bound is a panic, never EOF.
fn test_only_items(
text: &[char],
code: &[char],
file: &Path,
) -> (Vec<(usize, usize, usize)>, bool) {
let mut items = Vec::new();
let mut k = 0;
while k < code.len() {
let Some((inner, end)) = attribute_at(code, k) else {
k += 1;
continue;
};
let predicate: String = text[k..end].iter().collect();
if !is_test_only(&predicate) {
k = end;
continue;
}
if inner {
// An inner attribute heads the file or it heads an inline module;
// only the first is a shape this scan can honour.
let leading = code[..k].iter().all(|c| c.is_whitespace());
assert!(
leading,
"{}: an inner test-only cfg could not be bounded",
file.display()
);
return (Vec::new(), true);
}
let start = past_attributes(code, end);
let item_end = item_end(code, start).unwrap_or_else(|| {
panic!(
"{}: a test-only item at char {k} could not be bounded",
file.display()
)
});
items.push((k, start, item_end));
k = item_end;
}
(items, false)
}
/// Drop every test-only item. rustfmt puts an item's closing brace at the
/// attribute's own indentation, which is what makes this a line scan (§2).
fn strip_test_items(text: &str) -> String {
let lines: Vec<&str> = text.lines().collect();
let mut kept = String::new();
let mut i = 0;
while i < lines.len() {
let line = lines[i];
match is_test_cfg(line) {
false => {
kept.push_str(line);
kept.push('\n');
i += 1;
}
true => {
let indent = line.len() - line.trim_start().len();
let close = format!("{}}}", " ".repeat(indent));
i += 1;
while i < lines.len() && lines[i].trim_start().starts_with("#[") {
i += 1;
}
match lines.get(i).is_some_and(|l| l.trim_end().ends_with(';')) {
true => i += 1,
false => {
while i < lines.len() && lines[i].trim_end() != close {
i += 1;
}
i += 1;
}
/// An attribute starting at `k`: whether it is inner (`#!`) and where it ends.
fn attribute_at(code: &[char], k: usize) -> Option<(bool, usize)> {
if code[k] != '#' || (k > 0 && (code[k - 1].is_alphanumeric() || code[k - 1] == '_')) {
return None;
}
let inner = code.get(k + 1) == Some(&'!');
let open = k + 1 + usize::from(inner);
(code.get(open) == Some(&'[')).then(|| matching(code, open).map(|close| (inner, close + 1)))?
}
/// The index of the bracket closing the one at `open`, counting all three kinds.
fn matching(code: &[char], open: usize) -> Option<usize> {
let mut depth = 0usize;
for (k, &c) in code.iter().enumerate().skip(open) {
match c {
'(' | '[' | '{' => depth += 1,
')' | ']' | '}' => {
depth -= 1;
if depth == 0 {
return Some(k);
}
}
_ => {}
}
}
kept
None
}
/// Skip whitespace and any further outer attributes on the same item.
fn past_attributes(code: &[char], mut k: usize) -> usize {
loop {
while code.get(k).is_some_and(|c| c.is_whitespace()) {
k += 1;
}
let next = (k < code.len()).then(|| attribute_at(code, k)).flatten();
match next {
Some((false, end)) => k = end,
_ => return k,
}
}
}
/// Where the item starting at `start` ends: a `;` at depth zero, or the `}`
/// closing its body. `None` at the end of the file, which is not an end.
fn item_end(code: &[char], start: usize) -> Option<usize> {
let mut depth = 0usize;
let mut k = start;
while k < code.len() {
match code[k] {
'(' | '[' | '{' => depth += 1,
';' if depth == 0 => return Some(k + 1),
// The enclosing block closed first: the item was its last expression.
')' | ']' | '}' if depth == 0 => return Some(k),
')' | ']' => depth -= 1,
'}' => {
depth -= 1;
if depth == 0 && !continues(code, k + 1) {
return Some(k + 1);
}
}
_ => {}
}
k += 1;
}
None
}
/// Whether the item goes on after a `}` at depth zero: `S { .. };`, a method
/// call on a block, or an `if` with an `else`.
fn continues(code: &[char], after: usize) -> bool {
let rest: String = code[after..]
.iter()
.skip_while(|c| c.is_whitespace())
.take(5)
.collect();
rest.starts_with([';', '.', '?']) || rest.starts_with("else")
}
/// A `cfg` attribute whose predicate no shipped build satisfies. Evaluated,
/// not substring-matched (improvise-59q8): `context/code/protocol.md` §2.
fn is_test_only(attribute: &str) -> bool {
let tokens = cfg_tokens(attribute);
match tokens.as_slice() {
[Token::Word(cfg), Token::Open, rest @ ..] if cfg == "cfg" => {
let mut at = 0;
predicate(rest, &mut at) == Truth::No
}
_ => false,
}
}
/// What a `cfg` predicate is in a build without `test`. `Unknown` for what a
/// source scan cannot know, such as a target or an ordinary feature.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum Truth {
Yes,
No,
Unknown,
}
#[derive(Debug, Clone, PartialEq, Eq)]
enum Token {
Word(String),
Str(String),
Open,
Close,
Comma,
Equals,
}
/// The tokens of an attribute's text, from its first word: `#`, `!` and the
/// brackets around it are dropped.
fn cfg_tokens(attribute: &str) -> Vec<Token> {
let inner = attribute
.trim_start_matches('#')
.trim_start_matches('!')
.trim()
.trim_start_matches('[')
.trim_end()
.trim_end_matches(']');
let chars: Vec<char> = inner.chars().collect();
let mut tokens = Vec::new();
let mut k = 0;
while k < chars.len() {
let c = chars[k];
match c {
'(' => tokens.push(Token::Open),
')' => tokens.push(Token::Close),
',' => tokens.push(Token::Comma),
'=' => tokens.push(Token::Equals),
'"' => {
let end = quoted_end(&chars, k + 1, '"');
tokens.push(Token::Str(chars[k + 1..end - 1].iter().collect()));
k = end;
continue;
}
c if c.is_alphanumeric() || c == '_' => {
let word: String = chars[k..]
.iter()
.take_while(|c| c.is_alphanumeric() || **c == '_')
.collect();
k += word.chars().count();
tokens.push(Token::Word(word));
continue;
}
_ => {}
}
k += 1;
}
tokens
}
/// One predicate, advancing `at` past it. `test` is false; so is the
/// `test-support` feature, which this repo never ships (improvise-26dm).
fn predicate(tokens: &[Token], at: &mut usize) -> Truth {
let Some(Token::Word(word)) = tokens.get(*at) else {
return Truth::Unknown;
};
*at += 1;
match (word.as_str(), tokens.get(*at)) {
(op @ ("all" | "any" | "not"), Some(Token::Open)) => {
*at += 1;
let mut args = Vec::new();
while !matches!(tokens.get(*at), Some(Token::Close) | None) {
args.push(predicate(tokens, at));
if tokens.get(*at) == Some(&Token::Comma) {
*at += 1;
}
}
*at += 1;
combine(op, &args)
}
(key, Some(Token::Equals)) => {
*at += 1;
let value = match tokens.get(*at) {
Some(Token::Str(v)) => v.clone(),
_ => String::new(),
};
*at += 1;
match (key, value.as_str()) {
("feature", "test-support") => Truth::No,
_ => Truth::Unknown,
}
}
("test", _) => Truth::No,
_ => Truth::Unknown,
}
}
/// Kleene's three-valued `all`, `any` and `not`.
fn combine(op: &str, args: &[Truth]) -> Truth {
match op {
"all" if args.contains(&Truth::No) => Truth::No,
"all" if args.iter().all(|a| *a == Truth::Yes) => Truth::Yes,
"any" if args.contains(&Truth::Yes) => Truth::Yes,
"any" if args.iter().all(|a| *a == Truth::No) => Truth::No,
"not" => match args.first() {
Some(Truth::Yes) => Truth::No,
Some(Truth::No) => Truth::Yes,
_ => Truth::Unknown,
},
_ => Truth::Unknown,
}
}
/// `text` with comments, string, byte-string, raw-string and char literals
@@ -166,8 +402,8 @@ fn literal_or_comment_end(c: &[char], i: usize) -> Option<usize> {
(Some('/'), Some('/')) => Some(line_end(c, i)),
(Some('/'), Some('*')) => Some(block_end(c, i)),
(Some('"'), _) => Some(quoted_end(c, i + 1, '"')),
(Some('b'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')),
(Some('r' | 'b'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i),
(Some('b' | 'c'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')),
(Some('r' | 'b' | 'c'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i),
(Some('\''), _) => char_end(c, i),
_ => None,
}
@@ -211,9 +447,10 @@ fn quoted_end(c: &[char], mut k: usize, quote: char) -> usize {
c.len()
}
/// `r"…"`, `r#"…"#`, `br#"…"#`. `None` when the `r` is only an identifier.
/// `r"…"`, `r#"…"#`, `br#"…"#`, `cr#"…"#`. `None` when the `r` is only an
/// identifier.
fn raw_end(c: &[char], i: usize) -> Option<usize> {
let mut k = i + usize::from(c[i] == 'b');
let mut k = i + usize::from(matches!(c[i], 'b' | 'c'));
if c.get(k) != Some(&'r') {
return None;
}
@@ -283,21 +520,20 @@ mod tests {
"}\n",
"#[cfg(test)]\n",
"mod out_of_line;\n",
"#[cfg(any(test, feature = \"x\"))]\n",
"#[cfg(any(test, feature = \"test-support\"))]\n",
"pub mod gated;\n",
"#[cfg(not(test))]\n",
"fn production_only() {}\n",
"fn also_kept() {}\n",
);
let code = code_only(text);
let stripped = strip_test_items(&code);
let stripped = production_text(text);
for kept in ["fn kept", "fn production_only", "fn also_kept"] {
assert!(stripped.contains(kept), "{kept} was dropped: {stripped}");
}
assert!(!stripped.contains("gone_inline"), "{stripped}");
let lib = Path::new("/c/src/lib.rs");
assert_eq!(
test_module_paths(lib, &code),
test_module_paths(lib, &Source::of(text, lib)),
vec![
PathBuf::from("/c/src/out_of_line"),
PathBuf::from("/c/src/gated")
@@ -305,7 +541,7 @@ mod tests {
);
let nested = Path::new("/c/src/ui/reduce.rs");
assert_eq!(
test_module_paths(nested, &code)[0],
test_module_paths(nested, &Source::of(text, nested))[0],
PathBuf::from("/c/src/ui/reduce/out_of_line")
);
assert!(is_module_file(
@@ -322,6 +558,112 @@ mod tests {
));
}
/// improvise-59q8, the shapes that HID production code: a test-only item
/// whose end the line scan could not find. Evidence: test-evidence/protocol.md.
#[test]
fn a_test_only_item_hides_nothing_after_it() {
let cases = [
(
"a multi-line item ending in `];`",
concat!(
"impl X {\n",
" #[cfg(test)]\n",
" const NAMES: &[&str] = &[\n",
" \"a\",\n",
" ];\n",
" fn prod() { let c = Command::Hidden1(1); }\n",
"}\n",
"fn after() { Command::Visible(2) }\n",
),
),
(
"a multi-line statement ending in `);`",
concat!(
"fn f() {\n",
" #[cfg(test)]\n",
" let x = foo(\n",
" 1,\n",
" );\n",
" let c = Command::Hidden1(1);\n",
"}\n",
"fn after() { Command::Visible(2) }\n",
),
),
(
"a one-line `#[cfg(test)] use`",
concat!(
"#[cfg(test)] use foo::bar;\n",
"fn prod() { Command::Hidden1(1) }\n",
"fn after() { Command::Visible(2) }\n",
),
),
];
for (what, text) in cases {
let kept = production_text(text);
for name in ["Hidden1", "Visible"] {
assert!(kept.contains(name), "{what}: {name} was hidden: {kept:?}");
}
}
}
/// improvise-59q8, the shapes that COUNTED test code as production.
#[test]
fn a_test_only_cfg_is_recognised_whatever_its_spelling() {
let cases = [
(
"`all(test, not(..))`",
"#[cfg(all(test, not(target_arch = \"wasm32\")))]\nfn t() { Command::TestOnly(1) }\n",
),
(
"an inner `#![cfg(test)]` heading the file",
"#![cfg(test)]\nfn t() { Command::TestOnly(1) }\n",
),
(
"an attribute split across lines",
"#[cfg(\n test\n)]\nfn t() { Command::TestOnly(1) }\n",
),
];
for (what, text) in cases {
let kept = production_text(text);
assert!(
!kept.contains("TestOnly"),
"{what} was read as production: {kept:?}"
);
}
// The controls: a predicate a shipped build can satisfy is production.
for text in [
"#[cfg(not(test))]\nfn p() { Command::Shipped(1) }\n",
"#[cfg(any(test, feature = \"serde\"))]\nfn p() { Command::Shipped(1) }\n",
"#[cfg(target_arch = \"wasm32\")]\nfn p() { Command::Shipped(1) }\n",
] {
assert!(
production_text(text).contains("Shipped"),
"{text:?} was dropped"
);
}
}
/// improvise-59q8's minor case: a C string literal leaked its contents.
#[test]
fn a_c_string_literal_is_blanked() {
// The raw one quotes inside itself, so lexing it as ordinary strings
// leaves the text BETWEEN the inner quotes in the open.
let kept = code_only(
"let a = c\"Command::InC(1)\"; let b = cr#\"x\" Command::InRawC(2) \"y\"#;\n",
);
for gone in ["InC", "InRawC"] {
assert!(!kept.contains(gone), "{gone} survived: {kept}");
}
}
/// A test-only item the scan cannot bound is REFUSED, never read to the
/// end of the file, which is what silently hid every shape above.
#[test]
#[should_panic(expected = "could not be bounded")]
fn an_unbounded_test_only_item_is_refused_not_swallowed() {
let _ = production_text("#[cfg(test)]\nfn broken() {\n Command::X(1)\n");
}
#[test]
fn comments_and_literals_are_blanked_and_code_is_kept() {
let text = concat!(