Skip to content

Support using rooted macro with vectors.#697

Merged
jdm merged 6 commits intoservo:mainfrom
jdm:rooted-vec-value
Jan 18, 2026
Merged

Support using rooted macro with vectors.#697
jdm merged 6 commits intoservo:mainfrom
jdm:rooted-vec-value

Conversation

@jdm
Copy link
Member

@jdm jdm commented Jan 15, 2026

We created our RootedVec type back before we supported storing arbitrary traceable types in SpiderMonkey's Rooted class. Now that we can do that, we can replicate SpiderMonkey's RootedVec, which is just a Rooted wrapper around a vector.

Testing: New unit test added.
Servo PR: servo/servo#41921

Signed-off-by: Josh Matthews <josh@joshmatthews.net>
@jdm
Copy link
Member Author

jdm commented Jan 15, 2026

Ack, there was a new sccache release but it doesn't include x86_64 binaries for macOS, only arm64, so our x86_64 macOS build is busted.

Comment on lines +52 to +59
impl From<&super::RootedGuard<'_, Vec<JSVal>>> for JS::HandleValueArray {
fn from(vec: &super::RootedGuard<'_, Vec<JSVal>>) -> JS::HandleValueArray {
JS::HandleValueArray {
length_: vec.len(),
elements_: vec.deref().as_ptr(),
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make more sense to impl RootedGuard<'_, Vec<JSVal>> as that's how we do other stuff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example of what you mean? There's a matching impl for RootedVec right above this one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly,

impl<const N: usize> From<&Rooted<ValueArray<N>>> for JS::HandleValueArray {
fn from(array: &Rooted<ValueArray<N>>) -> JS::HandleValueArray {
JS::HandleValueArray {
length_: N,
elements_: array.data.get_ptr(),
}
}
}
impl From<&JS::CallArgs> for JS::HandleValueArray {
fn from(args: &JS::CallArgs) -> JS::HandleValueArray {
JS::HandleValueArray {
length_: args.argc_ as usize,
elements_: args.argv_ as *const _,
}
}
}
impl From<JS::Handle<JSVal>> for JS::HandleValueArray {
fn from(handle: JS::Handle<JSVal>) -> JS::HandleValueArray {
JS::HandleValueArray {
length_: 1,
elements_: handle.ptr,
}
}
}

Copy link
Member

@sagudev sagudev Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above is impl for RootedVec, but RootedGuard usually have handle() and handle_mut() methods so here it would be smth like:

impl<'a,> RootedGuard<'a, Vec<JSVal>>
{
fn handle_value_array(&self) -> JS::HandleValueArray {
        JS::HandleValueArray {
            length_: self.len(),
            elements_: self.deref().as_ptr(),
        }
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, given that HandleValueArray has no lifetime associated it can outlive the root so it should be unsafe (thus all from conversions are unsound).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the purposes of this PR, I'd rather match the existing HandleValueArray creation code that I linked in https://github.com/servo/mozjs/pull/697/changes/BASE..60cd1d696448318e06ec28f103b30641c9c8ffa6#r2700733569. If we want to change that, I think we should change all of them together.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair

@sagudev
Copy link
Member

sagudev commented Jan 15, 2026

Ack, there was a new sccache release but it doesn't include x86_64 binaries for macOS, only arm64, so our x86_64 macOS build is busted.

We can wait for mozilla/sccache#2554 or temporarily disable sccache on x86_64 macOS.

where
Vec<T>: RootKind,
{
pub fn push(&mut self, value: T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what I was trying to do here https://github.com/servo/mozjs/pull/630/changes#diff-737e12000758c48503126ab75c14db2c9d0854a95487ebdd0a51d99f991822efR289 but I ended up creating a whole new struct DynamicValueArray and struct FixedValueArray.

jdm added 2 commits January 17, 2026 02:49
Signed-off-by: Josh Matthews <josh@joshmatthews.net>
@jdm jdm enabled auto-merge January 18, 2026 07:15
@jdm jdm added this pull request to the merge queue Jan 18, 2026
Merged via the queue into servo:main with commit 48bf97d Jan 18, 2026
37 checks passed
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Jan 18, 2026
…s artifacts (#41971)

This bump includes
some new safe wrappers: servo/mozjs#699
servo/mozjs#700
debugmozjs artifacts: servo/mozjs#544
rooted macros with vectors: servo/mozjs#697

Testing: Done in mozjs repo.

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
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.

3 participants