fix(rome_js_analyzer, rome_js_parser): suppress false positive diagnostics related to index signature parameter #4660

nissy-dev · 2023-07-06T09:00:50Z

Summary

The reason of this issue is that semantic analyzer handles index signature parameter as JsIdentifierBinding.
Thus, I add a new CST Node, TsIndexSignatureIdentifierBinding and suppress false positive diagnostics.

Test Plan

I update some snapshot tests.

nissy-dev · 2023-07-06T09:01:57Z

crates/rome_js_analyze/src/semantic_analyzers/suspicious/no_redeclare.rs

+                    redeclarations.push(Redeclaration {
+                        name,
+                        declaration: *first_text_range,
+                        redeclaration: id_binding.syntax().text_trimmed_range(),
+                    })


This rule no longer handles the case like interface A { [index: number]: string; [index: number]: string; }, so I removed unnecessary logic.

netlify · 2023-07-06T09:02:05Z

✅ Deploy Preview for docs-rometools ready!

Name	Link
🔨 Latest commit	`86b1c92`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/64aaa70149419a00084c8669
😎 Deploy Preview	https://deploy-preview-4660--docs-rometools.netlify.app/playground
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

nissy-dev · 2023-07-06T09:04:17Z

crates/rome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.jsonc

@@ -24,7 +24,5 @@
 	"function f() { var a; var a; }",
 	"function f() { var a; if (test) { var a; } }",
 	"for (var a, a;;);",
-	"for (;;){ var a, a,;}",
-	"function f(x) { var x = 5; }",


