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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

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.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: errormageddon-wip-1.patch

Comment 3

[Jan de Mooij [:jandem]]

Very cool.

Comment 4

[Jason Orendorff [:jorendorff]]

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.

Comment 5

[Jason Orendorff [:jorendorff]]

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?

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: 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.)

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: errormageddon-wip-3.patch

Comment 8

[Nicholas Nethercote [inactive]]

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?

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: 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.)

Comment 10

[Jason Orendorff [:jorendorff]]

WIP 4 applies on top of rev 0ad7433c2159.

Comment 11

[Jason Orendorff [:jorendorff]]

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.

Comment 12

[Jason Orendorff [:jorendorff]]

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

Comment 13

[Jan de Mooij [:jandem]]

I can look at the Win32 thing.

Comment 14

[Jan de Mooij [:jandem]]

(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.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: 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).

Comment 16

[Terrence Cole [:terrence]]

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.

Comment 17

[Jason Orendorff [:jorendorff]]

Thank you both. This is excellent news.

Comment 18

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fff08b62957

Comment 19

[Chris Peterson [:cpeterson]]

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.

Comment 20

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea74e139f850

Comment 21

[Nicholas Nethercote [inactive]]

Jason, any progress here?

Comment 22

[Jan de Mooij [:jandem]]

Attachment: 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?

Comment 23

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 24

[Till Schneidereit [:till]]

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.

Comment 25

[Jason Orendorff [:jorendorff]]

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.)

Comment 26

[Till Schneidereit [:till]]

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.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 29

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 30

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 31

[Hannes Verschore [:h4writer]]

(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.

Comment 32

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 33

[Luke Wagner [:luke]]

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?

Comment 34

[Hannes Verschore [:h4writer]]

Just a drive by question.

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

Comment 35

[Jan de Mooij [:jandem]]

(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.

Comment 36

[Jan de Mooij [:jandem]]

(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.

Comment 37

[Jeff Walden [:Waldo]]

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.

Comment 38

[Jan de Mooij [:jandem]]

(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

[Luke Wagner [:luke]]

(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.

Comment 40

[Luke Wagner [:luke]]

(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.

Comment 41

[Jeff Walden [:Waldo]]

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

[Luke Wagner [:luke]]

(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

[Luke Wagner [:luke]]

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.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d29ab4cddb82
part 1 - Add JS::Result<> and use it in a few places. r=luke

Comment 45

[Jan de Mooij [:jandem]]

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.

Comment 46

[Jan de Mooij [:jandem]]

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

[Carsten Book [:Tomcat]]

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

Comment 48

[(use [e-mail] aka ˸sfink instead)]

(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.

Comment 49

[(use [e-mail] aka ˸sfink instead)]

Filed bug 1321014.

Comment 50

[Jan de Mooij [:jandem]]

(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!

Comment 51

[Nicholas Nethercote [inactive]]

Jan, does this bug still need to be open?

Comment 52

[Jan de Mooij [:jandem]]

(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.

Comment 53

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #51)
> Jan, does this bug still need to be open?

I think we should close this now because we did land patches so it's confusing to keep it open, and JS::Result<> is now a thing. Maybe we could file a new errormageddon meta bug.

Comment 54

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

(In reply to Jan de Mooij [:jandem] from comment #52)
> There's also a needinfo? sfink though so let's wait for that.

There's work I need to do for this in bug 1321014, but that's probably not a good reason to keep this particular bug alive.