Skip to content

Rewrite trailing setters and field setters as mapper.rebuild() chains - #153

Merged
steve-aom-elliott merged 2 commits into
mainfrom
migrate-mapper-setters-rebuild-and-field-access
Jul 3, 2026
Merged

Rewrite trailing setters and field setters as mapper.rebuild() chains#153
steve-aom-elliott merged 2 commits into
mainfrom
migrate-mapper-setters-rebuild-and-field-access

Conversation

@steve-aom-elliott

Copy link
Copy Markdown
Contributor

Summary

MigrateMapperSettersToBuilder previously left mapper.rebuild()...build() TODO comments on any setter it couldn't fold into a Mapper.builder()...build() chain, and silently skipped setters whose receiver was a field access (this.mapper.setX(...)).

This PR rewrites those setters as receiver = receiver.rebuild().<builderName>(args).build(); when the receiver is safely reassignable:

  • a non-final local declared in an enclosing block — method parameters are excluded because reassignment silently drops the caller's expected mutation
  • a J.FieldAccess with an identifier target and a non-final field

A doAfterVisit post-pass coalesces consecutive per-receiver rebuild assignments into a single chained .rebuild()....build(), so a run of N setters becomes one assignment rather than N.

Before

class A {
    JsonMapper create() {
        JsonMapper mapper = new JsonMapper();
        mapper.disable(SerializationFeature.INDENT_OUTPUT);
        System.out.println(mapper);
        mapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
        return mapper;
    }
}

The enable(...) call was left with a TODO because it fell outside the contiguous run following new JsonMapper(). A field-scoped variant like this.mapper.setX(...) produced no rewrite and no TODO at all.

After

class A {
    JsonMapper create() {
        JsonMapper mapper = JsonMapper.builder()
                .disable(SerializationFeature.INDENT_OUTPUT)
                .build();
        System.out.println(mapper);
        mapper = mapper.rebuild()
                .enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                .build();
        return mapper;
    }
}

And, newly covered, the this.mapper.setX(...) shape:

class A {
    private JsonMapper mapper = new JsonMapper();

    void configure() {
        this.mapper = this.mapper.rebuild()
                .disable(SerializationFeature.INDENT_OUTPUT)
                .enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                .build();
    }
}

How

  • Extend the guard in visitMethodInvocation to accept J.FieldAccess selects with a J.Identifier target, so field-shape setters are considered rather than silently dropped.
  • Add rewriteAsRebuildAssignment to build a JavaTemplate producing one <lhs> = <lhs>.rebuild().<builderName>(args).build() assignment per setter, applied at the setter's own coordinate — invoked before the TODO fallback when the receiver is reassignable.
  • Add coalesceRebuildAssignments as a doAfterVisit post-pass. Purely tree surgery (no template): it walks each block, parses consecutive <lhs> = <lhs>.rebuild()....build(); statements via tryParseRebuildAssignment, and if their LHSs are SemanticallyEqual, rewires the chain calls into one shared .rebuild() + .build().
  • Add rebuild() to mapperStub so the JavaTemplate parser resolves it against the Jackson 2 classpath used for template stubs.

Method parameters, final fields, and receivers behind a method chain still get the existing TODO comment.

Test plan

  • ./gradlew test — all tests pass, including five new cases under @Nested class RewriteAsRebuild:
    • trailingSetterAfterGapOnLocal — one straggler after a gap becomes a rebuild() assignment.
    • trailingSettersCoalesceIntoOneRebuildChain — multiple stragglers coalesce into one chain.
    • thisFieldSetterCoalescesIntoRebuildthis.mapper.setX(...) rewrites and coalesces.
    • qualifiedFieldSetterRewrittenholder.mapper.setX(...) rewrites.
    • finalFieldStillGetsTodoComment — final field left with TODO (no rewrite).
  • The existing CommentFallback.mapperFromParameter still leaves the TODO comment (parameters intentionally excluded).

`MigrateMapperSettersToBuilder` previously left `mapper.rebuild()...build()`
TODO comments on any setter it couldn't fold into a `Mapper.builder()...build()`
chain, and silently skipped setters whose receiver was a field access
(`this.mapper.setX(...)`).

This change rewrites those setters as
`receiver = receiver.rebuild().<builderName>(args).build();` when the receiver
is safely reassignable:

- a non-final local declared in an enclosing block (method parameters are
  excluded because reassignment silently drops the caller's expected
  mutation), or
- a `J.FieldAccess` with an identifier target and a non-final field.

A `doAfterVisit` post-pass coalesces consecutive per-receiver rebuild
assignments into a single chained `.rebuild()....build()`, so a run of N
setters becomes one assignment rather than N.

Setters that remain not-rewritable (final field, parameter, receiver behind
a method chain) still get the existing TODO comment.
@steve-aom-elliott steve-aom-elliott added enhancement New feature or request recipe labels Jul 3, 2026
@steve-aom-elliott steve-aom-elliott moved this from In Progress to Ready to Review in OpenRewrite Jul 3, 2026
Comment on lines +763 to +781
private static boolean isReassignableReceiver(Expression select, Cursor cursor) {
if (select instanceof J.Identifier) {
return isReassignableLocal((J.Identifier) select, cursor);
}
if (select instanceof J.FieldAccess) {
J.FieldAccess fa = (J.FieldAccess) select;
if (!(fa.getTarget() instanceof J.Identifier)) {
return false;
}
JavaType.Variable v = fa.getName().getFieldType();
return v != null && !v.hasFlags(Flag.Final);
}
return false;
}

private static boolean isReassignableLocal(J.Identifier ident, Cursor cursor) {
JavaType.Variable fieldType = ident.getFieldType();
if (fieldType != null && fieldType.hasFlags(Flag.Final)) {
return false;

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.

I wonder if we should have a Reassignable trait or something like it. There must be quite a few recipes which must ask a variation on this question.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can kick something off to try making one that would live in openrewrite/rewrite, as I feel that's likely a more appropriate place for it to live. Later on we could refactor this to use that trait instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sambsnyd You can see the initial pass of this here:

@steve-aom-elliott
steve-aom-elliott merged commit 99addc9 into main Jul 3, 2026
1 check passed
@steve-aom-elliott
steve-aom-elliott deleted the migrate-mapper-setters-rebuild-and-field-access branch July 3, 2026 20:52
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Jul 3, 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.

2 participants