refactor(semantic): make control flow generation optional. #3737

rzvxa · 2024-06-18T13:35:03Z

For maximum backward compatibility, we generate CFG by default.

Note: It can't be done with a simple method since lifetimes make it impossible(at least without unsafe trickery) I've tried to do it without a macro but it was just unintuitive.

graphite-app · 2024-06-18T13:35:09Z

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

Add the label “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 · 2024-06-18T13:35:14Z

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

Join @rzvxa and the rest of your teammates on Graphite

codspeed-hq · 2024-06-18T13:40:20Z

CodSpeed Performance Report

Merging #3737 will improve performances by 9.97%

_{Comparing 06-18-chore_semantic_make_control_flow_generation_optional (d8ad321) with main (5c38a0f)}

Summary

⚡ 1 improvements
✅ 21 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`06-18-chore_semantic_make_control_flow_generation_optional`	Change
⚡	`codegen_sourcemap[react.development.js]`	2.2 ms	2 ms	+9.97%

crates/oxc_semantic/src/builder.rs

Boshen · 2024-06-18T14:19:46Z

Nice! Would be nice if we get rid of the macro.

I'll make it off by default in the next PR.

rzvxa · 2024-06-18T14:22:01Z

Nice! Would be nice if we get rid of the macro.

I'll make it off by default in the next PR.

I'm working on making it disabled by default in the next few PRs, I have to first make linter tests to generate CFG optionally(so we can mark it if we need cfg for the rule to work).

Boshen · 2024-06-18T14:29:35Z

Ok, shall we merge or get rid of the macro?

rzvxa · 2024-06-18T14:32:50Z

Ok, shall we merge or get rid of the macro?

Do we want to feature gate the CFG in the future? If that's the case I think macro can help us, Otherwise, we should be fine with mapping the option.

I can also give the inline function another try, Maybe I can find a safe trick to make it work.

Boshen · 2024-06-18T14:34:33Z

Ok, let's merge and iterate.

rzvxa · 2024-06-18T14:38:37Z

Sounds good to me, I'll give the inline function another try and let you know about the results.

graphite-app · 2024-06-18T15:29:45Z

Merge activity

Jun 18, 11:29 AM EDT: Boshen added this pull request to the Graphite merge queue.
Jun 18, 11:34 AM EDT: The Graphite merge queue wasn't able to merge this pull request due to All comments must be resolved before merging.
Jun 18, 11:34 AM EDT: The Graphite merge queue removed this pull request due to downstack failures on PR #3728.
Jun 18, 11:57 AM EDT: Boshen added this pull request to the Graphite merge queue.
Jun 18, 12:05 PM EDT: Boshen merged this pull request with the Graphite merge queue.

For maximum backward compatibility, we generate CFG by default. Note: It can't be done with a simple method since lifetimes make it impossible(at least without unsafe trickery) I've tried to do it without a macro but it was just unintuitive.

overlookmotel · 2024-06-18T15:49:07Z

crates/oxc_linter/src/rules/eslint/getter_return.rs

@@ -55,16 +55,19 @@ declare_oxc_lint!(

 impl Rule for GetterReturn {
    fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
+        // control flow dependant
+        let Some(cfg) = ctx.semantic().cfg() else { return };


Should this be let cfg = ctx.semantic().cfg().unwrap()? i.e. Panic if linter has been passed a Semantic instance which doesn't have CFG.

It's not ideal to have runtime panics, but on the other hand, if there's some mistake and linter gets passed a Semantic with no CFG, then these lint rules will just silently fail to run, and user will be told "Lint successful! No problem with your code." regardless of whether that's true or not.

This is why my preference was for a Semantic<CfgType> generic type - then this would be enforced at compile time.

I'm not sure. There's arguments on both sides. Maybe a compromise would be:

debug_assert!(ctx.semantic().cfg().is_some()); let Some(cfg) = ctx.semantic().cfg() else { return };

I think this at least communicates to someone reading the code that cfg must be Some(_), and that it's not actually optional.

This was intentional, I was thinking that we can make the linter to omit CFG generation and disable the rules depending on it. Or the other way around if we have no rule dependent on CFG enabled we can disable the CFG generation altogether.
But we can go either way since it has no difference at the moment. I had no objection against unwrap/panics here.

if we have no rule dependent on CFG enabled we can disable the CFG generation altogether.

That's a nice optimization. I think that way around is the better option, in which case it could be unwrap here.

I have a broader feeling that we need a convention for communicating in code when (1) something is actually optional vs (2) there's an unstated invariant that means this branch must always go one way. When delving into parts of Oxc codebase which I'm new to, this ambiguity has left me scratching my head quite a few times, wondering "in what circumstances can this happen?", and it turns out the answer is "it can't".

unwrap/unreachable! is one way to communicate that, but it has the downside of bloating the ASM with panic code for the unreachable branch. So I don't have a good answer to this.

Anyway, it's a broader problem, so I think we can just leave it as is in this PR, since I was too late with my review.

overlookmotel · 2024-06-18T15:58:01Z

crates/oxc_semantic/src/builder.rs

@@ -31,6 +31,16 @@ use crate::{
    Semantic,
 };

+macro_rules! cfg {


Could this macro be called control_flow! rather than cfg!? Rust's standard library already has a cfg! macro, and I think it's potentially confusing to override it. https://doc.rust-lang.org/std/macro.cfg.html

Sure, I was thinking we can use std::cfg directly if needed, Named it cfg since it is a repeating pattern all over this file and it looks like the comments. I'll change it to control_flow.

Personally I find the use of "cfg" as abbreviation for "control flow graph" confusing in general. There's quite a strong convention in Rust that "cfg" is short for "config" e.g. #[cfg(...)].

When I first started looking at semantic's code, I kept asking myself "what's this 'config' all about?". It didn't take me too long to figure it out, but still I think it's useful to remember what your first encounter with the codebase felt like, to put yourself in the position of a new contributor (who of course we want to welcome). The principle of minimum surprise and all that...

That's a broader point, and not such a big deal. But on this specific one, personally I think overriding part of the standard prelude is a step too far! If you would be willing to change it, I think it'd be ideal. If you don't have time, just say so, and I'll do it.

I've already changed it in a subsequent PR.

Ha! I am too slow again! Thanks for doing that.

No worries mate! Winds are blowing fast, "Jibe Ho" ⚓

overlookmotel · 2024-06-18T16:01:53Z

crates/oxc_semantic/src/builder.rs

@@ -467,8 +484,11 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
        program.scope_id.set(Some(self.current_scope_id));

        /* cfg */


If you can be bothered, I think all these /* cfg */ comments could be removed, as the cfg! macro now performs same function of delimiting the code related to CFG. But this is just a style nit - feel free to ignore if you don't have time/don't agree.

I thought of it but didn't do it since there are a handful of instances where we have cfg related code outside of this macro, Wanted to keep it as a convention for that reason, and I'd love to remove these when it is a bit more mature and we know all of our cfg logics can be contained in these macro scopes.

overlookmotel · 2024-06-18T16:16:24Z

Oh poop. PR got merged while I was reviewing! @rzvxa I don't know if you think any of my comments above are worth considering and addressing in follow ups, or not?

rzvxa · 2024-06-18T16:28:08Z

@overlookmotel I've made some comments about the review conversations above, Let me know about your thoughts so I can integrate them in the next PR(s).

As requested here #3737 (comment)

## [0.15.0] - 2024-06-18 - 0537d29 cfg: [**BREAKING**] Move control flow to its own crate. (#3728) (rzvxa) - 5c38a0f codegen: [**BREAKING**] New code gen API (#3740) (Boshen) - 4bce59d semantic/cfg: [**BREAKING**] Re-export `petgraph` as `control_flow::graph`. (#3722) (rzvxa) - 534242a codegen: [**BREAKING**] Remove `CodegenOptions::enable_typescript` (#3674) (Boshen) - 0578ece ast: [**BREAKING**] Remove `ExportDefaultDeclarationKind::TSEnumDeclaration` (#3666) (Dunqing) ### Features - 5a99d30 codegen: Improve codegen formatting (#3735) (Boshen) - bf9b38a codegen: Improve codegen formatting (#3731) (Boshen) - 4a004e2 codegen: Print TSImport remaining fields (#3695) (Dunqing) - a56cb1b codegen: Print accessibility for MethodDefinition (#3690) (Dunqing) - 38a75e5 coverage: Improve codegen (#3729) (Boshen) - 750a534 coverage: Transformer idempotency test (#3691) (Boshen) - ee627c3 isolated-declarations: Create unique name for `_default` (#3730) (Dunqing) - 81e9526 isolated-declarations: Inferring set accessor parameter type from get accessor return type (#3725) (Dunqing) - 77d5533 isolated-declarations: Report errors that are consistent with typescript. (#3720) (Dunqing) - 8f5655d linter: Add eslint/no-useless-constructor (#3594) (Don Isaac) - 046ff3f linter/eslint: Add `no_unreachable` rule. (#3238) (rzvxa) - 0b8098a napi: Isolated-declaration (#3718) (Boshen) - 527bfc8 npm/oxc-transform: Setup npm/oxc-transform and publish (Boshen) - d65c652 parser: Display jsx mismatch error, e.g. `<Foo></Bar>` (#3696) (Boshen) - 9c31ed9 semantic/cfg: Propagate unreachable edges through subgraphs. (#3648) (rzvxa) - d9c5b33 semantic/cfg: Add `Condition` instruction. (#3567) (Ali Rezvani) - f2dfd66 semantic/cfg: Add iteration instructions. (#3566) (rzvxa) - 910193e transformer-dts: Report error for super class (#3711) (Dunqing) - 413d7be transformer-dts: Transform enum support (#3710) (Dunqing) - 35c382e transformer-dts: Remove type annotation from private field (#3689) (Dunqing) - 0e6d3ce transformer-dts: Report error for async function and generator (#3688) (Dunqing) - b22b59a transformer-dts: Transform namespace support (#3683) (Dunqing) - 4f2db46 transformer-dts: `--isolatedDeclarations` dts transform (#3664) (Dunqing) ### Bug Fixes - 2158268 ast: Incorrect visit order in function (#3681) (Dunqing) - da1e2d0 codegen: Improve typescript codegen (#3708) (Boshen) - f1b793f isolated-declarations: Function overloads reaching unreachable (#3739) (Dunqing) - 0fbecdc isolated-declarations: Should be added to references, not bindings (#3726) (Dunqing) - 8f64d99 minifier: Respect `join_vars: false` option (#3724) (mysteryven) - 70fc69b semantic: Add Eq to CtxFlags (#3651) (Yuji Sugiura) - 7a58fec semantic/cfg: Issue in unlabeled `Ctx`s. (#3678) (rzvxa) - abd6ac8 semantic/cfg: Discrete finalization path after `NewFunction`s. (#3671) (rzvxa) - e148a32 semantic/cfg: Correct unreachability propagation in try-finally. (#3667) (Ali Rezvani) - 59666e0 transformer: Do not rename accessible identifier references (#3623) (Dunqing) - 90743e2 traverse: Change visit order for `Function` (#3685) (overlookmotel) ### Performance - 2717a1a semantic/cfg: Lower the visits in `neighbors_filtered_by_edge_weight`. (#3676) (rzvxa) ### Refactor - fa7a6ba codegen: Add `gen` method to ast nodes (#3687) (Boshen) - 09b92b6 codegen: Move `gen_ts` into `gen` to make searching things easier (#3680) (Boshen) - 3c59735 isolated-declarations: Remove `TransformDtsCtx` (#3719) (Boshen) - 815260e isolated-declarations: Decouple codegen (#3715) (Boshen) - 7ec44f8 semantic: Rename `cfg` macro to `control_flow`. (#3742) (rzvxa) - d8ad321 semantic: Make control flow generation optional. (#3737) (rzvxa) - a94a72d semantic: Expose 1 checker function instead of 2 (#3694) (Boshen) - bd8d115 semantic/cfg: Remove unused types. (#3677) (rzvxa) - f702fb9 semantic/cfg: Cleanup control flow and it's builder. (#3650) (rzvxa) - 4f16664 transformer_dts: Create a `Program` for codegen (#3679) (Boshen) Co-authored-by: Boshen <Boshen@users.noreply.github.com>

## [0.5.0] - 2024-06-27 - 6796891 ast: [**BREAKING**] Rename all instances of `BigintLiteral` to `BigIntLiteral`. (#3898) (rzvxa) - ae09a97 ast: [**BREAKING**] Remove `Modifiers` from ts nodes (#3846) (Boshen) - 1af5ed3 ast: [**BREAKING**] Replace `Modifiers` with `declare` and `const` on `EnumDeclaration` (#3845) (Boshen) - ee6ec4e ast: [**BREAKING**] Replace `Modifiers` with `declare` and `abstract` on `Class` (#3841) (Boshen) - 4456034 ast: [**BREAKING**] Add `IdentifierReference` to `ExportSpecifier` (#3820) (Boshen) - 0537d29 cfg: [**BREAKING**] Move control flow to its own crate. (#3728) (rzvxa) - 5c38a0f codegen: [**BREAKING**] New code gen API (#3740) (Boshen) - 4bce59d semantic/cfg: [**BREAKING**] Re-export `petgraph` as `control_flow::graph`. (#3722) (rzvxa) ### Features - 3ae2628 linter: Change `no-import-assign` to correctness (#3928) (Boshen) - a89d501 linter: Implement @typescript-eslint/no-non-null-asserted-nulli… (#3850) (kaykdm) - fc48cb4 linter: �eslint-plugin-jest/prefer-jest-mocked (#3865) (cinchen) - 63b98bd linter: Accept multiple fixes when fix code (#3842) (mysteryven) - 328445b linter: Support `vitest/no-disabled-tests` (#3717) (mysteryven) - 8c61f9c linter: Implement @typescript-eslint/no-non-null-assertion (#3825) (kaykdm) - 080ecbd linter: Add `no-fallthrough`. (#3673) (rzvxa) - 9493fbe linter: Add `oxc/no-optional-chaining` rule (#3700) (mysteryven) - 139adfe linter: Add `@typescript-eslint/no-import-type-side_effects` (#3699) (mysteryven) - 5f84500 linter/eslint-plugin-react: Implement prefer-es6-class (#3812) (Jelle van der Waa) - fafe67c linter/import: Implement max-dependencies (#3814) (Jelle van der Waa) - d5f6aeb semantic: Check for illegal symbol modifiers (#3838) (Don Isaac) ### Bug Fixes - 4bd2c88 linter: Fix and promote `getter-return` to correctness. (#3777) (rzvxa) - 1190dee linter: False positives with setters in the `getter-return` rule. (#3714) (rzvxa) - de0690f linter: Do not run getter-return in typescript (#3693) (Boshen) - cf71c23 linter: Edge case with infinite loops. (#3672) (rzvxa) - 5902331 oxlint: Properly report error (#3889) (Luca Bruno) - 99a40ce semantic: `export default foo` should have `ExportLocalName::Default(NameSpan)` entry (#3823) (Boshen) - abd6ac8 semantic/cfg: Discrete finalization path after `NewFunction`s. (#3671) (rzvxa) ### Performance - 4f7ff7e Do not pass `&Atom` to functions (#3818) (overlookmotel) ### Refactor - 4d2b7f1 linter: `LintContext` can now only be constructed with a cfg enabled semantic. (#3761) (rzvxa) - 7302429 linter/prefer_number_properties: Remove the unused `IdentifierName` check (#3822) (Boshen) - d8ad321 semantic: Make control flow generation optional. (#3737) (rzvxa) ### Testing - 887da40 linter: Enable `no-fallthrough` test with `disable-next-line`. (#3766) (rzvxa) Co-authored-by: Boshen <Boshen@users.noreply.github.com>

rzvxa mentioned this pull request Jun 18, 2024

refactor(cfg)!: move control flow to its own crate. #3728

Merged

github-actions bot added A-linter Area - Linter A-semantic Area - Semantic labels Jun 18, 2024

rzvxa force-pushed the 06-18-chore_semantic_make_control_flow_generation_optional branch from 0b7ad53 to a282f8d Compare June 18, 2024 13:51

rzvxa changed the title ~~chore(semantic): make control flow generation optional.~~ refactor(semantic): make control flow generation optional. Jun 18, 2024

rzvxa requested a review from Boshen June 18, 2024 13:51

rzvxa marked this pull request as ready for review June 18, 2024 13:52

Boshen reviewed Jun 18, 2024

View reviewed changes

crates/oxc_semantic/src/builder.rs Show resolved Hide resolved

rzvxa mentioned this pull request Jun 18, 2024

chore(semantic)!: no longer generate CFG by default. #3738

Merged

Boshen force-pushed the 06-17-refactor_cfg_move_control_flow_to_its_own_crate branch from 3fdf236 to 61b69c3 Compare June 18, 2024 15:30

Boshen force-pushed the 06-18-chore_semantic_make_control_flow_generation_optional branch from d04848b to a4160d2 Compare June 18, 2024 15:31

rzvxa force-pushed the 06-17-refactor_cfg_move_control_flow_to_its_own_crate branch from 61b69c3 to 1ccbd3a Compare June 18, 2024 15:36

rzvxa force-pushed the 06-18-chore_semantic_make_control_flow_generation_optional branch from a4160d2 to 37adcb3 Compare June 18, 2024 15:36

rzvxa force-pushed the 06-18-chore_semantic_make_control_flow_generation_optional branch from 37adcb3 to ccb7d32 Compare June 18, 2024 15:39

rzvxa force-pushed the 06-18-chore_semantic_make_control_flow_generation_optional branch from ccb7d32 to 8bfea7b Compare June 18, 2024 15:40

Boshen force-pushed the 06-17-refactor_cfg_move_control_flow_to_its_own_crate branch from 97bcb64 to 0537d29 Compare June 18, 2024 16:00

Boshen force-pushed the 06-18-chore_semantic_make_control_flow_generation_optional branch from 8bfea7b to d8ad321 Compare June 18, 2024 16:00

Base automatically changed from 06-17-refactor_cfg_move_control_flow_to_its_own_crate to main June 18, 2024 16:04

graphite-app bot merged commit d8ad321 into main Jun 18, 2024
22 checks passed

graphite-app bot deleted the 06-18-chore_semantic_make_control_flow_generation_optional branch June 18, 2024 16:05

overlookmotel reviewed Jun 18, 2024

View reviewed changes

rzvxa mentioned this pull request Jun 18, 2024

refactor(semantic): rename cfg macro to control_flow. #3742

Merged

Boshen pushed a commit that referenced this pull request Jun 18, 2024

refactor(semantic): rename cfg macro to control_flow. (#3742)

7ec44f8

As requested here #3737 (comment)

github-actions bot mentioned this pull request Jun 18, 2024

Release crates v0.15.0 #3743

Merged

rzvxa mentioned this pull request Jun 18, 2024

feat(linter): add use_cfg attribute. #3744

Closed

This was referenced Jun 26, 2024

Release oxlint v0.5.0 #3931

Closed

Release oxlint v0.5.0 #3939

Closed

Release oxlint v0.5.0 #3940

Merged

github-actions bot mentioned this pull request Jun 27, 2024

Release oxlint v0.5.0 #3942

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(semantic): make control flow generation optional. #3737

refactor(semantic): make control flow generation optional. #3737

rzvxa commented Jun 18, 2024 •

edited

Loading

graphite-app bot commented Jun 18, 2024

rzvxa commented Jun 18, 2024 •

edited

Loading

codspeed-hq bot commented Jun 18, 2024 •

edited

Loading

Boshen commented Jun 18, 2024 •

edited

Loading

rzvxa commented Jun 18, 2024 •

edited

Loading

Boshen commented Jun 18, 2024

rzvxa commented Jun 18, 2024

Boshen commented Jun 18, 2024

rzvxa commented Jun 18, 2024

graphite-app bot commented Jun 18, 2024 •

edited

Loading

overlookmotel Jun 18, 2024

rzvxa Jun 18, 2024

overlookmotel Jun 18, 2024 •

edited

Loading

overlookmotel Jun 18, 2024

rzvxa Jun 18, 2024

overlookmotel Jun 18, 2024 •

edited

Loading

rzvxa Jun 18, 2024

overlookmotel Jun 18, 2024

rzvxa Jun 18, 2024

overlookmotel Jun 18, 2024

rzvxa Jun 18, 2024

overlookmotel commented Jun 18, 2024

rzvxa commented Jun 18, 2024

		@@ -467,8 +484,11 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
		program.scope_id.set(Some(self.current_scope_id));

		/* cfg */

refactor(semantic): make control flow generation optional. #3737

refactor(semantic): make control flow generation optional. #3737

Conversation

rzvxa commented Jun 18, 2024 • edited Loading

graphite-app bot commented Jun 18, 2024

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

rzvxa commented Jun 18, 2024 • edited Loading

codspeed-hq bot commented Jun 18, 2024 • edited Loading

Merging #3737 will improve performances by 9.97%

Summary

Benchmarks breakdown

Boshen commented Jun 18, 2024 • edited Loading

rzvxa commented Jun 18, 2024 • edited Loading

Boshen commented Jun 18, 2024

rzvxa commented Jun 18, 2024

Boshen commented Jun 18, 2024

rzvxa commented Jun 18, 2024

graphite-app bot commented Jun 18, 2024 • edited Loading

Merge activity

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overlookmotel Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overlookmotel Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

overlookmotel commented Jun 18, 2024

rzvxa commented Jun 18, 2024

rzvxa commented Jun 18, 2024 •

edited

Loading

rzvxa commented Jun 18, 2024 •

edited

Loading

codspeed-hq bot commented Jun 18, 2024 •

edited

Loading

Boshen commented Jun 18, 2024 •

edited

Loading

rzvxa commented Jun 18, 2024 •

edited

Loading

graphite-app bot commented Jun 18, 2024 •

edited

Loading

overlookmotel Jun 18, 2024 •

edited

Loading

overlookmotel Jun 18, 2024 •

edited

Loading