Skip to content

[BTA] Use typed copy in deepCopy() instead of string round-trip#5855

Merged
KotlinBuild merged 5 commits intomasterfrom
rr/ikvl/no_string
Apr 22, 2026
Merged

[BTA] Use typed copy in deepCopy() instead of string round-trip#5855
KotlinBuild merged 5 commits intomasterfrom
rr/ikvl/no_string

Conversation

@ywett02
Copy link
Copy Markdown
Collaborator

@ywett02 ywett02 commented Apr 13, 2026

Replace applyArgumentStrings(toArgumentStrings()) with applyCompilerArguments(toCompilerArguments()) in JvmCompilerArgumentsImpl.deepCopy().

@kodee-bot
Copy link
Copy Markdown

kodee-bot Bot commented Apr 13, 2026

Hi! It looks like there are no references to any YT issues in the commit messages.
If the change is non-functional, feel free to resolve the comment :)
Otherwise, please add them as requested in the guideline.

@kotlin-safe-merge
Copy link
Copy Markdown

kotlin-safe-merge Bot commented Apr 13, 2026

Code Owners

RuleOwnersApproval
/​compiler/​build-​tools/​
build-tools-api

@ALikhachev
/​compiler/​build-​tools/​kotlin-​build-​tools-​compat/​gen/​, /​compiler/​build-​tools/​kotlin-​build-​tools-​impl/​gen/​
kotlin

@ALikhachev

@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 13, 2026

/dry-run

@KotlinBuild
Copy link
Copy Markdown

THIS IS A DRY RUN

Quality gate is triggered at https://buildserver.labs.intellij.net/build/924729568 — use this link to get full insight.

Quality gate was triggered with the following revisions:

kotlin
Branch: refs/merge/GITHUB-5855/safe-merge
Commit: 4389b55

@KotlinBuild
Copy link
Copy Markdown

Triggered a retry attempt №1 out of 1.

@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 13, 2026

/dry-run

@KotlinBuild
Copy link
Copy Markdown

Quality gate failed. See https://buildserver.labs.intellij.net/build/924729568 to get full insight.

@KotlinBuild
Copy link
Copy Markdown

THIS IS A DRY RUN

Quality gate is triggered at https://buildserver.labs.intellij.net/build/924852109 — use this link to get full insight.

Quality gate was triggered with the following revisions:

kotlin
Branch: refs/merge/GITHUB-5855/safe-merge
Commit: 70b31ab

@KotlinBuild
Copy link
Copy Markdown

Quality gate finished successfully.

@ywett02 ywett02 force-pushed the rr/ikvl/no_string branch from 0949390 to 2f7689e Compare April 14, 2026 11:27
@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 14, 2026

/dry-run

@KotlinBuild
Copy link
Copy Markdown

THIS IS A DRY RUN

Quality gate is triggered at https://buildserver.labs.intellij.net/build/925590812 — use this link to get full insight.

Quality gate was triggered with the following revisions:

kotlin
Branch: refs/merge/GITHUB-5855/safe-merge
Commit: 693d55b

@KotlinBuild
Copy link
Copy Markdown

Triggered a retry attempt №1 out of 1.

@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 14, 2026

/dry-run

@KotlinBuild
Copy link
Copy Markdown

Quality gate failed. See https://buildserver.labs.intellij.net/build/925590812 to get full insight.

@KotlinBuild
Copy link
Copy Markdown

THIS IS A DRY RUN

Quality gate is triggered at https://buildserver.labs.intellij.net/build/925801681 — use this link to get full insight.

Quality gate was triggered with the following revisions:

kotlin
Branch: refs/merge/GITHUB-5855/safe-merge
Commit: f85b39b

@KotlinBuild
Copy link
Copy Markdown

Triggered a retry attempt №1 out of 1.

@KotlinBuild
Copy link
Copy Markdown

Quality gate failed. See https://buildserver.labs.intellij.net/build/925801681 to get full insight.

ALikhachev
ALikhachev previously approved these changes Apr 15, 2026
Copy link
Copy Markdown
Member

@ALikhachev ALikhachev left a comment

Choose a reason for hiding this comment

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

Other than the minor comment, LGTM

@ywett02 ywett02 force-pushed the rr/ikvl/no_string branch from 2a8f4e6 to 20a431c Compare April 16, 2026 12:11
@kotlin-safe-merge kotlin-safe-merge Bot dismissed ALikhachev’s stale review April 16, 2026 12:11

New changes affect files owned by this reviewer. Re-review required.

@kotlin-safe-merge kotlin-safe-merge Bot requested a review from ALikhachev April 16, 2026 12:11
@ywett02 ywett02 changed the base branch from master to rr/ikvl/KT-85414_delimiter_compatibility April 16, 2026 12:11
@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 16, 2026

/dry-run

@KotlinBuild
Copy link
Copy Markdown

THIS IS A DRY RUN

Quality gate is triggered at https://buildserver.labs.intellij.net/build/927796511 — use this link to get full insight.

Quality gate was triggered with the following revisions:

kotlin
Branch: refs/merge/GITHUB-5855/safe-merge
Commit: 20a431c

