fix(ast_codegen): detect "complex" type wrappers

[rzvxa]

Detect types such as Cell<Option<ScopeId>> and mark them as such! We didn't used to use this method for these options but now we have to check all types to calculate their layouts which means we need to process them correctly(instead of falling to their inner value).

[rzvxa]

• refactor(ast_codegen): more versatile generation pipeline. #4540
• perf(ast, ast_codegen): optimize AST structs' memory layouts #4404
• feat(ast_codegen): add alignment and size data to the schema. #4615
• refactor(ast): make AST structs repr(C). #4614
• fix(ast_codegen): detect "complex" type wrappers #4617 👈
• refactor(parser): use ast_builder in more places. #4612
• main

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

Join @rzvxa and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #4617 will not alter performance

_{Comparing 08-02-fix_ast_codegen_detect_complex_type_wrappers (bcfa297) with main (e2b15ac)}

Summary

✅ 32 untouched benchmarks

[overlookmotel]

What constitutes "complex"? In practice at present, is it only Cell<Option<T>>?

A couple of suggestions:

Could we add Cell and CellOpt variants to TypeWrapper, to explicitly handle these, rather than a catch-all Complex?

At present are you assuming that Cell<Option<T>> has same size and alignment as T? If so, that's only true accidentally, because ScopeId, SymbolId and ReferenceId are all NonZeroU32 under the hood, and so have a niche. If someone changed ScopeId to a u32, or wrapped some other field in a Cell, this assumption would break.

This isn't entirely academic - Dunqing was talking about making IdentifierReference::reference_flag a Cell in #4512.

Using the following rules would make type layout calculations more robust in the face of such changes:

• Cell<T> is same size and align as T (so where T is an Option, calculate size + alignment of the Option using same logic as for other Options).
• Cell<T> never has a niche, even if T does.

Taking it a bit further (which I'm not suggesting doing now, but we maybe could in future) could make TypeWrapper more flexible by turning it into a Vec<TypeWrapperPart>. So then you could handle any possible combination of wrappers e.g. my_field: Option<Vec<Option<Box<Something>>>.

NB: We don't have to do any of the above in this PR - it could be a follow-up.

[rzvxa]

What constitutes "complex"? In practice at present, is it only Cell<Option>?

Any type that has more than one part(e.g. Adt<T>) and Adt isn't covered by other wrappers.

Could we add Cell and CellOpt variants to TypeWrapper, to explicitly handle these, rather than a catch-all Complex?

We sure can but we don't need it at the moment. We never care for misc types such as Cell<Option<XXX>>. We do not run any analysis on them. It is always just a simple match to detect these and do a predefined behavior.

At present are you assuming that Cell<Option> has same size and alignment as T? If so, that's only true accidentally, because ScopeId, SymbolId and ReferenceId are all NonZeroU32 under the hood, and so have a niche. If someone changed ScopeId to a u32, or wrapped some other field in a Cell, this assumption would break.

No; Instead we have the whole type as well-known (as opposed to having well-known wrappers and calculating the layout).

oxc/tasks/ast_codegen/src/passes/calc_layout.rs

Lines 356 to 358 in 8cdd9d9

	Cell<Option<ScopeId>>: { _ => Layout::known(4, 4, 1), },
	Cell<Option<SymbolId>>: { _ => Layout::known(4, 4, 1), },
	Cell<Option<ReferenceId>>: { _ => Layout::known(4, 4, 1), },

As you can see unlike the other wrappers which we only mark the wrapper part(for example Vec) as well-known, These are fully identified types with predefined layouts.

oxc/tasks/ast_codegen/src/passes/calc_layout.rs

Lines 219 to 249 in 8cdd9d9

	let get_layout = |type_ref: Option<&TypeRef>| -> Result<PlatformLayout> {
	let result = if let Some(type_ref) = &type_ref {
	if calc_layout(&mut type_ref.borrow_mut(), ctx)? {
	type_ref.borrow().layouts().map(PlatformLayout::from)?
	} else {
	PlatformLayout::UNKNOWN
	}
	} else if let Some(well_known) =
	WELL_KNOWN.get(ty.typ.get_ident().inner_ident().to_string().as_str())
	{
	well_known.clone()
	} else {
	let Type::Path(typ) = &ty.typ else {
	panic!();
	};
	let typ = typ
	.path
	.segments
	.first()
	.map(|it| it.to_token_stream().to_string().replace(' ', ""))
	.expect("We only accept single segment types.");
	if let Some(typ) = WELL_KNOWN.get(typ.as_str()) {
	typ.clone()
	} else {
	panic!("Unsupported type: {:#?}", ty.typ.to_token_stream().to_string())
	}
	};
	Ok(result)
	};

If a type isn't AST And doesn't have well-known information to calculate its layout, We check if the whole type is well-known or not. If it is we use that layout otherwise we fail to do so.

As a follow up we can calculate these, But we don't have to. I assert all of the types so if a well-known layout changes we get an updated assertion or old assertions break(if we don't call just ast with new layouts).

I think the only scenario where calculating Cell is necessary is if we use it to wrap actual AST types. In that case, we should add Cell as an actual wrapper.

[overlookmotel]

Thanks for clarifying. I hadn't clocked that layout for Cell<Option<ScopeId>> etc are hard-coded. That's totally fine for now.

[graphite-app]

Merge activity

• Aug 3, 8:11 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Aug 3, 8:11 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
• Aug 3, 8:14 AM EDT: overlookmotel merged this pull request with the Graphite merge queue.