JavaScript: Detect empty diffs in rewriteRun() test harness and fix surfaced issues#6904
Merged
knutwannheden merged 5 commits intomainfrom Mar 10, 2026
Merged
JavaScript: Detect empty diffs in rewriteRun() test harness and fix surfaced issues#6904knutwannheden merged 5 commits intomainfrom
rewriteRun() test harness and fix surfaced issues#6904knutwannheden merged 5 commits intomainfrom
Conversation
Add allowEmptyDiff option to RecipeSpec (default false) that catches recipes which modify the AST without producing visible output changes. This surfaces parser/formatter bugs where whitespace is placed on the wrong AST element. Fix produceTree() to preserve object identity when neither the recipe nor marker visiting changed anything, preventing false positives from the base visitor infrastructure.
The empty diff detection added in the previous commit surfaced real
identity-preservation bugs across several visitors and recipes:
- MinimumViableSpacingVisitor: remove unconditional body prefix clear
that stripped the space before '{' in class declarations
- WrappingAndBracesVisitor: skip produce() when leadingAnnotations is
empty; restructure visitBlock/visitElse to check conditions first
- SpacesVisitor: add guard to skip produce() on already-correct imports;
avoid moving whitespace between equivalent AST positions
- WhitespaceReconciler: compare Space objects structurally instead of by
reference so identical whitespace doesn't create new objects
- OrderImports: early return when imports are already sorted
- RemoveImport: use updateIfChanged instead of object spreads; only
filter statements when removals actually occurred
knutwannheden
changed the title
rewriteRun() test harness and fix surfaced issues
- Remove duplicated importNeedsSpaceChanges guard; Mutative natively detects same-value assignments and preserves object identity, so conditional guards are unnecessary for scalar property writes. - Add TODO noting parser normalization issue for import keyword space. - Add clarifying comments: Mutative structural sharing invariant in produceTree, SpacesVisitor body.prefix responsibility in MVS, TextComment-only caveat in spacesEqual. - Fix test isolation: use local RecipeSpec instead of shared instance that leaked allowEmptyDiff between tests.
The parser always places the space after 'import' on importClause.prefix (both ImportClause and NamedBindings share the same trivia span, but ImportClause consumes it first). Remove the workaround that accepted the space in either position and the associated TODO. Also remove unused type parameter on ensurePrefixHasNewLine.
- Remove dead length check in remove-import.ts filter guard (mapAsync preserves array length, so the check was always equal) - Rename blockNeedsOneLine to blockIsMultiLine for clarity - Remove unnecessary allowEmptyDiff in test — ChangeText correctly preserves identity via produceTree when text is unchanged - Add comment noting spread cost trade-off in produceTree
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
rewriteRun()test harness — throws when a recipe changes the AST but printed output is identical (allowEmptyDiff = falseby default).produceTree()inTreeVisitorto preserve object identity when no changes occur.RemoveImport: useupdateIfChangedand eliminate dead length check in filter guard.OrderImports: add early return when imports are already sorted.MinimumViableSpacingVisitor: remove line that incorrectly stripped the space before{in class declarations.WrappingAndBracesVisitor: skipproduce()whenleadingAnnotationsis empty; renameblockNeedsOneLine→blockIsMultiLinefor clarity.SpacesVisitor: simplify import space handling — parser always places the space onimportClause.prefix, so no ambiguity workaround needed.WhitespaceReconciler: compareSpaceobjects structurally instead of by reference.Test plan