@ywett02 ywett02 force-pushed the rr/ikvl/KT-85414_delimiter_compatibility branch from 12c712e to f48f7eb Compare April 16, 2026 13:06
@KotlinBuild
Copy link
Copy Markdown

Quality gate finished successfully.

Base automatically changed from rr/ikvl/KT-85414_delimiter_compatibility to master April 16, 2026 17:08
@ywett02 ywett02 force-pushed the rr/ikvl/no_string branch from 20a431c to a15a274 Compare April 17, 2026 10:49
@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 17, 2026

/dry-run

@KotlinBuild
Copy link
Copy Markdown

THIS IS A DRY RUN

Quality gate is triggered at https://buildserver.labs.intellij.net/build/928672496 — use this link to get full insight.

Quality gate was triggered with the following revisions:

kotlin
Branch: refs/merge/GITHUB-5855/safe-merge
Commit: 66c7cff

@KotlinBuild
Copy link
Copy Markdown

Quality gate finished successfully.

@ywett02 ywett02 force-pushed the rr/ikvl/no_string branch from a15a274 to 6232776 Compare April 21, 2026 13:53
@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 21, 2026

/dry-run

@KotlinBuild
Copy link
Copy Markdown

THIS IS A DRY RUN

Quality gate is triggered at https://buildserver.labs.intellij.net/build/931568938 — use this link to get full insight.

Quality gate was triggered with the following revisions:

kotlin
Branch: refs/merge/GITHUB-5855/safe-merge
Commit: 25f053c

@KotlinBuild
Copy link
Copy Markdown

Triggered a retry attempt №1 out of 1.

Comment on lines +74 to +76
return JvmCompilationOperationWrapper(
base.build().also { it.compilerArguments.applyCompilerArguments(base.compilerArguments.toCompilerArguments()) })
}
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.

Could you clarify what this is needed for?

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.

What wasn't covered by the previous implementation calling deepCopy?

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.

Also, I see this change for Pre2_3_20 and Pre_2_4_20, but not Pre_2_4_0:

. Is that okay?

Copy link
Copy Markdown
Collaborator Author

@ywett02 ywett02 Apr 22, 2026

Choose a reason for hiding this comment

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

Could you clarify what this is needed for? & What wasn't covered by the previous implementation calling deepCopy?

This change is for backward compatibility with the current change.

Let's have -xfriend-paths=home/directory/pathwith,comma as an example.

Without a backward compatibility wrapper, you get different results based on the IMPL version after calling build on the JVM operation:

2.4.20(api)&2.4.20(impl)=>"User/home/directory/pathwith,comma"
2.4.20(api)&<2.4.20(impl)=>""User/home/directory/pathwith", "User/comma""

The implementation in the 2_4_20 wrapper ensures this behavior is correct across versions, as demonstrated by passing tests in: #5879.

In theory, if we are okay with the behavior being different across IMPL versions, it can be removed. However, such implementation would require forking tests since they would start failing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Also, I see this change for Pre2_3_20 and Pre_2_4_20, but not Pre_2_4_0. Is that okay?

2_4_20 traverses the tree of wrappers and tries to invoke certain functions at the closest parent. To make it work, it was necessary so that every wrapped object is called base. 2_4_0 already uses base. 2_3_20 was using baseCompilerArguments.

@KotlinBuild
Copy link
Copy Markdown

Quality gate failed. See https://buildserver.labs.intellij.net/build/931568938 to get full insight.

@ywett02 ywett02 force-pushed the rr/ikvl/no_string branch from 6232776 to 694b892 Compare April 22, 2026 07:36
@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 22, 2026

/dry-run

@KotlinBuild
Copy link
Copy Markdown

THIS IS A DRY RUN

Quality gate is triggered at https://buildserver.labs.intellij.net/build/932117259 — use this link to get full insight.

Quality gate was triggered with the following revisions:

kotlin
Branch: refs/merge/GITHUB-5855/safe-merge
Commit: e27d2d6

@ywett02 ywett02 force-pushed the rr/ikvl/no_string branch from 694b892 to 427c5dd Compare April 22, 2026 07:47
@KotlinBuild
Copy link
Copy Markdown

Triggered a retry attempt №1 out of 1.

@KotlinBuild
Copy link
Copy Markdown

Quality gate failed. See https://buildserver.labs.intellij.net/build/932117259 to get full insight.

@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 22, 2026

@ALikhachev, the last two commits are from #5878 (review).

@ywett02 ywett02 force-pushed the rr/ikvl/no_string branch from 6e4e6cd to 0f2b4eb Compare April 22, 2026 09:21
@ywett02
Copy link
Copy Markdown
Collaborator Author

ywett02 commented Apr 22, 2026

/safe-merge

@KotlinBuild
Copy link
Copy Markdown

Quality gate is triggered at https://buildserver.labs.intellij.net/build/932675858 — use this link to get full insight.

Quality gate was triggered with the following revisions:

kotlin
Branch: refs/merge/GITHUB-5855/safe-merge
Commit: ba7c4d6

@KotlinBuild
Copy link
Copy Markdown

Quality gate finished successfully.

@KotlinBuild KotlinBuild merged commit f5183ae into master Apr 22, 2026
1 check passed
@KotlinBuild KotlinBuild deleted the rr/ikvl/no_string branch April 22, 2026 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants