Skip to content

Fold rebuild reassignments into initializers and reclaim final locals - #156

Merged
steve-aom-elliott merged 4 commits into
mainfrom
unfinalize-locals-for-rebuild-rewrite
Jul 6, 2026
Merged

Fold rebuild reassignments into initializers and reclaim final locals#156
steve-aom-elliott merged 4 commits into
mainfrom
unfinalize-locals-for-rebuild-rewrite

Conversation

@steve-aom-elliott

@steve-aom-elliott steve-aom-elliott commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #155.

Two related improvements to MigrateMapperSettersToBuilder for mappers configured in a helper method away from their construction site.

1. Fold mapper = mapper.rebuild()...build(); into the initializer when adjacent

When a rebuild reassignment is the statement immediately following the declaration, fold the two into a single-statement form. The issue #155 shape:

final ObjectMapper mapper = initObjectMapper();
mapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);
mapper.disable(SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS);
return mapper;

now migrates to:

final JsonMapper mapper = initObjectMapper().rebuild()
        .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
        .disable(SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS)
        .build();
return mapper;

The fold intentionally requires the reassignment to be the very next statement — any intervening read of the local (as in the existing trailingSetterAfterGapOnLocal shape, where System.out.println(mapper) sits between the setters) prevents the fold and preserves observable semantics.

2. Reclaim final locals so the rebuild rewrite applies

Before this PR, final locals were unconditionally skipped by the rebuild-assignment path added in #153 — the receiver has to be reassignable — so #155's exact shape (final ObjectMapper mapper = ...; followed by two configuration calls) got the TODO fallback and shipped broken code once the SerializationFeatureDateTimeFeature rename ran. Now:

  • If the fold above succeeds, the declaration is initialized once and final is preserved.
  • If the fold can't apply (an intervening read forces the two-statement form), the recipe strips the final modifier so the rebuild reassignment is legal.
  • Un-finalizing is skipped when the local is referenced inside any lambda or anonymous-class body in its scope, because a subsequent reassignment would break Java's effective-final capture requirement. Those cases keep the existing TODO.

How

  • foldRebuildIntoInitializer post-pass: walks each block, and when it sees T name = <init>; immediately followed by name = name.rebuild()....build();, merges them into T name = <init>.rebuild()....build();. Uses the existing RebuildParts / tryParseRebuildAssignment helpers to reconstruct the chain rooted at the initializer.
  • foldRebuildIntoInitializer runs after coalesceRebuildAssignments (so consecutive rebuild reassignments become one chain before folding) and before unfinalizeDeclarations. All three post-passes are now scheduled from visitBlock so ordering is explicit; visitMethodInvocation records reclaim candidates in a per-visitor Set<UUID> instead of scheduling doAfterVisit directly.
  • unfinalizeDeclarations inspects each candidate declaration and only strips final when the initializer doesn't already contain a .rebuild() chain — i.e. only when the fold couldn't consume the reassignment.
  • findLocalDeclaration is factored out of isReassignableLocal and reused by reclaimableFinalLocalDeclarationId. isReferencedInsideCapture walks the enclosing method body for lambda/anonymous-class capture; those decls stay final and get the TODO.
  • Final fields intentionally still get the TODO — this change is scoped to locals.

Test plan

  • ./gradlew test — full suite passes, including five new cases under RewriteAsRebuild:
    • finalLocalIsUnfinalizedAndRewritten — final local with an intervening read: two-statement form, final stripped.
    • finalLocalFromMethodCallCoalescesRebuildChain — the issue Jackson 2 to 3 migration leaves broken code when ObjectMapper is created in one method and configured in another #155 reproducer: coalesce + fold + final preserved.
    • nonFinalLocalFromMethodCallFoldsIntoInitializer — proves the fold applies to non-final locals too.
    • finalLocalCapturedByLambdaKeepsTodoComment — capture guard for Supplier<JsonMapper> lambdas.
    • finalLocalCapturedByAnonymousClassKeepsTodoComment — capture guard for anonymous inner classes.
  • Existing trailingSetterAfterGapOnLocal still asserts the two-statement form (intervening read prevents fold).
  • Existing finalFieldStillGetsTodoComment unchanged: final fields intentionally excluded.

