From def11e21312bde623ca210b327c3c5babf036210 Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Sun, 16 Aug 2026 23:58:19 +0800 Subject: [PATCH] Fix backslash-splice data loss, then grow corpus A C or C++ "//" comment continued with a trailing "\" could lose the code line below it. The splice happens in translation phase 2, before the comment is lexed, so the grammar reports one node spanning both lines and the tool is free to rearrange bytes across the seam. Two routes reach the same damage. Reflow can park the "\" last on the last emitted line, where the splice swallows the following line of code. And deleting an interior line that is nothing but whitespace, which is correct treatment of comment-interior whitespace, re-points an existing splice onto the code line with no reflow involved at all, so no column limit is safe. Compiler-verified: a three-line file went from nm reporting "D _x" to no symbol, the declaration having become comment text. The second pass then reads the swallowed code as prose and packs it, so --check converges on the damage instead of reporting it. parse::spans_line_splice forces passthrough, beside spans_bare_cr and for the reason that one already states: it destroys code, no amount of downstream care can undo it, so refuse the comment outright. Gated to C and C++ line comments, because Rust has no line splicing, shell comments end at the newline, assembly claims only "/* */", and "*/" terminates a block whatever precedes the newline. The rule for "this line is continued" had three encodings in two files and they disagreed: two spelled it trim_end().ends_with('\\'), which strips Unicode whitespace, so a "\" followed by a no-break space was a splice to two callers and not to the third. No compiler splices that either, so the strict horizontal-whitespace form is the one that survived, as parse::ends_with_line_splice, and all three callers use it. Transform 5 no longer inserts a blank line under a "#!" shebang. It exempted only file offset 0, so a shebang on line 1 made the line-2 header flush against code and detached it, on essentially every shell script in existence, and split a "#! nix-shell -i bash" run off the shebang it belongs to. "#!" and "#if" are now one "#"-directive parser rather than two six lines apart, which is what left the BOM strip on only one of the pair: U+FEFF is not Unicode White_Space, so a BOM'd file whose first line is "#ifndef GUARD" failed a test its BOM-free twin passes. The corpus harness asserted every file's output contained the literal "int f(int a, int b);", which no Rust, shell, or assembly fixture could satisfy. Each input is now compared byte for byte against a recorded X.EXT.expected sibling, read with expect so a deleted sibling fails instead of silently skipping, and any file detect_language rejects other than a sibling is a hard error rather than a quiet skip. Recorded output rather than the simpler "input is its own fixed point": that form can only hold shapes the tool already leaves alone, so every input the tool is supposed to fix, which is the whole premature-wrap failure mode, would have been inexpressible. tests/corpus grows from one C header to one file per supported language, carrying the shapes that have historically broken this tool: CRLF throughout, a BOM before a shebang, nested Rust blocks, a macro continuation chain, unterminated blocks at EOF, fenced code, tables, and art. Two fixtures hold the splice shapes above. The two transforms that read a function_definition rather than a comment move to src/signature.rs. They are the only sanctioned uses of syntactic context beyond locating comments, and giving them a name and an address makes the ceiling TODO.md sets something a reviewer can point at. It is documentation, not a wall: both are still called from parse. ASan and UBSan jobs, since src/ has no unsafe and the only memory either can find a bug in is the linked C. CFLAGS is the load-bearing half, so each job ends by grepping the built grammar objects for instrumentation; a job reporting zero findings because it checked nothing is worse than no job. UBSan uses -fsanitize-trap rather than -fno-sanitize-recover: the runtime form does not link, because rustc drives the final link and never adds libclang_rt.ubsan. Both carry continue-on-error until they have run green twice, which puts that promise in the file it constrains instead of in a branch-protection setting no reviewer of the workflow can see. --- .github/workflows/ci.yml | 143 ++++++++++++++ src/lib.rs | 52 ++++- src/parse.rs | 317 +++++++++--------------------- src/signature.rs | 251 +++++++++++++++++++++++ tests/corpus/boot.S | 33 ++++ tests/corpus/boot.S.expected | 34 ++++ tests/corpus/headers.h | 9 +- tests/corpus/headers.h.expected | 70 +++++++ tests/corpus/nested.rs | 40 ++++ tests/corpus/nested.rs.expected | 42 ++++ tests/corpus/objects.cpp | 57 ++++++ tests/corpus/objects.cpp.expected | 57 ++++++ tests/corpus/script.sh | 28 +++ tests/corpus/script.sh.expected | 30 +++ tests/pipeline.rs | 167 +++++++++++++--- 15 files changed, 1066 insertions(+), 264 deletions(-) create mode 100644 src/signature.rs create mode 100644 tests/corpus/boot.S create mode 100644 tests/corpus/boot.S.expected create mode 100644 tests/corpus/headers.h.expected create mode 100644 tests/corpus/nested.rs create mode 100644 tests/corpus/nested.rs.expected create mode 100644 tests/corpus/objects.cpp create mode 100644 tests/corpus/objects.cpp.expected create mode 100644 tests/corpus/script.sh create mode 100644 tests/corpus/script.sh.expected diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3c9ffd2..3fb1881 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,6 +59,149 @@ jobs: - run: cargo install cargo-audit --locked - run: cargo audit --deny warnings + # "src/" contains no "unsafe" block, so the only memory these can find a bug in + # is the linked C: the four tree-sitter grammar crates and the runtime, driven + # by whatever "tests/corpus" and the suite feed them. That is also why "CFLAGS" + # is the load-bearing half of each job. Without it the "cc"-compiled grammars + # stay uninstrumented and the run reports zero findings because it checked + # nothing, which is worse than having no job at all. The "verify" step below + # exists to make that failure loud. + # + # "continue-on-error" on both, until each has run green twice. A nightly-only + # job that breaks on a toolchain roll must not block unrelated PRs on day one, + # and leaving that promise to a branch-protection setting puts it somewhere no + # reviewer of this file can see. Delete the two lines to make them blocking. + # + # "CC=clang" because rustc links LLVM's sanitizer runtime and the runner's + # default "CC" is gcc; gcc-instrumented objects against LLVM's runtime is a + # coin flip. The explicit "--target" is for "RUSTFLAGS", which cargo then keeps + # out of build scripts and proc macros; it does not stop "CFLAGS" reaching + # them, and does not need to ("cc" tries "CFLAGS_" and "TARGET_CFLAGS" + # first, then falls back to a bare "CFLAGS"). + # + # "-Zbuild-std" is deliberately omitted. Without it "std" stays uninstrumented, + # which is the right trade here: the target is the C, and rebuilding std per + # job costs more than it finds. Do not "fix" this later. + # + # Not here and not to be added: MSan needs every dependency including "std" + # rebuilt instrumented, and uninstrumented tree-sitter reports false positives + # all day. TSan waits for concurrency to return. Valgrind would duplicate ASan. + asan: + runs-on: ubuntu-24.04 + continue-on-error: true + env: + CC: clang + CFLAGS: -fsanitize=address + RUSTFLAGS: -Zsanitizer=address + steps: + - uses: actions/checkout@v7 + - uses: dtolnay/rust-toolchain@nightly + - uses: Swatinem/rust-cache@v2 + # "-Zsanitizer" is still unstable, hence the nightly toolchain above. No + # "+nightly" here: the action already made it the default, and naming it + # twice means a later pin to a dated nightly silently disagrees with it. + - run: cargo test --target x86_64-unknown-linux-gnu + # Prove the C was actually instrumented. "ASAN_OPTIONS=verbosity=1" does + # NOT do this: it prints runtime init and the shadow layout, not a module + # list, so it looks identical whether or not the grammars were built with + # "-fsanitize=address". Check for the symbols directly. + # + # "find" rather than a hardcoded "target//debug/build/..." glob: + # that path is cargo's business and it moves. The runner already proved + # it, laying the objects out as "build/tree-sitter-c//out" where + # the local cargo writes "build/tree-sitter-c-/out". Both spellings + # are matched, and the second is anchored on a whole path component so it + # cannot pick up tree-sitter-cpp. On no match this prints the objects that + # DO exist, so the next run says where they went instead of failing with + # an unexpanded glob and no information. + - name: Verify the grammars carry ASan instrumentation + run: | + objs=$(find target \( -path '*tree-sitter-c-*/out/*.o' \ + -o -path '*/tree-sitter-c/*/out/*.o' \)) + if [ -z "$objs" ]; then + echo "no tree-sitter-c objects under target/; what is there:" + find target -name '*.o' -path '*tree-sitter*' | head -20 + exit 1 + fi + echo "$objs" + nm $objs | grep -q __asan_ + + # UBSan is C-only: Rust has no "-Zsanitizer=undefined", so "RUSTFLAGS" is left + # alone here and only the grammars are instrumented. Stable toolchain, + # therefore, unlike the ASan job. + # + # The runtime form needs help linking: rustc drives the final link with "cc" + # and never adds libclang_rt.ubsan, so the build dies on undefined + # "__ubsan_handle_*" referenced from the tree-sitter scanners. "-Clinker=clang + # -Clink-arg=-fsanitize=undefined" supplies it, which is why "RUSTFLAGS" is + # set here even though Rust itself has no "-Zsanitizer=undefined" and none of + # the Rust code is instrumented. + # + # Trap mode ("-fsanitize-trap=undefined") was tried first because it needs no + # runtime at all. It works, but a finding then arrives as a test killed by + # SIGILL with no message, which is nearly useless in a CI log: the first real + # finding cost a full round trip to learn nothing but "something, somewhere". + # A sanitizer that cannot say what it found is not worth the job slot. + ubsan: + runs-on: ubuntu-24.04 + continue-on-error: true + env: + # "-fno-sanitize=function" is the one check turned off, and it is turned + # off because the finding is upstream and not ours to fix. tree-sitter's + # parser.c:369 stores every grammar's external-scanner entry points in a + # "void *(*)(void)" table and calls them through it with their real + # signatures, so the "function" check fires once per grammar with an + # external scanner (rust, bash). Calling through an incompatible function + # pointer type is UB by the letter of C, and it is also a decades-old C + # dispatch idiom that the check only started flagging when clang 17 began + # enabling it for C. Fixing it means patching a pinned third-party crate. + # Every other UBSan check stays on, so the memory-shaped UB this job + # exists to catch still fails the run. Retry without this flag whenever + # tree-sitter is bumped. + CC: clang + CFLAGS: -fsanitize=undefined -fno-sanitize=function + RUSTFLAGS: -Clinker=clang -Clink-arg=-fsanitize=undefined + # Print the file, line, and kind of UB rather than just the summary line, + # and keep going so one finding does not hide the rest. + UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=0 + steps: + - uses: actions/checkout@v7 + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + # Findings go to stderr and do not fail the process, so the run is teed + # and inspected below. Without that a "runtime error:" line scrolls past + # in a green log and nobody ever reads it. + # + # "pipefail" is not optional here: GitHub's default shell is "bash -e" + # WITHOUT it (the log line reads "shell: /usr/bin/bash -e {0}"), so the + # pipeline would report tee's exit status and a failing test suite would + # sail through as a green job. + - run: | + set -o pipefail + cargo test --no-fail-fast 2>&1 | tee ubsan.log + - name: Report UBSan findings + run: | + if grep -n 'runtime error:' ubsan.log; then + echo "::error::UBSan reported undefined behavior in the linked C" + exit 1 + fi + echo "no UBSan findings" + # Same reasoning as the ASan verify step: a green run proves nothing if + # "CFLAGS" never reached the grammars. + - name: Verify the grammars carry UBSan instrumentation + run: | + objs=$(find target -path '*tree-sitter*/out/*.o') + if [ -z "$objs" ]; then + echo "no tree-sitter objects under target/; what is there:" + find target -name '*.o' | head -20 + exit 1 + fi + echo "$objs" + # Across all the grammar objects, not one of them: which UBSan checks + # a given translation unit needs depends on what it does, and a table + # driven parser.c can legitimately need none. + nm $objs | grep -q __ubsan_ + build: name: ${{ matrix.name }} strategy: diff --git a/src/lib.rs b/src/lib.rs index 7a57bc9..b23292c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,6 +16,11 @@ pub mod normalize; pub mod parse; pub mod reflow; pub mod rewrite; + +// Nothing outside the crate calls into this module; "parse" is the only caller. +// Keeping it crate-private says so, rather than publishing a module whose every +// item is "pub(crate)" anyway. +pub(crate) mod signature; pub mod textline; /// Byte range to delete when a comment is moved out of its spot (transforms 3 @@ -75,9 +80,11 @@ fn comment_move_delete_span(source: &str, c: &parse::Comment, backward: bool) -> /// body-opening "{" (a comment between a function signature and its body, the /// unrelocated manual-page position, belongs with the function, not split off /// above), when a blank line is already there, when the comment sits at file -/// start, and when the previous line is itself a comment (don't fracture a -/// stacked comment). Doc comments are never touched at all: a blank line -/// detaches a Rust or Doxygen doc comment from the item it documents. +/// start, when it sits directly under the file's "#!" shebang (the header +/// belongs with the preamble, and the rule would otherwise fire on every shell +/// script there is), and when the previous line is itself a comment (don't +/// fracture a stacked comment). Doc comments are never touched at all: a blank +/// line detaches a Rust or Doxygen doc comment from the item it documents. /// "multiline" says the comment is still multi-line after reflow; one that the /// source split needlessly collapses to a single line no longer qualifies. /// @@ -110,7 +117,7 @@ fn blank_line_before( while directive_start > 0 { let before_end = directive_start - 1; let before_start = parse::line_start_before(source, before_end); - if !source[before_start..before_end].trim_end().ends_with('\\') { + if !parse::ends_with_line_splice(&source[before_start..before_end]) { break; } directive_start = before_start; @@ -121,16 +128,43 @@ fn blank_line_before( return None; } - // A comment opening a preprocessor conditional block ("#if"/"#ifdef"/ - // "#ifndef"/"#else"/"#elif") is the first thing inside that block: the same - // first-statement-in-a-block case as "{" above, so no blank line. - // "#endif"/"#define"/"#include" don't open a scope and are left alone. + // One parser for the two "#" lines that mean "this comment is not explaining + // me". Splitting them was how the BOM strip below ended up on only one of + // the pair, which left a BOM'd file whose first line is "#ifndef GUARD" + // failing a test its BOM-free twin passes. U+FEFF is not Unicode + // White_Space, so "trim" leaves it glued to the "#". + // + // - "#!" on line 1 of a SHELL file is the file's preamble. The header below it + // belongs flush against it, and without this the rule fires on + // essentially every shell script in existence, detaching its header from + // line 1 and splitting a "#! nix-shell -i bash" run off the shebang it + // belongs to, which some interpreters require. Gated on "prev_start == 0" + // because a "#!" anywhere else is an ordinary comment, per exec(2). + // - "#if"/"#ifdef"/"#ifndef"/"#else"/"#elif" open a conditional block, so + // the comment is the first thing inside it: the "{" case above in its + // "#"-directive form. "#endif"/"#define"/"#include" open no scope and are + // left alone. if let Some(rest) = source[directive_start..directive_end] .trim() + .trim_start_matches('\u{feff}') + .trim_start() .strip_prefix('#') { + // "#!" takes "rest" untrimmed: a shebang is the two bytes "#!" with + // nothing between them, so "# !x" is an ordinary comment. The + // directives take the trimmed form, because "# if" is valid cpp. + // + // Shell only, and that gate is load-bearing rather than tidiness: + // "#![no_std]" is a Rust inner attribute in exactly the same position, + // and reading it as a shebang suppressed the blank line under it. The + // extensionless-shebang carve-out lands on Shell too, so the scripts + // that need this still get it. let d = rest.trim_start(); - if d.starts_with("if") || d.starts_with("else") || d.starts_with("elif") { + if (lang == parse::Language::Shell && prev_start == 0 && rest.starts_with('!')) + || d.starts_with("if") + || d.starts_with("else") + || d.starts_with("elif") + { return None; } } diff --git a/src/parse.rs b/src/parse.rs index 78cccc9..e484290 100644 --- a/src/parse.rs +++ b/src/parse.rs @@ -3,6 +3,7 @@ use std::collections::HashMap; use std::path::Path; use tree_sitter::{Node, Parser, Tree}; +use crate::signature::{collect_param_shifts, manpage_relocate_target}; use crate::textline::{ block_is_doc, fence_marker_run, is_art, is_horizontal_rule, is_indented_code, is_table_row, }; @@ -353,228 +354,6 @@ fn merge_comment_groups(comments: Vec, source: &str) -> Vec { out } -/// Detect C/C++ signatures whose parameter comments all drifted forward by one: -/// every parameter after the first carries exactly one *leading* comment (the -/// ", comment param" shape inside "parameter_list"), plus exactly one comment -/// trailing the closing ")". That extra trailing comment is the tell that the -/// whole set is displaced: normal leading comments describe the *following* -/// parameter and leave nothing after ")". Returns a map from each drifted -/// comment's start byte to the offset where it should become a trailing comment -/// (the end of the parameter it actually describes: the previous one, or the -/// last parameter for the after-")" comment). Any deviation (a parameter with -/// zero or two leading comments, a missing trailing comment, a leading comment -/// on the first parameter) yields no entry for that function, so the transform -/// never fires on an ordinary signature. Idempotent: once shifted, each comment -/// sits as "param comment ," (before the comma), which is not the drift shape. -fn collect_param_shifts(root: Node, source: &str, lang: Language) -> HashMap { - let mut map = HashMap::new(); - if !matches!(lang, Language::C | Language::Cpp) { - return map; - } - let mut stack = vec![root]; - while let Some(node) = stack.pop() { - if node.kind() == "function_definition" { - detect_param_drift(node, source, lang, &mut map); - } - let mut cursor = node.walk(); - for child in node.children(&mut cursor) { - stack.push(child); - } - } - map -} - -fn detect_param_drift( - func: Node, - source: &str, - lang: Language, - map: &mut HashMap, -) { - let Some(decl) = func.child_by_field_name("declarator") else { - return; - }; - if decl.kind() != "function_declarator" { - return; - } - let Some(plist) = decl.child_by_field_name("parameters") else { - return; - }; - if plist.kind() != "parameter_list" { - return; - } - let text_of = |node: Node| &source[node.start_byte()..node.end_byte()]; - // The comment group trailing ")" (between the declarator and the body). - let body_start = func.child_by_field_name("body").map(|b| b.start_byte()); - let mut trailing: Vec = Vec::new(); - let mut cursor = func.walk(); - for child in func.children(&mut cursor) { - if child.kind() == "comment" - && child.start_byte() >= decl.end_byte() - && body_start.is_none_or(|b| child.end_byte() <= b) - { - trailing.push(child); - } - } - if trailing.is_empty() { - return; - } - // A real manual-page block after ")" belongs to transform 3, not here. - if trailing - .iter() - .any(|&t| has_manpage_section_headers(text_of(t))) - { - return; - } - - // Only "/* */" blocks shift; a "//" line comment would swallow the code - // after it once moved inline. - if trailing.iter().any(|&t| !is_block_comment(text_of(t))) { - return; - } - - // Parse the parameter_list into (param, leading comment group). Shape must - // be "(" param ("," comment* param)* ")": the first parameter has no - // leading comment; each later parameter may carry a whole group ("/* a */ - // /* b */"), all of which shift together. A comment before the first param - // or stray tokens bail. - let mut pcur = plist.walk(); - let kids: Vec = plist.children(&mut pcur).collect(); - let mut params: Vec<(Node, Vec)> = Vec::new(); - let mut i = 0; - if kids.first().map(Node::kind) != Some("(") { - return; - } - i += 1; - if kids.get(i).map(Node::kind) != Some("parameter_declaration") { - return; // no first param, or a comment leads it - } - params.push((kids[i], Vec::new())); - i += 1; - while kids.get(i).map(Node::kind) == Some(",") { - i += 1; - let mut group = Vec::new(); - while kids.get(i).map(Node::kind) == Some("comment") { - if !is_block_comment(text_of(kids[i])) { - return; // "//" line comment; see the trailing check - } - group.push(kids[i]); - i += 1; - } - if kids.get(i).map(Node::kind) != Some("parameter_declaration") { - return; // a missing param - } - params.push((kids[i], group)); - i += 1; - } - if kids.get(i).map(Node::kind) != Some(")") || i != kids.len() - 1 { - return; - } - - // The commented parameters must form a contiguous suffix ending at the last - // parameter (they and the after-")" group are all displaced forward by - // one). The first parameter must be uncommented (nowhere to shift back to). - let Some(m) = params.iter().position(|(_, g)| !g.is_empty()) else { - return; // no leading comments at all - }; - if m == 0 || !params[m..].iter().all(|(_, g)| !g.is_empty()) { - return; - } - - // A machine directive (NOLINT, ACSL, cppcheck, ...) in the drifted set must - // not move. This transform is atomic over the whole signature: shifting - // some comments while one stays put scrambles the parameter/comment pairing - // into a state that is neither the original nor a clean de-drift. So if any - // participant can't shift, abort the whole signature to passthrough. - if trailing - .iter() - .chain(params[m..].iter().flat_map(|(_, g)| g)) - .any(|&n| is_passthrough_directive(text_of(n), lang)) - { - return; - } - - // Shift each group back one: the group leading params[i] describes - // params[i-1]; the after-")" group describes the last parameter. - for i in m..params.len() { - let target = params[i - 1].0.end_byte(); - for comment in ¶ms[i].1 { - map.insert( - comment.start_byte(), - ParamShift { - insert_at: target, - after_paren: false, - }, - ); - } - } - let last_end = params[params.len() - 1].0.end_byte(); - for comment in &trailing { - map.insert( - comment.start_byte(), - ParamShift { - insert_at: last_end, - after_paren: true, - }, - ); - } -} - -/// A drifted parameter comment is only shiftable when it is a "/* … */" block: -/// a "//" line comment moved inline would comment out the following code. A -/// multi-line block is allowed but collapsed to one line on re-insert (see -/// "plan"), so it can't round-trip through the trailing-closer split. -fn is_block_comment(text: &str) -> bool { - text.starts_with("/*") -} - -fn has_manpage_section_headers(text: &str) -> bool { - let mut description = false; - let mut returns = false; - for part in text.split(['\n', '\r', '*']) { - let word = part - .trim_start_matches('/') - .split_whitespace() - .next() - .unwrap_or("") - .trim_end_matches(':'); - description |= word == "DESCRIPTION"; - returns |= matches!(word, "RETURN" | "RETURNS"); - } - description && returns -} - -/// A C/C++ block comment wedged between a function's signature and its body -/// ("type name(...) /* here */ { ... }") is the X11 manual-page placement. -/// When the comment carries "DESCRIPTION"/"RETURN" sections, normalize hoists -/// it ahead of the function; this returns the insert target (the physical line -/// start of the "function_definition"). Any other position yields "None", so -/// the relocation never fires for an ordinary comment. -/// -/// "parent" is the comment's enclosing node, handed down by the walk. Asking -/// tree-sitter for it instead ("Node::parent") costs a fresh descent from the -/// root, which is quadratic over a long run of sibling comments. -fn manpage_relocate_target( - node: Node, - parent: Option, - source: &str, - lang: Language, -) -> Option { - if !matches!(lang, Language::C | Language::Cpp) { - return None; - } - let parent = parent?; - if parent.kind() != "function_definition" { - return None; - } - let decl = parent.child_by_field_name("declarator")?; - let body = parent.child_by_field_name("body")?; - if node.start_byte() >= decl.end_byte() && node.end_byte() <= body.start_byte() { - Some(line_start_before(source, parent.start_byte())) - } else { - None - } -} - /// Distinct marker shapes for line-comment grouping. Hash-run kinds are /// length-tagged because "# foo" and "## bar" must NOT merge into one /// paragraph: shell convention treats the run length as a visual heading @@ -889,7 +668,7 @@ fn is_lint_directive(text: &str, lang: Language) -> bool { /// shellcheck, cbindgen. commentflow must never reflow, merge, relocate, or /// blank-line-detach these. (The Rust nested-"/* */" case is a grammar /// constraint, not a tool directive, so it stays inline at the one call site.) -fn is_passthrough_directive(text: &str, lang: Language) -> bool { +pub(crate) fn is_passthrough_directive(text: &str, lang: Language) -> bool { is_formatter_directive(text) || is_cppcheck_suppress(text) || (matches!(lang, Language::C | Language::Cpp) && is_acsl_annotation(text)) @@ -960,6 +739,54 @@ fn spans_bare_cr(source: &str, start_byte: usize, end_byte: usize) -> bool { .any(|(i, &b)| b == b'\r' && bytes.get(start_byte + i + 1) != Some(&b'\n')) } +/// True when a C/C++ line comment carries a backslash-newline line splice. +/// +/// A "\" at the end of a line splices the next line onto it before the comment +/// is even lexed, so a "//" comment written that way already extends past its +/// own line and the grammar reports one node spanning both. Two things then go +/// wrong, and neither is recoverable downstream: +/// +/// - Reflow can leave the "\" as the last thing on the LAST emitted line, and +/// the splice then swallows the CODE line below into the comment. It costs +/// nothing to write ("// a b c \" packs exactly that way at the right width). +/// - Deleting an interior line that is nothing but whitespace, which is +/// correct as comment-interior whitespace, re-points an existing splice from +/// that blank line onto the next code line. The comment text need not be +/// reflowed at all for this to fire, so no width is safe. +/// +/// The second pass then reads the swallowed code as comment prose and packs it, +/// so "--check" converges on the damage instead of reporting it. This is the +/// "spans_bare_cr" case exactly: it destroys code, no amount of downstream care +/// can undo it, so refuse the comment outright. +/// +/// Only C/C++ line comments. Rust has no line splicing, shell comments end at +/// the newline, and assembly claims only "/* */". A block comment is safe in +/// every language: "*/" terminates it whatever the preceding byte was. A "\" +/// anywhere else in the prose (a path, an escape) is untouched: only one +/// followed by nothing but horizontal whitespace to the line's end splices. +fn spans_line_splice(text: &str, lang: Language) -> bool { + if !matches!(lang, Language::C | Language::Cpp) || !text.starts_with("//") { + return false; + } + text.lines().any(ends_with_line_splice) +} + +/// True when this physical line is continued onto the next one by a trailing +/// "\", the translation-phase-2 splice. +/// +/// The one place this rule is written down. It had three encodings before, two +/// of them "trim_end().ends_with('\\')" and this one a byte-class walk, and +/// they disagreed: "trim_end" strips Unicode whitespace, so a "\" followed by a +/// no-break space was a splice to two callers and not to the third. It is not +/// one to any compiler either, which is why the horizontal-whitespace set is +/// the form that survived. The callers ask the same question for different +/// reasons (does this comment sit inside a macro, may transform 5 put a blank +/// line here, is this comment's extent load-bearing), and only one definition +/// of a cpp rule should exist to answer them. +pub(crate) fn ends_with_line_splice(line: &str) -> bool { + line.trim_end_matches([' ', '\t', '\r']).ends_with('\\') +} + /// Build a "Comment" from a byte span, deriving everything else from the /// source. Shared by the tree-sitter walk and the assembly scanner so the two /// producers can't drift on indent capture, trailing detection, or the @@ -975,7 +802,8 @@ fn make_comment( let text = source[start_byte..end_byte].to_string(); let force_passthrough = (lang == Language::Rust && rust_block_has_nested(&text)) || is_passthrough_directive(&text, lang) - || spans_bare_cr(source, start_byte, end_byte); + || spans_bare_cr(source, start_byte, end_byte) + || spans_line_splice(&text, lang); let line_start = line_start_before(source, start_byte); let line_prefix = &source[line_start..start_byte]; let is_trailing = line_prefix.chars().any(|c| !c.is_whitespace()); @@ -1154,7 +982,7 @@ fn is_inside_preprocessor_directive(start_byte: usize, source: &str) -> bool { // "\" line-continuation; otherwise this comment stands on its own. let prev_end = line_start - 1; let prev_start = line_start_before(source, prev_end); - if !source[prev_start..prev_end].trim_end().ends_with('\\') { + if !ends_with_line_splice(&source[prev_start..prev_end]) { return false; } line_end = prev_end; @@ -1330,6 +1158,45 @@ mod tests { assert!(directive.force_passthrough); } + #[test] + fn backslash_continued_line_comment_is_passthrough() { + // The shape that deleted a line of code: the comment's own continuation + // is whitespace-only, so removing it (correct, as comment-interior + // whitespace) re-points the splice onto "int x = 1;" and the next pass + // reads the declaration as prose. Nothing downstream can undo that, so + // the comment never enters the pipeline. + let cs = extract_comments("// aaa bbb \\\n \nint x = 1;\n", Language::C).unwrap(); + assert!(cs[0].force_passthrough); + + // Also when the continuation carries text: reflow is free to pack the + // "\" onto the last emitted line, where it swallows the code below. + let cs = extract_comments("// aaa \\\nbbb ccc\nint x = 1;\n", Language::Cpp).unwrap(); + assert!(cs[0].force_passthrough); + + // A "\" as the file's very last byte is NOT this: the grammar leaves it + // out of the node ("// aaa bbb " comes back), and there is no line + // after EOF for a splice to swallow anyway. + let cs = extract_comments("// aaa bbb \\", Language::C).unwrap(); + assert!(!cs[0].force_passthrough); + } + + #[test] + fn backslash_inside_prose_still_reflows() { + // Only a "\" with nothing but horizontal whitespace after it splices. A + // path or an escape mid-line is ordinary text, and freezing every + // comment that mentions one would gut the tool. + let cs = extract_comments("// see C:\\Users\\me for the log file\n", Language::C).unwrap(); + assert!(!cs[0].force_passthrough); + + // A block comment is safe whatever precedes the newline: "*/" ends it. + let cs = extract_comments("/* aaa \\\n * bbb */\n", Language::C).unwrap(); + assert!(!cs[0].force_passthrough); + + // Rust has no line splicing, so its line comments are never frozen. + let cs = extract_comments("// aaa \\\n// bbb\n", Language::Rust).unwrap(); + assert!(!cs[0].force_passthrough); + } + #[test] fn cppcheck_suppress_block_is_passthrough() { // The id sits on the first content line and prose fills the rest. diff --git a/src/signature.rs b/src/signature.rs new file mode 100644 index 0000000..f7fbd74 --- /dev/null +++ b/src/signature.rs @@ -0,0 +1,251 @@ +//! The two transforms that read a function's SIGNATURE, not just its comments. +//! +//! Everything else in "parse" locates comment nodes and builds "Comment"s from +//! them. These two ask tree-sitter what a "function_definition" looks like, to +//! decide where a comment BELONGS: the drifted-parameter shift (Scope transform +//! 4) and the X11 manual-page hoist (Scope transform 3), in that order below. +//! They are the only sanctioned uses of syntactic context beyond finding +//! comments, and they still move nothing but comment bytes. +//! +//! The module is documentation, not a wall: both are still CALLED from "parse" +//! (in "extract_comments_with" and "walk_collect"), so a third syntactic read +//! added at those call sites would not touch this file. What the boundary buys +//! is that the two sanctioned ones have a name and an address, so the ceiling +//! TODO.md sets is something a reviewer can point at. + +use std::collections::HashMap; + +use tree_sitter::Node; + +use crate::parse::{Language, ParamShift, is_passthrough_directive, line_start_before}; + +/// Detect C/C++ signatures whose parameter comments all drifted forward by one: +/// every parameter after the first carries exactly one *leading* comment (the +/// ", comment param" shape inside "parameter_list"), plus exactly one comment +/// trailing the closing ")". That extra trailing comment is the tell that the +/// whole set is displaced: normal leading comments describe the *following* +/// parameter and leave nothing after ")". Returns a map from each drifted +/// comment's start byte to the offset where it should become a trailing comment +/// (the end of the parameter it actually describes: the previous one, or the +/// last parameter for the after-")" comment). Any deviation (a parameter with +/// zero or two leading comments, a missing trailing comment, a leading comment +/// on the first parameter) yields no entry for that function, so the transform +/// never fires on an ordinary signature. Idempotent: once shifted, each comment +/// sits as "param comment ," (before the comma), which is not the drift shape. +pub(crate) fn collect_param_shifts( + root: Node, + source: &str, + lang: Language, +) -> HashMap { + let mut map = HashMap::new(); + if !matches!(lang, Language::C | Language::Cpp) { + return map; + } + let mut stack = vec![root]; + while let Some(node) = stack.pop() { + if node.kind() == "function_definition" { + detect_param_drift(node, source, lang, &mut map); + } + let mut cursor = node.walk(); + for child in node.children(&mut cursor) { + stack.push(child); + } + } + map +} + +fn detect_param_drift( + func: Node, + source: &str, + lang: Language, + map: &mut HashMap, +) { + let Some(decl) = func.child_by_field_name("declarator") else { + return; + }; + if decl.kind() != "function_declarator" { + return; + } + let Some(plist) = decl.child_by_field_name("parameters") else { + return; + }; + if plist.kind() != "parameter_list" { + return; + } + let text_of = |node: Node| &source[node.start_byte()..node.end_byte()]; + // The comment group trailing ")" (between the declarator and the body). + let body_start = func.child_by_field_name("body").map(|b| b.start_byte()); + let mut trailing: Vec = Vec::new(); + let mut cursor = func.walk(); + for child in func.children(&mut cursor) { + if child.kind() == "comment" + && child.start_byte() >= decl.end_byte() + && body_start.is_none_or(|b| child.end_byte() <= b) + { + trailing.push(child); + } + } + // Exactly one, which is what the tell means: a single comment displaced + // off the end of the list. Two or more after ")" is not one-step drift, and + // shifting them all onto the last parameter invents a grouping the author + // never wrote. The doc comment above has always said "exactly one"; the + // code only checked "at least one". + if trailing.len() != 1 { + return; + } + // A real manual-page block after ")" belongs to transform 3, not here. + if trailing + .iter() + .any(|&t| has_manpage_section_headers(text_of(t))) + { + return; + } + + // Only "/* */" blocks shift; a "//" line comment would swallow the code + // after it once moved inline. + if trailing.iter().any(|&t| !is_block_comment(text_of(t))) { + return; + } + + // Parse the parameter_list into (param, leading comment group). Shape must + // be "(" param ("," comment* param)* ")": the first parameter has no + // leading comment; each later parameter may carry a whole group ("/* a */ + // /* b */"), all of which shift together. A comment before the first param + // or stray tokens bail. + let mut pcur = plist.walk(); + let kids: Vec = plist.children(&mut pcur).collect(); + let mut params: Vec<(Node, Vec)> = Vec::new(); + let mut i = 0; + if kids.first().map(Node::kind) != Some("(") { + return; + } + i += 1; + if kids.get(i).map(Node::kind) != Some("parameter_declaration") { + return; // no first param, or a comment leads it + } + params.push((kids[i], Vec::new())); + i += 1; + while kids.get(i).map(Node::kind) == Some(",") { + i += 1; + let mut group = Vec::new(); + while kids.get(i).map(Node::kind) == Some("comment") { + if !is_block_comment(text_of(kids[i])) { + return; // "//" line comment; see the trailing check + } + group.push(kids[i]); + i += 1; + } + if kids.get(i).map(Node::kind) != Some("parameter_declaration") { + return; // a missing param + } + params.push((kids[i], group)); + i += 1; + } + if kids.get(i).map(Node::kind) != Some(")") || i != kids.len() - 1 { + return; + } + + // The commented parameters must form a contiguous suffix ending at the last + // parameter (they and the after-")" group are all displaced forward by + // one). The first parameter must be uncommented (nowhere to shift back to). + let Some(m) = params.iter().position(|(_, g)| !g.is_empty()) else { + return; // no leading comments at all + }; + if m == 0 || !params[m..].iter().all(|(_, g)| !g.is_empty()) { + return; + } + + // A machine directive (NOLINT, ACSL, cppcheck, ...) in the drifted set must + // not move. This transform is atomic over the whole signature: shifting + // some comments while one stays put scrambles the parameter/comment pairing + // into a state that is neither the original nor a clean de-drift. So if any + // participant can't shift, abort the whole signature to passthrough. + if trailing + .iter() + .chain(params[m..].iter().flat_map(|(_, g)| g)) + .any(|&n| is_passthrough_directive(text_of(n), lang)) + { + return; + } + + // Shift each group back one: the group leading params[i] describes + // params[i-1]; the after-")" group describes the last parameter. + for i in m..params.len() { + let target = params[i - 1].0.end_byte(); + for comment in ¶ms[i].1 { + map.insert( + comment.start_byte(), + ParamShift { + insert_at: target, + after_paren: false, + }, + ); + } + } + let last_end = params[params.len() - 1].0.end_byte(); + for comment in &trailing { + map.insert( + comment.start_byte(), + ParamShift { + insert_at: last_end, + after_paren: true, + }, + ); + } +} + +/// A drifted parameter comment is only shiftable when it is a "/* … */" block: +/// a "//" line comment moved inline would comment out the following code. A +/// multi-line block is allowed but collapsed to one line on re-insert (see +/// "plan"), so it can't round-trip through the trailing-closer split. +fn is_block_comment(text: &str) -> bool { + text.starts_with("/*") +} + +fn has_manpage_section_headers(text: &str) -> bool { + let mut description = false; + let mut returns = false; + for part in text.split(['\n', '\r', '*']) { + let word = part + .trim_start_matches('/') + .split_whitespace() + .next() + .unwrap_or("") + .trim_end_matches(':'); + description |= word == "DESCRIPTION"; + returns |= matches!(word, "RETURN" | "RETURNS"); + } + description && returns +} + +/// A C/C++ block comment wedged between a function's signature and its body +/// ("type name(...) /* here */ { ... }") is the X11 manual-page placement. +/// When the comment carries "DESCRIPTION"/"RETURN" sections, normalize hoists +/// it ahead of the function; this returns the insert target (the physical line +/// start of the "function_definition"). Any other position yields "None", so +/// the relocation never fires for an ordinary comment. +/// +/// "parent" is the comment's enclosing node, handed down by the walk. Asking +/// tree-sitter for it instead ("Node::parent") costs a fresh descent from the +/// root, which is quadratic over a long run of sibling comments. +pub(crate) fn manpage_relocate_target( + node: Node, + parent: Option, + source: &str, + lang: Language, +) -> Option { + if !matches!(lang, Language::C | Language::Cpp) { + return None; + } + let parent = parent?; + if parent.kind() != "function_definition" { + return None; + } + let decl = parent.child_by_field_name("declarator")?; + let body = parent.child_by_field_name("body")?; + if node.start_byte() >= decl.end_byte() && node.end_byte() <= body.start_byte() { + Some(line_start_before(source, parent.start_byte())) + } else { + None + } +} diff --git a/tests/corpus/boot.S b/tests/corpus/boot.S new file mode 100644 index 0000000..771301d --- /dev/null +++ b/tests/corpus/boot.S @@ -0,0 +1,33 @@ +/* + * DO NOT REFLOW THIS FILE. It is test input, not source: running the tool on it + * destroys the very shapes it exists to hold. Exclude tests/corpus when + * reflowing the repo's own sources. + * + * Assembly shapes: a macro whose backslash continuation makes a newline a + * terminator rather than whitespace, target-specific line-comment characters + * that are never claimed, a "/*" that a line comment already swallowed, and + * quoting that must not leak. + */ + +#define SAVE_REGS \ + /* inside a continuation chain a newline ends the macro, so this comment is frozen: no reflow, and no blank line above it either */ \ + push %rbx; \ + push %rbp + +/* Register usage. This block is ordinary prose and is the one shape here that reflows, so it is written long enough to prove the packer ran. + * + * %rax return value + * %rdi first argument + */ + + .section .rodata,"a",@progbits /* an ELF section-type marker, not a comment */ + .ascii "a /* inside a string is not a comment opener" + .byte '/', '*' + +# a "#" line comment on x86 is left alone, /* even when it holds an opener */ +@ an ARM32 line comment is left alone too, /* opener and all */ +; a GAS statement separator, /* likewise */ +// an AArch64 line comment, /* likewise */ + + mov $1, %eax /* trailing block comment, the one form claimed on every target */ + ret diff --git a/tests/corpus/boot.S.expected b/tests/corpus/boot.S.expected new file mode 100644 index 0000000..62bf0d2 --- /dev/null +++ b/tests/corpus/boot.S.expected @@ -0,0 +1,34 @@ +/* + * DO NOT REFLOW THIS FILE. It is test input, not source: running the tool on it + * destroys the very shapes it exists to hold. Exclude tests/corpus when + * reflowing the repo's own sources. + * + * Assembly shapes: a macro whose backslash continuation makes a newline a + * terminator rather than whitespace, target-specific line-comment characters + * that are never claimed, a "/*" that a line comment already swallowed, and + * quoting that must not leak. + */ + +#define SAVE_REGS \ + /* inside a continuation chain a newline ends the macro, so this comment is frozen: no reflow, and no blank line above it either */ \ + push %rbx; \ + push %rbp + +/* Register usage. This block is ordinary prose and is the one shape here that + * reflows, so it is written long enough to prove the packer ran. + * + * %rax return value + * %rdi first argument + */ + + .section .rodata,"a",@progbits /* an ELF section-type marker, not a comment */ + .ascii "a /* inside a string is not a comment opener" + .byte '/', '*' + +# a "#" line comment on x86 is left alone, /* even when it holds an opener */ +@ an ARM32 line comment is left alone too, /* opener and all */ +; a GAS statement separator, /* likewise */ +// an AArch64 line comment, /* likewise */ + + mov $1, %eax /* trailing block comment, the one form claimed on every target */ + ret diff --git a/tests/corpus/headers.h b/tests/corpus/headers.h index ab2e685..2caf511 100644 --- a/tests/corpus/headers.h +++ b/tests/corpus/headers.h @@ -6,9 +6,12 @@ * Shapes taken from real C headers that broke this tool, reproduced here rather * than vendored (the originals are SDK files with their own licenses). Every * one of these was found by running the binary over 228 macOS SDK headers, not - * by writing test cases. The corpus test only asserts that the file is STABLE: - * the in-process helper reruns the pipeline on its own output and requires a - * no-op. Add a shape here whenever a real file surprises you. + * by writing test cases. The corpus test asserts that the pipeline turns each + * input into its ".expected" sibling byte for byte, and the in-process helper + * separately reruns the pipeline on that output and requires a no-op. This file + * is already settled, so its ".expected" is a copy of it; a file holding an + * unsettled shape differs from its sibling, which is the point of having one. + * Add a shape here whenever a real file surprises you. */ /* Public header file for the library. bzlib.h diff --git a/tests/corpus/headers.h.expected b/tests/corpus/headers.h.expected new file mode 100644 index 0000000..2caf511 --- /dev/null +++ b/tests/corpus/headers.h.expected @@ -0,0 +1,70 @@ +/* + * DO NOT REFLOW THIS FILE. It is test input, not source: running the tool on it + * destroys the very shapes it exists to hold. Exclude tests/corpus when + * reflowing the repo's own sources. + * + * Shapes taken from real C headers that broke this tool, reproduced here rather + * than vendored (the originals are SDK files with their own licenses). Every + * one of these was found by running the binary over 228 macOS SDK headers, not + * by writing test cases. The corpus test asserts that the pipeline turns each + * input into its ".expected" sibling byte for byte, and the in-process helper + * separately reruns the pipeline on that output and requires a no-op. This file + * is already settled, so its ".expected" is a copy of it; a file holding an + * unsettled shape differs from its sibling, which is the point of having one. + * Add a shape here whenever a real file surprises you. + */ + +/* Public header file for the library. bzlib.h + * + * A run of one-line block comments that merge: rule, labeled rule, rule. + * Deleting the rules used to leave the dashes on the label for the next pass to + * strip. See is_bsd_dash_opener and strip_merged_member. + */ + +/* CAPI3REF: Run-Time Library Version Numbers + * KEYWORDS: sqlite3_version sqlite3_sourceid + * + * sqlite3.h's doc-generator directives. Packing these into the prose below them + * destroys the markup, so a run of banner rows holds its lines. + */ + +/* File: aio.h + Author: Somebody + * 05-Feb-2003 created. + * + * A tab-indented banner with no star markers. + */ + +/* Any application code which uses these declarations will get the following: + * + * compile link run + * + * funcA: normal normal normal funcB: warning normal normal funcF: normal weak + * on 10.3 normal typeA: warning + * + * AvailabilityMacros.h's table. Its separator row reads as a bookend whose + * label is another dash run; collapsing that made a fake setext underline and + * split the header off, which the next run could not see. See bookend_match. + */ + +/* unsigned int isn't 100% accurate as it should be a strict 4-byte value. + * XXX: Tcl is currently UCS-2 and planning UTF-16 for the Unicode + * XXX: string rep that Tcl_UniChar represents. + * XXX: Changing the size of Tcl_UniChar is not supported. + */ + +/* Reproduction of the COPYRIGHT file: + * +Copyright 1995-2002 University Corporation for Atmospheric Research + * + * A metadata line that lost its marker in the original. It must not be + * rewrapped into the prose around it, and it must not be retouched either. + */ + +/* Interaction flags (should be passed about in a control) Automatic (default): + * use defaults, prompt otherwise + * Interactive: prompt always + * Quiet: never prompt + */ + +int f(int a, int b); diff --git a/tests/corpus/nested.rs b/tests/corpus/nested.rs new file mode 100644 index 0000000..7e7f950 --- /dev/null +++ b/tests/corpus/nested.rs @@ -0,0 +1,40 @@ +//! DO NOT REFLOW THIS FILE. It is test input, not source: running the tool on +//! it destroys the very shapes it exists to hold. Exclude tests/corpus when +//! reflowing the repo's own sources. +//! +//! Rust shapes: nested block comments, a fenced code block, a markdown table, a +//! `//` sequence inside a raw string that is not a comment, and an unterminated +//! nested block at the end of the file. + +/// Parse one line of input into a token, returning `None` at end of input, which is the shape a badly wrapped doc comment takes. +/// +/// # Examples +/// +/// ``` +/// let t = parse("x = 1"); +/// assert!(t.is_some()); +/// ``` +/// +/// # Panics +/// +/// Never. The table below documents what each mode does, and its bars are alignment rather than prose. +/// +/// | mode | reads | writes | +/// |-------|-------|--------| +/// | strict| yes | no | +/// | lax | yes | yes | +pub fn parse(_s: &str) -> Option<()> { + None +} + +/* Rust block comments nest, so /* this inner opener really does open a comment + * and the closer below belongs to it */ and the outer comment is still open + * here until this line ends it. + */ +pub const NESTED: u32 = 1; + +pub const NOT_A_COMMENT: &str = r"a raw string holding // and /* which the grammar never reports as comments"; + +/* An unterminated /* nested block at the end of the file swallows the rest, so + * nothing may follow it. + */ diff --git a/tests/corpus/nested.rs.expected b/tests/corpus/nested.rs.expected new file mode 100644 index 0000000..2805a4c --- /dev/null +++ b/tests/corpus/nested.rs.expected @@ -0,0 +1,42 @@ +//! DO NOT REFLOW THIS FILE. It is test input, not source: running the tool on +//! it destroys the very shapes it exists to hold. Exclude tests/corpus when +//! reflowing the repo's own sources. +//! +//! Rust shapes: nested block comments, a fenced code block, a markdown table, a +//! `//` sequence inside a raw string that is not a comment, and an unterminated +//! nested block at the end of the file. + +/// Parse one line of input into a token, returning `None` at end of input, +/// which is the shape a badly wrapped doc comment takes. +/// +/// # Examples +/// +/// ``` +/// let t = parse("x = 1"); +/// assert!(t.is_some()); +/// ``` +/// +/// # Panics +/// +/// Never. The table below documents what each mode does, and its bars are +/// alignment rather than prose. +/// +/// | mode | reads | writes | +/// |-------|-------|--------| +/// | strict| yes | no | +/// | lax | yes | yes | +pub fn parse(_s: &str) -> Option<()> { + None +} + +/* Rust block comments nest, so /* this inner opener really does open a comment + * and the closer below belongs to it */ and the outer comment is still open + * here until this line ends it. + */ +pub const NESTED: u32 = 1; + +pub const NOT_A_COMMENT: &str = r"a raw string holding // and /* which the grammar never reports as comments"; + +/* An unterminated /* nested block at the end of the file swallows the rest, so + * nothing may follow it. + */ diff --git a/tests/corpus/objects.cpp b/tests/corpus/objects.cpp new file mode 100644 index 0000000..b0e1dcd --- /dev/null +++ b/tests/corpus/objects.cpp @@ -0,0 +1,57 @@ +/* + * DO NOT REFLOW THIS FILE. It is test input, not source: running the tool on it + * destroys the very shapes it exists to hold. Exclude tests/corpus when + * reflowing the repo's own sources. + * + * C++ shapes: a Doxygen block whose tags must survive, a pipe-aligned table, + * ASCII art, two backslash-continued line comments (frozen: a splice would + * swallow the code below; the second's whitespace-only continuation line is + * load-bearing, do not strip it), a fake nested opener, an unterminated block + * at the end of the file, and CRLF line endings on every line, which every + * emitted line must keep. The corpus test asserts the pipeline turns this file + * into its ".expected" sibling. + */ + +/** + * @brief Construct a widget. + * + * @param name the widget's name, which must outlive the widget itself + * @param size how many slots to reserve up front + * @return a widget, or nullptr when the allocation fails + */ +class Widget; + +/* State transitions. The bars are alignment, not prose, so the rows hold their + * lines while the paragraphs around them reflow. + * + * | from | event | to | + * |---------|-------|---------| + * | idle | start | running | + * | running | pause | idle | + * | running | stop | done | + */ + +/* A diagram passes through byte for byte, including the interior spacing that + * lines the boxes up. + * + * +--------+ +--------+ + * | parser | ---> | reflow | + * +--------+ +--------+ + */ + +// A line comment ending in a backslash continues onto the next physical \ +line, so the comment node spans both lines. Reflowing it can park the \ +as the last thing on the last line, where it swallows the code below. +int continued_comment = 0; + +// The shape that actually deleted a line of code. The continuation below \ + +int whitespace_continuation = 1; + +/* A block comment holding what looks like a nested opener: /* the inner opener + * is only text, and the first closer ends the comment. + */ +int after_fake_nesting = 1; + +/* An unterminated block comment at the end of the file swallows everything + * after it, so nothing may follow. diff --git a/tests/corpus/objects.cpp.expected b/tests/corpus/objects.cpp.expected new file mode 100644 index 0000000..b0e1dcd --- /dev/null +++ b/tests/corpus/objects.cpp.expected @@ -0,0 +1,57 @@ +/* + * DO NOT REFLOW THIS FILE. It is test input, not source: running the tool on it + * destroys the very shapes it exists to hold. Exclude tests/corpus when + * reflowing the repo's own sources. + * + * C++ shapes: a Doxygen block whose tags must survive, a pipe-aligned table, + * ASCII art, two backslash-continued line comments (frozen: a splice would + * swallow the code below; the second's whitespace-only continuation line is + * load-bearing, do not strip it), a fake nested opener, an unterminated block + * at the end of the file, and CRLF line endings on every line, which every + * emitted line must keep. The corpus test asserts the pipeline turns this file + * into its ".expected" sibling. + */ + +/** + * @brief Construct a widget. + * + * @param name the widget's name, which must outlive the widget itself + * @param size how many slots to reserve up front + * @return a widget, or nullptr when the allocation fails + */ +class Widget; + +/* State transitions. The bars are alignment, not prose, so the rows hold their + * lines while the paragraphs around them reflow. + * + * | from | event | to | + * |---------|-------|---------| + * | idle | start | running | + * | running | pause | idle | + * | running | stop | done | + */ + +/* A diagram passes through byte for byte, including the interior spacing that + * lines the boxes up. + * + * +--------+ +--------+ + * | parser | ---> | reflow | + * +--------+ +--------+ + */ + +// A line comment ending in a backslash continues onto the next physical \ +line, so the comment node spans both lines. Reflowing it can park the \ +as the last thing on the last line, where it swallows the code below. +int continued_comment = 0; + +// The shape that actually deleted a line of code. The continuation below \ + +int whitespace_continuation = 1; + +/* A block comment holding what looks like a nested opener: /* the inner opener + * is only text, and the first closer ends the comment. + */ +int after_fake_nesting = 1; + +/* An unterminated block comment at the end of the file swallows everything + * after it, so nothing may follow. diff --git a/tests/corpus/script.sh b/tests/corpus/script.sh new file mode 100644 index 0000000..9086a9f --- /dev/null +++ b/tests/corpus/script.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# DO NOT REFLOW THIS FILE. It is test input, not source: running the tool on it +# destroys the very shapes it exists to hold. Exclude tests/corpus when +# reflowing the repo's own sources. +# +# Shell shapes: a BOM before the shebang, a "#" run that reflows, a "#!" line in +# the middle of the file that is an ordinary comment, a "#" inside a string and +# inside a heredoc that is not a comment at all, and ASCII art. +set -eu + +# This paragraph is one long source line that has to wrap, which is the whole reason the reflow exists, and the run above it must keep its own marker column. + +#!/bin/sh is a comment here, not a shebang: only byte 0 of the file gets that +# reading, per exec(2). + +# +-------+ +--------+ +# | input | --> | output | +# +-------+ +--------+ + +echo 'a # inside single quotes is not a comment' +echo "neither is a # inside double quotes" + +cat <<'EOF' +# a heredoc body is data, not shell source, so this line is untouched even +# though it looks exactly like a comment run that wants reflowing badly enough +EOF + +exit 0 diff --git a/tests/corpus/script.sh.expected b/tests/corpus/script.sh.expected new file mode 100644 index 0000000..495dc44 --- /dev/null +++ b/tests/corpus/script.sh.expected @@ -0,0 +1,30 @@ +#!/usr/bin/env bash +# DO NOT REFLOW THIS FILE. It is test input, not source: running the tool on it +# destroys the very shapes it exists to hold. Exclude tests/corpus when +# reflowing the repo's own sources. +# +# Shell shapes: a BOM before the shebang, a "#" run that reflows, a "#!" line in +# the middle of the file that is an ordinary comment, a "#" inside a string and +# inside a heredoc that is not a comment at all, and ASCII art. +set -eu + +# This paragraph is one long source line that has to wrap, which is the whole +# reason the reflow exists, and the run above it must keep its own marker +# column. + +#!/bin/sh is a comment here, not a shebang: only byte 0 of the file gets that +# reading, per exec(2). + +# +-------+ +--------+ +# | input | --> | output | +# +-------+ +--------+ + +echo 'a # inside single quotes is not a comment' +echo "neither is a # inside double quotes" + +cat <<'EOF' +# a heredoc body is data, not shell source, so this line is untouched even +# though it looks exactly like a comment run that wants reflowing badly enough +EOF + +exit 0 diff --git a/tests/pipeline.rs b/tests/pipeline.rs index 9eaf895..4214c36 100644 --- a/tests/pipeline.rs +++ b/tests/pipeline.rs @@ -345,39 +345,87 @@ fn metadata_copyright_with_year_preserved() { ); } -/// Every file under "tests/corpus/" reproduces a shape from a real header that -/// this tool got wrong. Running them here is mostly about the convergence -/// assertion inside the shared "pipeline" helper: hand-written cases kept -/// missing the shapes that actually break, so the corpus is where real-world -/// input lives. -#[test] -fn corpus_files_are_stable() { +/// Every file under "tests/corpus/" reproduces a shape from real source that +/// this tool got wrong. Hand-written cases kept missing the shapes that +/// actually break, so the corpus is where real-world input lives: a whole file +/// at a time, which is the only way to hold the shapes that need file context +/// (a byte-0 shebang, a heredoc body, an unterminated block at EOF). Byte +/// equality against the recorded sibling is the assertion; the convergence +/// check inside the shared "pipeline" helper rides along on top of it. +#[test] +fn corpus_files_match_expected() { let dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/corpus"); - let mut seen = 0; - for entry in std::fs::read_dir(&dir).expect("read tests/corpus") { - let path = entry.expect("corpus entry").path(); - let Some(lang) = parse::detect_language(&path).ok() else { - continue; - }; - let src = std::fs::read_to_string(&path).expect("read corpus file"); - - // A corpus file holds shapes in their SETTLED form, so the first pass - // must already be a no-op. Byte equality is the whole assertion: it - // covers the code lines, the comment text, and the layout at once, and - // it stays meaningful for any file added later, which a check for one - // known code line did not. ("pipeline" separately asserts a second pass - // changes nothing, which catches a file that oscillates.) + + // Sorted, so a failure names the same file on every filesystem; "read_dir" + // order is unspecified. The ".expected" siblings are the one thing held + // back from the run. Anything else "detect_language" rejects is a file + // somebody put here expecting it to be tested, and skipping it silently is + // exactly the failure this test is supposed to make impossible. + let mut entries: Vec = std::fs::read_dir(&dir) + .expect("read tests/corpus") + .map(|e| e.expect("corpus entry").path()) + .collect(); + entries.sort(); + let (siblings, inputs): (Vec, Vec) = entries + .into_iter() + .partition(|p| p.extension().is_some_and(|e| e == "expected")); + + assert!( + !inputs.is_empty(), + "no corpus files found in {}", + dir.display() + ); + + for path in &inputs { + let lang = parse::detect_language(path) + .unwrap_or_else(|e| panic!("corpus file {}: {e}", path.display())); + let src = std::fs::read_to_string(path).expect("read corpus file"); + + // Append rather than "with_extension": a corpus file may legitimately + // have NO extension (the extensionless-shebang carve-out makes one a + // supported shell input), and "with_extension" would then produce + // "script..expected" and, for a dotfile, eat the name itself. + let mut expected_name = path.clone().into_os_string(); + expected_name.push(".expected"); + let expected_path = PathBuf::from(expected_name); + + // Byte equality against a recorded output is the whole assertion: it + // covers the code lines, the comment text, and the layout at once, for + // any language, which the old check for one known C declaration did + // not. Reading the sibling with "expect" rather than skipping is what + // makes a deleted ".expected" a failure. ("pipeline" separately asserts + // a second pass changes nothing, so a file that oscillates is caught + // even when its first pass matches.) + let expected = std::fs::read_to_string(&expected_path).unwrap_or_else(|e| { + panic!("read {}: {e}", expected_path.display()); + }); let out = pipeline(&src, lang, 80); assert_eq!( - src, + expected, out, - "corpus file {} is not a fixed point; if the tool is right, replace \ - the file with this output, otherwise the shape found a bug", - path.display() + "corpus file {} no longer produces {}; if the tool is right, \ + replace the sibling with this output, otherwise the shape found a \ + bug", + path.display(), + expected_path.display() + ); + } + + // The mirror of the missing-sibling failure above. A ".expected" whose + // input is gone (a rename that only touched one of the pair, a deleted + // shape) is a file this loop never opens, so nothing else in the suite + // would ever notice it went stale. + for sibling in &siblings { + // The inverse of the push above: "partition" guarantees the extension + // here is exactly "expected", so stripping it cannot eat anything else. + let input = sibling.with_extension(""); + assert!( + inputs.contains(&input), + "{} has no corpus input {}; delete the orphan or restore its input", + sibling.display(), + input.display() ); - seen += 1; } - assert!(seen > 0, "no corpus files found in {}", dir.display()); } #[test] @@ -1168,6 +1216,71 @@ void f(void) assert_eq!(pipeline(&out, detect("font.c"), 80), out); } +#[test] +fn plan_no_blank_before_comment_under_bom_directive() { + // Both "#" lines that suppress the blank line go through one parser, so the + // BOM strip reaches each of them. It used to reach only the shebang arm, + // which left a BOM'd guard failing the test its BOM-free twin passes: + // U+FEFF is not Unicode White_Space, so "trim" leaves it glued to the "#". + let guard = "\u{feff}#ifndef GUARD\n/* A header long enough to stay multi-line once the reflow has run over it. */\nint x;\n#endif\n"; + let out = pipeline(guard, detect("g.h"), 80); + assert!( + out.contains("#ifndef GUARD\n/* A header"), + "blank line wrongly inserted after a BOM-prefixed #ifndef:\n{out}" + ); + + // The shebang arm, same rule, with the BOM the corpus fixture carries. + let script = "\u{feff}#!/bin/sh\n# A header long enough to stay multi-line once the reflow has run over it here.\nexit 0\n"; + let out = pipeline(script, detect("s.sh"), 80); + assert!( + out.contains("#!/bin/sh\n# A header"), + "blank line wrongly inserted under a BOM-prefixed shebang:\n{out}" + ); +} + +#[test] +fn plan_blank_line_under_rust_inner_attribute() { + // "#![no_std]" sits exactly where a shebang does and starts with "#!", so + // the shell carve-out read it as one and suppressed the blank line under + // it. It is a Rust inner attribute: ordinary code, and the comment below it + // is explaining that code. + let src = "#![no_std]\n// A comment long enough that it stays multi-line once the packer has run over it.\nfn f() {}\n"; + let out = pipeline(src, detect("a.rs"), 80); + assert!( + out.contains("#![no_std]\n\n//"), + "no blank line under a Rust inner attribute:\n{out}" + ); + + // The shell shebang it was confused with still suppresses it. + let sh = "#!/bin/sh\n# A comment long enough that it stays multi-line once the packer has run over it.\nexit 0\n"; + let out = pipeline(sh, detect("s.sh"), 80); + assert!( + out.contains("#!/bin/sh\n#"), + "blank line wrongly inserted under a shebang:\n{out}" + ); +} + +#[test] +fn param_drift_needs_exactly_one_comment_after_paren() { + // One comment after ")" is the tell that the set is displaced by one. Two + // is not that shape, and shifting both onto the last parameter invents a + // grouping the author never wrote, so the whole signature passes through. + let two = "void f(int a, /* the b */ int b) /* one */ /* two */ {\n g();\n}\n"; + assert_eq!( + pipeline(two, detect("d.c"), 80), + two, + "two trailing comments must not shift" + ); + + // The documented one-comment drift still fires. + let one = "void f(int a, /* the b */ int b) /* the c */ {\n g();\n}\n"; + let out = pipeline(one, detect("e.c"), 80); + assert!( + out.contains("void f(int a /* the b */, int b /* the c */)"), + "one-step drift no longer shifts:\n{out}" + ); +} + #[test] fn plan_no_blank_after_preprocessor_conditional() { // A multi-line comment that is the first thing inside a preprocessor