-
-
Notifications
You must be signed in to change notification settings - Fork 538
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.
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
feat(linter/eslint): add no_unreachable
rule.
#3238
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd 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. |
57f6ca1
to
94c3001
Compare
CodSpeed Performance ReportMerging #3238 will not alter performanceComparing Summary
|
94c3001
to
ece1ff3
Compare
4b0951e
to
a4f9ee3
Compare
658e5d5
to
ae657bc
Compare
a4f9ee3
to
ba99160
Compare
ae657bc
to
857840a
Compare
ba99160
to
842eccc
Compare
6bb0206
to
9bdf934
Compare
306c912
to
3397176
Compare
ca3307c
to
0478a0b
Compare
5faf0e9
to
0cf1b04
Compare
0478a0b
to
298791a
Compare
0cf1b04
to
2efdc8b
Compare
Merge activity
|
298791a
to
79a71f5
Compare
closes #621 [no-unreachable](https://github.com/eslint/eslint/blob/069aa680c78b8516b9a1b568519f1d01e74fb2a2/lib/rules/no-unreachable.js#L196) [oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9406195143/job/25909079029) This rule is done but since it is running for every possible statement and does quite a bit of work on them to determine whether it is 100% reachable or not; The performance in my opinion is kind of abysmal. I'll try to work it out, I know Biome does 2 types of checks to simplify the rule for some nodes, However, they have a lot more false negatives than our implementation. ##### Here is one example of those [false negatives](https://biomejs.dev/playground/?code=ZgB1AG4AYwB0AGkAbwBuACAAeAAoACkAIAB7ACAAZABvACAAewAgAGEAKAApADsAIAB9ACAAdwBoAGkAbABlACgAdAByAHUAZQApADsAIABiACgAKQA7ACAAfQA%3D) ------------- ### Update 1: I've benchmarked this rule using only the simplified reachability checks and it was around 5% faster, To be honest, it isn't much improvement especially considering that we can only use this check for a small portion of nodes and even that is accompanied by newly introduced checks which would lessen the amount of performance gain further. Most of the performance regression is because of allocations during our depth first search since we have to store both the visited and finished nodes which results in a bunch of rapid-fire allocations back to back. Currently, At the moment I don't have a great idea of how to improve it, We may have to implement our own graph to use arenas underneath. Given that this rule is the most extensive use case of control flow (It doesn't come with a limited scope similar to property and constructor rules already implemented) this performance drop might be reasonable to some extent. ------------ ### Update 2: I reworked my approach in 2 senses, First I used @Boshen's suggestion inspired by TypeScript and kept some of the reachability information in the basic block structure instead of calculating it on the fly. It is done by propagating the `Unreachable` edge and `Unreachable` instruction throughout subgraphs. This for sure helped with the performance but the next part is what never failed to amaze me, Going from something near `O(n!)` in the worst-case scenario to `O(n^2)` (in the worst-case scenario). By changing the approach instead of checking the reachability of each statement we do it in 3 paths; First, we do a path on the entire CFG and query all reachable but suspicious cases, and then we do another path on each of these suspicions subgraphs to determine the reachability with higher confidence. Finally, we iterate all of the appropriate nodes and check their reachability status according to the information collected in 2 previous paths. With these 2 this rule went from `-24%` to `~-2%`. This performance gain doesn't come for free though; It increases the likelihood of false positives/negatives, But as long as we are passing our `ecosystem-ci` it should be fine. We can always sacrifice some performance to check for edge cases if there are any. [new oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9490791181)
2efdc8b
to
707d42e
Compare
79a71f5
to
ddab23c
Compare
closes #621 [no-unreachable](https://github.com/eslint/eslint/blob/069aa680c78b8516b9a1b568519f1d01e74fb2a2/lib/rules/no-unreachable.js#L196) [oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9406195143/job/25909079029) This rule is done but since it is running for every possible statement and does quite a bit of work on them to determine whether it is 100% reachable or not; The performance in my opinion is kind of abysmal. I'll try to work it out, I know Biome does 2 types of checks to simplify the rule for some nodes, However, they have a lot more false negatives than our implementation. ##### Here is one example of those [false negatives](https://biomejs.dev/playground/?code=ZgB1AG4AYwB0AGkAbwBuACAAeAAoACkAIAB7ACAAZABvACAAewAgAGEAKAApADsAIAB9ACAAdwBoAGkAbABlACgAdAByAHUAZQApADsAIABiACgAKQA7ACAAfQA%3D) ------------- ### Update 1: I've benchmarked this rule using only the simplified reachability checks and it was around 5% faster, To be honest, it isn't much improvement especially considering that we can only use this check for a small portion of nodes and even that is accompanied by newly introduced checks which would lessen the amount of performance gain further. Most of the performance regression is because of allocations during our depth first search since we have to store both the visited and finished nodes which results in a bunch of rapid-fire allocations back to back. Currently, At the moment I don't have a great idea of how to improve it, We may have to implement our own graph to use arenas underneath. Given that this rule is the most extensive use case of control flow (It doesn't come with a limited scope similar to property and constructor rules already implemented) this performance drop might be reasonable to some extent. ------------ ### Update 2: I reworked my approach in 2 senses, First I used @Boshen's suggestion inspired by TypeScript and kept some of the reachability information in the basic block structure instead of calculating it on the fly. It is done by propagating the `Unreachable` edge and `Unreachable` instruction throughout subgraphs. This for sure helped with the performance but the next part is what never failed to amaze me, Going from something near `O(n!)` in the worst-case scenario to `O(n^2)` (in the worst-case scenario). By changing the approach instead of checking the reachability of each statement we do it in 3 paths; First, we do a path on the entire CFG and query all reachable but suspicious cases, and then we do another path on each of these suspicions subgraphs to determine the reachability with higher confidence. Finally, we iterate all of the appropriate nodes and check their reachability status according to the information collected in 2 previous paths. With these 2 this rule went from `-24%` to `~-2%`. This performance gain doesn't come for free though; It increases the likelihood of false positives/negatives, But as long as we are passing our `ecosystem-ci` it should be fine. We can always sacrifice some performance to check for edge cases if there are any. [new oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9490791181)
707d42e
to
c97a3b4
Compare
ddab23c
to
94828e4
Compare
closes #621 [no-unreachable](https://github.com/eslint/eslint/blob/069aa680c78b8516b9a1b568519f1d01e74fb2a2/lib/rules/no-unreachable.js#L196) [oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9406195143/job/25909079029) This rule is done but since it is running for every possible statement and does quite a bit of work on them to determine whether it is 100% reachable or not; The performance in my opinion is kind of abysmal. I'll try to work it out, I know Biome does 2 types of checks to simplify the rule for some nodes, However, they have a lot more false negatives than our implementation. ##### Here is one example of those [false negatives](https://biomejs.dev/playground/?code=ZgB1AG4AYwB0AGkAbwBuACAAeAAoACkAIAB7ACAAZABvACAAewAgAGEAKAApADsAIAB9ACAAdwBoAGkAbABlACgAdAByAHUAZQApADsAIABiACgAKQA7ACAAfQA%3D) ------------- ### Update 1: I've benchmarked this rule using only the simplified reachability checks and it was around 5% faster, To be honest, it isn't much improvement especially considering that we can only use this check for a small portion of nodes and even that is accompanied by newly introduced checks which would lessen the amount of performance gain further. Most of the performance regression is because of allocations during our depth first search since we have to store both the visited and finished nodes which results in a bunch of rapid-fire allocations back to back. Currently, At the moment I don't have a great idea of how to improve it, We may have to implement our own graph to use arenas underneath. Given that this rule is the most extensive use case of control flow (It doesn't come with a limited scope similar to property and constructor rules already implemented) this performance drop might be reasonable to some extent. ------------ ### Update 2: I reworked my approach in 2 senses, First I used @Boshen's suggestion inspired by TypeScript and kept some of the reachability information in the basic block structure instead of calculating it on the fly. It is done by propagating the `Unreachable` edge and `Unreachable` instruction throughout subgraphs. This for sure helped with the performance but the next part is what never failed to amaze me, Going from something near `O(n!)` in the worst-case scenario to `O(n^2)` (in the worst-case scenario). By changing the approach instead of checking the reachability of each statement we do it in 3 paths; First, we do a path on the entire CFG and query all reachable but suspicious cases, and then we do another path on each of these suspicions subgraphs to determine the reachability with higher confidence. Finally, we iterate all of the appropriate nodes and check their reachability status according to the information collected in 2 previous paths. With these 2 this rule went from `-24%` to `~-2%`. This performance gain doesn't come for free though; It increases the likelihood of false positives/negatives, But as long as we are passing our `ecosystem-ci` it should be fine. We can always sacrifice some performance to check for edge cases if there are any. [new oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9490791181)
c97a3b4
to
433e053
Compare
closes #621 [no-unreachable](https://github.com/eslint/eslint/blob/069aa680c78b8516b9a1b568519f1d01e74fb2a2/lib/rules/no-unreachable.js#L196) [oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9406195143/job/25909079029) This rule is done but since it is running for every possible statement and does quite a bit of work on them to determine whether it is 100% reachable or not; The performance in my opinion is kind of abysmal. I'll try to work it out, I know Biome does 2 types of checks to simplify the rule for some nodes, However, they have a lot more false negatives than our implementation. ##### Here is one example of those [false negatives](https://biomejs.dev/playground/?code=ZgB1AG4AYwB0AGkAbwBuACAAeAAoACkAIAB7ACAAZABvACAAewAgAGEAKAApADsAIAB9ACAAdwBoAGkAbABlACgAdAByAHUAZQApADsAIABiACgAKQA7ACAAfQA%3D) ------------- ### Update 1: I've benchmarked this rule using only the simplified reachability checks and it was around 5% faster, To be honest, it isn't much improvement especially considering that we can only use this check for a small portion of nodes and even that is accompanied by newly introduced checks which would lessen the amount of performance gain further. Most of the performance regression is because of allocations during our depth first search since we have to store both the visited and finished nodes which results in a bunch of rapid-fire allocations back to back. Currently, At the moment I don't have a great idea of how to improve it, We may have to implement our own graph to use arenas underneath. Given that this rule is the most extensive use case of control flow (It doesn't come with a limited scope similar to property and constructor rules already implemented) this performance drop might be reasonable to some extent. ------------ ### Update 2: I reworked my approach in 2 senses, First I used @Boshen's suggestion inspired by TypeScript and kept some of the reachability information in the basic block structure instead of calculating it on the fly. It is done by propagating the `Unreachable` edge and `Unreachable` instruction throughout subgraphs. This for sure helped with the performance but the next part is what never failed to amaze me, Going from something near `O(n!)` in the worst-case scenario to `O(n^2)` (in the worst-case scenario). By changing the approach instead of checking the reachability of each statement we do it in 3 paths; First, we do a path on the entire CFG and query all reachable but suspicious cases, and then we do another path on each of these suspicions subgraphs to determine the reachability with higher confidence. Finally, we iterate all of the appropriate nodes and check their reachability status according to the information collected in 2 previous paths. With these 2 this rule went from `-24%` to `~-2%`. This performance gain doesn't come for free though; It increases the likelihood of false positives/negatives, But as long as we are passing our `ecosystem-ci` it should be fine. We can always sacrifice some performance to check for edge cases if there are any. [new oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9490791181)
closes #621 [no-unreachable](https://github.com/eslint/eslint/blob/069aa680c78b8516b9a1b568519f1d01e74fb2a2/lib/rules/no-unreachable.js#L196) [oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9406195143/job/25909079029) This rule is done but since it is running for every possible statement and does quite a bit of work on them to determine whether it is 100% reachable or not; The performance in my opinion is kind of abysmal. I'll try to work it out, I know Biome does 2 types of checks to simplify the rule for some nodes, However, they have a lot more false negatives than our implementation. ##### Here is one example of those [false negatives](https://biomejs.dev/playground/?code=ZgB1AG4AYwB0AGkAbwBuACAAeAAoACkAIAB7ACAAZABvACAAewAgAGEAKAApADsAIAB9ACAAdwBoAGkAbABlACgAdAByAHUAZQApADsAIABiACgAKQA7ACAAfQA%3D) ------------- ### Update 1: I've benchmarked this rule using only the simplified reachability checks and it was around 5% faster, To be honest, it isn't much improvement especially considering that we can only use this check for a small portion of nodes and even that is accompanied by newly introduced checks which would lessen the amount of performance gain further. Most of the performance regression is because of allocations during our depth first search since we have to store both the visited and finished nodes which results in a bunch of rapid-fire allocations back to back. Currently, At the moment I don't have a great idea of how to improve it, We may have to implement our own graph to use arenas underneath. Given that this rule is the most extensive use case of control flow (It doesn't come with a limited scope similar to property and constructor rules already implemented) this performance drop might be reasonable to some extent. ------------ ### Update 2: I reworked my approach in 2 senses, First I used @Boshen's suggestion inspired by TypeScript and kept some of the reachability information in the basic block structure instead of calculating it on the fly. It is done by propagating the `Unreachable` edge and `Unreachable` instruction throughout subgraphs. This for sure helped with the performance but the next part is what never failed to amaze me, Going from something near `O(n!)` in the worst-case scenario to `O(n^2)` (in the worst-case scenario). By changing the approach instead of checking the reachability of each statement we do it in 3 paths; First, we do a path on the entire CFG and query all reachable but suspicious cases, and then we do another path on each of these suspicions subgraphs to determine the reachability with higher confidence. Finally, we iterate all of the appropriate nodes and check their reachability status according to the information collected in 2 previous paths. With these 2 this rule went from `-24%` to `~-2%`. This performance gain doesn't come for free though; It increases the likelihood of false positives/negatives, But as long as we are passing our `ecosystem-ci` it should be fine. We can always sacrifice some performance to check for edge cases if there are any. [new oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9490791181)
433e053
to
9cc5fb5
Compare
closes #621 [no-unreachable](https://github.com/eslint/eslint/blob/069aa680c78b8516b9a1b568519f1d01e74fb2a2/lib/rules/no-unreachable.js#L196) [oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9406195143/job/25909079029) This rule is done but since it is running for every possible statement and does quite a bit of work on them to determine whether it is 100% reachable or not; The performance in my opinion is kind of abysmal. I'll try to work it out, I know Biome does 2 types of checks to simplify the rule for some nodes, However, they have a lot more false negatives than our implementation. ##### Here is one example of those [false negatives](https://biomejs.dev/playground/?code=ZgB1AG4AYwB0AGkAbwBuACAAeAAoACkAIAB7ACAAZABvACAAewAgAGEAKAApADsAIAB9ACAAdwBoAGkAbABlACgAdAByAHUAZQApADsAIABiACgAKQA7ACAAfQA%3D) ------------- ### Update 1: I've benchmarked this rule using only the simplified reachability checks and it was around 5% faster, To be honest, it isn't much improvement especially considering that we can only use this check for a small portion of nodes and even that is accompanied by newly introduced checks which would lessen the amount of performance gain further. Most of the performance regression is because of allocations during our depth first search since we have to store both the visited and finished nodes which results in a bunch of rapid-fire allocations back to back. Currently, At the moment I don't have a great idea of how to improve it, We may have to implement our own graph to use arenas underneath. Given that this rule is the most extensive use case of control flow (It doesn't come with a limited scope similar to property and constructor rules already implemented) this performance drop might be reasonable to some extent. ------------ ### Update 2: I reworked my approach in 2 senses, First I used @Boshen's suggestion inspired by TypeScript and kept some of the reachability information in the basic block structure instead of calculating it on the fly. It is done by propagating the `Unreachable` edge and `Unreachable` instruction throughout subgraphs. This for sure helped with the performance but the next part is what never failed to amaze me, Going from something near `O(n!)` in the worst-case scenario to `O(n^2)` (in the worst-case scenario). By changing the approach instead of checking the reachability of each statement we do it in 3 paths; First, we do a path on the entire CFG and query all reachable but suspicious cases, and then we do another path on each of these suspicions subgraphs to determine the reachability with higher confidence. Finally, we iterate all of the appropriate nodes and check their reachability status according to the information collected in 2 previous paths. With these 2 this rule went from `-24%` to `~-2%`. This performance gain doesn't come for free though; It increases the likelihood of false positives/negatives, But as long as we are passing our `ecosystem-ci` it should be fine. We can always sacrifice some performance to check for edge cases if there are any. [new oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9490791181)
## [0.4.4] - 2024-06-14 ### Features - 8f5655d linter: Add eslint/no-useless-constructor (#3594) (Don Isaac) - 29c78db linter: Implement @typescript-eslint/explicit-function-return-type (#3455) (kaykdm) - 21d3425 linter: Typescript-eslint no-useless-empty-export (#3605) (keita hino) - 85c3b83 linter: Eslint-plugin-jest/max-nested-describes (#3585) (cinchen) - f6d9ca6 linter: Add `eslint/sort-imports` rule (#3568) (Wang Wenzhe) - 046ff3f linter/eslint: Add `no_unreachable` rule. (#3238) (rzvxa) - e32ce00 linter/jsdoc: Implement require-param-name rule (#3636) (Yuji Sugiura) - 110661c linter/jsdoc: Implement require-param-description (#3621) (Yuji Sugiura) - d6370f1 linter/jsdoc: Implement require-param-type rule (#3601) (Yuji Sugiura) - d9c5b33 semantic/cfg: Add `Condition` instruction. (#3567) (Ali Rezvani) - f2dfd66 semantic/cfg: Add iteration instructions. (#3566) (rzvxa) ### Bug Fixes - f0b689d linter: Panic in jsdoc/require-param (#3590) (Don Isaac) - e148a32 semantic/cfg: Correct unreachability propagation in try-finally. (#3667) (Ali Rezvani) ### Refactor - 84304b4 linter: Add a `ctx.module_record()` method (#3637) (Boshen) - f98f777 linter: Add rule fixer (#3589) (Don Isaac) - fa11644 linter: Pass `Rc` by value (#3587) (overlookmotel) - f702fb9 semantic/cfg: Cleanup control flow and it's builder. (#3650) (rzvxa) - 5793ff1 transformer: Replace `&’a Trivias` with `Rc<Trivias>` (#3580) (Dunqing) Co-authored-by: Boshen <Boshen@users.noreply.github.com>
## [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>
closes #621
no-unreachable
oxlint-echosystem-ci result
This rule is done but since it is running for every possible statement and does quite a bit of work on them to determine whether it is 100% reachable or not; The performance in my opinion is kind of abysmal.
I'll try to work it out, I know Biome does 2 types of checks to simplify the rule for some nodes, However, they have a lot more false negatives than our implementation.
Here is one example of those false negatives
Update 1:
I've benchmarked this rule using only the simplified reachability checks and it was around 5% faster, To be honest, it isn't much improvement especially considering that we can only use this check for a small portion of nodes and even that is accompanied by newly introduced checks which would lessen the amount of performance gain further.
Most of the performance regression is because of allocations during our depth first search since we have to store both the visited and finished nodes which results in a bunch of rapid-fire allocations back to back. Currently, At the moment I don't have a great idea of how to improve it, We may have to implement our own graph to use arenas underneath.
Given that this rule is the most extensive use case of control flow (It doesn't come with a limited scope similar to property and constructor rules already implemented) this performance drop might be reasonable to some extent.
Update 2:
I reworked my approach in 2 senses, First I used @Boshen's suggestion inspired by TypeScript and kept some of the reachability information in the basic block structure instead of calculating it on the fly. It is done by propagating the
Unreachable
edge andUnreachable
instruction throughout subgraphs.This for sure helped with the performance but the next part is what never failed to amaze me, Going from something near
O(n!)
in the worst-case scenario toO(n^2)
(in the worst-case scenario). By changing the approach instead of checking the reachability of each statement we do it in 3 paths; First, we do a path on the entire CFG and query all reachable but suspicious cases, and then we do another path on each of these suspicions subgraphs to determine the reachability with higher confidence. Finally, we iterate all of the appropriate nodes and check their reachability status according to the information collected in 2 previous paths.With these 2 this rule went from
-24%
to~-2%
.This performance gain doesn't come for free though; It increases the likelihood of false positives/negatives, But as long as we are passing our
ecosystem-ci
it should be fine. We can always sacrifice some performance to check for edge cases if there are any.new oxlint-echosystem-ci result