If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.
Bug 1277368 (errormageddon)

[meta] Use mozilla::Result<T, E> for fallible return values in the JS engine

NEW
Assigned to

Status

()

Core
JavaScript Engine
P3
normal
a year ago
2 days ago

People

(Reporter: jorendorff, Assigned: jandem, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {leave-open, meta, triage-deferred})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

a year ago
Currently we use nullptr and false to indicate most errors.

This is OK in the normal case but we have a *lot* of places when error handling starts to get a little bit atypical, and then we have bugs. JS::ObjectOpResult is a whole thing with its own rules; CanGC/NoGC functions are another; Pure functions are another; AllocPolicies add another angle; DenseElementResult, jit::MethodStatus, and jit::JitExecStatus are more variations on the theme. TokenStream::flags::hadError is an example of a custom error-handling scheme that I'm not sure could possibly be correct.

So the plan is to introduce a template, mozilla::Result<T, E>, that's just a tagged variant that holds either a success value of type T or an error value of type E. Where we currently return `bool`, we'll return `Result<Ok, JS::Error>`, where Ok and JS::Error are maybe dummy types for now. Where we currently return `JSScript*`, we can return `Result<JSScript*, JS::Error>`.

Then, add some macros like `MOZ_TRY(expr)` to make it easy to use Result correctly. If we get lucky, Result<> can supplant most of the error-handling schemes we've got today.

Long term, where I'd love to go with this is to delete all error-handling state from JSContext (throwing, unwrappedException_, overRecursed_, and probably propagatingForcedReturn_). Instead it will all be part of the JS::Error type, indicated in the type signature of all JSAPI functions.
(Reporter)

Comment 1

a year ago
A nice side benefit here is that `Result` can be a MOZ_MUST_USE_TYPE (see Attributes.h), so we don't have to remember to add `MOZ_MUST_USE` to every bool return.

The patch I've got right now may not be totally correct, but it's about -540 lines net of everything, mainly due to many individual hunks like this:

> -    if (!LookupNameUnqualified(cx, name, scopeChain, &scope))
> -        return false;
> +    MOZ_TRY(LookupNameUnqualified(cx, name, scopeChain, &scope));

Each of these saves a line of code. What's left is more compact and I *think* reads better, though this patch leaves the codebase only slightly Result-ified, so it's a bit of a mishmash.

There are already several types named Result in various places in the codebase, but I think we can live with it.
(Reporter)

Comment 2

a year ago
Created attachment 8758883 [details] [diff] [review]
errormageddon-wip-1.patch
Assignee: nobody → jorendorff
Very cool.
(Reporter)

Comment 4

a year ago
In comment 0 I should of course have noted that Terrence came up with the whole idea.

Another thing this might help with is the kind of bug where an allocation fails, but we return false/null without reporting the OOM. We can rule that out by making our non-reporting functions return a Result with a different error type. You wouldn't be able to convert freely between the two, you'd have to be explicit about it, and we'd have a method or a macro to make that easy.

The sheer massiveness of this change gives me pause, but I am really having trouble coming up with reasonable arguments against it. It seems to be as fast, and the code is shorter. Places where the error handling is weird look visually weird, which... no, that's actually a good thing too.
(Reporter)

Comment 5

a year ago
Other arguments against:

There are too many different macros. MOZ_TRY(res), JS_TRY_OR_RETURN_FALSE(cx, res), JS_TRY_OR_RETURN_NULL(cx, res). And a separate assigning version of each, MOZ_TRY_VAR(var, res) etc. But once everything is converted, the JS_ versions go away: they are only to handle the boundaries between Result-using and non-Result-using code.

The compiler error messages can be gross, because templates. But whatever, right?
(Reporter)

Comment 6

a year ago
Created attachment 8759478 [details] [diff] [review]
errormageddon-wip-2.patch

New in this WIP: `MOZ_TRY_VAR(var, res)` and a few places that use it; a special `ReportedOOM` error type for functions that can't fail any other way.
(I kind of like that you can see that in the type. If it turns out to be a pain, we could always rip it out and use JS::Error* throughout the engine.)
(Reporter)

Comment 7

a year ago
Created attachment 8762840 [details] [diff] [review]
errormageddon-wip-3.patch
Comment on attachment 8758883 [details] [diff] [review]
errormageddon-wip-1.patch

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

A few comments from a quick skim...

In mfbt/Result.h you've used SpiderMonkey style instead of Gecko style :(  The biggest problems:

