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

Reusing Codeblocks between Contexts is not properly supported #2626

Closed
jedel1043 opened this issue Mar 1, 2023 · 2 comments · Fixed by #3424
Closed

Reusing Codeblocks between Contexts is not properly supported #2626

jedel1043 opened this issue Mar 1, 2023 · 2 comments · Fixed by #3424
Labels
API bug Something isn't working discussion Issues needing more discussion

Comments

@jedel1043
Copy link
Member

jedel1043 commented Mar 1, 2023

Describe the bug
Trying to run a Codeblock compiled by a Context in a separate Context causes a panic on runtime.

To Reproduce

use boa_engine::{Context, Source};

fn main() {
    let script = r#"
        a = 5;
    "#;

    let context = &mut Context::default();
    let code = context.parse_script(Source::from_bytes(script)).unwrap();
    let code = context.compile_script(&code).unwrap();

    println!(
        "First execution: {}",
        context.execute(code.clone()).unwrap().display()
    );

    let context = &mut Context::default();

    println!(
        "Second execution: {}",
        context.execute(code.clone()).unwrap().display()
    );
}

This uses the main branch of Boa, but it should be also reproducible from the latest released version.

Expected behaviour

Either run correctly or throw an error saying that codeblocks cannot be reused on other Contexts.

Current behaviour

First execution: 5
thread 'main' panicked at 'string disappeared', /home/jedel/SDK/src/Rust/boa/boa_interner/src/lib.rs:422:30
... (stack trace)

Build environment:

  • OS: Arch linux
  • Version: 6.1.11-arch1-1
  • Target triple: stable-x86_64-unknown-linux-gnu
  • Rustc version: rustc 1.67.1 (d5a82bbd2 2023-02-07)

Additional context (no pun intented):

This is where the fun part begins. AFAIK, most engines support using codeblocks between contexts, but we don't support that because of two reasons:

  • Codeblocks use Syms to internally represent variable bindings. This assumes that the containing Context has on its interner the binding names used in the execution.
  • Our compiler doesn't consider already defined global bindings in the Context itself.

To illustrate the second point, this code correctly throws:

use boa_engine::{Context, Source};

fn main() {
    let script = r#"
        let a = 50;
        {
            var a = 5;
        }
        a
    "#;

    let context = &mut Context::default();

    println!(
        "First execution: {}",
        context
            .parse_script(Source::from_bytes(script))
            .unwrap_err()
    );
}

While this code runs and returns 5:

fn main() {
    let script = r#"
        {
            var a = 5;
        }
        a
    "#;

    let context = &mut Context::default();
    let code = context.parse_script(Source::from_bytes(script)).unwrap();
    let code = context.compile_script(&code).unwrap();

    println!(
        "First execution: {}",
        context.execute(code.clone()).unwrap().display()
    );

    let context = &mut Context::default();

    context.eval_script(Source::from_bytes("let a = 50")).unwrap();
    println!(
        "Second execution: {}",
        context.execute(code.clone()).unwrap().display()
    );
}

Discussion

Having said all of that, we have possibly two paths forward:

  • Support this properly, which implies that we need to separate our compilation pipeline from the execution context itself and add runtime checks that disallow reusing lexical binding names on contexts that already have them defined, with the downside of having to do a possibly big refactor on how we currently handle environments and bindings.
  • Disallow sharing compiled scripts between contexts, which eases our implementation work but now users don't have a way to cache compiled scripts for several contexts.
@jedel1043 jedel1043 added bug Something isn't working discussion Issues needing more discussion API labels Mar 1, 2023
@jedel1043 jedel1043 changed the title Reusing Codeblocks between Contexts is not property supported. Reusing Codeblocks between Contexts is not properly supported. Mar 2, 2023
@jedel1043 jedel1043 changed the title Reusing Codeblocks between Contexts is not properly supported. Reusing Codeblocks between Contexts is not properly supported Mar 2, 2023
@Razican
Copy link
Member

Razican commented Mar 23, 2023

For the first issue (the ussage of Sym), we could just embed the interned strings in the codeblock and serialize that into an immutable struct. This could also allow caching it on disk.
The second issue I didn't understand fully XD

@jedel1043
Copy link
Member Author

jedel1043 commented Mar 23, 2023

For the first issue (the ussage of Sym), we could just embed the interned strings in the codeblock and serialize that into an immutable struct. This could also allow caching it on disk.

Yeah, I've recently thought that it would be better if the interner was stored in the AST.

The second issue I didn't understand fully XD

The crux of the matter would be that our compiler only considers the internal bindings of a codeblock to check for errors, but it should also consider context bindings defined in previous executions for it to be correct, and those checks can only be done at runtime after passing the compiled codeblock to the context.

This implies that codeblocks should be independent from contexts for them to be reusable between executions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API bug Something isn't working discussion Issues needing more discussion
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants