feat(ast): add AstBuilder::vec_convert

[Dunqing]

Give a try for oxc-project/backlog#153

I am not sure whether it a 100% safe, we only can guarantee that we use it safely

[Dunqing]

• feat(ast): add AstBuilder::vec_convert #7860 👈 (View in Graphite)
• refactor(transformer/class-properties): rename *_owner to owned_* #7855
• main

---

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.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #7860 will not alter performance

_{Comparing 12-13-feat_ast_add_astbuilder_vec_convert (7f641c9) with main (0f5e078)}

Summary

✅ 29 untouched benchmarks

[overlookmotel]

Sorry to say this is wildly unsound!

2 types being same size and alignment is not enough. They also need to be the same type.

The only reason why we can convert from Expression to Argument safely is because Argument is defined with the inherit_variants! macro. This makes Argument a superset of Expression, so any Expression is also a valid Argument.

• Expression::BooleanLiteral -> Argument::BooleanLiteral
• Expression::ArrayExpression -> Argument::ArrayExpression
• ...same for all Expression's variants

The inherit_variants! macro generates code that checks at compile time that this is true, and that both enums have same discriminants i.e. discriminant for Expression::BooleanLiteral is same as discriminant for Argument::BooleanLiteral, etc for all the variants.

oxc/crates/oxc_ast/src/ast/macros.rs

Lines 858 to 870 in 0804916

	// Ensure discriminants match for all variants between parent and child types
	const _: () = {
	$(
	assert!(
	$crate::ast::macros::discriminant!($parent::$variant)
	== $crate::ast::macros::discriminant!($child::$variant),
	concat!(
	"Non-matching discriminants for `", stringify!($variant),
	"` between `", stringify!($parent), "` and `", stringify!($child), "`"
	)
	);
	)+
	};

So there's a lot of checks that make sure this conversion is sound.

But it's not sound, for example, to go in the other direction - from Argument back to Expression - because Argument::SpreadElement has no equivalent in Expression.

And it's even more unsound to perform conversion between any types which have same size and alignment, which is what vec_convert allows.

vec_convert could convert from Vec<u8> to Vec<bool>, for example. But that's not valid. 0u8 -> false and 1u8 -> true, but what is the bool value that corresponds to 2? Or 255? There isn't one, so it's undefined behavior.

I think we probably can make a sound implementation of Vec<Expression> to Vec<Argument> conversion, but we should lock it down to only allow conversions which are valid, and codegen the code with either inherit_variants! macro or ast_tools to make sure we don't make mistakes.

Beyond soundness issues, there's also a question of whether allowing this conversion loses us anything else useful. At present, we have an invariant that any pointer to an element in a Vec<Expression> remains a valid Expression forever. but if you can convert it to a Vec<Argument>, then that invariant breaks. We don't currently make use of that invariant, but we may want to in future (e.g. Traverse v2). That's the part I'm not sure about.

Anyway... TLDR... it's complicated!

[Dunqing]

Anyway... TLDR... it's complicated!

Ah, closing it as you said!