Skip to content

Commit

Permalink
refactor(ast, isolated_declarations, transformer): mark `AstBuilder::…
Browse files Browse the repository at this point in the history
…copy` as an unsafe function (#4907)

`AstBuilder::copy` is completely unsound (#3483), and we need to remove it. Make it an `unsafe` function to discourage any further usage of it in meantime.
  • Loading branch information
overlookmotel committed Aug 15, 2024
1 parent 2476dce commit 229e38d
Show file tree
Hide file tree
Showing 18 changed files with 267 additions and 115 deletions.
4 changes: 2 additions & 2 deletions crates/oxc_ast/src/ast_builder_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@ impl<'a> AstBuilder<'a> {
/// This method is completely unsound and should not be used.
/// We need to remove all uses of it. Please don't add any more!
/// <https://github.com/oxc-project/oxc/issues/3483>
#[allow(clippy::missing_safety_doc)]
#[inline]
pub fn copy<T>(self, src: &T) -> T {
pub unsafe fn copy<T>(self, src: &T) -> T {
// SAFETY: Not safe (see above)

unsafe { std::mem::transmute_copy(src) }
}

Expand Down
79 changes: 54 additions & 25 deletions crates/oxc_isolated_declarations/src/class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ impl<'a> IsolatedDeclarations<'a> {

if property.accessibility.map_or(true, |a| !a.is_private()) {
if property.type_annotation.is_some() {
type_annotations = self.ast.copy(&property.type_annotation);
// SAFETY: `ast.copy` is unsound! We need to fix.
type_annotations = unsafe { self.ast.copy(&property.type_annotation) };
} else if let Some(expr) = property.value.as_ref() {
let ts_type = if property.readonly {
// `field = 'string'` remain `field = 'string'` instead of `field: 'string'`
Expand All @@ -71,7 +72,8 @@ impl<'a> IsolatedDeclarations<'a> {
.transform_template_to_string(lit)
.map(Expression::StringLiteral);
} else {
value = Some(self.ast.copy(expr));
// SAFETY: `ast.copy` is unsound! We need to fix.
value = Some(unsafe { self.ast.copy(expr) });
}
None
}
Expand All @@ -91,7 +93,8 @@ impl<'a> IsolatedDeclarations<'a> {
property.r#type,
property.span,
self.ast.vec(),
self.ast.copy(&property.key),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&property.key) },
value,
property.computed,
property.r#static,
Expand All @@ -116,12 +119,15 @@ impl<'a> IsolatedDeclarations<'a> {
let value = self.ast.alloc_function(
FunctionType::TSEmptyBodyFunctionExpression,
function.span,
self.ast.copy(&function.id),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&function.id) },
false,
false,
false,
self.ast.copy(&function.type_parameters),
self.ast.copy(&function.this_param),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&function.type_parameters) },
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&function.this_param) },
params,
return_type,
Option::<FunctionBody>::None,
Expand All @@ -131,7 +137,8 @@ impl<'a> IsolatedDeclarations<'a> {
definition.r#type,
definition.span,
self.ast.vec(),
self.ast.copy(&definition.key),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&definition.key) },
value,
definition.kind,
definition.computed,
Expand Down Expand Up @@ -209,7 +216,8 @@ impl<'a> IsolatedDeclarations<'a> {
};
self.create_class_property(
r#type,
self.ast.copy(&method.key),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&method.key) },
method.r#static,
method.r#override,
self.transform_accessibility(method.accessibility),
Expand Down Expand Up @@ -247,7 +255,8 @@ impl<'a> IsolatedDeclarations<'a> {
None
} else {
// transformed params will definitely have type annotation
self.ast.copy(&params.items[index].pattern.type_annotation)
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&params.items[index].pattern.type_annotation) }
};
if let Some(new_element) =
self.transform_formal_parameter_to_class_property(param, type_annotation)
Expand Down Expand Up @@ -289,7 +298,10 @@ impl<'a> IsolatedDeclarations<'a> {
MethodDefinitionKind::Get => {
let return_type = self.infer_function_return_type(function);
if let Some(return_type) = return_type {
inferred_accessor_types.insert(name, self.ast.copy(&return_type));
inferred_accessor_types.insert(name, {
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&return_type) }
});
}
}
MethodDefinitionKind::Set => {
Expand All @@ -300,7 +312,10 @@ impl<'a> IsolatedDeclarations<'a> {
self.infer_type_from_formal_parameter(param)
.map(|x| self.ast.alloc_ts_type_annotation(SPAN, x))
},
|t| Some(self.ast.copy(t)),
|t| {
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { Some(self.ast.copy(t)) }
},
);
if let Some(type_annotation) = type_annotation {
inferred_accessor_types.insert(name, type_annotation);
Expand Down Expand Up @@ -373,9 +388,10 @@ impl<'a> IsolatedDeclarations<'a> {
|n| {
self.transform_set_accessor_params(
&function.params,
inferred_accessor_types
.get(&self.ast.atom(&n))
.map(|t| self.ast.copy(t)),
inferred_accessor_types.get(&self.ast.atom(&n)).map(|t| {
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(t) }
}),
)
},
)
Expand Down Expand Up @@ -403,9 +419,10 @@ impl<'a> IsolatedDeclarations<'a> {
}
MethodDefinitionKind::Get => {
let rt = method.key.static_name().and_then(|name| {
inferred_accessor_types
.get(&self.ast.atom(&name))
.map(|t| self.ast.copy(t))
inferred_accessor_types.get(&self.ast.atom(&name)).map(|t| {
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(t) }
})
});
if rt.is_none() {
self.error(accessor_must_have_explicit_return_type(
Expand Down Expand Up @@ -446,14 +463,18 @@ impl<'a> IsolatedDeclarations<'a> {
property.r#type,
property.span,
self.ast.vec(),
self.ast.copy(&property.key),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&property.key) },
None,
property.computed,
property.r#static,
);
elements.push(new_element);
}
ClassElement::TSIndexSignature(_) => elements.push(self.ast.copy(element)),
ClassElement::TSIndexSignature(_) => elements.push({
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(element) }
}),
}
}

