Skip to content

AddToTagVisitor: skip insert when a semantically-equal child already exists#7567

Merged
steve-aom-elliott merged 1 commit intomainfrom
fix/add-to-tag-visitor-idempotent
May 4, 2026
Merged

AddToTagVisitor: skip insert when a semantically-equal child already exists#7567
steve-aom-elliott merged 1 commit intomainfrom
fix/add-to-tag-visitor-idempotent

Conversation

@steve-aom-elliott
Copy link
Copy Markdown
Contributor

@steve-aom-elliott steve-aom-elliott commented May 4, 2026

Summary

Make AddToTagVisitor idempotent by default: if the target scope already contains a child for which SemanticallyEqual.areEqual(existingChild, tagToAdd) holds, return without mutating the tree. SemanticallyEqual ignores comments and attribute order, so this catches semantic duplicates that only differ in whitespace, attribute ordering, or interleaved comments.

An allowDuplicates opt-out flag is provided for the rare caller that wants the prior unconditional-append behavior. Two new constructor overloads accept the flag without forcing callers to pass null for tagComparator just to opt out:

  • (Xml.Tag, Xml.Tag, boolean)
  • (Xml.Tag, Xml.Tag, @Nullable Comparator<Content>, boolean)

Why default-on

I audited every in-tree call site of AddToTagVisitor (~22, across rewrite-maven, rewrite-csharp, and the static addToTag(...) helpers used by rewrite-xml's AddOrUpdateChild and rewrite-maven's AddPluginDependency). None want duplicate sibling tags:

  • rewrite-maven: Maven POM schema forbids duplicate <dependency>, <plugin>, <property>, etc.
  • rewrite-csharp (AddNuGetPackageReferenceVisitor): csproj schema forbids duplicate <PackageReference>.
  • rewrite-xml/AddOrUpdateChild: already gates externally by checking for an existing child before delegating to addToTag, so the new check is redundant rather than behavior-changing for it.

Centralizing the check here removes existing per-site duplicate prevention and prevents the semantically-equal-but-different-identity output that has tripped up downstream tree-identity checks.

Why default-on with an opt-out

Raw XML technically allows duplicate sibling tags. Although no in-tree caller wants them, an unknown third-party consumer of rewrite-xml could in principle. allowDuplicates=true keeps that path available without forcing every in-tree consumer to flip a flag.

Behavior change

Default-off-then-on. Anyone passing a tagToAdd that is SemanticallyEqual to a child already present in scope will now see a no-op instead of a duplicate insert. A literal "I want a duplicate" use case isn't expected for in-tree callers (XML duplicates of <dependency>/<plugin>/<property> etc. are usually bugs); flagging here so reviewers can call out anything that needs adjustment.

Test plan

  • New doesNotAddSemanticallyEqualDuplicate test exercises the basic skip path
  • New doesNotAddDuplicateIgnoringAttributeOrder test exercises attribute-order tolerance
  • New addsWhenChildrenShareNameButDifferentAttributes test confirms additions still happen when children only share a tag name
  • Existing AddToTagTest cases still pass
  • Targeted maven recipe tests using AddToTagVisitor (Add{Dependency,Property,Repository,ManagedDependency,Profile,Plugin}, ExcludeDependency) all pass

@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite May 4, 2026
@steve-aom-elliott steve-aom-elliott added the bug Something isn't working label May 4, 2026
@steve-aom-elliott steve-aom-elliott force-pushed the fix/add-to-tag-visitor-idempotent branch from 8727772 to 52a80c5 Compare May 4, 2026 18:23
@steve-aom-elliott steve-aom-elliott changed the title AddToTagVisitor: skip insert when a semantically-equal child exists AddToTagVisitor: opt-in flag to skip insert when a semantically-equal child exists May 4, 2026
@steve-aom-elliott steve-aom-elliott force-pushed the fix/add-to-tag-visitor-idempotent branch from 52a80c5 to 40c8f2c Compare May 4, 2026 18:53
@steve-aom-elliott steve-aom-elliott changed the title AddToTagVisitor: opt-in flag to skip insert when a semantically-equal child exists AddToTagVisitor: skip insert when a semantically-equal child already exists May 4, 2026
…exists

Make `AddToTagVisitor` idempotent by default: if `scope` already contains
a child where `SemanticallyEqual.areEqual(existingChild, tagToAdd)` holds,
return without mutating the tree. `SemanticallyEqual` ignores comments
and attribute order, so this catches semantic duplicates that only differ
in whitespace, attribute ordering, or interleaved comments.

Preserves reference identity on no-op runs so that downstream "did this
recipe change anything" checks (dirty-marker tracking, conditional
`doAfterVisit` chains, recipe-cycle convergence) remain accurate.

An `allowDuplicates` opt-out is provided for the rare caller that wants
the prior unconditional-append behavior. Two new constructor overloads
accept the flag without forcing callers to pass `null` for `tagComparator`:
- `(Xml.Tag, Xml.Tag, boolean)`
- `(Xml.Tag, Xml.Tag, @nullable Comparator<Content>, boolean)`

An audit of every in-tree caller (~22 sites in `rewrite-maven`,
`rewrite-csharp`, and `rewrite-xml`) confirmed none want duplicate
sibling tags — Maven POM, csproj, and `AddOrUpdateChild` (which already
gates externally) all rely on the schemas forbidding duplicates or on
caller-side checks that have been redundant until now.
@steve-aom-elliott steve-aom-elliott force-pushed the fix/add-to-tag-visitor-idempotent branch from 40c8f2c to 7a9d305 Compare May 4, 2026 18:59
@steve-aom-elliott steve-aom-elliott added enhancement New feature or request test provided Already replicated with a unit test, using JUnit pioneer's ExpectedToFail xml labels May 4, 2026
@steve-aom-elliott steve-aom-elliott moved this from In Progress to Ready to Review in OpenRewrite May 4, 2026
@steve-aom-elliott steve-aom-elliott marked this pull request as ready for review May 4, 2026 19:37
@steve-aom-elliott steve-aom-elliott merged commit 1791ef5 into main May 4, 2026
1 check passed
@steve-aom-elliott steve-aom-elliott deleted the fix/add-to-tag-visitor-idempotent branch May 4, 2026 20:01
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request test provided Already replicated with a unit test, using JUnit pioneer's ExpectedToFail xml

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants