eliminate: full-file AST reprint destroys comments, blank lines, and structural formatting

[NikolaRHristov]

Summary

Maintain eliminate uses syn to parse and prettyplease to reprint the entire file whenever any single-use binding is inlined. This means even one eliminated let causes the entire file to be reformatted — removing inline // comments, collapsing blank lines, restructuring use imports, and changing code layout far beyond the site of the inlined binding.

Reproduction

~/Developer/Application/CodeEditorLand/Land/Target/release/Maintain eliminate \
  --path ./Source/Air \
  --verbose

Run against Element/Mountain/Source/Air, the diff shows:

• All section-separator comments (e.g. // --- Authentication Operations ---) removed
• All blank lines between methods and blocks collapsed or dropped
• Multi-line use declarations reformatted into a single line
• pub mod declarations (e.g. pub mod AirMetrics, pub mod AirStatus, etc.) removed from the output
• Doc-comment blocks on structs and constants stripped
• Structural labels like // Address of the Air daemon, // Default gRPC server address gone
• let intermediate variables collapsed into deeply nested method chains, making the code harder to follow

Root Cause

In Source/Eliminate/Transform/mod.rs, Run() unconditionally calls prettyplease::unparse(&Ast) on the entire syn::File whenever AnyChanged == true:

// Transform/mod.rs
if !AnyChanged {
    return Ok(None);
}
Ok(Some(prettyplease::unparse(&Ast)))

syn does not preserve non-doc // comments, blank lines, or original whitespace structure — these are not represented in the AST. prettyplease then regenerates canonical formatting with no knowledge of the original layout. The result is that a single inlined binding triggers a total rewrite of every file it touches.

Desired Behaviour (aggressive mode)

We prefer aggressive inlining — single-use variables should be eliminated without hesitation — but the output should preserve the original file's:

• Inline // comments and section-separator comments
• Blank lines used for visual grouping between methods, sections, and logical blocks
• Multi-line use and pub mod formatting
• Doc-comment blocks on items not being eliminated

Suggested Fixes

Option A — Preserve original whitespace/comment regions via text-level patching

Instead of reprinting the whole file, identify the line ranges of each inlined let statement in the original source and splice only those lines out, substituting the inlined expression at the usage site using a text-level replacement. This preserves all surrounding lines verbatim.

Requires tracking byte/line spans from syn's Span information through the substitution phase.

Option B — Use a comment-preserving Rust formatter

Switch from prettyplease (which drops comments) to a formatter that round-trips comments — for example rustfmt directly via rustfmt_nightly API or rust-analyzer's formatter, which both preserve comments. Then the full-file reprint would at least keep comments intact.

Option C — Split into two passes: eliminate first, normalize separately

Only write back the minimal diff (just the affected let + usage lines) when running in default mode. Provide a separate --normalize flag (or a second command) for users who explicitly want full-file canonical formatting. This way eliminate is safe to run on code with hand-crafted layout and comments.

Option D — Pre/post comment stitching

Before calling prettyplease::unparse, extract all // comment lines and their surrounding blank lines from the original source indexed by line number. After reprinting, attempt to re-insert them at matching structural positions. This is lossy for comments that were inline on the same line as code, but preserves section-separator blocks.

Impact

• Files that have been through eliminate no longer compile in some cases (see separate issue for E0382 borrow-after-move).
• All formatting conventions, section structure, and documentation in files under Source/Air have been erased.
• Running eliminate a second time re-triggers all files as "changed" if the formatter output differs from the stored version, making it non-idempotent at the project level even though individual transforms are idempotent.

Context

• Invocation: Maintain eliminate --path ./Source/Air --verbose
• Affected files: Source/Air/AirClient.rs, Source/Air/AirServiceProvider.rs (and likely all .rs files under Source/Air)
• Branch: Current
• Eliminate::Definition::Options::InlineComments defaults to false — doc-commented bindings are excluded from inlining — but this flag does not protect surrounding comments or blank lines from being erased during the reprint step.

[NikolaRHristov]

Additional findings from deeper analysis

C — Loop-body uses counted as 1: inlining into a loop changes evaluation semantics

Count.rs has no loop-awareness and this is explicitly documented as known broken in its own test suite:

/// A binding used only in a loop body: count = 1 (we do not model loops).
#[test]
fn LoopBodyCountedAsOne() {
    // ...
    // Tool sees one textual reference; it has no loop-awareness.
    assert_eq!(Count, 1);
}

Consider:

let expensive = compute_expensive_value();  // evaluated once
for item in &collection {
    process(item, expensive);               // Count sees: 1 reference
}

eliminate inlines this to:

for item in &collection {
    process(item, compute_expensive_value());  // now evaluated N times
}

This changes runtime behaviour for any initializer that has side effects, is expensive to compute, or involves I/O. For move-only types it also introduces a compile error on the second iteration. The fix is to detect when the substitution site is inside a for/while/loop body and skip inlining in that case, or at minimum count loop-body references as >1 to conservatively prevent inlining.

---

D — BindingsInlined stat is computed by naïve line-counting, not by actual eliminator count

In Process.rs:

let Before = Source.lines().filter(|L| L.trim_start().starts_with("let ")).count();
let After  = Transformed.lines().filter(|L| L.trim_start().starts_with("let ")).count();
Stats.BindingsInlined += Before.saturating_sub(After);

This is wrong in multiple ways:

• let inside string literals or macro bodies is counted.
• Multi-line let statements (where let appears mid-expression after prettyplease reformats) are miscounted.
• prettyplease may split or join lines independently of the number of bindings removed, skewing the delta.

The Eliminator struct already has Changed: bool but tracks no count. The fix is to add a Count: usize field to Eliminator that is incremented on each Block.stmts.remove() and bubble it up through Transform::Run → ProcessFile.

---

E — MaxIterations = 100 with no warning when the limit is hit

Constant::MaxIterations is 100. Because each inner loop pass eliminates at most one binding per block before restarting candidate collection, a single function with more than 100 single-use variables will silently stop short. Transform::Run returns Ok(Some(...)) with no indication that the fixed point was not reached, and ProcessFile has no way to detect this. Under --verbose, the stats will appear to show fewer inlined bindings than actually exist in the file.

The fix: expose whether MaxIterations was exhausted (e.g. return a (String, bool) from Run where the bool is reached_fixed_point), and emit a log::warn! in ProcessFile when it was not.