Skip to content

Improved support for indentation formatting using records and single arg method declarations#6158

Merged
sambsnyd merged 17 commits intomainfrom
indentation-issue
Oct 22, 2025
Merged

Improved support for indentation formatting using records and single arg method declarations#6158
sambsnyd merged 17 commits intomainfrom
indentation-issue

Conversation

@Jenson3210
Copy link
Contributor

Summary

This PR improves indentation formatting support for Java records and method declarations, particularly for cases with single or multiple parameters that span multiple lines. The changes ensure consistent continuation indentation and proper alignment of parameters in both record components and method declarations.

Changes

Core Formatting Improvements

TabsAndIndentsVisitor.java

  • Extended indentation logic to support record components (primary constructor parameters) using the same alignment rules as method parameters
  • Added support for single-parameter indentation when the parameter is on a new line
  • Improved computeFirstParameterColumn() to handle both J.MethodDeclaration and J.ClassDeclaration (for records)
  • Added IndentType message passing to maintain proper indentation context
  • Fixed alignment calculation to consistently use declPrefixLength + continuationIndent when first parameter is on a new line
  • Added RECORD_STATE_VECTOR and METHOD_DECLARATION_PARAMETER to argument alignment cases

MinimumViableSpacingVisitor.java

  • Added spacing normalization for record primary constructor parameters
  • Added spacing normalization for method declaration parameters
  • Ensures parameters on the same line have proper spacing (no space before first param, single space before subsequent params)
  • Preserves newlines when parameters are already on separate lines

Style Configuration

TabsAndIndentsStyle.java

  • Added new RecordComponents configuration class with alignWhenMultiple setting
  • Mirrors the existing MethodDeclarationParameters structure for consistency

Autodetect.java

  • Updated style auto-detection to include RecordComponents configuration
  • Uses the same detection logic as method parameters for now (aligned vs non-aligned)

IntelliJ.java

  • Updated IntelliJ style defaults to include RecordComponents with alignment enabled

Test Coverage

AutoFormatTest.java

Added comprehensive test coverage for:

  • Record single parameter indentation - Handles various incorrect indentation scenarios and preserves correct formatting
  • Record multiple parameter indentation - Ensures consistent continuation indent for multiple record components
  • Method single parameter indentation - Fixes incorrect indentation for single parameters on new lines
  • Method multiple parameter indentation - Ensures proper alignment for multiple method parameters
  • Nested method invocations - Tests indentation when method calls are nested as arguments with multiple parameters

StyleHelperTest.java

  • Updated test to verify RecordComponents style merging works correctly

Example Transformations

Record with single parameter

// Before
record someRecord(
String name) {
}

// After
record someRecord(
        String name) {
}

Record with multiple parameters

// Before
record someRecord(
String name,
int age) {
}

// After
record someRecord(
        String name,
        int age) {
}

Method with single parameter

// Before
void someMethod(
String name) {
}

// After
void someMethod(
        String name) {
}

Technical Notes

  • The implementation uses a NoSuchMethodError catch block for backward compatibility when RecordComponents is not available in older runtime environments/lsts
  • Indentation calculations now properly account for the declaration prefix length plus continuation indent
  • The changes maintain consistency between record component formatting and method parameter formatting

@Jenson3210 Jenson3210 self-assigned this Oct 20, 2025
@Jenson3210 Jenson3210 added the bug Something isn't working label Oct 20, 2025
@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Oct 20, 2025
timtebeek and others added 3 commits October 20, 2025 15:06
…ViableSpacingVisitor.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
if (elements.size() > 1) {
try {
if ((loc == JRightPadded.Location.METHOD_DECLARATION_PARAMETER && style.getMethodDeclarationParameters().getAlignWhenMultiple()) ||
((loc == JRightPadded.Location.RECORD_STATE_VECTOR && style.getRecordComponents() != null && style.getRecordComponents().getAlignWhenMultiple()))) {
Copy link
Member

@timtebeek timtebeek Oct 20, 2025

Choose a reason for hiding this comment

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

style.getRecordComponents() is not marked as @Nullable; should it be? Or can we remove this guard since we seem to rely on the NoSuchMethodError below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as with the try/catch, we had NPE's, No SuchMethodErrors last time when introducing new style classes. So added like this to be safe. We should then later be able to remove the try/catch's and the if != null statements

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense for these temporary backwards compatibility null checks to exist within the relevant style class. You can always provide an implementation for getRecordComponents() which returns the default value when the underlying field is null. Then when the null check is no longer relevant there is only one place to remove it from, rather than from every call site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense in this occasion as we will introduce both the style and the default getter in a single go. It will hence be either a NoSuchMethod or a defaulted getter. Last time we also had the possibilitiy of an old (non-defaulted) getter being parent loaded.
Adapted!

…IndentsVisitor.java

Co-authored-by: Tim te Beek <tim@moderne.io>
@Jenson3210
Copy link
Contributor Author

Jenson3210 commented Oct 22, 2025

ran with cli against apache org -> no errors datatable

@sambsnyd sambsnyd merged commit 6c8a11a into main Oct 22, 2025
2 checks passed
@sambsnyd sambsnyd deleted the indentation-issue branch October 22, 2025 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants