766347 - It's not possible to pass Rooted<S> to a Foo method if it has Foo(Handle<S>) and Foo(Handle<T>) overloads

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

The problem is that Handle<T> can be constructed from Rooted<S> for any S.  The testAssign method is supposed to enforce convertibility, but it doesn't affect type signatures in a way that triggers correct argument conversion.  You know what that means: SFINAE time!  Excited yet?

Comment 1

[Jeff Walden [:Waldo]]

Attachment: mozilla::EnableIf<bool, typename>

Apparently the foundational element of SFINAE.  Fun fun fun!

Comment 2

[Jeff Walden [:Waldo]]

Attachment: mozilla::IsConvertible<typename From, typename To>

Also includes tests.  It doesn't work quite right in one far edge case, commented out in the test, but if someone were trying that, I'm pretty sure they'd be trying to be dumb, more or less.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Fix the actual bug now

I plan to add some users soon, but I haven't added any here.  I did test that a use did work with this patch.

I haven't tryservered any of these patches, but I'll be doing so now.  :-)

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 634615 [details] [diff] [review]
mozilla::EnableIf<bool, typename>

u r become death, the destroyer of worlds

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 634617 [details] [diff] [review]
mozilla::IsConvertible<typename From, typename To>

s/D/C/

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 634618 [details] [diff] [review]
Fix the actual bug now

Review of attachment 634618 [details] [diff] [review]:
-----------------------------------------------------------------

1337 C++ h4x0r

::: js/src/gc/Root.h
@@ +78,5 @@
> +    /* Creates a handle from a handle of a type convertible to T. */
> +    template <typename S>
> +    Handle(Handle<S> handle,
> +           typename mozilla::EnableIf<mozilla::IsConvertible<S, T>::value,
> +                                      int>::Type dummy = 0)

This line-break is awkward.  Can you hoist the IsConvertible expression up to a static const bool and reuse in the other two constructor decls.

@@ +104,5 @@
> +    template <typename S>
> +    inline
> +    Handle(const Rooted<S> &root,
> +           typename mozilla::EnableIf<mozilla::IsConvertible<S, T>::value,
> +                                      int>::Type dummy = 0);

I think you already did this in a prev patch, but could you remove the 'const'?

Comment 7

[Jeff Walden [:Waldo]]

The IsConvertible isn't hoistable because it depends on typename S, but those lines were short enough to fit in 99ch anyway, so I removed the linebreaks.

I had these patches in my queue before the const removal one, but I ended up reordering my queue to land that one first, so no const to remove any more.

https://hg.mozilla.org/integration/mozilla-inbound/rev/54a17b76f488
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe817bf85f36
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0fd1627db78

And yes, death.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/54a17b76f488
https://hg.mozilla.org/mozilla-central/rev/fe817bf85f36
https://hg.mozilla.org/mozilla-central/rev/b0fd1627db78