Open Bug 1659200 Opened 4 years ago Updated 4 years ago

Add `nullable` and `notnull` attributes to XPIDL

Categories

(Core :: XPConnect, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: lina, Unassigned)

References

(Blocks 1 open bug)

Details

Currently, XPIDL doesn't have nullability annotations. Interface, pointer, and webidl parameters and return values are nullable by default. Numbers aren't—you can't pass nullptr for an integer or float when calling an XPIDL method from C++, and XPConnect will coerce it to 0 when calling from JS. Strings are "voided": if you pass null for a string argument from JS, it'll look like an empty string, but aStr.IsVoid() will return true.

Is it worth making this more explicit through nullability annotations? Maybe something like this:

  • [nullable] in T would be reflected as mozilla::Maybe<T> in C++, for all types. In JS, it would be either null or a value of type T.
  • [notnull] in T would be mozilla::NotNull<T*>, and only apply to interface, pointers, and webidl types...others are already not null by design. (Except strings...I'm kind of leaning toward [notnull] in AString throwing if you pass null. If you really want a void string, you can leave off the annotation). In JS, this would just be T, which you could use without null-checking.
  • In JS, passing null or undefined for something annotated [notnull] would throw an XPC exception at the call site. In C++, we'd rely on the release assert in WrapNotNull to make sure T* isn't null before calling the method.
  • Ditto for return values. In C++, a [notnull] return parameter could be reflected as a NotNull<RefPtr<T>>& out parameter (instead of T** and using getter_AddRefs at the call site; for primitives and strings, this would just be T&), and nullable could likewise be Maybe<RefPtr<T>>& or Maybe<T>&.

I think this would help in a few cases:

  1. Code generation. We're currently exploring generating XPCOM bindings for Rust components in Desktop. We can add runtime checks in the form of NS_ENSURE_ARG{_POINTER}, but it would be great to have more of the XPConnect and XPIDL machinery do this for us, especially since it's already doing some type checking.
  2. Nullable primitives. For integers, sometimes it's important to tell the difference between "not passed" and "passed but is 0". Right now, I think we can use in nsIVariant as a workaround, but variants are clunky to use from C++.
  3. Stronger guarantees about whether something is allowed to be null or not. Right now, I think we're mostly relying on doc comments in the interface, and the NS_ENSURE_* macros in the implementations.

(In reply to Lina Cambridge [:lina] from comment #0)

Currently, XPIDL doesn't have nullability annotations. Interface, pointer, and webidl parameters and return values are nullable by default. Numbers aren't—you can't pass nullptr for an integer or float when calling an XPIDL method from C++, and XPConnect will coerce it to 0 when calling from JS. Strings are "voided": if you pass null for a string argument from JS, it'll look like an empty string, but aStr.IsVoid() will return true.

Is it worth making this more explicit through nullability annotations? Maybe something like this:

I think it would be valuable.

  • [nullable] in T would be reflected as mozilla::Maybe<T> in C++, for all types. In JS, it would be either null or a value of type T.

This probably gets complicated at the xptcall level because how Maybe<T> is passed depends on the trivialness of T. (e.g. I think Maybe<int> gets passed in a single register on x86-64, but as a pointer on x86; Maybe<int64_t> gets passed in...two registers on x86-64, I think? Maybe<nsString> gets passed as a pointer always, I believe.) We also have to consider how this gets passed to Rust; I think the idiomatic translation would be Option<T>...but I don't think we have any guarantees on how Option is laid out or passed?

  • [notnull] in T would be mozilla::NotNull<T*>, and only apply to interface, pointers, and webidl types...others are already not null by design. (Except strings...I'm kind of leaning toward [notnull] in AString throwing if you pass null. If you really want a void string, you can leave off the annotation). In JS, this would just be T, which you could use without null-checking.

Same comments apply here as far as argument passing goes, but I think it's easier in this case, both because NotNull<T*> is likely more uniform in calling conventions and the idiomatic translation on the C++ and the Rust side is a reference, which ought to be easily handled.

  • In JS, passing null or undefined for something annotated [notnull] would throw an XPC exception at the call site. In C++, we'd rely on the release assert in WrapNotNull to make sure T* isn't null before calling the method.
  • Ditto for return values. In C++, a [notnull] return parameter could be reflected as a NotNull<RefPtr<T>>& out parameter (instead of T** and using getter_AddRefs at the call site; for primitives and strings, this would just be T&), and nullable could likewise be Maybe<RefPtr<T>>& or Maybe<T>&.

I think we have an (unstated) convention that outparams generally should be pointer values (so we would have NotNull<RefPtr<T>>* as the outparam type), so that you can see at callsites that an address is being taken and therefore something might be getting modified. I know that kind of conflicts with the pointer (almost?) never being null.

I think this would help in a few cases:

  1. Code generation. We're currently exploring generating XPCOM bindings for Rust components in Desktop. We can add runtime checks in the form of NS_ENSURE_ARG{_POINTER}, but it would be great to have more of the XPConnect and XPIDL machinery do this for us, especially since it's already doing some type checking.
  2. Nullable primitives. For integers, sometimes it's important to tell the difference between "not passed" and "passed but is 0". Right now, I think we can use in nsIVariant as a workaround, but variants are clunky to use from C++.
  3. Stronger guarantees about whether something is allowed to be null or not. Right now, I think we're mostly relying on doc comments in the interface, and the NS_ENSURE_* macros in the implementations.

+1 to all of this.

(In reply to Nathan Froyd [:froydnj] from comment #1)

  • [nullable] in T would be reflected as mozilla::Maybe<T> in C++, for all types. In JS, it would be either null or a value of type T.

This probably gets complicated at the xptcall level because how Maybe<T> is passed depends on the trivialness of T. (e.g. I think Maybe<int> gets passed in a single register on x86-64, but as a pointer on x86; Maybe<int64_t> gets passed in...two registers on x86-64, I think? Maybe<nsString> gets passed as a pointer always, I believe.)

It would not be possible to implement this with Rust support, due to the differences in how types with destruction are handled in the calling convention (namely Rust transfers ownership to the callee, whereas C++ requires the caller to run argument destructors - at least on Linux). The code would need to accept types more like const mozilla::Maybe<T>& so that it's clear that ownership isn't being transferred into the callee.

This is also inconsistent with how nullability is handled in WebIDL, which I think is somewhat unfortunate. If we were to switch xpidl to have a more advanced concept of nullability, I'd like to keep it aligned with how it's handled in WebIDL to avoid awkward compatibility hazards.

We also have to consider how this gets passed to Rust; I think the idiomatic translation would be Option<T>...but I don't think we have any guarantees on how Option is laid out or passed?

We don't have any guarantees about that, so we'd need to define our own rust Maybe type with a known fixed layout. It could also pose difficulties for us down the line, as it would require us to lock in our C++ Maybe<T>'s layout. For example, things like changing our template type to automatically optimize Maybe<NotNull<T*>> to be a single pointer may be impossible, as we wouldn't be able to represent the SFINAE template logic from C++ in Rust code.

In order to make this correct, we'd probably need a standard-layout type like struct MaybeRepr<T>, and to have all of the fancy C++ template stuff inherit from it, in order to ensure that the actual layout is known.

  • [notnull] in T would be mozilla::NotNull<T*>, and only apply to interface, pointers, and webidl types...others are already not null by design.

Same comments apply here as far as argument passing goes, but I think it's easier in this case, both because NotNull<T*> is likely more uniform in calling conventions and the idiomatic translation on the C++ and the Rust side is a reference, which ought to be easily handled.

I agree that something like this might be easier to implement, but we probably can't directly use &T as the type on the rust side due to ABI concerns.

We already have one type which is effectively a pointer wrapper which we support passing to methods like this, which is TD_JSVAL. The type JS::Handle<JS::Value> is a thin wrapper around a pointer to the JS::Value. For most ABIs, the type is passed in a pointer register, as for the other types, but there are a few ABIs which actually have special logic in their xptcall code because the ABI is changed by it being a struct.

Taking a quick look, I see that we have special handling for TD_JSVAL for ppc_linux, ppc_openbsd, sparc_netbsd, sparc_openbsd, and sparc_solaris (e.g. https://searchfox.org/mozilla-central/rev/23dd7c485a6525bb3a974abeaafaf34bfb43d76b/xpcom/reflect/xptcall/md/unix/xptcinvoke_ppc_linux.cpp#49-50). In order to support these types, the Rust side would need to receive the method wrapped in a newtype struct, or behind a reference.

This should be somewhat hide-able from rust xpcom code, however, as we already perform some amount of conversion logic between what the user sees and the raw xpcom APIs for rust code.

(Except strings...I'm kind of leaning toward [notnull] in AString throwing if you pass null. If you really want a void string, you can leave off the annotation). In JS, this would just be T, which you could use without null-checking.

This seems fine to me. There would be no difference in the actual type which is reflected into C++ and Rust code though.

  • In JS, passing null or undefined for something annotated [notnull] would throw an XPC exception at the call site. In C++, we'd rely on the release assert in WrapNotNull to make sure T* isn't null before calling the method.
  • Ditto for return values. In C++, a [notnull] return parameter could be reflected as a NotNull<RefPtr<T>>& out parameter (instead of T** and using getter_AddRefs at the call site; for primitives and strings, this would just be T&), and nullable could likewise be Maybe<RefPtr<T>>& or Maybe<T>&.

I think we have an (unstated) convention that outparams generally should be pointer values (so we would have NotNull<RefPtr<T>>* as the outparam type), so that you can see at callsites that an address is being taken and therefore something might be getting modified. I know that kind of conflicts with the pointer (almost?) never being null.

IIRC this is done by convention going back to when XPCOM had support for other languages like C. I would expect the outparam to have a type like what :froydnj suggested.

I think I'd like to consider having XPIDL's types generally match the behaviour of WebIDL. This sometimes means that the C++ code will have types which are less precise, but we can make sure the types being generated in Rust code, and the JS<->native conversion logic, are aware and check these properties. As an example, WebIDL still uses nsString for both DOMString and DOMString?, but only allows a null value in the ? case, and uses Interface* for both Interface and Interface? types, and just only allows null in one of them.

This would reduce the number of places which need to have an extra Maybe type wrapped around them, potentially to the point that we can somehow guarantee the layout and ABI of specific instances of those types so they can be passed over FFI by-value. (though I might want to add things like maybe-wrapping in a separate series of patches)

We could then have the code generated by #[xpimplements] and such perform the conversion from the native C++ types which we're using, and which may be somewhat imprecise, to the more precise rust types we're interested in, unwrapping *const nsIFoo, for example, into &nsIFoo.

An approach like this doesn't give us great type safety in C++, but it still improves documentation, and makes it easier and more clean to communicate the interface between Rust/C++/JS code.

As a bit of a side note, if we're going to be going the route of using more complex types in XPConnect-compatible interfaces in the longer-term, we'll probably need to find some way to move away from XPTCall so we can avoid some of the ABI issues, though we'll still need to figure out how to make the types we use in C++ be reflected into Rust code safely as well.

(In reply to Nika Layzell [:nika] (ni? for response) from comment #2)

As a bit of a side note, if we're going to be going the route of using more complex types in XPConnect-compatible interfaces in the longer-term,

I think the guidance here for now is: use XPIDL where you can, and improve XPIDL where it's straightforward to do. I don't think we've yet made a decision to invest heavily in expanding XPConnect's capabilities around complex types. Not quite sure where nullability falls on the spectrum.

we'll probably need to find some way to move away from XPTCall

Not sure what that would look like. The reason xptcall exists is that there was no other way.

(In reply to Bobby Holley (:bholley) from comment #3)

I think the guidance here for now is: use XPIDL where you can, and improve XPIDL where it's straightforward to do. I don't think we've yet made a decision to invest heavily in expanding XPConnect's capabilities around complex types. Not quite sure where nullability falls on the spectrum.

In terms of where nullability falls on the spectrum, I think it depends a lot on how we end up implementing it. If we implement it for all types using Maybe and NotNull, this would be a fairly large change due to the complex types in signatures. With a limited approach which generally avoids impacting C++ signatures or using complex types in parameters, only changing Rust signatures and xpconnect's parameter validation, it could be a much smaller change.

Not sure what that would look like. The reason xptcall exists is that there was no other way.

It was definitely the only way to make it work when we could dynamically load new interfaces. With the new system of statically known interfaces, it's theoretically possible to make a huge binary size trade-off, and generate bespoke code for each unique method shape, which would allow us to rely on the compiler to generate the correct ABI logic.

I don't think this is the direction we want to go, considering the complexity of the change and the likely large binary size increase it would cause, but I think it's the main alternative to xptcall, and perhaps the direction we'd need to take if we want to support complex types in xpconnect signatures.

(In reply to Nika Layzell [:nika] (ni? for response) from comment #4)

Not sure what that would look like. The reason xptcall exists is that there was no other way.

It was definitely the only way to make it work when we could dynamically load new interfaces. With the new system of statically known interfaces, it's theoretically possible to make a huge binary size trade-off, and generate bespoke code for each unique method shape, which would allow us to rely on the compiler to generate the correct ABI logic.

I don't think this is the direction we want to go, considering the complexity of the change and the likely large binary size increase it would cause, but I think it's the main alternative to xptcall, and perhaps the direction we'd need to take if we want to support complex types in xpconnect signatures.

This was discussed a bit in bug 1483885 (and bugs linked from there); the document linked in that comment has a rough overview of what would be necessary.

The binary size increase is probably ~100-200k for the per-interface vtables, which I think is non-shared memory between processes on at least Mac (Windows doesn't take a hit -- I think, and we are using forkservers on Linux (?)).

P3

Severity: -- → N/A
Type: task → enhancement
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.