Add lifetimes for Handle and MutableHandle #393

marmistrz · 2018-03-04T19:16:58Z

Servo compiled fine for me with the following patch set.

Since the From trait consumes the object, I created a raw method for MutableHandle. This is not ideal, since into() would be more idiomatic. Is there anything better I could do or should I just leave it as it is?

There are two questions marked with // REVIEW since I'm not sure if the explicit lifetimes are the same what compiler inferred.

Closes #153.

This change is

jdm · 2018-03-05T20:16:02Z

This will require some updates to the unit tests, too.

jdm

This is exciting work! There are some cases where it's really easy to create 'static lifetimes that I'd like to see if we could avoid, though.

jdm · 2018-03-05T20:35:03Z

src/conversions.rs

@@ -180,7 +181,7 @@ fn clamp_to<D>(d: f64) -> D
 // https://heycam.github.io/webidl/#es-void
 impl ToJSValConvertible for () {
    #[inline]
-    unsafe fn to_jsval(&self, _cx: *mut JSContext, rval: MutableHandleValue) {
+    unsafe fn to_jsval(&self, _cx: *mut JSContext, mut rval: MutableHandleValue) {


We should modify the ToJSValConvertible trait to accept a lifetime argument and use it for the rval argument.

jdm · 2018-03-05T20:35:03Z

src/rust.rs

-        }
+    // REVIEW: is this equivalent to?
+    // pub fn handle(&self) -> Handle<T>
+    pub fn handle(&'a self) -> Handle<'a, T> {


I believe it is equivalent. Let's use the explicit form.

jdm · 2018-03-05T20:35:03Z

src/rust.rs

+        RawMutableHandle::from_marked_location(ptr).into()
+    }
+
+    pub fn handle(&self) -> RawHandle<T> {


We should probably return a non-raw handle from this API.

jdm · 2018-03-05T20:35:03Z

src/rust.rs

@@ -853,9 +984,9 @@ impl<T: GCMethods + Copy> Heap<T> {
        self.ptr.get()
    }

-    pub fn handle(&self) -> Handle<T> {
+    pub fn handle(&self) -> RawHandle<T> {


We should definitely return a non-raw Handle here, and return a MutableHandle from handle_mut that is not using the 'static lifetime.

Are you sure that it's not using the 'static lifetime now? I'm not so sure of it.

I'm not even sure which code is being referred to any more. I believe that any code with a &self argument that returns a Handle will have the lifetime inferred to match the self argument, but it would be valuable to write that out explicitly.

jdm · 2018-03-05T20:35:03Z

src/rust.rs

        }
    }
 }

+impl<'a> HandleValue<'a> {
+    pub fn null() -> Self {


Let's be explicit and use HandleValue<'static'> for these methods.

jdm · 2018-03-05T20:35:03Z

src/rust.rs

+    }
+}
+
+impl<'a, T> From<RawMutableHandle<T>> for MutableHandle<'a, T> {


How important are the From<Raw> impls for the safe handles? It makes me uncomfortable that we can create safe handles with 'static lifetimes from arbitrary handles that do not actually have static lifetimes.

jdm · 2018-03-05T20:35:03Z

src/rust.rs

+        Handle { ptr: ptr }
+    }
+
+    pub unsafe fn from_marked_location(ptr: *const T) -> Self {


Is this used outside of this crate? If we could avoid making it public, that would be useful.

marmistrz · 2018-03-07T16:11:31Z

Honestly, I don't really have an idea what we could test here. That's just mimicking the Handle's behavior, all the checks are done by the compiler.

jdm · 2018-03-08T17:01:26Z

We could write some compile_fail doctests that demonstrate that handle values with lifetimes can't outlive the objects from which they were obtained.

bors-servo · 2018-03-08T17:06:05Z

☔ The latest upstream changes (presumably #394) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2018-03-08T17:06:25Z

Oh, and to be clear when I said "updates to the unit tests", I was specifically referring to the fact that they do not compile any longer according to TravisCI :)


        Make Handle take lifetimes


        Add lifetimes for MutableHandle


        [Do not merge] A dummy PR to track if servo/rust-mozjs#393 doesn\'t b…

…reak servo


        Fix the tests and MutableHandle::handle


        Remove the conversions from Handle to RawHandle


        Make null and undefined handlevalues static


        Make Heap::handle return a Handle

jdm · 2018-03-12T21:32:04Z

The changes here look good to me as long as we can make Servo build with them before merging them.

bors-servo · 2018-03-13T21:48:31Z

☔ The latest upstream changes (presumably #397) made this pull request unmergeable. Please resolve the merge conflicts.


        Add wrappers for raw handle methods

jdm · 2018-03-23T14:17:27Z

src/generate_jsapi_wrappers.sh

@@ -0,0 +1,17 @@
+#!/bin/sh
+# This is one big heuristic but seems to work well enough


This is a clever solution! Let's add documentation like the following:

The script extracts a list of all the JSAPI functions that accept FFI Handle and MutableHandle arguments.


        Merge remote-tracking branch 'upstream/master'

marmistrz · 2018-03-23T14:23:50Z

@jdm Could some macros guru to review my macro? The macro rule duplication here is atrocious but I don't know if the macro system allows for anything more than copy paste.

nox · 2018-03-23T14:29:10Z

src/conversions.rs

@@ -238,7 +239,7 @@ unsafe fn convert_int_from_jsval<T, M>(cx: *mut JSContext, value: HandleValue,
 // https://heycam.github.io/webidl/#es-boolean
 impl ToJSValConvertible for bool {
    #[inline]
-    unsafe fn to_jsval(&self, _cx: *mut JSContext, rval: MutableHandleValue) {
+    unsafe fn to_jsval(&self, _cx: *mut JSContext, mut  rval: MutableHandleValue) {


Nit: double space.

jdm · 2018-03-23T14:29:55Z

src/rust.rs

+        wrap!(@inner $saved <> ($($acc,)* $arg,) <> $($rest)*);
+    };
+    (@inner ($func_name:ident ($($args:tt)*) -> $outtype:ty) <> ($($argexprs:expr,)*) <> ) => {
+        pub unsafe fn $func_name($($args)*) -> $outtype {


Let's add #[inline] here.

jdm · 2018-03-23T14:29:55Z

src/rust.rs

+    use libc::FILE;
+    use super::{Handle, HandleId, HandleObject, HandleValue};
+    use super::{MutableHandle, MutableHandleObject, MutableHandleValue};
+    include!("jsapi_wrappers.rs");


The file in this PR is called jsapi_wrappers.in. Is something missing?

that was fixed while merging upstream :)

jdm · 2018-03-23T14:29:55Z

src/rust.rs

+        wrap!(pub fn $func_name($($args)*) -> ());
+    }
+}
+pub mod wrappers {


Let's add documentation like:

Wrappers for JSAPI methods that accept FFI Handle and MutableHandle arguments. The new methods are identical except that they accept Handle and MutableHandle arguments that include lifetimes instead.

You mean:

Wrappers for JSAPI methods that accept lifetimed Handle and MutableHandle arguments.
?

My comment scrolls horizontally, so keep reading.

nox · 2018-03-23T14:32:51Z

@marmistrz AFAICT skimming through that macro, the only way to require less clauses is to unconditionally transform all arguments, as opposed to just the ones with MutableHandle etc. This would require a trait implemented for all arguments found in invocations of wrap!. I think this is good enough for now.


        review fixes


        Fix a double space

bors-servo · 2018-03-23T15:14:36Z

☔ The latest upstream changes (presumably #404) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2018-03-23T16:07:02Z

Hmm, Unbox seems to be a platform-specific function for some reason:

error[E0425]: cannot find function `Unbox` in module `jsapi`
    --> src\rust.rs:1680:23
     |
1680 |         wrap!(@inner ($func_name ($($args)*) -> $outtype) <> () <> $($args)* ,);
     |                       ^^^^^^^^^^ not found in `jsapi`
     | 
    ::: src\jsapi_wrappers.in:3:1
     |
3    | wrap!(pub fn Unbox(cx: *mut JSContext, obj: HandleObject, vp: MutableHandleValue) -> bool);
     | ------------------------------------------------------------------------------------------- in this macro invocation
help: possible candidate is found in another module, you can import it into scope
     |
1693 |     use rust::wrappers::Unbox;
     |


        Make from_raw public, it's used in interface.rs in servo


        Add wrappers for glue


        Get rid of Unbox


        Use a pointer and a phantom for MutableHandle


        Merge remote-tracking branch 'upstream/master'


        bump the version

jdm · 2018-03-27T18:15:47Z

@bors-servo r+

bors-servo · 2018-03-27T18:15:48Z

📌 Commit 709357f has been approved by jdm

bors-servo · 2018-03-27T18:15:51Z

⌛ Testing commit 709357f with merge 03db1ae...


        Auto merge of #393 - marmistrz:master, r=jdm

Add lifetimes for Handle and MutableHandle Servo compiled fine for me with the following patch set. Since the `From` trait consumes the object, I created a `raw` method for `MutableHandle`. This is not ideal, since `into()` would be more idiomatic. Is there anything better I could do or should I just leave it as it is? There are two questions marked with `// REVIEW` since I'm not sure if the explicit lifetimes are the same what compiler inferred.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/393)

bors-servo · 2018-03-27T22:15:55Z

☀️ Test successful - status-appveyor, status-travis
Approved by: jdm
Pushing 03db1ae to master...


        Adapt Servo for mozjs 0.6 and the changes introduced in servo/rust-mo…

…zjs#393


        Remove stray comments asking for review :) introduced by servo#393


        Remove stray comments asking for review :)

They were introduced by servo#393 and meant to be removed once the questions had been answered - but they ended up merged.


        Auto merge of #421 - marmistrz:review-rm, r=cbrewster

Remove stray comments asking for review :) They were introduced by #393 and meant to be removed once the questions had been answered - but they ended up merged.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/421)


        Create a second set of borrowing wrappers

Our current wrappers held in rust::wrappers accept the Handles by consume/Copy. This was introduced in servo#393 to make it feasible to migrate the API calls on the Servo side in finite time. This, in particular, requires unsafe code in the implementation of MutableHandle. This change introduces a second set of wrappers, which will now borrow MutableHandles instead of relying on impl Copy for MutableHandle. The Servo code should gradually move to the new wrappers, rust::jsapi_wrapped, so that we can eventually remove rust::wrappers.


        Adapt Servo for mozjs 0.6 and the changes introduced in servo/rust-mo…

…zjs#393


        Adapt Servo for mozjs 0.6 and the changes introduced in servo/rust-mo…

…zjs#393

jdm requested changes Mar 5, 2018

View changes

marmistrz added 2 commits Mar 4, 2018

Make Handle take lifetimes

b90c4ba

Add lifetimes for MutableHandle

c534a12

marmistrz force-pushed the marmistrz:master branch from e204a99 to f62f7df Mar 8, 2018

marmistrz added a commit to marmistrz/servo that referenced this pull request Mar 8, 2018

[Do not merge] A dummy PR to track if servo/rust-mozjs#393 doesn\'t b…

8bcfd37

…reak servo

Fix the tests and MutableHandle::handle

Loading status checks…

4331642

marmistrz force-pushed the marmistrz:master branch from f62f7df to 4331642 Mar 8, 2018

marmistrz added 3 commits Mar 11, 2018

Remove the conversions from Handle to RawHandle

Loading status checks…

ac7db1c

Make null and undefined handlevalues static

Loading status checks…

2c207ee

Make Heap::handle return a Handle

Loading status checks…

0372723

jdm approved these changes Mar 12, 2018

View changes

Add wrappers for raw handle methods

Loading status checks…

cfa8046

jdm reviewed Mar 23, 2018

View changes

Merge remote-tracking branch 'upstream/master'

Loading status checks…

f4cd26d

nox reviewed Mar 23, 2018

View changes

jdm requested changes Mar 23, 2018

View changes

marmistrz added 2 commits Mar 23, 2018

review fixes

Loading status checks…

fdaa1a0

Fix a double space

Loading status checks…

a28e3e7

Make from_raw public, it's used in interface.rs in servo

Loading status checks…

343ee62

marmistrz force-pushed the marmistrz:master branch from 47000fa to 343ee62 Mar 25, 2018

marmistrz added 5 commits Mar 25, 2018

Add wrappers for glue

Loading status checks…

dc4decc

Get rid of Unbox

Loading status checks…

8ebb20f

Use a pointer and a phantom for MutableHandle

Loading status checks…

855b4ee

Merge remote-tracking branch 'upstream/master'

Loading status checks…

d58120c

bump the version

Loading status checks…

709357f

bors-servo merged commit 709357f into servo:master Mar 27, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors-servo mentioned this pull request Mar 27, 2018

Added RootKind and GCMethods implementation for PropertyDescriptor #414

Merged

marmistrz added a commit to marmistrz/servo that referenced this pull request Mar 28, 2018

Adapt Servo for mozjs 0.6 and the changes introduced in servo/rust-mo…

356c57e

…zjs#393

marmistrz added a commit to marmistrz/rust-mozjs that referenced this pull request Apr 5, 2018

Remove stray comments asking for review :) introduced by servo#393

db1bdb1

marmistrz added a commit to marmistrz/rust-mozjs that referenced this pull request Apr 5, 2018

Remove stray comments asking for review :)

488a9c8

They were introduced by servo#393 and meant to be removed once the questions had been answered - but they ended up merged.

marmistrz mentioned this pull request Apr 5, 2018

Remove stray comments asking for review :) #421

Merged

Moggers added a commit to Moggers/servo that referenced this pull request Jun 13, 2018

Adapt Servo for mozjs 0.6 and the changes introduced in servo/rust-mo…

6421de0

…zjs#393

thomaskrause pushed a commit to korpling/graphannis-malloc_size_of that referenced this pull request Oct 11, 2018

Adapt Servo for mozjs 0.6 and the changes introduced in servo/rust-mo…

32c0804

…zjs#393

servo / rust-mozjs

Add lifetimes for Handle and MutableHandle #393

Add lifetimes for Handle and MutableHandle #393

marmistrz commented Mar 4, 2018 •

edited

jdm commented Mar 5, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

marmistrz commented Mar 7, 2018

jdm commented Mar 8, 2018

bors-servo commented Mar 8, 2018

jdm commented Mar 8, 2018

jdm commented Mar 12, 2018

bors-servo commented Mar 13, 2018

This comment has been minimized.

marmistrz commented Mar 23, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Mar 23, 2018

bors-servo commented Mar 23, 2018

jdm commented Mar 23, 2018

jdm commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 27, 2018

		@@ -0,0 +1,17 @@
		#!/bin/sh
		# This is one big heuristic but seems to work well enough

servo / rust-mozjs

Join GitHub today

Add lifetimes for Handle and MutableHandle #393

Add lifetimes for Handle and MutableHandle #393

Conversation

marmistrz commented Mar 4, 2018 • edited

jdm commented Mar 5, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

marmistrz commented Mar 7, 2018

jdm commented Mar 8, 2018

bors-servo commented Mar 8, 2018

jdm commented Mar 8, 2018

jdm commented Mar 12, 2018

bors-servo commented Mar 13, 2018

This comment has been minimized.

jdm Mar 23, 2018 • edited

marmistrz commented Mar 23, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm Mar 23, 2018 • edited

This comment has been minimized.

This comment has been minimized.

nox commented Mar 23, 2018

bors-servo commented Mar 23, 2018

jdm commented Mar 23, 2018

jdm commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 27, 2018

marmistrz commented Mar 4, 2018 •

edited

jdm Mar 23, 2018 •

edited

jdm Mar 23, 2018 •

edited