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

Lifetimes of AST entangled with parser? #79

Open
Fee0 opened this issue Nov 28, 2022 · 2 comments
Open

Lifetimes of AST entangled with parser? #79

Fee0 opened this issue Nov 28, 2022 · 2 comments

Comments

@Fee0
Copy link

Fee0 commented Nov 28, 2022

It seems the lifetime of an AST is entangled with the parser itself:

pub fn js_to_ast(js: &str) -> Program {
    let mut p = Parser::new(js).unwrap();
    let ast = p.parse().unwrap();
    ast
}
// error[E0515]: cannot return value referencing local variable `p`
// |
// 177 |     let ast = p.parse().unwrap();
// |               --------- `p` is borrowed here
// 178 |     ast
//     |     ^^^ returns a value referencing data owned by the current function

I think the AST should be completely independent from parser/scanner etc.
Otherwise we always needs to have the parser object around with our AST.

@FreeMasen
Copy link
Collaborator

At the very least the AST will always be tied to the lifetime of the js text used to parse construct the parser. The parser's lifetime is only there to allow for the provided &str to not need to be copied. It should be possible to line up the lifetimes so that the parser's lifetime is explicitly smaller than the &str's lifetime and then apply the lifetime of the &str to the AST returned. I haven't tried to line that up as of yet.

Since the AST is really just a wrapper around Cow<'a, str>s, the process of detaching an AST from the original &str ends up re-allocating the full string in chunks which I thought would be quite expensive. We could potentially provide helpers for the process of deep cloning the AST nodes into owned variants of the Cow but the work hasn't been done there just yet.

@Fee0
Copy link
Author

Fee0 commented Nov 29, 2022

Ah makes sense to not force a copy of the js text.
But maybe giving the parser builder an option to create a copy while parsing?
This would prevent walking and cloning the full AST later again. And we also don't need to provide extra deep clone functionality? It seems simply ast.clone() does not work with the Cow's.

Making the parser lifetime shorter than the string is probably a good idea too.

Thank you for clarifying, I was a bit confused about these things.

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