Special-case "optionals with a default value" and fix compilation errors in Gecko JS #314
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These types are different than optionals without a default value. Their C++ type is just `T`, not `Optional<T>`, since the default value is a fallback if the caller doesn't pass one. But they must be serialized as if they were `Optional<T>`, because the Rust side of the FFI will try to deserialize them as optionals instead of instances of the value.
eoger
approved these changes
Oct 7, 2020
Comment on lines
+53
to
+57
| /// Optionals with a default value are a grab bag of special cases in Gecko. | ||
| /// In the generated C++ bindings, the type of an optional with a default | ||
| /// value is `T`, not `Optional<T>`. However, it must be serialized as if | ||
| /// it's an `Optional<T>`, since that's what the Rust side of the FFI | ||
| /// expects. |
There was a problem hiding this comment.
What a headache, I wonder if we shouldn't have disallowed optionals without default values in IDL files.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Optional arguments and members with a default value are a bag of special cases.
On the C++ side,
optionalarguments with default values are passed asconst T&, and optional dictionary members are typeT(for optional arguments, they'reconst dom::Optional<T>&anddom::Optional<T>, respectively). But the Rust side is expecting them to be serialized asOption<T>. It's basically the same problem as strings, where null and non-null strings have the sameconst nsAString&/nsStringtype—the Rust side is expecting to read1/0followed by the serialization of the type, but the C++ side isn't passing that1/0.So the first commit adds a special
WebIDLType::OptionalWithDefaultValuethat's emitted asT, except when we callViaFfi, where it passesNullable = true. It also adds a helper for serializing non-null things on the C++ side as if they wereSome<T>for Rust.The second commit fixes some errors in the generated Nimbus bindings.
dom::Optional<T>can't be constructed directly, andThrowOperationErrortakes aconst ACString&, not a rawchar*, so we have to wrap it in annsDependentCString.@dmose and I paired on this yesterday, and got the Nimbus SDK building with these changes. We haven't written an xpcshell test for it yet—it's crashing because it's trying to use Viaduct to make a network request from the main thread, which is forbidden—but it compiles and runs.