Add some tests for default-initialized roots

[jdm]

https://github.com/servo/rust-mozjs/pull/410/ added new functionality, but we don't have any tests that exercise it yet. We should add some to https://github.com/servo/rust-mozjs/blob/master/tests/rooting.rs.

[ghost]

Hi, I'm very interested in learning Rust by working on this issue. I have cloned the repository and run cargo test on the code, but I'm still not sure what to test in src/rust.rs and what to assert in tests/rooting.rs

Are there some materials or pointers I can read first before taking this task? Thank you very much for your help.

[jdm]

https://github.com/servo/rust-mozjs/blob/master/src/rust.rs#L396-L403 were added in #410. They declare rooted values that default to null, rather than a passed in expression like https://github.com/servo/rust-mozjs/blob/master/src/rust.rs#L388-L395. We should add some tests that verify that calling .is_null() on the resulting root values is true, for the various JS types (mut JSObject, mut JSString, *mut JSFunction, JSValue).

[jdm]

These tests belong in https://github.com/servo/rust-mozjs/blob/master/tests/rooting.rs.

[ergunsh]

Hey! I can take this

[jdm]

@ergunsh That would be great! Please ask questions if anything is unclear :)

[ergunsh]

Hey!

I have two questions: 1 - As far as I understand, newly added macro waits Default trait implemented. However, the types you have said in https://github.com/servo/rust-mozjs/issues/411#issuecomment-404350995 doesn't implement it. Do I miss a point 😅

2- When I've tried with JSVal which implements Default trait; it also didn't work and compiler said expression expected instead of type. After, I've updated the macro to use <$type as Default>::default() instead of just $type::default() it worked.

I think, I'm missing a point but couldn't find what that is 😅

[jdm]

1. No, it just means that we should implement the Default trait for those types that evaluates to a null pointer
2. Feel free to update the macro if that makes it work!

[ergunsh]

Sorry for my inactivity; I have tried something but it didn't work out. Is there a usage example where the newly added macro is used?

[jdm]

I don't think we ended up using it after it was added.

[bytesnail]

I try to implement the Default trait for those types that evaluates to a null pointer, like this:

impl Default for JSObject {
    fn default() -> Self {
        ptr::null()
    }
}

After do that, I get some error message:

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
   --> src/rust.rs:418:1
    |
418 | impl Default for JSObject {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate
    |
    = note: the impl does not reference any types defined in this crate
    = note: define and implement a trait or new type instead

@jdm What should I do next ?

[jdm]

You're right! However, it looks like instead of using Default we can rely on the GCMethods trait that already exists and has a initial static method: https://doc.servo.org/mozjs/rust/trait.GCMethods.html . Want to replace the implementation of the macro with that instead?

[bytesnail]

GCMethods implement for JSVal

default value for JSVal implement in mozjs_sys:

impl Default for JSVal {
    fn default() ->JSVal { UndefinedValue() }
}

We can see that the initial value of JSVal is actually undefined, not the null that everyone expects. This can lead to many problems, such as: thread 'type_rooting' panicked at 'assertion failed: !parent_runtime.is_null()', src/rust.rs:175:17

[bytesnail]

Is this default value specified by the js standard, should we change the default value of JSVal?

[jdm]

No, undefined is expected. The error that you see on travisCI separate and should be fixed by https://github.com/servo/rust-mozjs/pull/450.

[bytesnail]

@jdm

Ok, in this case, has this task been completed? Do you have time to review the code for me?

[bytesnail]

@jdm

The code to solve this problem has been successfully merged, now we can close this issue.