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

Add extend method for UserDataMap #350

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sytten
Copy link
Contributor

@Sytten Sytten commented Aug 3, 2024

I am working on a new ModuleDef extension that is stateful.
I decided to use the userdata to store the options that are later retrieved.
I faced an issue where I wanted my module loader builder to not have to lock the runtime on each new module.

My solution is to allow users to build a UserDataMap themselves and then allow to extend the runtime one when you get the lock. Lifetimes other than static obviously won't work, but my options are static anyway so I don't care about that.

I am open to other suggestions, it would obviously be easier if we made the ModuleDef stateful, but that would break a lot of existing code.

#[derive(Debug, Default)]
pub struct ModuleLoaderBuilder {
    definitions: HashMap<&'static str, LoadFn>,
    globals: HashMap<&'static str, GlobalFn>,
    // Accumulate the options in UserDataMap
}

impl ModuleLoaderBuilder {
    pub async fn add_module<M: ModuleDefExt>(&mut self, module: M, global: bool) -> &mut Self {
        self.definitions.insert(M::NAME, load_func::<M>);
        if global {
            self.globals.insert(M::NAME, global_func::<M>);
        }
        // Store the options
        self
    }
}

My trait

pub trait ModuleDefExt: ModuleDef {
    const NAME: &'static str;

    type Options<'js>: UserData<'js>;

    fn declare(decl: &Declarations<'_>) -> Result<()> {
        let _ = decl;
        Ok(())
    }

    fn evaluate<'js>(
        options: &Self::Options<'js>,
        ctx: &Ctx<'js>,
        exports: &Exports<'js>,
    ) -> Result<()> {
        let _ = (options, exports, ctx);
        Ok(())
    }

    fn globals<'js>(options: &Self::Options<'js>, globals: &Object<'js>) -> Result<()> {
        let _ = (globals);
        Ok(())
    }

    fn options(self) -> Result<Self::Options<'static>>;
}

Full code for my experiment is here: https://github.com/rquickjs/rquickjs-moduledef-ext

@DelSkayn
Copy link
Owner

DelSkayn commented Aug 5, 2024

This is unsound, UserDataMap is not designed to be used outside of internal rquickjs code. It erases the 'js lifetime from rquickjs types which is critical to rquickjs it's safety. With an exposed UserDataMap smuggling a quickjs type outside of the with closure is as easy as:

/// Ctx object now outside of the with closure!
let ctx: Ctx = ctx.with(|ctx|{
    let mut map = UserDataMap::default();
    map.insert(ctx).ok();
    map.remove::<Ctx>().unwrap()
});

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

Successfully merging this pull request may close these issues.

2 participants