1769518 - Explore way to allow mozilla::Result<JS::Value>

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

It could be good to support Result to contain JS values (Result<Ok, JS::Value> or such) to support the Result-style error handling convention. Currently it's disallowed by hazard analysis as it doesn't know whether it's rooted.

See also https://phabricator.services.mozilla.com/D139525#4785809:

One short term solution might be to make Rooted<Result<Ok, JS::Value>> work and root the return value immediately.

A better fix, or set of fixes, would be to make the analysis more aware of Result states, so that if the arm that contains a GC pointer is not being used, then it would not require rooting. And perhaps use move semantics somehow to extract the error value and leave behind Ok or something?

Comment 1

[Steve Fink [:sfink] [:s:]]

I looked into doing this, but ran into a problem: Result<V,E> represents an immutable value, so afaict there's no way to trace the V or E member. Specifically, there's no way to implement GCPolicy<Result<V,E>>::trace() because you can't update the V or E.

The only way I see to implement this would be to specialize Result for GC pointer types, but that specialization would have to redo the entire Result implementation. And you can't even do something like make JS::Result for those types inherit from mozilla::Result, because the latter is marked final (and the needed members are private).

I got Result<JS::Value, Err> to compile by cutting & pasting the entire struct mozilla::Result body and replacing V with JS::Value, but:

• that required a const_cast that smells of undefined behavior
• it's a bit brittle, dependent on the packing strategy to be PackedVariant (though this can be forced with a specialization of mozilla::detail::SelectResultImpl)
• it would need to be duplicated for Result<Ok, JS::Value>
• the Variant packing is suboptimal; it would be better to use UndefinedValue or MagicValue to distinguish.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1769518 - Support Rooted<Result<V,E>> as long as V and E have GCPolicy<> defined for them. (Use IgnoreGCPolicy for whichever of them does not need tracing.)

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)

• the Variant packing is suboptimal; it would be better to use UndefinedValue or MagicValue to distinguish.

We could teach NaN-boxing to the Result type if needed.

(In reply to Steve Fink [:sfink] [:s:] from comment #1)

I looked into doing this, but ran into a problem: Result<V,E> represents an immutable value, so afaict there's no way to trace the V or E member. Specifically, there's no way to implement GCPolicy<Result<V,E>>::trace() because you can't update the V or E.

Looking at the code of Result, I do not see the constness of the content reflected in the type.
It is reflected in the API for manipulating the values, but the implementation of the Value it-self seems like it could be made internally-mutable.

So, we could potentially add specialization which provides this internal mutability to implement a trace function.

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)

Looking at the code of Result, I do not see the constness of the content reflected in the type.
It is reflected in the API for manipulating the values, but the implementation of the Value it-self seems like it could be made internally-mutable.

So, we could potentially add specialization which provides this internal mutability to implement a trace function.

That's right, it's not in the API. It's just that the Result class doesn't provide any way to mutate contained values, so the GCPolicy specialization doesn't have an API to go through. I took a stab at making the specializing just the implementation to return a MutableValue that contained a mutable JS::Value v field, but that ran into problems. It might be workable somehow. This stuff is tricky, though; I use inspect() to access the value to update. Depending on how things get packed, you might end up with something that copies the value and returns a reference to the copy.

I'm wondering if the general mozilla::Result could use a separate detail::RequiresMutability<T> template to provide the needed access, and thread that all the way through...

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)

I'm wondering if the general mozilla::Result could use a separate detail::RequiresMutability<T> template to provide the needed access, and thread that all the way through...

I think this is doable, to have a detail::RequiresAddressOfMutableContent, and only expose in mozilla::Result a method to return either a pointer to the value or a nullptr if this is the other data type.

Comment 6

[Steve Fink [:sfink] [:s:]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)

(In reply to Steve Fink [:sfink] [:s:] from comment #4)

I'm wondering if the general mozilla::Result could use a separate detail::RequiresMutability<T> template to provide the needed access, and thread that all the way through...

I think this is doable, to have a detail::RequiresAddressOfMutableContent, and only expose in mozilla::Result a method to return either a pointer to the value or a nullptr if this is the other data type.

I have something like this working now. It feels a little dangerous, since the behavior changes depending on what headers you have included—if a Result user doesn't have TraceExternalEdge available for the types being used, then a Rooted<Result> won't trace. But it doesn't seem too bad; if you're using Rooted, then you have to include js/RootingAPI.h which includes js/GCPolicyAPI.h which then includes js/TracingAPI.h. So it's pretty difficult to get into trouble that way.

But perhaps more worrisome, if you use a class or struct that is traceable (as in, GCPolicy<T> is declared appropriately), it won't get traced (because there's no TraceExternalEdge that accepts it.) And I can't test whether GCPolicy<T>::trace exists, because it is instantiable for anything. It will then fail to compile the body of GCStructPolicy<T>::trace. So an actual error, not SFINAE.

This applies to recursive Results, for example Result<Result<Ok, JS::Value>, SomeError>. But also when V or E is OwningNonNull<T> or Maybe<T> or UniquePtr<T>. If these failed to compile, that would be fine, but I haven't gotten them to fail without making eg Result<Ok, int> also fail.

Perhaps we should make GCPolicy<T>::trace fail to exist? eg by breaking the instantiation of GCStructPolicy<T> if T does not in fact define a trace method. I haven't tried that.

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Steve Fink [:sfink] [:s:] from comment #6)

Perhaps we should make GCPolicy<T>::trace fail to exist? eg by breaking the instantiation of GCStructPolicy<T> if T does not in fact define a trace method. I haven't tried that.

An alternative could be to always define a trace function and make it meaningless unless the content is traceable.

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)

An alternative could be to always define a trace function and make it meaningless unless the content is traceable.

If you had SomeStructWithAGCPointer that did not contain a trace method, then Rooted<SomeStructWithAGCPointer> would successfully compile, and even pass the hazard analysis, but would not actually root. Same if it defined a trace method with the wrong signature, which is easy to do.

An added wrinkle here is that we actually allow structs to implement only a subset of trace/traceWeak if not all of them are going to be used. I'm not sure if that was intentional, but it does work that way and there are many GCPolicy specializations that don't define both. GCPolicy<RefPtr<T>> for example only has trace. GCPolicy<js::UnsafeBarePtr<T>> only has traceWeak.

I seem to have something that works now, though.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1687f633995
Support Rooted<Result<V,E>> as long as V and E have GCPolicy<> defined for them. (Use IgnoreGCPolicy for whichever of them does not need tracing.) r=emilio,nbp,jonco

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/c1687f633995