Skip to content

Comments

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

Closed
rzvxa wants to merge 1 commit intomainfrom
09-07-refactor_ast_regular_expression_converge_oxc_ast_regexpflags_and_oxc_regular_expression_ast_flags_
Closed

refactor(ast, regular_expression)!: converge oxc_ast::RegExpFlags and oxc_regular_expression::ast::Flags.#5592
rzvxa wants to merge 1 commit intomainfrom
09-07-refactor_ast_regular_expression_converge_oxc_ast_regexpflags_and_oxc_regular_expression_ast_flags_

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

3 participants

@rzvxa @leaysgur @Boshen