This case was not correctly checked, so I removed. (see: #4291)

nissy-dev · 2023-07-06T09:05:38Z

crates/rome_js_analyze/tests/specs/suspicious/noRedeclare/invalid.jsonc

@@ -24,7 +24,5 @@
 	"function f() { var a; var a; }",
 	"function f() { var a; if (test) { var a; } }",
 	"for (var a, a;;);",
-	"for (;;){ var a, a,;}",
-	"function f(x) { var x = 5; }",
-  "interface A { [index: number]: string; [index: number]: string; }"


semantic analyzer no longer handle this case, so I removed.

github-actions · 2023-07-06T09:08:57Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	48863	48863	0
Passed	47810	47810	0
Failed	1053	1053	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	1736	❌ ⏬ -28
Failed	4448	4476	❌ ⏫ +28
Panics	0	0	0
Coverage	28.40%	27.95%	-0.45%

🔥 Regression (28):

classDoesNotDependOnBaseTypes.symbols
classIndexer.symbols
computedPropertyNamesContextualType4_ES5.symbols
computedPropertyNamesContextualType4_ES6.symbols
computedPropertyNamesContextualType5_ES5.symbols
computedPropertyNamesContextualType5_ES6.symbols
declFileClassWithIndexSignature.symbols
declFileIndexSignatures.symbols
hidingIndexSignatures.symbols
incrementOnNullAssertion.symbols
indexerAssignability.symbols
inferredIndexerOnNamespaceImport.symbols
nestedIndexer.symbols
parserClassDeclarationIndexSignature1.symbols
parserES5SymbolIndexer1.symbols
parserES5SymbolIndexer2.symbols
parserES5SymbolIndexer3.symbols
parserIndexMemberDeclaration1.symbols
parserIndexMemberDeclaration6.symbols
parserSymbolIndexer1.symbols
parserSymbolIndexer2.symbols
parserSymbolIndexer3.symbols
parserSymbolIndexer4.symbols
spreadObjectWithIndexDoesNotAddUndefinedToLocalIndex.symbols
staticIndexSignature6.symbols
staticIndexSignatureAndNormalIndexSignature.symbols
tsxPreserveEmit3.symbols
typeGuardOnContainerTypeNoHang.symbols

ts/babel

Test result	`main` count	This PR count	Difference
Total	639	639	0
Passed	573	573	0
Failed	66	66	0
Panics	0	0	0
Coverage	89.67%	89.67%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	17224	17224	0
Passed	13121	13121	0
Failed	4103	4103	0
Panics	0	0	0
Coverage	76.18%	76.18%	0.00%

ematipico · 2023-07-06T14:24:36Z

@nissy-dev have you looked at the regressions? Seems like the change of logic broke some test

nissy-dev · 2023-07-07T00:44:58Z

@ematipico I'll check it in a few days! But, I have never seen these .symbol files, so I would appreciate if you could tell me some information about symbols/microsoft tests.

ematipico · 2023-07-07T07:17:08Z

@ematipico I'll check it in a few days! But, I have never seen these .symbol files, so I would appreciate if you could tell me some information about symbols/microsoft tests.

No worries! In Rome we have some conformance tests that run against some code bases, and one of them in the TypeScript repository. We do that using git submodules.

Our test suite runs against TypeScript conformance tests, and we check if our parser and their parser result match. If our parser doesn't match TS result, it's a fail.

In your case, it seems that what we could parse, now doesn't parse anymore as expected. The files listed in the comment come from the TypeScript repository, here's an example:

Our test suite parsers the symbol file to collect the correct data, and eventually we run it using our xtask.

Now, I don't precisely know the assertions that we do against our parser and what caused the regression, unfortunately. Although, a regression is something that we should not overlook.

Something to consider: the TS parser identifies those as normal identifiers. Try to hover x and s, and both are Identifier. Maybe that's the reason why we used the same node. Perhaps, we could change the semantic model to ignore those identifiers if they are inside an interface/type.

nissy-dev · 2023-07-09T12:19:44Z

@ematipico Thank you for your thoughtful comments!

After checking these regressions, the reason is that the conformance test depends on the semantic analyzer.

tools/xtask/coverage/src/symbols/msts.rs

Line 83 in 54b2ab7

let mut actual: Vec<_> = rome_js_semantic::semantic_events(r.syntax())

Considering this point, the regressions are reasonable, so we can ignore.

Perhaps, we could change the semantic model to ignore those identifiers if they are inside an interface/type.

Because of the reason, I confirmed the regressions will also happens even if I fix it as you pointed out.

ematipico · 2023-07-09T19:54:34Z

I believe it's fine if you decide to create a new node, but I think that the semantic analyzer should be updated to track the new node as a symbol.

It's fine if we fix the rules, but I think it's not fine if we regress the conformance without a specific reason.

nissy-dev · 2023-07-10T04:44:13Z

I think that the semantic analyzer should be updated to track the new node as a symbol.

The symbol collected in this conformance test is some kinds of the semantic events.

Implementation:

tools/xtask/coverage/src/symbols/msts.rs

Lines 83 to 101 in 54b2ab7

    
           let mut actual: Vec<_> = rome_js_semantic::semantic_events(r.syntax()) 
        
               .into_iter() 
        
               .filter(|x| { 
        
                   // We filter any event pointing to string literals. 
        
                   // We do the same below because TS classifies some string literals as symbols and we also 
        
                   // filter them below. 
        
                   match x { 
        
                       SemanticEvent::DeclarationFound { .. } 
        
                       | SemanticEvent::Read { .. } 
        
                       | SemanticEvent::HoistedRead { .. } 
        
                       | SemanticEvent::Write { .. } 
        
                       | SemanticEvent::HoistedWrite { .. } => { 
        
                           let name = x.str(&code); 
        
                           !name.contains('\"') && !name.contains('\'') 
        
                       } 
        
                       _ => false, 
        
                   } 
        
               }) 
        
               .collect();

In my PR, the semantic analyzer no longer push the semantic event when entering an identifier binding node of index signatures. Your suggestion is that it would be better to solve this issue while handling an identifier binding of index signatures as the semantic event?

I am not sure if this is possible yet. I have felt that many of the semantic analyzer functions don't take into account the identifier of index signatures, so a lot of changes may be needed.

ematipico · 2023-07-10T05:55:07Z

I am not sure if this is possible yet. I have felt that many of the semantic analyzer functions don't take into account the identifier of index signatures, so a lot of changes may be needed.

I know. Although, I think this is not the correct fix. For example, the usNamingConvetion rule had a regression where those bindings aren't checked anymore.

So, as long as we want to add a new node, we should still keep tracking it in the semantic analyzer. Then, inside the noDeclare rule, we ignore all those bindings that belong to this new node.

nissy-dev · 2023-07-10T08:36:10Z

So, as long as we want to add a new node, we should still keep tracking it in the semantic analyzer. Then, inside the noDeclare rule, we ignore all those bindings that belong to this new node.

OK! I will try to reimplement it with your remarks in mind.

nissy-dev added 4 commits July 5, 2023 22:12

feat: add TsIndexSignatureParameterIdentifierBinding node

3cadd71

fix: update linter logic

afb48a2

refactor: rename to TsIndexSignatureIdentifierBinding

bc95d98

docs: update changelog

73f249a

github-actions bot added A-Linter Area: linter A-Formatter Area: formatter A-Parser Area: parser A-Tooling Area: our own build, development, and release tooling labels Jul 6, 2023

nissy-dev changed the title ~~fix(rome_js_analyzer, rome_js_parser):~~ fix(rome_js_analyzer, rome_js_parser): suppress false positive diagnostics related to index signature parameter Jul 6, 2023

nissy-dev commented Jul 6, 2023

View reviewed changes

fix: apply fmt

5e06019

Conaclos approved these changes Jul 6, 2023

View reviewed changes

Merge branch 'main' into fix-4634-rework

86b1c92

nissy-dev closed this Jul 10, 2023

nissy-dev deleted the fix-4634-rework branch July 30, 2023 02:39

Conaclos mentioned this pull request Sep 27, 2023

🐛 noUnusedVariables misreporting imports used for type definitions biomejs/biome#104

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rome_js_analyzer, rome_js_parser): suppress false positive diagnostics related to index signature parameter #4660

