Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement conversion methods between Vecs of AST enums which inherit from each other #153

Open
overlookmotel opened this issue Dec 13, 2024 · 6 comments

Comments

@overlookmotel
Copy link

overlookmotel commented Dec 13, 2024

We have zero-cost conversion methods for converting e.g. From<Expression> for Argument. It's zero cost because Argument inherits Expression's variants, so a valid Expression is also a valid Argument, and the conversion is just an in-place transmute.

Implement the same for converting Vecs. e.g. Vec<'a, Expression<'a>> should be able to be converted to Vec<'a, Argument<'a>> as a zero cost transmute - without memory copy or allocation.

Ditto From<Box<'a, Expression<'a>>> for Box<'a, Argument<'a>>. Edit: No point. We never box enums.

The code touched in oxc-project/oxc#7854 is an example of where we could use this for a more efficient operation.

@Dunqing
Copy link
Member

Dunqing commented Dec 13, 2024

I am interested in trying it

@overlookmotel
Copy link
Author

The current conversions use safe code only and rely on compiler to optimize it. But I think these ones would need unsafe code (mem::transmute) - I don't think compiler would understand enough to do the optimization itself with only safe code.

impl<'a> From<Vec<'a, Expression<'a>> for Vec<'a, Argument<'a>> {
    fn from(vec: Vec<'a, Expression<'a>) -> Self {
        // SAFETY: Maybe this is safe!
        unsafe { std::mem::transmute(vec) }
    }
}

@overlookmotel overlookmotel changed the title Implement conversion methods between Vecs and Boxes of AST enums which inherit from each other Implement conversion methods between Vecs of AST enums which inherit from each other Dec 13, 2024
@overlookmotel
Copy link
Author

We could maybe also add APIs to map from a Vec of one type to a Vec of another type, reusing the same allocation.

@Dunqing
Copy link
Member

Dunqing commented Dec 13, 2024

The current conversions use safe code only and rely on compiler to optimize it. But I think these ones would need unsafe code (mem::transmute) - I don't think compiler would understand enough to do the optimization itself with only safe code.

impl<'a> From<Vec<'a, Expression<'a>> for Vec<'a, Argument<'a>> {
    fn from(vec: Vec<'a, Expression<'a>) -> Self {
        // SAFETY: Maybe this is safe!
        unsafe { std::mem::transmute(vec) }
    }
}

Vec is not defined in ast crate, so it seems we can't do that

@overlookmotel
Copy link
Author

overlookmotel commented Dec 13, 2024

I'm not 100% sure if this is safe. I think it is, but would like to think about it more carefully.

If we decide it is safe, and we want to do it, I think we should implement for all inherited enums with ast_tools.

(a) It's less error-prone to generate unsafe code than to write it by hand, and
(b) we can do it for loads of types, e.g. also Vec<MemberExpression> -> Vec<Expression>, Vec<Declaration> -> Vec<Statement>.

It's not going to be a massive perf win, so I think no hurry.

@overlookmotel
Copy link
Author

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 if you get a pointer to an element in a Vec<Expression>, that pointer remains a valid *const Expression for the life of the allocator, even if the Vec reallocates. This is because allocator is simple and doesn't reuse blocks of memory.

But if you can convert a Vec<Expression> to a Vec<Argument>, then that invariant no longer holds.

We don't currently make use of that invariant, but we may want to in future. So by doing this, we close off other possibilities.

That's the part I'm not sure about - is it a good idea or not? Do we care about this invariant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants