fix(ast/estree): Align BindingIdentifier output with TS-ESTree

[therewillbecode]

Add an empty decorators field which is always an empty array to the BindingIdentifier node in order to fix the conformance test case for topLevelLambda3.ts.

This test was failing because of the failing diff for the BindingIdentifier f in var f = () => {this.window;}.

@@ -17,7 +17,6 @@
             "type": "Identifier",
             "start": 4,
             "end": 5,
-            "decorators": [],
             "name": "f",
             "optional": false,
             "typeAnnotation": null

This is my first attempt at contributing to our efforts to align our AST's ESTree output with that of TS-ESLint's ESTree output. See #9705 . Let me know if I am missing something.

AST Playgrounds for the test case below:

Oxc playground

TS-ESlint playground

[graphite-app]

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[codspeed-hq]

CodSpeed Performance Report

Merging #9928 will not alter performance

_{Comparing therewillbecode:fix/estree-binding-identifier-decorator (7f52aac) with main (33c1c76)}

Summary

✅ 33 untouched benchmarks

[therewillbecode]

Actually judging by the snapshot failing in CI , seems like I have added some mismatches now that need fixed.

[hi-ogawa]

Thanks for joining forces 👍
I didn't link to the issue, but I've also attempted to fix BindingIdentifier in #9822 and it's waiting for review. Sorry for causing duplicate work 😢

[overlookmotel]

#9822 is now merged and it makes the same change, so this PR is now defunct.

But, just to give feedback in case you'd like to continue hacking on this (please please do!)...

The approach you took here was exactly right. The #[estree] attributes give various ways to alter the JS-side AST, without altering the actual Rust types. They're all listed in this function in the codegen oxc_ast_tools which auto-generates the ESTree impls:

oxc/tasks/ast_tools/src/derives/estree.rs

Lines 87 to 203 in db642eb

	fn parse_estree_attr(location: AttrLocation, part: AttrPart) -> Result<()> {
	// No need to check attr name is `estree`, because that's the only attribute this derive handles
	match location {
	// `#[estree]` attr on struct
	AttrLocation::Struct(struct_def) => match part {
	AttrPart::Tag("skip") => struct_def.estree.skip = true,
	AttrPart::Tag("flatten") => struct_def.estree.flatten = true,
	AttrPart::Tag("no_type") => struct_def.estree.no_type = true,
	AttrPart::Tag("no_ts_def") => struct_def.estree.no_ts_def = true,
	AttrPart::List("add_fields", list) => {
	for list_element in list {
	let (name, value) = list_element.try_into_string()?;
	struct_def.estree.add_fields.push((name, value));
	}
	}
	AttrPart::List("field_order", list) => {
	// Get iterator over all field names (including added fields)
	let all_field_names = struct_def.fields.iter().map(FieldDef::name).chain(
	struct_def.estree.add_fields.iter().map(|(field_name, _)| field_name.as_str()),
	);
	// Convert field names to indexes.
	// Added fields (`#[estree(add_fields(...))]`) get indexes after the real fields.
	// Error if same field name included more than once.
	let mut field_indices = vec![];
	for list_element in list {
	let field_name = list_element.try_into_tag()?;
	let field_name = field_name.trim_start_matches("r#");
	let field_index = all_field_names
	.clone()
	.position(|this_field_name| this_field_name == field_name)
	.ok_or(())?;
	let field_index = u8::try_from(field_index).map_err(|_| ())?;
	if field_indices.contains(&field_index) {
	return Err(());
	}
	field_indices.push(field_index);
	}
	struct_def.estree.field_indices = field_indices;
	}
	AttrPart::String("ts_alias", value) => struct_def.estree.ts_alias = Some(value),
	AttrPart::String("add_ts_def", value) => struct_def.estree.add_ts_def = Some(value),
	AttrPart::String("rename", value) => struct_def.estree.rename = Some(value),
	AttrPart::String("via", value) => struct_def.estree.via = Some(value),
	_ => return Err(()),
	},
	// `#[estree]` attr on enum
	AttrLocation::Enum(enum_def) => match part {
	AttrPart::Tag("skip") => enum_def.estree.skip = true,
	AttrPart::Tag("no_rename_variants") => enum_def.estree.no_rename_variants = true,
	AttrPart::Tag("no_ts_def") => enum_def.estree.no_ts_def = true,
	AttrPart::String("ts_alias", value) => enum_def.estree.ts_alias = Some(value),
	AttrPart::String("add_ts_def", value) => {
	enum_def.estree.add_ts_def = Some(value);
	}
	AttrPart::String("via", value) => enum_def.estree.via = Some(value),
	_ => return Err(()),
	},
	// `#[estree]` attr on struct field
	AttrLocation::StructField(struct_def, field_index) => match part {
	AttrPart::Tag("skip") => struct_def.fields[field_index].estree.skip = true,
	AttrPart::Tag("flatten") => struct_def.fields[field_index].estree.flatten = true,
	AttrPart::Tag("no_flatten") => struct_def.fields[field_index].estree.no_flatten = true,
	AttrPart::Tag("json_safe") => struct_def.fields[field_index].estree.json_safe = true,
	AttrPart::String("rename", value) => {
	struct_def.fields[field_index].estree.rename = Some(value);
	}
	AttrPart::String("via", value) => {
	struct_def.fields[field_index].estree.via = Some(value);
	}
	AttrPart::String("append_to", value) => {
	// Find field this field is to be appended to
	let target_field_index = struct_def
	.fields
	.iter()
	.enumerate()
	.find(|(_, other_field)| other_field.name() == value)
	.map(|(field_index, _)| field_index)
	.ok_or(())?;
	if target_field_index == field_index {
	// Can't append field to itself
	return Err(());
	}
	let target_field = &mut struct_def.fields[target_field_index];
	if target_field.estree.append_field_index.is_some() {
	// Can't append twice to same field
	return Err(());
	}
	target_field.estree.append_field_index = Some(field_index);
	struct_def.fields[field_index].estree.skip = true;
	}
	AttrPart::String("ts_type", value) => {
	struct_def.fields[field_index].estree.ts_type = Some(value);
	}
	_ => return Err(()),
	},
	// `#[estree]` attr on enum variant
	AttrLocation::EnumVariant(enum_def, variant_index) => match part {
	AttrPart::String("rename", value) => {
	enum_def.variants[variant_index].estree.rename = Some(value);
	}
	AttrPart::String("via", value) => {
	enum_def.variants[variant_index].estree.via = Some(value);
	}
	_ => return Err(()),
	},
	// `#[estree]` attr on meta type
	AttrLocation::Meta(meta) => match part {
	AttrPart::String("ts_type", ts_type) => meta.estree.ts_type = Some(ts_type),
	AttrPart::String("raw_deser", raw_deser) => meta.estree.raw_deser = Some(raw_deser),
	_ => return Err(()),
	},
	_ => unreachable!(),
	}
	Ok(())
	}

But they're not documented (sorry, my bad! was coding in a hurry).

The change to the snapshot file in this PR wasn't due to any fault in your work. There's an unfortunate thing that when you run the conformance tester with a --filter, it overwrites the snapshot file with a result excluding the filtered out files. i.e. wipes all/most of the test cases from the snapshot.

The solution is to run it again without --filter to restore the snapshot. And, if your change is working, you should see some red lines in the diff on the snapshot file, as more tests pass.

Sorry that poor co-ordination on our part meant that your work here was wasted. I hope it's not put you off completely, and you still have appetite to continue.

Please feel free to reach out on Discord if you have any questions.

[therewillbecode]

Thanks so much @overlookmotel for the feedback!