refactor(ast): type params field before params in TS function declaration types

[overlookmotel]

Our convention is that AST type fields are ordered in order they appear in source. Move type_parameters fields in TS function declaration types to before this_param and formal parameters.

[overlookmotel]

• refactor(ast): type params field before params in TS function declaration types #6391 👈
• main

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

Join @overlookmotel and the rest of your teammates on Graphite

[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 #6391 will not alter performance

_{Comparing 10-09-fix_ast_type_params_field_before_params_in_ts_function_declaration_types (073b02a) with main (06ea121)}

Summary

✅ 29 untouched benchmarks

[overlookmotel]

What is odd is that, because prior to #6388, we resolved references in FormalParameters early, I imagine U in the function params and return type in this example would not have been resolved correctly:

type T = <U>(x: U) => U;

But, if I'm right about that, our tests did not catch this. Am I wrong? Or do we not have enough test coverage?

@Dunqing calling on your TS expertise!

[overlookmotel]

PS: I am slowly learning TypeScript!

[Boshen]

Our convention is that AST type fields are ordered in order they appear in source.

In source or runtime order? This should be documented.

[Dunqing]

What is odd is that, because prior to #6388, we resolved references in FormalParameters early, I imagine U in the function params and return type in this example would not have been resolved correctly:

type T = <U>(x: U) => U;

But, if I'm right about that, our tests did not catch this. Am I wrong? Or do we not have enough test coverage?

@Dunqing calling on your TS expertise!

Well, good question, we have enough tests to cover these ASTs. This case is correct before, check out the Playground

Why resolve correctly even if the order of the fields is incorrect

It is related to our resolving algorithms. Our process is collecting bindings/references first, and then resolving references in exit_scope.

So in type T = <U>(x: U) => U;, the resolving steps are as follows.

1. enter_scope (TypeAlias)
2. declare reference x (FormalParameter)
3. declare reference U (FormalParameter's TSTypeAnnotation)
4. declare reference U (ReturnType)
5. bind U (TSTypeParameter)
6. exit_scope (calling resolve_references_for_current_scope here)

Based on the above steps, we know that we have known all the bindings before resolving so that the references are resolved correctly no matter what the order of fields.

But attention if there is a nested scope, the order is crucial.

[overlookmotel]

Why resolve correctly even if the order of the fields is incorrect

I think I understand it now.

What I was thinking is that due to this code, there would be an extra call to resolve_references_for_current_scope after the FormalParameters are traversed, but before the type_parameters field is visited (i.e. between (3) and (4) in your list of operations):

oxc/crates/oxc_semantic/src/builder.rs

Lines 1983 to 1991 in c822b48

	AstKind::FormalParameters(parameters) => {
	if parameters.kind != FormalParameterKind::Signature && parameters.has_parameter() {
	// `function foo({bar: identifier_reference}) {}`
	// ^^^^^^^^^^^^^^^^^^^^ Parameter initializer must be resolved
	// after all parameters have been declared
	// to avoid binding to variables inside the scope
	self.resolve_references_for_current_scope();
	}
	}

But I now realize that wasn't happening because parameters.kind == FormalParameterKind::Signature in all these cases.

So this PR is not a bug fix, just a refactor.

Thanks for clarifying this for me.

[overlookmotel]

In source or runtime order? This should be documented.

I've opened #6453 to discuss this question. But in this particular case, the concept of "runtime order" is a bit slippery - TS types don't even exist at runtime - they're a compile-time concept only.

So I think regardless of the answer to #6453, the change this PR makes is good. Either way, type_parameters being last field is definitely not our convention.

[Dunqing]

LGTM

[overlookmotel]

Merge activity

• Oct 11, 5:56 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Oct 11, 5:56 AM EDT: A user added this pull request to the Graphite merge queue.
• Oct 11, 6:02 AM EDT: A user merged this pull request with the Graphite merge queue.

[overlookmotel]

LGTM

Thanks Dunqing!