fix(rome_js_analyzer, rome_js_parser): suppress false positive diagnostics related to index signature parameter #4660

nissy-dev commented Jul 6, 2023 •

edited

Loading

nissy-dev Jul 6, 2023

netlify bot commented Jul 6, 2023 •

edited

Loading

nissy-dev Jul 6, 2023

nissy-dev Jul 6, 2023

github-actions bot commented Jul 6, 2023 •

edited

Loading

ematipico commented Jul 6, 2023 •

edited

Loading

nissy-dev commented Jul 7, 2023

ematipico commented Jul 7, 2023

nissy-dev commented Jul 9, 2023 •

edited

Loading

ematipico commented Jul 9, 2023

nissy-dev commented Jul 10, 2023

ematipico commented Jul 10, 2023 •

edited

Loading

nissy-dev commented Jul 10, 2023 •

edited

Loading

fix(rome_js_analyzer, rome_js_parser): suppress false positive diagnostics related to index signature parameter #4660

fix(rome_js_analyzer, rome_js_parser): suppress false positive diagnostics related to index signature parameter #4660

Conversation

nissy-dev commented Jul 6, 2023 • edited Loading

Summary

Test Plan

nissy-dev Jul 6, 2023

Choose a reason for hiding this comment

netlify bot commented Jul 6, 2023 • edited Loading

✅ Deploy Preview for docs-rometools ready!

nissy-dev Jul 6, 2023

Choose a reason for hiding this comment

nissy-dev Jul 6, 2023

Choose a reason for hiding this comment

github-actions bot commented Jul 6, 2023 • edited Loading

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

ematipico commented Jul 6, 2023 • edited Loading

nissy-dev commented Jul 7, 2023

ematipico commented Jul 7, 2023

nissy-dev commented Jul 9, 2023 • edited Loading

ematipico commented Jul 9, 2023

nissy-dev commented Jul 10, 2023

ematipico commented Jul 10, 2023 • edited Loading

nissy-dev commented Jul 10, 2023 • edited Loading

nissy-dev commented Jul 6, 2023 •

edited

Loading

netlify bot commented Jul 6, 2023 •

edited

Loading

github-actions bot commented Jul 6, 2023 •

edited

Loading

ematipico commented Jul 6, 2023 •

edited

Loading

nissy-dev commented Jul 9, 2023 •

edited

Loading

ematipico commented Jul 10, 2023 •

edited

Loading

nissy-dev commented Jul 10, 2023 •

edited

Loading