rusty-ecma / RESSA

Rusty EcmaScript Syntax Analyzer
MIT License
111 stars 14 forks source link

Lifetimes of AST entangled with parser? #79

Open Fee0 opened 1 year ago

Fee0 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.