Skip to content

Support name-based destructuring declarations#630

Closed
goutamadwant wants to merge 4 commits into
facebook:mainfrom
goutamadwant:fix/ktfmt-629-name-destructuring
Closed

Support name-based destructuring declarations#630
goutamadwant wants to merge 4 commits into
facebook:mainfrom
goutamadwant:fix/ktfmt-629-name-destructuring

Conversation

@goutamadwant

Copy link
Copy Markdown
Contributor

Summary

  • Preserve square-bracket destructuring delimiters instead of always emitting parentheses.
  • Format name-based destructuring entries with renames such as x = b.
  • Add regression coverage for bracket destructuring and rename spacing.

Fixes #629

Tests

  • ./gradlew :ktfmt:test --tests "com.facebook.ktfmt.format.FormatterTest.handle name based destructuring declaration" --no-daemon --console=plain --stacktrace
  • ./gradlew :ktfmt:test --tests "com.facebook.ktfmt.format.FormatterTest" --no-daemon --console=plain --stacktrace
  • ./gradlew :ktfmt:build --no-daemon --console=plain --stacktrace

Note: I also tried ./gradlew build --no-daemon --console=plain --stacktrace; the local run reached ktfmt checks, then failed in unrelated :lambda:test dependency resolution because local Maven is missing io.netty:netty-transport-native-epoll:4.1.42.Final:linux-x86_64.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 18, 2026
@hick209

hick209 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

This is terrific! Thank you for your contribution here. Don't forget to update the CHANGELOG.md.

I think we will still need to handle the case of (val mail = email, val name = username) = user, right?
But this PR as is is already an improvement, so I'd take as it is, just update the CHANGELOG.md.

@goutamadwant

Copy link
Copy Markdown
Contributor Author

Thanks @hick209 updated CHANGELOG.md

@hick209

hick209 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

That was fast!

@meta-codesync

meta-codesync Bot commented Jun 18, 2026

Copy link
Copy Markdown

@hick209 has imported this pull request. If you are a Meta employee, you can view this in D108969079.

@goutamadwant

Copy link
Copy Markdown
Contributor Author

@hick209 I see Facebook Internal - Linter and Facebook Internal - Builds & Tests are failing behind internal links that I cannot access.
are these actionable for this PR? I can follow up if there is a code change/fix needed on my side.

@cortinico cortinico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but minor comment

Comment thread core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt Outdated
@hick209

hick209 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

@goutamadwant we got this error here

core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt:2261:38: error: unresolved reference 'hasSquareBrackets'.
        if (destructuringDeclaration.hasSquareBrackets()) "[" to "]" else "(" to ")"
                                     ^^^^^^^^^^^^^^^^^

Internally we are still on Kotlin 2.2, which might be the reason for the issue. Do you know if there's an alternative here? Else we could work to make this backwards compatible for a while

…itor.kt

Co-authored-by: Nicola Corti <corti.nico@gmail.com>
@goutamadwant

Copy link
Copy Markdown
Contributor Author

Thanks @hick209 yes.. I think the compatibility path is to avoid hasSquareBrackets() and use lPar / rPar text instead.

From what I checked, getLPar() / getRPar() exist in Kotlin 2.2, and with Kotlin 2.3 they still resolve the bracket delimiters for this syntax. I can also guard the new regression test with KotlinVersion.CURRENT < KotlinVersion(2, 3) so internal Kotlin 2.2 builds do not try to parse the newer syntax.

I can push this follow up if this sounds good to you.

@hick209

hick209 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Yep, sounds good!

@goutamadwant

Copy link
Copy Markdown
Contributor Author

addressed the issue @hick209.. it should compile against Kotlin 2.2 while still preserving bracket delimiters on Kotlin 2.3. Let me know if it looks good now. Thanks!

.trimMargin()
)
fun `handle name based destructuring declaration`() {
if (KotlinVersion.CURRENT < KotlinVersion(2, 3)) return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this check here necessary?
I feel like it would pass regardless

@meta-codesync meta-codesync Bot closed this in dcb5f0e Jun 18, 2026
@meta-codesync meta-codesync Bot added the Merged label Jun 18, 2026
@meta-codesync

meta-codesync Bot commented Jun 18, 2026

Copy link
Copy Markdown

@hick209 merged this pull request in dcb5f0e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing support for name-based destructuring

3 participants