Skip to content
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

fix(analyzer): False positive in rule noUndeclaredVariables #243 #350

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

denbezrukov
Copy link
Contributor

@denbezrukov denbezrukov commented Sep 20, 2023

Summary

Fix #243

We don't need to check the correct position of infer types in semantics because our parser raises an error if they are in an invalid position. #308

Thank to @strager for the explanation.

Test Plan

cargo test

@denbezrukov denbezrukov temporarily deployed to Website deployment September 20, 2023 21:08 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Linter Area: linter A-Parser Area: parser A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Changelog Area: changelog labels Sep 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 20, 2023

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47808 47808 0
Failed 1055 1055 0
Panics 0 0 0
Coverage 97.84% 97.84% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6212 6212 0
Passed 1764 1767 ✅ ⏫ +3
Failed 4448 4445 ✅ ⏬ -3
Panics 0 0 0
Coverage 28.40% 28.44% +0.05%
🎉 Fixed (3):
conditionalTypeGenericInSignatureTypeParameterConstraint.symbols
inferTInParentheses.symbols
typeAliasFunctionTypeSharedSymbol.symbols

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 571 571 0
Failed 68 68 0
Panics 0 0 0
Coverage 89.36% 89.36% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13123 13123 0
Failed 4101 4101 0
Panics 0 0 0
Coverage 76.19% 76.19% 0.00%

@denbezrukov denbezrukov temporarily deployed to Website deployment September 20, 2023 21:28 — with GitHub Actions Inactive
@ematipico
Copy link
Member

!bench_analyzer

I want to check if the workflow is fixed

@github-actions
Copy link
Contributor

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      4.4±0.00ms     2.7 MB/sec    1.01      4.4±0.02ms     2.7 MB/sec
analyzer/index.js         1.00      9.0±0.03ms     3.4 MB/sec    1.01      9.1±0.04ms     3.3 MB/sec
analyzer/lint.ts          1.00      6.8±0.04ms     6.1 MB/sec    1.02      6.9±0.01ms     6.0 MB/sec
analyzer/parser.ts        1.00     13.3±0.10ms     3.7 MB/sec    1.02     13.6±0.03ms     3.6 MB/sec
analyzer/router.ts        1.00      4.3±0.00ms     5.4 MB/sec    1.01      4.4±0.00ms     5.4 MB/sec
analyzer/statement.ts     1.00     12.5±0.06ms     2.8 MB/sec    1.01     12.7±0.02ms     2.8 MB/sec
analyzer/typescript.ts    1.00     19.7±0.15ms     2.8 MB/sec    1.02     20.0±0.08ms     2.7 MB/sec

@denbezrukov
Copy link
Contributor Author

denbezrukov commented Sep 21, 2023

!bench_analyzer

I want to check if the workflow is fixed

What do you think if there is a better way to check if a type is inside a conditional branch?
We have to go to a parent and compare a node with a true type for every TsNode.
I'm wondering if we can somehow know that the next node is a true type.

If its a performance regression we can try to narrow node kinds, because not every type node needs its own scope in a true branch. But it makes code more complex.

@ematipico
Copy link
Member

I think the implementation is fine as is!

Sorry if I used your PR to trigger the benchmark. The workflows were broken for a long time, and I wanted to check if they were finally fixed.

@github-actions
Copy link
Contributor

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      4.4±0.00ms     2.7 MB/sec    1.01      4.4±0.01ms     2.7 MB/sec
analyzer/index.js         1.00      9.0±0.02ms     3.4 MB/sec    1.01      9.1±0.01ms     3.4 MB/sec
analyzer/lint.ts          1.00      6.7±0.01ms     6.2 MB/sec    1.02      6.9±0.03ms     6.0 MB/sec
analyzer/parser.ts        1.00     13.2±0.05ms     3.7 MB/sec    1.02     13.5±0.12ms     3.6 MB/sec
analyzer/router.ts        1.00      4.3±0.00ms     5.5 MB/sec    1.02      4.4±0.00ms     5.4 MB/sec
analyzer/statement.ts     1.00     12.4±0.03ms     2.9 MB/sec    1.01     12.6±0.14ms     2.8 MB/sec
analyzer/typescript.ts    1.00     19.5±0.03ms     2.8 MB/sec    1.02     19.8±0.09ms     2.7 MB/sec

crates/biome_js_semantic/src/events.rs Outdated Show resolved Hide resolved
crates/biome_js_semantic/src/events.rs Outdated Show resolved Hide resolved
@denbezrukov denbezrukov temporarily deployed to Website deployment September 21, 2023 19:29 — with GitHub Actions Inactive
@denbezrukov denbezrukov merged commit 7de063c into main Sep 21, 2023
18 checks passed
@denbezrukov denbezrukov deleted the feat/infer-semantic branch September 21, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Linter Area: linter A-Parser Area: parser A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 False positive in rule noUndeclaredVariables
3 participants