- Please use 2 space indents
- Please use mFoo/aFoo var naming
- Please use upper case letters for the first letter in function names

::: mfbt/Result.h
@@ +81,5 @@
> + * Specialization for the result of allocation, when the success type is a
> + * pointer and the error type is OutOfMemory.
> + *
> + * Result<T*, Ok> is guaranteed to be pointer-sized, and all zero bits on error.
> + * Do not change this representation!

Why not? (For the next specialization you say that JIT code depends on the representation. Is that true here too? Or is there another reason?)

@@ +122,5 @@
> +{
> +    E* err;
> +
> +  public:
> +    /* DEPRECATED */

Explain that this is only needed during the transition period when Result is being introduced?
(Reporter)

Comment 9

a year ago
Created attachment 8764933 [details] [diff] [review]
errormageddon-wip-4.patch

Now works in `-m32` Linux builds.

(Browser still doesn't build in MSVC because of the pkix namespace collision -- apparently MSVC handles C++ namespaces differently in some weird corner cases involving templates and the Koenig lookup. Working on it.)
Attachment #8758883 - Attachment is obsolete: true
Attachment #8759478 - Attachment is obsolete: true
Attachment #8762840 - Attachment is obsolete: true
(Reporter)

Comment 10

a year ago
WIP 4 applies on top of rev 0ad7433c2159.
(Reporter)

Comment 11

a year ago
The next step here is to get this running on Win32 and then see if it makes us any slower.

Unfortunately, on Win32 the ABI for returning a struct is wonky (and not well documented).
Terrence helped me with this last night.

Here is code, generated by MSVC, that calls a Result<>-returning function:
  https://pastebin.mozilla.org/8880155

Here is the function that it's calling:
  https://pastebin.mozilla.org/8880157

Unfortunately it looks like the ABI uses EAX, which we use as a scratch register whenever we call a function.
(Reporter)

Comment 12

a year ago
And here's the current code we're emitting, which works on Linux gcc -m32. Unfortunately that's a different ABI. :-(

https://pastebin.mozilla.org/8880159
I can look at the Win32 thing.
Flags: needinfo?(jdemooij)
(In reply to Jason Orendorff [:jorendorff] from comment #11)
> Unfortunately it looks like the ABI uses EAX, which we use as a scratch
> register whenever we call a function.

Hm I don't think the ABI uses EAX, but it is weird we use EAX as scratch. I'll post a patch to fix that and then I can test this on Win32.
Created attachment 8766345 [details] [diff] [review]
Fix 32-bit OS X and Windows

With this patch applied on top of WIP 4, jit-tests pass on Win32 and OS X 32-bit.

There are 3 different conventions for this on x86:

* OS X: Result<> is returned in eax \o/

* Linux (System V): pointer to stack memory is passed, callee discards this pointer.

* Windows: also passes a pointer to stack memory, but caller discards it (like all other arguments).
Flags: needinfo?(jdemooij)
Testing performance on windows 10 with 32bit builds, we have:

>          Run1  Run2  Run3      Avg
> Before: 27660 28165 28113    27979
> After:  27879 27946 28150    27991

So no discernible performance difference.
(Reporter)

Comment 17

a year ago
Thank you both. This is excellent news.
(Reporter)

Comment 18

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fff08b62957
(Reporter)

Updated

a year ago
Depends on: 1283562
Summary: Introduce mozilla::Result<T, E> for fallible return values → [meta] Use mozilla::Result<T, E> for fallible return values in the JS engine
Comment on attachment 8764933 [details] [diff] [review]
errormageddon-wip-4.patch

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

::: mfbt/Result.h
@@ +76,5 @@
> + * the error type is a pointer.
> + *
> + * In this case, Result<T, E*> is guaranteed to be pointer-sized, and all zero
> + * bits on success. Do not change this representation! There is JIT code that
> + * depends on it.

Can you static_assert that sizeof this Result type is pointer-sized?

@@ +92,5 @@
> +  explicit ResultImplementation(bool ok)
> +      : mErrorValue(reinterpret_cast<E*>(uintptr_t(!ok))) {}
> +
> +  explicit ResultImplementation(Ok) : mErrorValue(nullptr) {}
> +  explicit ResultImplementation(E* aErrorValue) : mErrorValue(aErrorValue) {}

Should you assert aErrorValue is not null here? Or will the IsNull asserts in Result's ctor prevent that case here?

Why are there ctor asserts in the Result class but not the ResultImplementation detail classes when the unwrap() asserts are in the ResultImplementation detail classes but not the Result class? Should the asserts be everywhere (belt and suspenders) or just the public Result class?

@@ +97,5 @@
> +
> +  bool isOk() const { return mErrorValue == nullptr; }
> +
> +  Ok unwrap() const {
> +    MOZ_ASSERT(isOk());

In addition to asserting isOk(), can you assert that the caller actually checked isOk()? Or would that be too onerous for callers that *know* the value is always Ok or error?

Just asserting isOk() would only catch misuse when a (possibly rare) error is unwrapped in a debug build, but asserting that isOk() has already been checked would always detect that misuse of Result.

@@ +192,5 @@
> +   * (The rule is for efficiency reasons.)
> +   */
> +  explicit Result(E aErrorValue) : mImpl(aErrorValue) {
> +    MOZ_ASSERT(!detail::IsNull(aErrorValue));
> +    MOZ_ASSERT(isErr());

You might consider upgrading some of these asserts to MOZ_DIAGNOSTIC_ASSERT so they are checked in opt builds of Nightly and Aurora.
(Reporter)

Comment 20

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea74e139f850
Blocks: 1289662
Blocks: 1115005
Jason, any progress here?
Flags: needinfo?(jorendorff)
Keywords: meta
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 22

11 months ago
Created attachment 8802571 [details] [diff] [review]
Rebased patch

Here's a rebased patch. Applies to revision bc91be30f2aa and passes shell tests on OS X 32-bit and 64-bit.

jorendorff, mind if I work on getting this landed or do you want to finish it?
Attachment #8764933 - Attachment is obsolete: true
Attachment #8766345 - Attachment is obsolete: true
Attachment #8802571 - Flags: feedback?(jorendorff)
(Assignee)

Comment 23

11 months ago
Created attachment 8802891 [details] [diff] [review]
Rebased

This fixes a bunch of issues. Android linker weirdness, and JIT changes for Win64 and the ARM simulator. Try should be green now.

Passing types like Result<> to JIT code sucks unfortunately, every platform has its own calling convention.
Attachment #8802571 - Attachment is obsolete: true
Attachment #8802571 - Flags: feedback?(jorendorff)
(Assignee)

Updated

11 months ago
Depends on: 1311993
(Assignee)

Updated

11 months ago
Depends on: 1311996
(Assignee)

Updated

11 months ago
Depends on: 1312003
Depends on: 1314639
According to emilio, having this in any functions we need to create Rust bindings for would be a problem right now. Mostly, that means it'd be great if we could hold off on using it in any JSAPI functions or the public headers. There might be a few additional places where it'd be problematic, though, because the Rust bindings reimplement various (mostly rooting-related) functions. Not sure about that, though.

Presumably, impl specialization (https://github.com/rust-lang/rfcs/pull/1210) will make it much easier to support this in rust-bindgen. Not sure if there are feasible solutions for that right now.
(Reporter)

Comment 25

11 months ago
Thanks for taking this, Jan.

Till, we can get most of the benefit from this without changing the JSAPI. That said, we do want to change the API eventually because experience suggests that downstream code rarely handles JSAPI errors correctly. So it'd be good to know what the problem is exactly. C++ mozilla::Result isn't meant to be layout-compatible with Rust's std::result::Result, but it seems like the sort of type-translation work that bindgen would automate.

(In particular: it seems like a "dumb" problem with a "dumb" solution, not the sort of case where you'd want to block on a powerful new Rust language feature. More likely, I've not understood the problem.)
Flags: needinfo?(jorendorff)
The problems are, unsurprisingly, related to templates, which work just differently enough in C++ and Rust for the bindings to be non-trivial.

I do think that using mozilla::Result in JSAPI makes a lot of sense, and agree that it certainly shouldn't be blocked by Rust language features. (And I'm not even really sure that impl specialization would help substantially.)

Even if it turns out that we really can't feasibly support mozilla::Result, I think my concerns are probably better addressed by introducing a wrapper that we *can* bind to and ignoring the unsupported items in jsapi.h.

Needinfo fitzgen for maybe slightly more elaboration on what's problematic here.
Flags: needinfo?(nfitzgerald)
So the main problem is that Rust doesn't have the concept of struct specialization, so creating a generic Result<T, E> that can be layout compatible with the C++ implementation automatically for every T and E is pretty hard, both manually, and even more automatically.

impl specialization may help if we parameterize in terms of a trait that could define different types for the members, but that's extremely hard to do automatically, and I don't know:

 * How smart is rustc passing repr(C) types with template parameters and zero-sized types (presumably not a huge deal because we do that in Stylo).
 * How smart both the C++ and Rust's compiler are to handle passing proper return values with templates. We've had problems with that in stylo, where gcc and clang only agreed with rustc if the return value didn't have a destructor.

Assuming the last point is not a problem (we might be lucky, and if the types don't have destructors I seem to recall that there weren't huge problems), and that we get proper specialization, it might be doable.

It might be easier for rust-mozjs if you typedef the concrete specializations of the errors you use in the public API, though:

    typedef Result<Foo, Bar> FoobarResult;
    FoobarResult JS_DoWhatever();

That way we can make bindgen not try to generate FoobarResult, and replace it with a manual struct that is compatible for Foo and Bar (and so on). Even though that'd be a manual task, it's better than trying to reimplement C++'s template specialization in bindgen with the limited information that libclang gives us.
Oh, other solution to that problem is getting cross-language LTO, and adding a proper wrapper to the C++ API that Rust can use, but I'm not too confident that's going to happen really soon.
I don't have anything to add on top of what Emilio has said.

I want to reiterate that generating Rust bindings should not interfere with improving SpiderMonkey/JSAPI.

If need be, we can make a separate header with a C API for use only by bindgen that dumbs down everything.
Flags: needinfo?(nfitzgerald)
Assignee: jorendorff → jdemooij
(Assignee)

Comment 30

11 months ago
Created attachment 8808552 [details] [diff] [review]
Rebased

Rebased to revision d370d74e76d7 on top of the new mozilla::Result<> patch. In particular, the error type is now a reference instead of a pointer, per Waldo's suggestion.
Attachment #8802891 - Attachment is obsolete: true
(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)
> It might be easier for rust-mozjs if you typedef the concrete
> specializations of the errors you use in the public API, though:
> 
>     typedef Result<Foo, Bar> FoobarResult;
>     FoobarResult JS_DoWhatever();
> 
> That way we can make bindgen not try to generate FoobarResult, and replace
> it with a manual struct that is compatible for Foo and Bar (and so on). Even
> though that'd be a manual task, it's better than trying to reimplement C++'s
> template specialization in bindgen with the limited information that
> libclang gives us.

I think throughout SM we will prefer to use a typedef'd Result over a bare Result<>.
I don't immediately see a reason why we wouldn't/couldn't do this on the public API
or even make it a requirement on the public API.
(Assignee)

Comment 32

10 months ago
Created attachment 8812769 [details] [diff] [review]
Part 1 - Add JS::Result

This adds JS::Result and uses it in a few places. It's a bit awkward because most code hasn't been converted yet, so we have to convert back and forth. js::CheckPropertyDescriptorAccessors shows what this will look like though.

Unlike the mega patch, this one doesn't have the JIT integration bits.

I also renamed JS::ReportedOOM in the big patch to JS::OOM, I hope the shorter name will encourage people to use it instead of the more generic JS::Error.
Flags: needinfo?(nicolas.b.pierron)
Attachment #8812769 - Flags: review?(luke)

Comment 33

10 months ago
Comment on attachment 8812769 [details] [diff] [review]
Part 1 - Add JS::Result

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

::: js/public/Result.h
@@ +66,5 @@
> + *     JS_TRY_OR_RETURN_FALSE(cx, DefenestrateObject(cx, obj));
> + *     JS_TRY_VAR_OR_RETURN_FALSE(cx, v, GetObjectThrug(cx, obj));
> + *
> + *     JS_TRY_OR_RETURN_NULL(cx, DefenestrateObject(cx, obj));
> + *     JS_TRY_VAR_OR_RETURN_NULL(cx, v, GetObjectThrug(cx, obj));

Initially I was confused by the "OR" (we're *always* trying, and only sometimes returning false/null), but than I realized that we're probably referring to the short-circuiting behavior of ||.  Given the importance of terseness for what is intended to be such a common utility, this makes sense; just recording my reasoning here.

@@ +109,5 @@
> + * value and a saved stack).
> + *
> + * The long-term plan is to remove JS_IsExceptionPending and
> + * JS_GetPendingException in favor of JS::Error. Exception state will no longer
> + * exist.

Is this plan compatible with using Error& types?  It seems like currently we are able to return a & by keeping global variables we take the address of.  If these become stateful, then we'd need to have a similar stateful (at most one at a time) scheme as we are getting rid of with JSContext which seems like it kindof defeats the purpose.

::: js/src/jscntxt.h
@@ +324,5 @@
> +    inline JS::Result<> boolToResult(bool ok);
> +
> +    /**
> +     * Intentionally awkward signpost method that is stationed on the
> +     * boundary between Result-using and non-Result-using code.

Why do we put this on the cx; is the intention to later do something like asserting an invariants that actually involve cx/rt?

::: js/src/jsobj.h
@@ +182,5 @@
>       * Make a non-array object with the specified initial state. This method
>       * takes ownership of any extantSlots it is passed.
>       */
> +    static inline JS::Result<JSObject*, JS::OOM&> create(js::ExclusiveContext* cx,
> +                                                                 js::gc::AllocKind kind,

nit: column alignment

But actually, I think we have a stylistic choice here: if we use Result everywhere, it's going to crunch up all of our signatures on the right which I think is kindof ugly.  What about starting a new style:

  static etc etc JS::Result<...>
  create(params);

that is allowed for Result types?
Just a drive by question.

Shouldn't we create a typedef for "Result<...>" to remove those <> and make it more clear?
(Assignee)

Comment 35

10 months ago
(In reply to Luke Wagner [:luke] from comment #33)
> Is this plan compatible with using Error& types?  It seems like currently we
> are able to return a & by keeping global variables we take the address of. 
> If these become stateful, then we'd need to have a similar stateful (at most
> one at a time) scheme as we are getting rid of with JSContext which seems
> like it kindof defeats the purpose.

We could mark js::Result<V, E> as a GC type for the static analysis (we probably need to do this anyway for the case where V is a GC type like JSObject*, not sure the analysis is smart enough to figure that out since Result<> may use uintptr_t + pointer tagging to store it).

Then we can js_new<JS::Error> and store GC pointers in it (say JS::Value for the exception value) without worrying about GC hazards. We could maintain a Vector of allocated JS::Error allocations and free all of them on GC (and of course we could add a separate class to "root" a JS::Error across GC calls for the few places that need to do this). Because we don't want to allocate on OOM, the JS::OOM instance could stay as static singleton like in this patch (similar to the "out of memory" constant atom we throw now).

Does something like that seem reasonable?

> Why do we put this on the cx; is the intention to later do something like
> asserting an invariants that actually involve cx/rt?

Since the exception state is on the cx currently, it doesn't seem unreasonable to put these methods there as well. This is intended to be temporary anyway, while we convert code. I'd be happy to change it to a separate function that takes a cx, though.
Flags: needinfo?(luke)
(Assignee)

Comment 36

10 months ago
(In reply to Hannes Verschore [:h4writer] from comment #34)
> Just a drive by question.
> 
> Shouldn't we create a typedef for "Result<...>" to remove those <> and make
> it more clear?

What would we call this typedef? Since it will be used everywhere, it should be something short... I'm also not sure it's clearer, Result<> makes it obvious that it's similar to, say, Result<JSObject*>, which seems useful.
A typedef is going to obscure more than it helps, especially when you consider there are two separate axes to Result.  Revealing similarity as comment 36 notes is also an excellent reason to not typedef (and goes for other typedefs as well).

(In reply to Luke Wagner [:luke] from comment #33)
> I think we have a stylistic choice here: if we use Result
> everywhere, it's going to crunch up all of our signatures on the right which
> I think is kindof ugly.  What about starting a new style:
> 
>   static etc etc JS::Result<...>
>   create(params);
> 
> that is allowed for Result types?

I'm pretty sure this "new" style is already haphazardly adopted in numerous places for member function signatures that grow particularly unwieldy.  Doesn't seem to me it's explicitly sanctioned (in either sense), so no reason anyone shouldn't use it.
(Assignee)

Comment 38

10 months ago
(In reply to Jan de Mooij [:jandem] from comment #35)
> Then we can js_new<JS::Error> and store GC pointers in it (say JS::Value for
> the exception value) without worrying about GC hazards. We could maintain a
> Vector of allocated JS::Error object and free all of them on GC

Hm, maybe we don't even need this Vector if we treat any function that can dynamically allocate JS::Error (so excluding JS::OOM!) as a GC function. This way, the hazard analysis will complain whenever a JS::Result is used across a call that can throw a different JS::Result, other than OOM.

Comment 39

10 months ago
(In reply to Jan de Mooij [:jandem] from comment #35)
Ah, ok, that sounds like a good plan w.r.t allocated error objects.  Mostly I was just a little weirded out by seeing the & in the Result types (& in stack objects returned by value sets off spider-senses for obvious reasons), but it seems like that's what we need to do for performance reasons.

> > Why do we put this on the cx; is the intention to later do something like
> > asserting an invariants that actually involve cx/rt?
> 
> Since the exception state is on the cx currently, it doesn't seem
> unreasonable to put these methods there as well.

Ah, I was asking about JSContext::resultTo*, which don't currently seem to use the JSContext's exception state, and whether later they would.  If they do, then member functions seems great as you've done.
Flags: needinfo?(luke)

Comment 40

10 months ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #37)
> I'm pretty sure this "new" style is already haphazardly adopted in numerous
> places for member function signatures that grow particularly unwieldy. 
> Doesn't seem to me it's explicitly sanctioned (in either sense), so no
> reason anyone shouldn't use it.

Well then perhaps we could just start off by setting the style precedent in these initial landings?  These things tend to propagate.
We could do that.  I confess that I'm not sure I care that much -- anything that's long enough lines is probably going to be wrapped as early as possible even if there's no requirement in place.

Comment 42

10 months ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #41)
Great!  I've seen Gecko code that shows late wrapping, which is why I asked in the first place.

Comment 43

10 months ago
Comment on attachment 8812769 [details] [diff] [review]
Part 1 - Add JS::Result

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

Any, r+ with the suggestion of wrapping long declarations after the return value as discussed.
Attachment #8812769 - Flags: review?(luke) → review+

Comment 44

10 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d29ab4cddb82
part 1 - Add JS::Result<> and use it in a few places. r=luke
(Assignee)

Updated

10 months ago
Keywords: leave-open
(Assignee)

Comment 45

10 months ago
Steve, can you help me figure out how to make JS::Result<> a GC type, for the static analysis? We want this for 2 reasons:

(1) JS::Result<JSObject*, Error&> will be a rooting hazard when unwrap()ped across a GC.
(2) Eventually, JS::Error will store the exception JS::Value and we don't want rooting hazards when that happens. Furthermore, not holding JS::Error across a GC will simplify things when we dynamically allocate JS::Error instances (see comment 35).

I know about JS_HAZ_GC_POINTER, but JS::Result is not a class/struct:

  template <typename V = Ok, typename E = Error&>
  using Result = mozilla::Result<V, E>;

The hazard analysis seems to ignore the JS_HAZ_GC_POINTER attribute here.

Worst case we can apply the attribute to mozilla::Result or make JS::Result inherit from mozilla::Result (it's |final| though), but it's not great.
Flags: needinfo?(sphink)
(Assignee)

Comment 46

10 months ago
If the analysis sees mozilla::Result and JS::Result as the same type, we could also do some pattern matching on the template parameters maybe? But it's pretty annoying.

Comment 47

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d29ab4cddb82

Comment 48

10 months ago
(In reply to Jan de Mooij [:jandem] from comment #46)
> If the analysis sees mozilla::Result and JS::Result as the same type, we
> could also do some pattern matching on the template parameters maybe? But
> it's pretty annoying.

Looking at this, it seems like the nicest way is for it to figure out whether Result<T,E> is a GC pointer from the template parameters T and E. But as you say, the raw storage prevents that from happening now. But it appears that mozilla::Result already has to handle this for other static analyses -- it stores its contents as a mozilla::Variant, which stores them as raw but uses the attribute MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS to tell it to do the right thing.

Filing a bug to implement.

Updated

10 months ago
Depends on: 1321014

Comment 49

10 months ago
Filed bug 1321014.
(Assignee)

Comment 50

10 months ago
(In reply to sfink from comment #48)
> it stores its contents as a mozilla::Variant, which stores them
> as raw but uses the attribute
> MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS to tell it to do the right
> thing.

mozilla::Result *may* use mozilla::Variant, but it also has some more optimized ResultImplementation's. We should probably apply MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS to mozilla::Result as well...

Thanks for looking at this!
Jan, does this bug still need to be open?
Flags: needinfo?(jdemooij)
(In reply to Nicholas Nethercote [:njn] from comment #51)
> Jan, does this bug still need to be open?

Not sure. I thought of it more as a meta-bug that could be kept open as long as there's more bool -> Result work to be done, but I did land patches here so we should mark it fixed (for FF 53).

There's also a needinfo? sfink though so let's wait for that.
Flags: needinfo?(jdemooij)
Keywords: triage-deferred
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.