refactor(transform): retag_stack use AncestorType (#3173)

Make the code for `retag_stack` in `walk_*` functions more
comprehensible, by replacing hard-coded enum discriminants as integers
with an `AncestorType` type.

Alternative solution to the problem raised in #3170.

close: #3170
This commit is contained in:
overlookmotel 2024-05-06 18:53:41 +01:00 committed by GitHub
parent 4e9b9d9e09
commit 762677e17b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 1000 additions and 463 deletions

View file

@ -2,7 +2,8 @@ import {camelToSnake, snakeToCamel} from './utils.mjs';
export default function generateAncestorsCode(types) { export default function generateAncestorsCode(types) {
const variantNamesForEnums = Object.create(null); const variantNamesForEnums = Object.create(null);
let enumVariants = '', let ancestorTypeEnumVariants = '',
ancestorEnumVariants = '',
isFunctions = '', isFunctions = '',
ancestorTypes = '', ancestorTypes = '',
discriminant = 1; discriminant = 1;
@ -62,8 +63,8 @@ export default function generateAncestorsCode(types) {
const variantName = `${type.name}${fieldNameCamel}`; const variantName = `${type.name}${fieldNameCamel}`;
variantNames.push(variantName); variantNames.push(variantName);
enumVariants += `${variantName}(${structName}) = ${discriminant},\n`; ancestorTypeEnumVariants += `${variantName} = ${discriminant},\n`;
field.ancestorDiscriminant = discriminant; ancestorEnumVariants += `${variantName}(${structName}) = AncestorType::${variantName} as u16,\n`;
discriminant++; discriminant++;
if (fieldType.kind === 'enum') { if (fieldType.kind === 'enum') {
@ -96,8 +97,6 @@ export default function generateAncestorsCode(types) {
`; `;
} }
const discriminantType = discriminant <= 256 ? 'u8' : 'u16';
return ` return `
#![allow( #![allow(
unsafe_code, unsafe_code,
@ -117,20 +116,34 @@ export default function generateAncestorsCode(types) {
AssignmentOperator, BinaryOperator, LogicalOperator, UnaryOperator, UpdateOperator, AssignmentOperator, BinaryOperator, LogicalOperator, UnaryOperator, UpdateOperator,
}; };
pub(crate) type AncestorDiscriminant = ${discriminantType}; /// Type of [\`Ancestor\`].
/// Used in [\`crate::TraverseCtx::retag_stack\`].
#[repr(u16)]
#[derive(Clone, Copy)]
#[allow(dead_code)]
pub(crate) enum AncestorType {
None = 0,
${ancestorTypeEnumVariants}
}
/// Ancestor type used in AST traversal. /// Ancestor type used in AST traversal.
/// ///
/// Encodes both the type of the parent, and child's location in the parent. /// Encodes both the type of the parent, and child's location in the parent.
/// i.e. variants for \`BinaryExpressionLeft\` and \`BinaryExpressionRight\`, not just \`BinaryExpression\`. /// i.e. variants for \`BinaryExpressionLeft\` and \`BinaryExpressionRight\`, not just \`BinaryExpression\`.
// //
// SAFETY: This type MUST be \`#[repr(u8)]\` or \`#[repr(u16)]\` (depending on number of variants) // SAFETY:
// to maintain the safety of \`TraverseCtx::retag_stack\`. // * This type must be \`#[repr(u16)]\`.
#[repr(C, ${discriminantType})] // * Variant discriminants must correspond to those in \`AncestorType\`.
//
// These invariants make it possible to set the discriminant of an \`Ancestor\` without altering
// the "payload" pointer with:
// \`*(ancestor as *mut _ as *mut AncestorType) = AncestorType::Program\`.
// \`TraverseCtx::retag_stack\` uses this technique.
#[repr(C, u16)]
#[derive(Debug)] #[derive(Debug)]
pub enum Ancestor<'a> { pub enum Ancestor<'a> {
None = 0, None = AncestorType::None as u16,
${enumVariants} ${ancestorEnumVariants}
} }
impl<'a> Ancestor<'a> { impl<'a> Ancestor<'a> {

View file

@ -27,7 +27,7 @@ export default function generateWalkFunctionsCode(types) {
#[allow(clippy::wildcard_imports)] #[allow(clippy::wildcard_imports)]
use oxc_ast::ast::*; use oxc_ast::ast::*;
use crate::{ancestor, Ancestor, Traverse, TraverseCtx}; use crate::{ancestor::{self, AncestorType}, Ancestor, Traverse, TraverseCtx};
${walkMethods} ${walkMethods}
@ -51,8 +51,10 @@ function generateWalkForStruct(type, types) {
const fieldsCodes = visitedFields.map((field, index) => { const fieldsCodes = visitedFields.map((field, index) => {
const fieldWalkName = `walk_${camelToSnake(field.innerTypeName)}`; const fieldWalkName = `walk_${camelToSnake(field.innerTypeName)}`;
const retagCode = index === 0 ? '' : `ctx.retag_stack(${field.ancestorDiscriminant});`, const retagCode = index === 0
fieldCode = `(node as *mut u8).add(ancestor::${field.offsetVarName}) as *mut ${field.typeName}`; ? ''
: `ctx.retag_stack(AncestorType::${type.name}${snakeToCamel(field.name)});`;
const fieldCode = `(node as *mut u8).add(ancestor::${field.offsetVarName}) as *mut ${field.typeName}`;
if (field.wrappers[0] === 'Option') { if (field.wrappers[0] === 'Option') {
let walkCode; let walkCode;

File diff suppressed because it is too large Load diff

View file

@ -1,7 +1,7 @@
use oxc_allocator::{Allocator, Box}; use oxc_allocator::{Allocator, Box};
use oxc_ast::AstBuilder; use oxc_ast::AstBuilder;
use crate::ancestor::{Ancestor, AncestorDiscriminant}; use crate::ancestor::{Ancestor, AncestorType};
const INITIAL_STACK_CAPACITY: usize = 64; const INITIAL_STACK_CAPACITY: usize = 64;
@ -119,25 +119,21 @@ impl<'a> TraverseCtx<'a> {
/// of pointer to the ancestor node. /// of pointer to the ancestor node.
/// ///
/// This is purely a performance optimization. If the last item on stack already contains the /// This is purely a performance optimization. If the last item on stack already contains the
/// correct pointer, then `ctx.retag_stack(3)` is equivalent to: /// correct pointer, then `ctx.retag_stack(AncestorType::ProgramBody)` is equivalent to:
/// ///
/// ```nocompile /// ```nocompile
/// ctx.pop_stack(); /// ctx.pop_stack();
/// ctx.push_stack(Ancestor::ProgramBody(ProgramWithoutBody(node_ptr))); /// ctx.push_stack(Ancestor::ProgramBody(ProgramWithoutBody(node_ptr)));
/// ``` /// ```
/// ///
/// (3 is the discriminant for `Ancestor::ProgramBody`)
///
/// `retag_stack` is only a single 2-byte write operation. /// `retag_stack` is only a single 2-byte write operation.
/// ///
/// # SAFETY /// # SAFETY
/// * Stack must not be empty. /// * Stack must not be empty.
/// * `discriminant` must be valid discriminant value for `Ancestor` enum. /// * Last item on stack must contain pointer to type corresponding to provided `AncestorType`.
/// * Last item on stack must contain pointer to type corresponding to provided discriminant.
#[inline] #[inline]
#[allow(unsafe_code, clippy::ptr_as_ptr, clippy::ref_as_ptr)] #[allow(unsafe_code, clippy::ptr_as_ptr, clippy::ref_as_ptr)]
pub(crate) unsafe fn retag_stack(&mut self, discriminant: AncestorDiscriminant) { pub(crate) unsafe fn retag_stack(&mut self, ty: AncestorType) {
*(self.stack.last_mut().unwrap_unchecked() as *mut _ as *mut AncestorDiscriminant) = *(self.stack.last_mut().unwrap_unchecked() as *mut _ as *mut AncestorType) = ty;
discriminant;
} }
} }

File diff suppressed because it is too large Load diff