`MigrateMapperSettersToBuilder` previously left a TODO comment on any setter
whose receiver was a `final` local, because the rebuild-assignment rewrite
introduced by #153 requires the receiver to be reassignable. Issue #155 hits
this exact case: `final ObjectMapper mapper = initObjectMapper();` followed
by two `mapper.disable(...)` calls in a helper method.

When the local is not referenced inside any lambda or anonymous-class body
in its scope, stripping `final` is behavior-preserving: the only new
reassignment is the one the recipe itself introduces. If the local is
captured, un-finalizing (and reassigning) would break Java's effective-final
requirement, so those cases still fall back to the TODO.

- `findLocalDeclaration` factored out of `isReassignableLocal` and reused
  by the new `reclaimableFinalLocalDeclarationId` helper.
- `isReferencedInsideCapture` walks the enclosing method body and returns
  true if the identifier is used under any `J.Lambda` or `J.NewClass` with
  a body.
- `unfinalizeDeclaration` post-pass strips the `final` modifier from the
  matched `J.VariableDeclarations`, preserving the declaration's leading
  whitespace when `final` was the first modifier.
- Final fields keep the existing TODO — this change is scoped to locals.
… adjacent

When a rebuild reassignment is the statement immediately following its
declaration (i.e. no read of the local sits between them), fold the two
into a single `T mapper = <init>.rebuild()...build();` declaration. This
produces the one-line shape when the shape allows it, and — critically —
lets the reclaim-final-local path preserve `final` rather than strip it,
since the declaration is initialized once and never reassigned.

- New `foldRebuildIntoInitializer` post-pass runs after
  `coalesceRebuildAssignments` and before `unfinalizeDeclarations`.
- Ordering matters, so all three post-passes are now scheduled from
  `visitBlock`; `visitMethodInvocation` records reclaim candidates in a
  per-visitor `Set<UUID>` instead of scheduling `doAfterVisit` directly.
- `unfinalizeDeclarations` checks whether the declaration's initializer
  already contains a `.rebuild()` chain — if so, the fold consumed the
  reassignment and `final` is preserved.
- The fold intentionally requires the reassignment to be the *next*
  statement. Any intervening read (as in `trailingSetterAfterGapOnLocal`)
  prevents the fold, and the two-statement form + `final`-strip apply.
@steve-aom-elliott steve-aom-elliott changed the title Un-finalize reclaimable final locals so mapper.rebuild() rewrite applies Fold rebuild reassignments into initializers and reclaim final locals Jul 6, 2026
@steve-aom-elliott steve-aom-elliott added enhancement New feature or request recipe labels Jul 6, 2026
@steve-aom-elliott steve-aom-elliott moved this from In Progress to Ready to Review in OpenRewrite Jul 6, 2026
@steve-aom-elliott

Copy link
Copy Markdown
Contributor Author

Trimming comments now

timtebeek and others added 2 commits July 6, 2026 18:33
Address review feedback on the reclaim-final-local post-passes:
- isReferencedInsideCapture: use the TreeVisitor.reduce(tree, Set) idiom
  already used elsewhere in this file instead of a boolean[] holder.
- unfinalizeDeclarations: strip the final modifier via ListUtils.map/mapFirst,
  dropping the manual index scan and ArrayList copy.
- foldRebuildIntoInitializer: fold adjacent pairs with an index-based
  ListUtils.map (null drops the consumed tail), removing the merged list and
  changed flag. Folds never overlap since a reassignment tail is never a
  declaration head.

@timtebeek timtebeek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see this expanded once again, thanks!

@steve-aom-elliott
steve-aom-elliott merged commit 302735d into main Jul 6, 2026
1 check passed
@steve-aom-elliott
steve-aom-elliott deleted the unfinalize-locals-for-rebuild-rewrite branch July 6, 2026 16:46
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request recipe

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Jackson 2 to 3 migration leaves broken code when ObjectMapper is created in one method and configured in another

2 participants