fix(semantic): correctly resolve binding for return type of functions

[overlookmotel]

Fixes #6387.

[overlookmotel]

• test(semantic): add a test for UpdateExpression #7495
• test(semantic): re-organize snapshot-based tests #7494
• fix(semantic): correctly resolve binding for return type of functions #6388 👈 (View in Graphite)
• main

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

[graphite-app]

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

Add the label “0-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.

[codspeed-hq]

CodSpeed Performance Report

Merging #6388 will create unknown performance changes

_{Comparing 10-09-fix_semantic_correctly_resolve_binding_for_return_type_of_functions (f3850eb) with main (b0e1c03)}

Summary

🆕 30 new benchmarks

Benchmarks breakdown

	Benchmark	main	10-09-fix_semantic_correctly_resolve_binding_for_return_type_of_functions	Change
🆕	codegen[checker.ts]	N/A	24.7 ms	N/A
🆕	codegen_sourcemap[checker.ts]	N/A	77.5 ms	N/A
🆕	isolated-declarations[vue-id.ts]	N/A	406.4 ms	N/A
🆕	lexer[RadixUIAdoptionSection.jsx]	N/A	24.2 µs	N/A
🆕	lexer[antd.js]	N/A	22.3 ms	N/A
🆕	lexer[cal.com.tsx]	N/A	5.5 ms	N/A
🆕	lexer[checker.ts]	N/A	13.3 ms	N/A
🆕	lexer[pdf.mjs]	N/A	3.6 ms	N/A
🆕	linter[RadixUIAdoptionSection.jsx]	N/A	2.5 ms	N/A
🆕	linter[cal.com.tsx]	N/A	1.1 s	N/A
🆕	linter[checker.ts]	N/A	2.6 s	N/A
🆕	minifier[antd.js]	N/A	151 ms	N/A
🆕	minifier[react.development.js]	N/A	1.4 ms	N/A
🆕	minifier[typescript.js]	N/A	242.3 ms	N/A
🆕	parser[RadixUIAdoptionSection.jsx]	N/A	78.7 µs	N/A
🆕	parser[antd.js]	N/A	106.9 ms	N/A
🆕	parser[cal.com.tsx]	N/A	24.8 ms	N/A
🆕	parser[checker.ts]	N/A	53.5 ms	N/A
🆕	parser[pdf.mjs]	N/A	17.4 ms	N/A
🆕	semantic[RadixUIAdoptionSection.jsx]	N/A	98.3 µs	N/A
...	...	...	...	...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

[overlookmotel]

...or at least I think it's a fix. @Dunqing could you please help me with this one?

Questions:

1. There are various TS types which contain FormalParameters:

• TSCallSignatureDeclaration
• TSMethodSignature
• TSConstructSignatureDeclaration
• TSFunctionType
• TSConstructorType

None of these function types have a body, so I assume we're OK to skip the extra call to resolve_references_for_current_scope for them, right?

2. Is the func.params.kind != FormalParameterKind::Signature check required for Function and ArrowFunctionExpression? Or can their params never have kind: FormalParameterKind::Signature anyway?

3. How/where would I add tests for this?

[Dunqing]

Questions:

1. There are various TS types which contain FormalParameters:

• TSCallSignatureDeclaration
• TSMethodSignature
• TSConstructSignatureDeclaration
• TSFunctionType
• TSConstructorType

None of these function types have a body, so I assume we're OK to skip the extra call to resolve_references_for_current_scope for them, right?

Right

2. Is the func.params.kind != FormalParameterKind::Signature check required for Function and ArrowFunctionExpression? Or can their params never have kind: FormalParameterKind::Signature anyway?

Skipping func.params.kind != FormalParameterKind::Signature is ok

3. How/where would I add tests for this?

I think as long as there are no test failures, it’s fine, so we don’t need to add tests for it.

[overlookmotel]

2. Is the func.params.kind != FormalParameterKind::Signature check required for Function and ArrowFunctionExpression? Or can their params never have kind: FormalParameterKind::Signature anyway?

Skipping func.params.kind != FormalParameterKind::Signature is ok

Great. I've removed that.

3. How/where would I add tests for this?

I think as long as there are no test failures, it’s fine, so we don’t need to add tests for it.

But #6387 was a bug, and this PR fixes it. So there should have been a test failing before, but there wasn't. So I think we should add one. I just don't know where to put it. Can you point me in right direction please?

[Dunqing]

But #6387 was a bug, and this PR fixes it. So there should have been a test failing before, but there wasn't. So I think we should add one. I just don't know where to put it. Can you point me in right direction please?

Oh, sorry, I didn't notice that. We have two ways to write semantic tests:

1. The snapshot-based tests, you can add a test here, and then run https://github.com/oxc-project/oxc/blob/c822b48d4f7eedf4db803d158cfe7bcde24c25ea/crates/oxc_semantic/tests/main.rs#L158-L172

2. The integration test, you can add here

Either way is ok. I prefer the first way because I can see what scopes/symbols have in the test code

[Dunqing]

Let me add tests to it

[Boshen]

Merge activity

• Nov 26, 10:41 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Nov 26, 10:47 AM EST: A user added this pull request to the Graphite merge queue.
• Nov 26, 10:53 AM EST: A user merged this pull request with the Graphite merge queue.

[overlookmotel]

Thank you very much for cleaning up the mess I left!