fix(analyze/js): fix bindings in bogus imports not being detected as imports

[dyc3]

Summary

This fix attempts to fix #4568 such that the same issue won't pop up in other lint rules.

I suppose a better fix could be to not evaluate linter rules if the file has parsing errors. Would it be better to add a JS_BOGUS_IMPORT instead?

fixes #4568

Test Plan

Tested it manually against the sample in the issue.

[github-actions]

Parser conformance results on

js/262

Test result	main count	This PR count	Difference
Total	48595	48595	0
Passed	47403	47403	0
Failed	1192	1192	0
Panics	0	0	0
Coverage	97.55%	97.55%	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	6586	6586	0
Passed	2208	2208	0
Failed	4378	4378	0
Panics	0	0	0
Coverage	33.53%	33.53%	0.00%

ts/babel

Test result	main count	This PR count	Difference
Total	680	680	0
Passed	608	608	0
Failed	72	72	0
Panics	0	0	0
Coverage	89.41%	89.41%	0.00%

ts/microsoft

Test result	main count	This PR count	Difference
Total	18618	18618	0
Passed	14273	14273	0
Failed	4345	4345	0
Panics	0	0	0
Coverage	76.66%	76.66%	0.00%

[arendjr]

I suppose a better fix could be to not evaluate linter rules if the file has parsing errors.

I think we do want to run rules on files that have bogus nodes, because bogus states are to be expected while the user is typing and we don't want diagnostics to temporarily disappear just because they are temporarily in an invalid syntax state.

Would it be better to add a JS_BOGUS_IMPORT instead?

Sometimes having a more specific bogus node can indeed help the error recovery become more resilient. But if the change you made now already resolves the issue, my guess is that's it's not needed here? This is something @denbezrukov is usually quite knowledgeable about, if I'm not mistaken.

[dyc3]

my guess is that's it's not needed here?

Technically, sure, to resolve the issue. But I want to make sure we resolve this with a more preventative measure to make sure this doesn't pop up again.

Ultimately, this issue was caused by an assumption (as shown by usage of unreachable!()) that was proven to be false (via fuzzing). So I'm thinking it would make more sense to add a JS_BOGUS_IMPORT node, whether that's in this PR or a different one.

I don't think I have the bandwidth right now to add the node, but I could write up a task detailed enough for someone else to pick up.

[ematipico]

Out of curiosity, can't we remove the unreachable from the rule's code?

[dyc3]

Yup, could do that too. I'll go ahead and add that to this PR.

[ematipico]

Looks good! We should update the changelog and - if possible - add a test with the code coming from the issue

[codspeed-hq]

CodSpeed Performance Report

Merging #4578 will not alter performance

_{Comparing 11-18-fix_analyze_js_fix_some_imports_not_being_detected (389c0bc) with main (b8ca13e)}

Summary

✅ 99 untouched benchmarks