Skip to content

Commit 5b6e265

Browse files
jamesadevineCopilot
andcommitted
fix(compile): address codemod-refactor PR review
Two findings + two suggestions from the latest Rust PR Reviewer pass: 1. Naming clash in `perform_source_rewrite_if_needed`: the parameter `codemods: &codemods::CodemodReport` shadowed the sibling `codemods` module. Functionally fine (the module isn't used inside the function body) but a future caller adding a `codemods::something()` reference would hit a confusing type error. Renamed to `report`. 2. `docs/codemods.md` claimed "Front-matter key order is preserved" without qualification. Renamed keys move to the END of the front-matter block because `Mapping::insert` appends new keys when absent. Updated the doc to call this out, and added the same note to the stderr warning emitted when codemods rewrite the source. 3. Marked the two registry-health tests (`registry_ids_are_unique`, `codemod_filenames_match_registry_count`) as vacuously true while CODEMODS is empty, so future reviewers don't mistake the green test for a meaningful invariant today. 4. Added a comment on the `pub use helpers::*` re-export and on the helper-level `#[allow(dead_code)]`s explaining they're defensive against the empty-registry warning surface and should be stripped once the first real codemod lands. Plus, while in the docs, added an "unsanitized input" invariant to the codemod author contract: the runner sanitizes the typed `FrontMatter` AFTER codemods run, so the raw `Mapping` codemods receive is whatever the user wrote. Codemods should treat values as opaque rather than parse/interpolate them. All 1349 tests pass. No behavior changes outside the docs and the warning text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent c8cec49 commit 5b6e265

3 files changed

Lines changed: 34 additions & 7 deletions

File tree

docs/codemods.md

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,13 @@ every compile; codemods that don't match are essentially free.
4343
the closing `---`).
4444
- **Leading whitespace** before the opening `---` is preserved
4545
byte-for-byte (BOM-strippers and editor blank lines).
46-
- **Front-matter key order** is preserved by `serde_yaml`'s
47-
insertion-ordered mapping.
46+
- **Front-matter key order** is preserved for keys the codemod
47+
doesn't touch (`serde_yaml`'s mapping is insertion-ordered).
48+
Renamed keys, however, move to the **end** of the front-matter
49+
block: `Mapping::insert` appends new keys, so when a codemod
50+
removes `old-key` and inserts `new-key`, the new key lands at
51+
the bottom regardless of where the old one was. The compile
52+
warning calls this out so users aren't surprised.
4853
- **Front-matter comments** are NOT preserved. `serde_yaml`
4954
round-trip drops them. The warning emitted on rewrite calls this
5055
out so it isn't a surprise. If you have important context in a
@@ -191,6 +196,15 @@ review + per-codemod tests:
191196
6. **Order-aware.** If codemod B depends on shapes produced by
192197
codemod A, A must precede B in the registry. Document the
193198
ordering requirement in B's doc comment.
199+
7. **Receives unsanitized input.** The compiler runs sanitization
200+
(`##vso[` neutralization, control-character stripping, length
201+
limits) on the typed `FrontMatter` *after* codemods run, but the
202+
raw `Mapping` you receive is whatever the user wrote — including
203+
any pipeline-injection attempts, control characters, or
204+
over-length strings. Codemods should therefore treat values as
205+
opaque (move them around, wrap them in objects, etc.) rather
206+
than parse or interpolate them. If a codemod must inspect a
207+
value, treat it defensively.
194208

195209
### Use the helpers
196210

src/compile/codemods/mod.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,13 @@ use anyhow::{Context, Result};
3333
use serde_yaml::Mapping;
3434

3535
mod helpers;
36+
// `#[allow(unused_imports)]` here mirrors the per-item
37+
// `#[allow(dead_code)]` annotations in `helpers.rs`. While the
38+
// CODEMODS registry is empty, no in-crate caller exercises these
39+
// re-exports — the lint warns. Once the first real codemod lands
40+
// and uses one of these helpers, both the re-export attribute and
41+
// the per-item `dead_code` allows on `helpers.rs` should be
42+
// removed.
3643
#[allow(unused_imports)]
3744
pub use helpers::{insert_no_overwrite, rename_key, take_key, ConflictPolicy};
3845

@@ -159,6 +166,9 @@ mod tests {
159166

160167
#[test]
161168
fn registry_ids_are_unique() {
169+
// Vacuously true while CODEMODS is empty; the assertion
170+
// machinery still compiles so this test guards against
171+
// duplicate ids the moment a real codemod ships.
162172
let mut seen = std::collections::BTreeSet::new();
163173
for c in CODEMODS {
164174
assert!(
@@ -171,8 +181,10 @@ mod tests {
171181

172182
#[test]
173183
fn codemod_filenames_match_registry_count() {
174-
// Scan src/compile/codemods/*.rs and check that each numeric
175-
// codemod file is present exactly once in the registry.
184+
// Vacuously true while CODEMODS is empty (the directory
185+
// contains only `mod.rs` and `helpers.rs`, which are
186+
// skipped). Once a numeric `<NNNN>_<id>.rs` file lands, this
187+
// test asserts the registry was updated to match.
176188
let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
177189
.join("src/compile/codemods");
178190
let mut numeric_files: Vec<String> = Vec::new();

src/compile/mod.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ async fn perform_source_rewrite_if_needed(
263263
front_matter_mapping: &serde_yaml::Mapping,
264264
body_raw: &str,
265265
source_sha256: &[u8; 32],
266-
codemods: &codemods::CodemodReport,
266+
report: &codemods::CodemodReport,
267267
) -> Result<bool> {
268268
let new_content =
269269
common::reconstruct_source(leading_whitespace, front_matter_mapping, body_raw)?;
@@ -303,11 +303,12 @@ async fn perform_source_rewrite_if_needed(
303303
})?;
304304

305305
eprintln!("warning: applied codemods to {}:", input_path.display());
306-
for applied in &codemods.applied {
306+
for applied in &report.applied {
307307
eprintln!(" - {}: {}", applied.id, applied.summary);
308308
}
309309
eprintln!(
310-
"note: front-matter comments are not preserved when codemods rewrite the source."
310+
"note: front-matter comments are not preserved when codemods rewrite the source. \
311+
Renamed keys may move to the end of the front-matter block."
311312
);
312313

313314
Ok(true)

0 commit comments

Comments
 (0)