Skip to content

refactor(ast, regular_expression)!: converge oxc_ast::RegExpFlags and oxc_regular_expression::ast::Flags. #5592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
rzvxa wants to merge 1 commit into from
Closed

refactor(ast, regular_expression)!: converge oxc_ast::RegExpFlags and oxc_regular_expression::ast::Flags. #5592

rzvxa wants to merge 1 commit into from

Conversation

@rzvxa
Copy link
Contributor

@rzvxa rzvxa commented Sep 7, 2024 •
edited
Loading

closes #5562

@graphite-app
Copy link
Contributor

graphite-app bot commented Sep 7, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@rzvxa Graphite App
Copy link
Contributor Author

rzvxa commented Sep 7, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rzvxa and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added A-linter Area - Linter A-parser Area - Parser A-semantic Area - Semantic A-ast Area - AST A-transformer Area - Transformer / Transpiler A-prettier A-ast-tools Area - AST tools labels Sep 7, 2024
@rzvxa rzvxa force-pushed the 09-07-refactor_ast_regular_expression_converge_oxc_ast_regexpflags_and_oxc_regular_expression_ast_flags_ branch from dd1e537 to 9e157cb Compare September 7, 2024 18:54
@rzvxa rzvxa changed the title refactor(ast, regular_expression): converge oxc_ast::RegExpFlags and oxc_regular_expression::ast::Flags. refactor(ast, regular_expression)!: converge oxc_ast::RegExpFlags and oxc_regular_expression::ast::Flags. Sep 7, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 7, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #5592 will not alter performance

Comparing 09-07-refactor_ast_regular_expression_converge_oxc_ast_regexpflags_and_oxc_regular_expression_ast_flags_ (4d8d090) with main (8c9179d)

Summary

✅ 29 untouched benchmarks

@rzvxa rzvxa force-pushed the 09-07-refactor_ast_regular_expression_converge_oxc_ast_regexpflags_and_oxc_regular_expression_ast_flags_ branch from 9e157cb to 4d8d090 Compare September 7, 2024 20:47
rzvxa
rzvxa commented Sep 7, 2024
View reviewed changes
crates/oxc_regular_expression/src/ast.rs

#[cfg(feature = "serialize")]
#[wasm_bindgen::prelude::wasm_bindgen(typescript_custom_section)]
const TS_APPEND_CONTENT: &'static str = r#"
Copy link
Contributor Author

@rzvxa rzvxa Sep 7, 2024

Choose a reason for hiding this comment

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

Should I change any javascript code now that this binding is renamed?

@rzvxa rzvxa marked this pull request as ready for review September 7, 2024 20:57
leaysgur
leaysgur reviewed Sep 8, 2024
View reviewed changes
crates/oxc_regular_expression/src/options.rs
pub fn with_unicode_sets_mode(self) -> ParserOptions {
ParserOptions { unicode_mode: true, unicode_sets_mode: true, ..self }
pub fn with_flags(self, flags: RegularExpressionFlags) -> Self {
Self { flags, ..self }
Copy link
Member

@leaysgur leaysgur Sep 8, 2024

Choose a reason for hiding this comment

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

I prefer this in order to split the domain more explicit. 👀

Suggested change
Self { flags, ..self }
Self {
unicode_mode: flags.intersects(RegularExpressionFlags::U | RegularExpressionFlags::V),
unicode_sets_mode: flags.contains(RegularExpressionFlags::V),
..self
}

@leaysgur leaysgur mentioned this pull request Sep 11, 2024
Merged
@leaysgur leaysgur mentioned this pull request Oct 3, 2024
Merged
Boshen pushed a commit that referenced this pull request Oct 3, 2024
@leaysgur
refactor(regular_expression)!: Simplify public APIs (#6262)
5a73a66 
This PR makes 2 changes to improve the existing API that are not very useful.

- Remove `(Literal)Parser` and `FlagsParser` and their ASTs
- Add `with_flags(flags_text)` helper to `ParserOptions`

Here are the details.

> Remove `(Literal)Parser` and `FlagsParser` and their ASTs

Previously, the `oxc_regular_expression` crate exposed 3 parsers.

- `(Literal)Parser`: assumes `/pattern/flags` format
- `PatternParser`: assumes `pattern` part only
- `FlagsParser`: assumes `flags` part only

However, it turns out that in actual usecases, only the `PatternParser` is actually sufficient, as the pattern and flags are validated and sliced in advance on the `oxc_parser` side.

The current usecase for `(Literal)Parser` is mostly for internal testing.

There were also some misuses of `(Literal)Parser` that restore `format!("/{pattern}/{flags}")` back and use `(Literal)Parser`.

Therefore, only `PatternParser` is now published, and unnecessary ASTs have been removed.
(This also obsoletes #5592 .)

> Added `with_flags(flags_text)` helper to `ParserOptions`

Strictly speaking, there was a subtle difference between the "flag" strings that users were aware of and the "mode" recognised by the parser.

Therefore, it was a common mistake to forget to enable `unicode_mode` when using the `v` flag.

With this helper, crate users no longer need to distinguish between flags and modes.
@leaysgur
Copy link
Member

leaysgur commented Oct 3, 2024

Closing, oxc_regular_expression::ast::Flags is removed in #6262

@leaysgur leaysgur closed this Oct 3, 2024
@leaysgur leaysgur deleted the 09-07-refactor_ast_regular_expression_converge_oxc_ast_regexpflags_and_oxc_regular_expression_ast_flags_ branch October 3, 2024 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leaysgur leaysgur leaysgur left review comments

@Boshen Boshen Boshen approved these changes

@overlookmotel overlookmotel Awaiting requested review from overlookmotel overlookmotel is a code owner

Assignees

No one assigned

Labels

A-ast Area - AST A-ast-tools Area - AST tools A-linter Area - Linter A-parser Area - Parser A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use bitflags for oxc_regular_expression::ast::Flags

4 participants

@rzvxa @leaysgur @Boshen