Add `nullable` and `notnull` attributes to XPIDL
Categories
(Core :: XPConnect, enhancement, P3)
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 asmozilla::Maybe<T>
in C++, for all types. In JS, it would be eithernull
or a value of typeT
.[notnull] in T
would bemozilla::NotNull<T*>
, and only apply to interface, pointers, andwebidl
types...others are already not null by design. (Except strings...I'm kind of leaning toward[notnull] in AString
throwing if you passnull
. If you really want a void string, you can leave off the annotation). In JS, this would just beT
, which you could use without null-checking.- In JS, passing
null
orundefined
for something annotated[notnull]
would throw an XPC exception at the call site. In C++, we'd rely on the release assert inWrapNotNull
to make sureT*
isn't null before calling the method. - Ditto for return values. In C++, a
[notnull]
return parameter could be reflected as aNotNull<RefPtr<T>>&
out parameter (instead ofT**
and usinggetter_AddRefs
at the call site; for primitives and strings, this would just beT&
), andnullable
could likewise beMaybe<RefPtr<T>>&
orMaybe<T>&
.
I think this would help in a few cases:
- 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. - 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++. - 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.
Comment 1•4 years ago
|
||
(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 passnullptr
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 passnull
for a string argument from JS, it'll look like an empty string, butaStr.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 asmozilla::Maybe<T>
in C++, for all types. In JS, it would be eithernull
or a value of typeT
.
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 bemozilla::NotNull<T*>
, and only apply to interface, pointers, andwebidl
types...others are already not null by design. (Except strings...I'm kind of leaning toward[notnull] in AString
throwing if you passnull
. If you really want a void string, you can leave off the annotation). In JS, this would just beT
, 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
orundefined
for something annotated[notnull]
would throw an XPC exception at the call site. In C++, we'd rely on the release assert inWrapNotNull
to make sureT*
isn't null before calling the method.- Ditto for return values. In C++, a
[notnull]
return parameter could be reflected as aNotNull<RefPtr<T>>&
out parameter (instead ofT**
and usinggetter_AddRefs
at the call site; for primitives and strings, this would just beT&
), andnullable
could likewise beMaybe<RefPtr<T>>&
orMaybe<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:
- 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.- 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++.- 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.
Comment 2•4 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #1)
[nullable] in T
would be reflected asmozilla::Maybe<T>
in C++, for all types. In JS, it would be eithernull
or a value of typeT
.This probably gets complicated at the xptcall level because how
Maybe<T>
is passed depends on the trivialness ofT
. (e.g. I thinkMaybe<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 howOption
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 bemozilla::NotNull<T*>
, and only apply to interface, pointers, andwebidl
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 passnull
. If you really want a void string, you can leave off the annotation). In JS, this would just beT
, 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
orundefined
for something annotated[notnull]
would throw an XPC exception at the call site. In C++, we'd rely on the release assert inWrapNotNull
to make sureT*
isn't null before calling the method.- Ditto for return values. In C++, a
[notnull]
return parameter could be reflected as aNotNull<RefPtr<T>>&
out parameter (instead ofT**
and usinggetter_AddRefs
at the call site; for primitives and strings, this would just beT&
), andnullable
could likewise beMaybe<RefPtr<T>>&
orMaybe<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.
Comment 3•4 years ago
|
||
(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.
Comment 4•4 years ago
|
||
(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.
Comment 5•4 years ago
|
||
(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 (?)).
Description
•