refactor(ast)!: remove TSMappedTypeModifierOperator::None variant#10749
Conversation
github-actions
bot
added
A-parser
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. |
CodSpeed Instrumentation Performance ReportMerging #10749 will not alter performanceComparing Summary
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes revise the representation of the Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
crates/oxc_ast/src/generated/ast_builder.rsis excluded by!**/generated/**crates/oxc_ast/src/generated/derive_estree.rsis excluded by!**/generated/**crates/oxc_traverse/src/generated/ancestor.rsis excluded by!**/generated/**napi/parser/generated/deserialize/js.jsis excluded by!**/generated/**napi/parser/generated/deserialize/ts.jsis excluded by!**/generated/**
📒 Files selected for processing (5)
crates/oxc_ast/src/ast/ts.rs(1 hunks)crates/oxc_codegen/src/gen.rs(2 hunks)crates/oxc_formatter/src/write/mod.rs(2 hunks)crates/oxc_parser/src/ts/types.rs(2 hunks)npm/oxc-types/types.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
crates/oxc_formatter/src/write/mod.rs (2)
npm/oxc-types/types.d.ts (1)
TSMappedTypeModifierOperator(1384-1384)crates/oxc_formatter/src/formatter/builders.rs (1)
space(592-594)
crates/oxc_ast/src/ast/ts.rs (1)
npm/oxc-types/types.d.ts (1)
TSMappedTypeModifierOperator(1384-1384)
crates/oxc_codegen/src/gen.rs (2)
npm/oxc-types/types.d.ts (1)
TSMappedTypeModifierOperator(1384-1384)crates/oxc_transformer/src/es2018/object_rest_spread.rs (1)
p(937-941)
🪛 GitHub Actions: CI
crates/oxc_formatter/src/write/mod.rs
[error] 2908-2913: Rust compiler error E0308: mismatched types in match arms returning write! macro calls which return Result<(), FormatError>, but the surrounding code expects (). Suggested fixes include returning the Result or unwrapping it with expect().
[error] 2909-2911: Repeated Rust compiler error E0308: mismatched types in match arms with write! macro calls returning Result<(), FormatError> instead of (). Suggested fixes include returning the Result or unwrapping it with expect().
[error] 2911-2911: Repeated Rust compiler error E0308: mismatched types in match arm with write! macro call returning Result<(), FormatError> instead of (). Suggested fixes include returning the Result or unwrapping it with expect().
🔇 Additional comments (9)
crates/oxc_ast/src/ast/ts.rs (2)
1448-1448: Improved type representation for optional fields.The change from
TSMappedTypeModifierOperatortoOption<TSMappedTypeModifierOperator>foroptionalandreadonlyfields is a good refactoring. This follows Rust's idiomatic pattern of usingOption::Noneto represent absence rather than having a specialNonevariant in an enum.Also applies to: 1458-1458
1462-1475: Streamlined enum by removing redundantNonevariant.Removing the
Nonevariant fromTSMappedTypeModifierOperatorand usingOption::Noneinstead is a cleaner approach. This change doesn't affect the memory footprint since Rust's niche optimization ensuresOption<TSMappedTypeModifierOperator>remains a 1-byte type.crates/oxc_formatter/src/write/mod.rs (1)
2931-2935:⚠️ Potential issueFix the same type mismatch in
optionalmatch arms.The match arms for
optionalhave the same issue - they're returningResult<(), FormatError>but the code expects().Apply this fix to properly handle the return types:
match self.optional { - Some(TSMappedTypeModifierOperator::True) => write!(f, "?")?, - Some(TSMappedTypeModifierOperator::Plus) => write!(f, "+?")?, - Some(TSMappedTypeModifierOperator::Minus) => write!(f, "-?")?, - None => {} + Some(TSMappedTypeModifierOperator::True) => write!(f, "?")?, + Some(TSMappedTypeModifierOperator::Plus) => write!(f, "+?")?, + Some(TSMappedTypeModifierOperator::Minus) => write!(f, "-?")?, + None => {} }Note that while the diff appears identical (the
?operators were already present), the context is that these need to be fixed together with the changes to thereadonlymatch arms above.Likely an incorrect or invalid review comment.
npm/oxc-types/types.d.ts (2)
1378-1379: Proper implementation of the Option patternThe change to allow
nullvalues for theoptionalandreadonlyproperties aligns with the Rust-side change from using aNoneenum variant to usingOption<TSMappedTypeModifierOperator>. This is a more idiomatic approach in Rust for representing optional values.
1384-1384: Correctly updated type definitionThe
TSMappedTypeModifierOperatortype has been properly updated to excludenull, as the absence of a value is now represented bynullat the property level rather than as a variant within the enum itself.crates/oxc_codegen/src/gen.rs (2)
3170-3174: Correctly updated pattern matching for thereadonlyfield.The code now properly matches on
Option<TSMappedTypeModifierOperator>instead of directly onTSMappedTypeModifierOperator, aligning with the removal of theNonevariant from the enum. This follows Rust's convention of usingOption::Noneto represent absence of a value.
3191-3195: Correctly updated pattern matching for theoptionalfield.The code now properly matches on
Option<TSMappedTypeModifierOperator>instead of directly onTSMappedTypeModifierOperator, consistent with the change made for thereadonlyfield. This ensures both fields use the same pattern for handling the absence of modifiers.crates/oxc_parser/src/ts/types.rs (2)
559-566: Approve the refactoring of thereadonlyfieldThe code changes correctly initialize
readonlyasNoneand set appropriateSome(...)variants based on the token presence, aligning with Rust's idiomatic use ofOptionto represent absence of a value.
592-605: Approve the refactoring of theoptionalfieldSimilar to the
readonlyfield refactoring, theoptionalfield is now properly handled as anOption<TSMappedTypeModifierOperator>instead of using a dedicatedNonevariant in the enum. This change is consistent with the PR's objective and follows Rust conventions.
overlookmotel
force-pushed
the
05-01-refactor_ast_remove_tsmappedtypemodifieroperator_none_variant
branch
from
e9c53ba to
4e64155
Compare
overlookmotel
changed the base branch from
graphite-base/10749
to
05-01-fix_formatter_do_not_ignore_result_of_write_
Merge activity
|
Merge activity
|
graphite-app
bot
force-pushed
the
05-01-fix_formatter_do_not_ignore_result_of_write_
branch
from
c7c4b79 to
e5a24c6
Compare
…10749) It seems odd for `TSMappedTypeModifierOperator` to have a `None` variant. It's more conventional to represent "doesn't exist" as `Option::None`. Remove the `TSMappedTypeModifierOperator::None` variant, and use `Option<TSMappedTypeModifierOperator>` instead of `TSMappedTypeModifierOperator`. Both are the same to the compiler due to the niche optimization on `Option` (`Option<TSMappedTypeModifierOperator>` is 1 byte), but it just seems more logical to me this way. Also an unrelated change: Shorten code in codegen and formatter.
graphite-app
bot
force-pushed
the
05-01-refactor_ast_remove_tsmappedtypemodifieroperator_none_variant
branch
from
4e64155 to
fe3fc85
Compare
…10749) It seems odd for `TSMappedTypeModifierOperator` to have a `None` variant. It's more conventional to represent "doesn't exist" as `Option::None`. Remove the `TSMappedTypeModifierOperator::None` variant, and use `Option<TSMappedTypeModifierOperator>` instead of `TSMappedTypeModifierOperator`. Both are the same to the compiler due to the niche optimization on `Option` (`Option<TSMappedTypeModifierOperator>` is 1 byte), but it just seems more logical to me this way. Also an unrelated change: Shorten code in codegen and formatter.
graphite-app
bot
force-pushed
the
05-01-fix_formatter_do_not_ignore_result_of_write_
branch
from
e5a24c6 to
38b8184
Compare
graphite-app
bot
force-pushed
the
05-01-refactor_ast_remove_tsmappedtypemodifieroperator_none_variant
branch
from
fe3fc85 to
28ceb90
Compare
graphite-app
bot
deleted the
05-01-refactor_ast_remove_tsmappedtypemodifieroperator_none_variant
branch
2 participants
It seems odd for
TSMappedTypeModifierOperatorto have aNonevariant. It's more conventional to represent "doesn't exist" asOption::None.Remove the
TSMappedTypeModifierOperator::Nonevariant, and useOption<TSMappedTypeModifierOperator>instead ofTSMappedTypeModifierOperator.Both are the same to the compiler due to the niche optimization on
Option(Option<TSMappedTypeModifierOperator>is 1 byte), but it just seems more logical to me this way.Also an unrelated change: Shorten code in codegen and formatter.