Expand Down Expand Up @@ -490,11 +511,16 @@ impl<'a> IsolatedDeclarations<'a> {
decl.r#type,
decl.span,
self.ast.vec(),
self.ast.copy(&decl.id),
self.ast.copy(&decl.type_parameters),
self.ast.copy(&decl.super_class),
self.ast.copy(&decl.super_type_parameters),
self.ast.copy(&decl.implements),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.id) },
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.type_parameters) },
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.super_class) },
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.super_type_parameters) },
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.implements) },
body,
decl.r#abstract,
declare.unwrap_or_else(|| self.is_declare()),
Expand All @@ -510,7 +536,10 @@ impl<'a> IsolatedDeclarations<'a> {
if items.first().map_or(true, |item| item.pattern.type_annotation.is_none()) {
let kind = items.first().map_or_else(
|| self.ast.binding_pattern_kind_binding_identifier(SPAN, "value"),
|item| self.ast.copy(&item.pattern.kind),
|item| {
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&item.pattern.kind) }
},
);

self.create_formal_parameters(kind, type_annotation)
Expand Down
32 changes: 22 additions & 10 deletions crates/oxc_isolated_declarations/src/declaration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ impl<'a> IsolatedDeclarations<'a> {
init =
self.transform_template_to_string(lit).map(Expression::StringLiteral);
} else {
init = Some(self.ast.copy(init_expr));
// SAFETY: `ast.copy` is unsound! We need to fix.
init = Some(unsafe { self.ast.copy(init_expr) });
}
} else if !decl.kind.is_const()
|| !matches!(init_expr, Expression::TemplateLiteral(_))
Expand All @@ -94,10 +95,14 @@ impl<'a> IsolatedDeclarations<'a> {
}
}
let id = binding_type.map_or_else(
|| self.ast.copy(&decl.id),
|| {
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.id) }
},
|ts_type| {
self.ast.binding_pattern(
self.ast.copy(&decl.id.kind),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.id.kind) },
Some(self.ast.ts_type_annotation(SPAN, ts_type)),
decl.id.optional,
)
Expand Down Expand Up @@ -149,19 +154,22 @@ impl<'a> IsolatedDeclarations<'a> {
decl: &Box<'a, TSModuleDeclaration<'a>>,
) -> Box<'a, TSModuleDeclaration<'a>> {
if decl.declare {
return self.ast.copy(decl);
// SAFETY: `ast.copy` is unsound! We need to fix.
return unsafe { self.ast.copy(decl) };
}

let Some(body) = &decl.body else {
return self.ast.copy(decl);
// SAFETY: `ast.copy` is unsound! We need to fix.
return unsafe { self.ast.copy(decl) };
};

match body {
TSModuleDeclarationBody::TSModuleDeclaration(decl) => {
let inner = self.transform_ts_module_declaration(decl);
self.ast.alloc_ts_module_declaration(
decl.span,
self.ast.copy(&decl.id),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.id) },
Some(TSModuleDeclarationBody::TSModuleDeclaration(inner)),
decl.kind,
self.is_declare(),
Expand All @@ -171,7 +179,8 @@ impl<'a> IsolatedDeclarations<'a> {
let body = self.transform_ts_module_block(block);
self.ast.alloc_ts_module_declaration(
decl.span,
self.ast.copy(&decl.id),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.id) },
Some(TSModuleDeclarationBody::TSModuleBlock(body)),
decl.kind,
self.is_declare(),
Expand Down Expand Up @@ -213,15 +222,17 @@ impl<'a> IsolatedDeclarations<'a> {
Declaration::TSTypeAliasDeclaration(alias_decl) => {
self.visit_ts_type_alias_declaration(alias_decl);
if !check_binding || self.scope.has_reference(&alias_decl.id.name) {
Some(self.ast.copy(decl))
// SAFETY: `ast.copy` is unsound! We need to fix.
Some(unsafe { self.ast.copy(decl) })
} else {
None
}
}
Declaration::TSInterfaceDeclaration(interface_decl) => {
self.visit_ts_interface_declaration(interface_decl);
if !check_binding || self.scope.has_reference(&interface_decl.id.name) {
Some(self.ast.copy(decl))
// SAFETY: `ast.copy` is unsound! We need to fix.
Some(unsafe { self.ast.copy(decl) })
} else {
None
}
Expand Down Expand Up @@ -250,7 +261,8 @@ impl<'a> IsolatedDeclarations<'a> {
}
Declaration::TSImportEqualsDeclaration(decl) => {
if !check_binding || self.scope.has_reference(&decl.id.name) {
Some(Declaration::TSImportEqualsDeclaration(self.ast.copy(decl)))
// SAFETY: `ast.copy` is unsound! We need to fix.
Some(Declaration::TSImportEqualsDeclaration(unsafe { self.ast.copy(decl) }))
} else {
None
}
Expand Down
6 changes: 4 additions & 2 deletions crates/oxc_isolated_declarations/src/enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ impl<'a> IsolatedDeclarations<'a> {

let member = self.ast.ts_enum_member(
member.span,
self.ast.copy(&member.id),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&member.id) },
value.map(|v| match v {
ConstantValue::Number(v) => {
let is_negative = v < 0.0;
Expand Down Expand Up @@ -94,7 +95,8 @@ impl<'a> IsolatedDeclarations<'a> {

Some(self.ast.declaration_ts_enum(
decl.span,
self.ast.copy(&decl.id),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&decl.id) },
members,
decl.r#const,
self.is_declare(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ pub struct FormalParameterBindingPattern<'a> {
impl<'a> VisitMut<'a> for FormalParameterBindingPattern<'a> {
fn visit_binding_pattern_kind(&mut self, kind: &mut BindingPatternKind<'a>) {
if let BindingPatternKind::AssignmentPattern(assignment) = kind {
*kind = self.ast.copy(&assignment.left.kind);
// SAFETY: `ast.copy` is unsound! We need to fix.
*kind = unsafe { self.ast.copy(&assignment.left.kind) };
}

walk_binding_pattern_kind(self, kind);
Expand Down
29 changes: 20 additions & 9 deletions crates/oxc_isolated_declarations/src/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,15 @@ impl<'a> IsolatedDeclarations<'a> {
Some(self.ast.alloc_function(
func.r#type,
func.span,
self.ast.copy(&func.id),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&func.id) },
false,
false,
declare.unwrap_or_else(|| self.is_declare()),
self.ast.copy(&func.type_parameters),
self.ast.copy(&func.this_param),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&func.type_parameters) },
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&func.this_param) },
params,
return_type,
Option::<FunctionBody>::None,
Expand All @@ -61,9 +64,11 @@ impl<'a> IsolatedDeclarations<'a> {
let is_assignment_pattern = pattern.kind.is_assignment_pattern();
let mut pattern =
if let BindingPatternKind::AssignmentPattern(pattern) = &param.pattern.kind {
self.ast.copy(&pattern.left)
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&pattern.left) }
} else {
self.ast.copy(&param.pattern)
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&param.pattern) }
};

FormalParameterBindingPattern::remove_assignments_from_kind(self.ast, &mut pattern.kind);
Expand All @@ -72,7 +77,10 @@ impl<'a> IsolatedDeclarations<'a> {
let type_annotation = pattern
.type_annotation
.as_ref()
.map(|type_annotation| self.ast.copy(&type_annotation.type_annotation))
.map(|type_annotation| {
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&type_annotation.type_annotation) }
})
.or_else(|| {
// report error for has no type annotation
let new_type = self.infer_type_from_formal_parameter(param);
Expand Down Expand Up @@ -106,7 +114,8 @@ impl<'a> IsolatedDeclarations<'a> {
});

pattern = self.ast.binding_pattern(
self.ast.copy(&pattern.kind),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&pattern.kind) },
type_annotation,
// if it's assignment pattern, it's optional
pattern.optional || (!is_remaining_params_have_required && is_assignment_pattern),
Expand All @@ -121,7 +130,8 @@ impl<'a> IsolatedDeclarations<'a> {
params: &FormalParameters<'a>,
) -> Box<'a, FormalParameters<'a>> {
if params.kind.is_signature() || (params.rest.is_none() && params.items.is_empty()) {
return self.ast.alloc(self.ast.copy(params));
// SAFETY: `ast.copy` is unsound! We need to fix.
return self.ast.alloc(unsafe { self.ast.copy(params) });
}

let items =
Expand All @@ -143,7 +153,8 @@ impl<'a> IsolatedDeclarations<'a> {
params.span,
FormalParameterKind::Signature,
items,
self.ast.copy(&params.rest),
// SAFETY: `ast.copy` is unsound! We need to fix.
unsafe { self.ast.copy(&params.rest) },
)
}
}
Expand Down
Loading

0 comments on commit 229e38d

Please sign in to comment.