At the moment, the generated code has lots of codes that might lead to undefined behavior: https://github.com/dprint/dprint-swc-ecma-ast-view/blob/dfc9347015603495579eec592b27ca301abb093e/rs-lib/src/generated.rs#L7097-L7111
Here MaybeUninit::uninit().assume_init()
is called, but Rust doesn't allow us to invoke assume_init()
on values that are not properly initialized in memory. It looks like the code works fine for now, but there's no guarantee that it will keep working.
So I'd like to propose replacing these codes to avoid undefined behavior. I think this refactoring is not so easy or simple but it's worth it.
Here are my thoughts on how to refactor:
- make all fields of Node structs (such as
BindingIdent
) private and create getter methods for them
- change type of fields that currently use
MaybeUninit::uninit().assume_init()
(such as id
of BindingIdent
) to Cell<Option<..>>
. They become None
only when being constructed. Once the AST view has been completely created, they should be Some
. Therefore, in the getter methods, we can safely call .unwrap()
- rewrite the construction of the view
As a whole, BindingIdent
could be refactored into the following:
// Make all fields private
pub struct BindingIdent<'a> {
parent: Node<'a>,
inner: &'a swc_ast::BindingIdent,
id: Cell<Option<&'a Ident<'a>>>, // wrap in Cell
type_ann: Cell<Option<&'a TsTypeAnn<'a>>>, // wrap in Cell
}
// Instead, expose getters for all fields
impl<'a> BindingIdent<'a> {
pub fn parent(&self) -> Node<'a> {
self.parent
}
pub fn inner(&self) -> &'a swc_ast::BindingIdent {
self.inner
}
pub fn id(&self) -> &'a Ident<'a> {
self.id.get().unwrap() // when the library users call it, this `unwrap` is totally safe
}
pub fn type_ann(&self) -> Option<&'a TsTypeAnn<'a>> {
self.type_ann.get()
}
}
fn get_view_for_binding_ident<'a>(inner: &'a swc_ast::BindingIdent, parent: Node<'a>, bump: &'a Bump) -> &'a BindingIdent<'a> {
let node = bump.alloc(BindingIdent {
inner,
parent,
id: Cell::new(None), // wrap in Cell
type_ann: Cell::new(None), // wrap in Cell
});
let parent: Node<'a> = (&*node).into();
node.id.set(Some(get_view_for_ident(&inner.id, parent.clone(), bump)));
node.type_ann.set(match &inner.type_ann {
Some(value) => Some(get_view_for_ts_type_ann(value, parent, bump)),
None => None,
});
node
}
As a side note: In the above example, type_ann
is not required to wrap in Cell
as it is not related to MaybeUninit
. But it's preferable because the assignment node.type_ann = ...
requires mutable reference to node
while parent
holds immutable reference to node
. The Rust compiler is supposed to warn it, but somehow it compiles (probably due to std::mem::transmute
in From
trait impl.) For immutable reference and mutable reference not to exist at the same time, Cell
is helpful.
Let me know what you think about this refactoring :)