fix(formatter): Complete is_complex_type_arguments()#17353
fix(formatter): Complete is_complex_type_arguments()#17353
is_complex_type_arguments()#17353Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
is_complex_type_arguments()
There was a problem hiding this comment.
Pull request overview
This PR completes the implementation of the is_complex_type_arguments function by adding the previously missing will_break() logic. This allows the formatter to correctly identify complex type arguments in call expressions and apply appropriate formatting.
Key Changes
- Completed the
is_complex_type_argumentsfunction by implementing thewill_break()check that was previously marked as TODO - Changed function signatures from
&Formatterto&mut Formatterto support the newinspect()method - Simplified the explicit type matching by removing
TSMappedType, which is now handled by thewill_break()logic
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/oxc_formatter/src/utils/assignment_like.rs |
Completed is_complex_type_arguments implementation with will_break() logic and updated function signatures to accept mutable Formatter references |
crates/oxc_formatter/tests/fixtures/ts/assignments/complex-type-arguments.ts |
Added test case for TSTypeReference with nested TSTypeLiteral to verify the fix |
crates/oxc_formatter/tests/fixtures/ts/assignments/complex-type-arguments.ts.snap |
Updated snapshot to include expected formatting output for the new test case |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CodSpeed Performance ReportMerging #17353 will not alter performanceComparing Summary
Footnotes
|
|
Talked with @Dunqing , I will retry this. roughly: let is_first_argument_complex = ts_type_argument_list.first().is_some_and(|first_argument| {
+ match first_argument {
+ TSType::TSTypeReference(r) => {
+ if r.type_arguments.as_ref().is_some_and(|type_args| type_args.params.len() > 1) {
+ return true;
+ }
+
+ return matches!(
+ r.type_arguments.as_ref().unwrap().params.first().unwrap(),
+ TSType::TSUnionType(_)
+ | TSType::TSIntersectionType(_)
+ | TSType::TSTypeLiteral(_)
+ // TODO: This is not part of Prettier's logic, but it makes sense to consider mapped types as
+ // complex because it is the same as type literals in terms of structure.
+ // NOTE: Once the `will_break` logic is added, this will have to be revisited.
+ | TSType::TSMappedType(_)
+ );
+ }
+ _ => {}
+ }
+
matches!(
first_argument,
TSType::TSUnionType(_)
Need refactoring though. NOTE to myself: |
|
We discussed concerns that comments might be discarded when attempting to print again, since the results of However, even after modifying the code in the @Dunqing After investigating, I understood the reason. oxc/crates/oxc_formatter/src/utils/assignment_like.rs Lines 915 to 917 in e8444bf This check runs first, so But in any case, I think it's better not to use |

Fixes #17275.
At fitst, I just added
TSTypeReferenceafterTSMappedType. However it produced another formatting difference (= fails current snapshot tests).