[rustmojo] Add skeleton for Mojom bindings user interface [chromium/src : main]
Devon Loehr (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Daniel Cheng
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 14 comments![]( "Open in Gerrit")
//! fn Div(&mut self, dividend: i32, divisor: i32, response_callback: FnOnce(i32, i32));nit: `impl FnOnce(...)`, right? Or maybe `Box<dyn FnOnce(...)>`?
Can `response_callback` be called after `self` goes away? If not, then I wonder if the type of `response_callback` should refer to the lifetime of `self` (e.g. `FnOnce(...) + `a`).
Can `response_callback` be passed to another thread and then invoked from that other thread? If so, then maybe we should say `+ Send`?
//! impl MathService for MathServiceImpl { ... }`SaturatingMathService`, right?
//! // Undbing the receiver, getting the state object outUnbind?
//! let (p_rec, final_count) = counting_receiver.unbind();Side-note: If this didn't return a **pair**, then I would suggest naming this `into_inner()`. But I think `unbind` is okay and better if we need to return both the inner impl + the strongly-typed pipe/pending-receiver.
Question: would it make sense to say `count_state` rather than `final_count` to signify that this is the same object that was created earlier on line 120?
// rules and lack of support for multiple blanket implementations of a trait.Disclaimer: I think that a macro is a reasonable, clean solution to all of this (and it gives us a centralized point of control where we can potentially tweak things in the future if needed). But for completeness, let me try to see if there are other solutions to the problems listed here.
IIUC the `.mojom` => `.rs` generator will generate `trait MathService`. Is it allowed / would it work to have a blanket impl **there**?:
```
impl<T> MojoService for T where T: MojoService {}
```
// `dyn SomeInterface` as the parameter to our objects, there's no way to specifyAck. I can't think of anything smart here. I wondered about using `T: !Sized` bound for `Remote`/`Received` (which any `dyn ...` would satisfy), but negative bounds are not supported. Maybe there are some hacks to make this work, but I think we should flush out this approach before (optionally) coming back to this later.
// macro like they're supposed to), this ensures that the only valid parametersI wonder if we can:
* Define `trait UsedTheMacro` (name TBD) that is:
- internal (`mod internal`) and undocumented (doc hidden)
- Marked with [`diagnostic::on_unimplemented`](https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-diagnosticon_unimplemented-attribute) attribute so that when a user doesn't implement this hidden trait, then our error message wags a finger and says "please use the macro!".
* And then have our macro "automatically" impl this trait
// FOR_RELEASE: This is a very clever solution, and possibly too clever. With
// good API design, we can probably make it impossible to construct a remote/
// receiver of the wrong type. But it's still not checked by the compiler, so
// that solution requires all future API designers to maintain the invariant.Meh... if it works, it works. But... we probably **should** try to solicit a review by people who are more familiar with Rust type system tricks. @manishearth may be a good start I think + is a Chromium committer.
payload: Vec<u8>,`Rc<[u8]>` to indicate immutability. I forgot how to construct values of this type - I think there is a helper somewhere...
fn dispatch_response(message: Message, response_callback: Self::CallbackTy);I don't understand the `response_callback` parameter. The map of response_id-to-resonse_callback will be maintained by a `Remote`, right? If so, then should this take `&mut self` as the first parameter instead of taking `response_callback`? But maybe I just need to wait for an implementation...
pub trait SameTypeAs<T: ?Sized> {}A thought: can we compare `TypeId::of<T>()` as an alternative approach for checking type equality at compile time?
pending_responses: std::collections::HashMap<u32, T::CallbackTy>,Would it simplify things if `pending_responses`:
- Was a field of the `mojom`-generated struct instead?
- Was split into multiple maps - one per method / per response callback type?
?
fn new_with_runner(endpoint: MessageEndpoint, runner: SequencedTaskRunnerHandle) -> Self {I wonder if C++ checks that interface methods are called on the right sequence. And if so, then how we could duplicate this behavior.
Related details
- Daniel Cheng
- Devon Loehr
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devon Loehr (Gerrit)
Devon Loehr voted and added 12 comments![]( "Open in Gerrit")
Votes added by Devon Loehr
| Commit-Queue | +1 |
12 comments
//! fn Div(&mut self, dividend: i32, divisor: i32, response_callback: FnOnce(i32, i32));nit: `impl FnOnce(...)`, right? Or maybe `Box<dyn FnOnce(...)>`?
Can `response_callback` be called after `self` goes away? If not, then I wonder if the type of `response_callback` should refer to the lifetime of `self` (e.g. `FnOnce(...) + `a`).
Can `response_callback` be passed to another thread and then invoked from that other thread? If so, then maybe we should say `+ Send`?
I don't think it's related to `self` at all; adding `Send` makes sense, though.
//! impl MathService for MathServiceImpl { ... }Devon Loehr`SaturatingMathService`, right?
Done
//! // Undbing the receiver, getting the state object outDevon LoehrUnbind?
Done
//! let (p_rec, final_count) = counting_receiver.unbind();Side-note: If this didn't return a **pair**, then I would suggest naming this `into_inner()`. But I think `unbind` is okay and better if we need to return both the inner impl + the strongly-typed pipe/pending-receiver.
Question: would it make sense to say `count_state` rather than `final_count` to signify that this is the same object that was created earlier on line 120?
Yeah, I think the `bind`/`unbind` names are meaningful (they're not just data conversions) so I think we should consistently used them throughout.
Changed the `final_count` name as suggested.
// rules and lack of support for multiple blanket implementations of a trait.Disclaimer: I think that a macro is a reasonable, clean solution to all of this (and it gives us a centralized point of control where we can potentially tweak things in the future if needed). But for completeness, let me try to see if there are other solutions to the problems listed here.
IIUC the `.mojom` => `.rs` generator will generate `trait MathService`. Is it allowed / would it work to have a blanket impl **there**?:
```
impl<T> MojoService for T where T: MojoService {}
```
No; the problem is that we need to write
```
impl<T> MojomInterface for T where T: MathService {}
```
So the trait we're `impl`ing is `MojomInterface`, which we didn't define. And of course, `T` is arbitrary, so we don't control that either.
// `dyn SomeInterface` as the parameter to our objects, there's no way to specifyAck. I can't think of anything smart here. I wondered about using `T: !Sized` bound for `Remote`/`Received` (which any `dyn ...` would satisfy), but negative bounds are not supported. Maybe there are some hacks to make this work, but I think we should flush out this approach before (optionally) coming back to this later.
Even the `!Sized` bound wouldn't be enough because it might let you use the wrong `dyn`. I think the comment below about the "possibly-too-clever" solution is where this is really addressed.
// macro like they're supposed to), this ensures that the only valid parametersI wonder if we can:
* Define `trait UsedTheMacro` (name TBD) that is:
- internal (`mod internal`) and undocumented (doc hidden)
- Marked with [`diagnostic::on_unimplemented`](https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-diagnosticon_unimplemented-attribute) attribute so that when a user doesn't implement this hidden trait, then our error message wags a finger and says "please use the macro!".
* And then have our macro "automatically" impl this trait
Ah, I actually considered this exact thing, but I didn't know about that attribute. I thought that a trait named `YouNeedToUseTheMacro` was a little overkill, but if we can give a custom error message then that's a good idea. I'll look into it for a later CL.
`Rc<[u8]>` to indicate immutability. I forgot how to construct values of this type - I think there is a helper somewhere...
I'm going to leave it for now because this is just a dummy type so I can write the rest of the interface; we can come back to this when we've got an actual message type in front of us.
fn dispatch_response(message: Message, response_callback: Self::CallbackTy);I don't understand the `response_callback` parameter. The map of response_id-to-resonse_callback will be maintained by a `Remote`, right? If so, then should this take `&mut self` as the first parameter instead of taking `response_callback`? But maybe I just need to wait for an implementation...
Hopefully this will be clearer with the code (I only just realized we needed this myself). My intention is that the bindings will generate something like
```
CallbackTy = enum {
AddCallback(Box<dyn FnOnce(i32)>),
DivCallback(Box<dyn FnOnce(i32, i32)>),
}
fn dispatch_response(message, callback) {
match (message_ID, callback) {
(ADD_ID, AddCallback(fun)) => fun(parse_add(message));
(DIV_ID, DivCallback(fun)) => fun(parse_div(message));
_ => panic!("Mismatched callback and message")
}
}
```The remote will just look up the stashed callback in its map, and pass it along. It can't actually deconstruct the callback type because this generic implementation doesn't know how many methods the associated interface will have.
pub trait SameTypeAs<T: ?Sized> {}A thought: can we compare `TypeId::of<T>()` as an alternative approach for checking type equality at compile time?
Hmmm, maybe? I'm not sure if we can do that in "trait position" without doing something like this that uses a const generic instead of a type parameter.
pending_responses: std::collections::HashMap<u32, T::CallbackTy>,Would it simplify things if `pending_responses`:
- Was a field of the `mojom`-generated struct instead?
- Was split into multiple maps - one per method / per response callback type?
?
I'm not sure what `mojom`-generated struct you're talking about -- I think that for `Remote` we don't end up generating any structs (but we'll do some `impls` for `dyn Whatever`).
We could split it into multiple maps, but we'd have to do so dynamically, because `Remote<T>` doesn't know how many methods the associated interface has as long as `T` is generic.
fn new_with_runner(endpoint: MessageEndpoint, runner: SequencedTaskRunnerHandle) -> Self {I wonder if C++ checks that interface methods are called on the right sequence. And if so, then how we could duplicate this behavior.
I know C++ has a `DCHECK_CALLED_ON_VALID_SEQUENCE`, which seems to be the way they check this. I just filed b/470438844 which is related to this, so let's continue the discussion there.
Related details
- Daniel Cheng
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devon Loehr (Gerrit)
Devon Loehr added 1 comment![]( "Open in Gerrit")
// FOR_RELEASE: This is a very clever solution, and possibly too clever. With
// good API design, we can probably make it impossible to construct a remote/
// receiver of the wrong type. But it's still not checked by the compiler, so
// that solution requires all future API designers to maintain the invariant.Meh... if it works, it works. But... we probably **should** try to solicit a review by people who are more familiar with Rust type system tricks. @manishearth may be a good start I think + is a Chromium committer.
Sounds good - @manishearth, could you take a look at this trick to see if it makes sense/there's not a better way?
Related details
- Daniel Cheng
- Manish Goregaokar
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manish Goregaokar (Gerrit)
Manish Goregaokar added 3 comments![]( "Open in Gerrit")
// FOR_RELEASE: This is a very clever solution, and possibly too clever. With
// good API design, we can probably make it impossible to construct a remote/
// receiver of the wrong type. But it's still not checked by the compiler, so
// that solution requires all future API designers to maintain the invariant.Devon LoehrMeh... if it works, it works. But... we probably **should** try to solicit a review by people who are more familiar with Rust type system tricks. @manishearth may be a good start I think + is a Chromium committer.
Sounds good - @manishearth, could you take a look at this trick to see if it makes sense/there's not a better way?
I'm not really sure I understand this setup. Is this designed the way it is so that people can only write `Receiver<dyn Interface>` and not `Receiver<ConcreteType>`?
You could potentially achieve that via a trait DynMojomInterface that is only implemented on `dyn Interface`. I think that might be cleaner? It would be better for docs, too, because you can have `T: DynMojomInterface` vs `T: MojomInterface` and they are both useful bounds meaning different things.
type DynTy: MojomInterface + ?Sized + SameTypeAs<Self::DynTy>;question: why is this SameTypeAs necessary? It feels like this will trivially always be true?
Or is this to help other bounds assume it?
pub trait SameTypeAs<T: ?Sized> {}Devon LoehrA thought: can we compare `TypeId::of<T>()` as an alternative approach for checking type equality at compile time?
Hmmm, maybe? I'm not sure if we can do that in "trait position" without doing something like this that uses a const generic instead of a type parameter.
You can't, SameTypeAs is a useful trick, though most of the time you can avoid it in my experience.
Related details
- Daniel Cheng
- Devon Loehr
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devon Loehr (Gerrit)
Devon Loehr added 3 comments![]( "Open in Gerrit")
// FOR_RELEASE: This is a very clever solution, and possibly too clever. With
// good API design, we can probably make it impossible to construct a remote/
// receiver of the wrong type. But it's still not checked by the compiler, so
// that solution requires all future API designers to maintain the invariant.Devon LoehrMeh... if it works, it works. But... we probably **should** try to solicit a review by people who are more familiar with Rust type system tricks. @manishearth may be a good start I think + is a Chromium committer.
Manish GoregaokarSounds good - @manishearth, could you take a look at this trick to see if it makes sense/there's not a better way?
I'm not really sure I understand this setup. Is this designed the way it is so that people can only write `Receiver<dyn Interface>` and not `Receiver<ConcreteType>`?
You could potentially achieve that via a trait DynMojomInterface that is only implemented on `dyn Interface`. I think that might be cleaner? It would be better for docs, too, because you can have `T: DynMojomInterface` vs `T: MojomInterface` and they are both useful bounds meaning different things.
There are two reasons: first, we want to ensure that people only write `Remote<dyn SomeInterface>`, but we also want to make sure that they can't confuse `Remote<dyn SomeInterface>` and `Remote<dyn SomeOtherInterface>`. Implementing a marker trait only for `dyn Interface` would achieve the first, but it would (in theory) leave the door open for the second.
In essence, what we really want to say is to parameterize on a trait, like `Remote<SomeInterface>`, for readability, consistency with C++, and to ensure that each `Remote/Receiver` pair is using the same interface. But since that's not allowed, we're using `dyn SomeInterface` instead of `Interface`, and this trickery is just to try to prevent people from writing other things in that position.
type DynTy: MojomInterface + ?Sized + SameTypeAs<Self::DynTy>;question: why is this SameTypeAs necessary? It feels like this will trivially always be true?
Or is this to help other bounds assume it?
Yes, it was to help later bounds assume it. That said, I don't think I ended up keeping any of the code that took advantage of it, so we can remove it.
pub trait SameTypeAs<T: ?Sized> {}Devon LoehrA thought: can we compare `TypeId::of<T>()` as an alternative approach for checking type equality at compile time?
Manish GoregaokarHmmm, maybe? I'm not sure if we can do that in "trait position" without doing something like this that uses a const generic instead of a type parameter.
You can't, SameTypeAs is a useful trick, though most of the time you can avoid it in my experience.
- Daniel Cheng
- Manish Goregaokar
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devon Loehr (Gerrit)
Devon Loehr added 1 comment![]( "Open in Gerrit")
// macro like they're supposed to), this ensures that the only valid parametersDevon LoehrI wonder if we can:
* Define `trait UsedTheMacro` (name TBD) that is:
- internal (`mod internal`) and undocumented (doc hidden)
- Marked with [`diagnostic::on_unimplemented`](https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-diagnosticon_unimplemented-attribute) attribute so that when a user doesn't implement this hidden trait, then our error message wags a finger and says "please use the macro!".
* And then have our macro "automatically" impl this trait
Ah, I actually considered this exact thing, but I didn't know about that attribute. I thought that a trait named `YouNeedToUseTheMacro` was a little overkill, but if we can give a custom error message then that's a good idea. I'll look into it for a later CL.
Ah, unfortunately I think this doesn't quite work because we can't `impl UsedTheMacro for Remote<dyn MathInterface>` in the generated code, since it didn't define either `UsedTheMacro` or `Remote`. It's very close to working, so maybe I'll come up with a variant, but for now it doesn't seem possible.
Related details
- Daniel Cheng
- Manish Goregaokar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devon Loehr (Gerrit)
Devon Loehr added 1 comment![]( "Open in Gerrit")
// macro like they're supposed to), this ensures that the only valid parametersDevon LoehrI wonder if we can:
* Define `trait UsedTheMacro` (name TBD) that is:
- internal (`mod internal`) and undocumented (doc hidden)
- Marked with [`diagnostic::on_unimplemented`](https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-diagnosticon_unimplemented-attribute) attribute so that when a user doesn't implement this hidden trait, then our error message wags a finger and says "please use the macro!".
* And then have our macro "automatically" impl this trait
Devon LoehrAh, I actually considered this exact thing, but I didn't know about that attribute. I thought that a trait named `YouNeedToUseTheMacro` was a little overkill, but if we can give a custom error message then that's a good idea. I'll look into it for a later CL.
Ah, unfortunately I think this doesn't quite work because we can't `impl UsedTheMacro for Remote<dyn MathInterface>` in the generated code, since it didn't define either `UsedTheMacro` or `Remote`. It's very close to working, so maybe I'll come up with a variant, but for now it doesn't seem possible.
Ah ah, nevermind, I got this to work by using blanket `impl<T> for Remote<T>`, which is fine because users aren't implementing anything for `Remote`s, only for state objects.
Manish Goregaokar (Gerrit)
Manish Goregaokar added 1 comment![]( "Open in Gerrit")
// FOR_RELEASE: This is a very clever solution, and possibly too clever. With
// good API design, we can probably make it impossible to construct a remote/
// receiver of the wrong type. But it's still not checked by the compiler, so
// that solution requires all future API designers to maintain the invariant.Devon LoehrMeh... if it works, it works. But... we probably **should** try to solicit a review by people who are more familiar with Rust type system tricks. @manishearth may be a good start I think + is a Chromium committer.
Manish GoregaokarSounds good - @manishearth, could you take a look at this trick to see if it makes sense/there's not a better way?
Devon LoehrI'm not really sure I understand this setup. Is this designed the way it is so that people can only write `Receiver<dyn Interface>` and not `Receiver<ConcreteType>`?
You could potentially achieve that via a trait DynMojomInterface that is only implemented on `dyn Interface`. I think that might be cleaner? It would be better for docs, too, because you can have `T: DynMojomInterface` vs `T: MojomInterface` and they are both useful bounds meaning different things.
There are two reasons: first, we want to ensure that people only write `Remote<dyn SomeInterface>`, but we also want to make sure that they can't confuse `Remote<dyn SomeInterface>` and `Remote<dyn SomeOtherInterface>`. Implementing a marker trait only for `dyn Interface` would achieve the first, but it would (in theory) leave the door open for the second.
In essence, what we really want to say is to parameterize on a trait, like `Remote<SomeInterface>`, for readability, consistency with C++, and to ensure that each `Remote/Receiver` pair is using the same interface. But since that's not allowed, we're using `dyn SomeInterface` instead of `Interface`, and this trickery is just to try to prevent people from writing other things in that position.
Hmmmm. Still not convinced you should ever need SameTypeAs, then.
I think what you probably want is T: MojomInterface and T: DynMojomInterface, with MojomInterface having `type Dyn: DynMojomInterface` (and autogenerated with the macro). I think that would anyway be a more easier to understand refactor, docs wise.
Then Remote<T> can have most methods be T: DynMojomInterace, and where you need to ensure the types are the same with Receiver, just use Receiver<T>.
It's a bit hard for me to be sure without looking at something end-to-end, but I think `T: MojomInterface + SameTypeAs<T::DynTy>` is much more conveniently represented as DynMojomInterface
Related details
- Daniel Cheng
- Devon Loehr
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devon Loehr (Gerrit)
Devon Loehr added 1 comment![]( "Open in Gerrit")
// FOR_RELEASE: This is a very clever solution, and possibly too clever. With
// good API design, we can probably make it impossible to construct a remote/
// receiver of the wrong type. But it's still not checked by the compiler, so
// that solution requires all future API designers to maintain the invariant.Devon LoehrMeh... if it works, it works. But... we probably **should** try to solicit a review by people who are more familiar with Rust type system tricks. @manishearth may be a good start I think + is a Chromium committer.
Manish GoregaokarSounds good - @manishearth, could you take a look at this trick to see if it makes sense/there's not a better way?
Devon LoehrI'm not really sure I understand this setup. Is this designed the way it is so that people can only write `Receiver<dyn Interface>` and not `Receiver<ConcreteType>`?
You could potentially achieve that via a trait DynMojomInterface that is only implemented on `dyn Interface`. I think that might be cleaner? It would be better for docs, too, because you can have `T: DynMojomInterface` vs `T: MojomInterface` and they are both useful bounds meaning different things.
Manish GoregaokarThere are two reasons: first, we want to ensure that people only write `Remote<dyn SomeInterface>`, but we also want to make sure that they can't confuse `Remote<dyn SomeInterface>` and `Remote<dyn SomeOtherInterface>`. Implementing a marker trait only for `dyn Interface` would achieve the first, but it would (in theory) leave the door open for the second.
In essence, what we really want to say is to parameterize on a trait, like `Remote<SomeInterface>`, for readability, consistency with C++, and to ensure that each `Remote/Receiver` pair is using the same interface. But since that's not allowed, we're using `dyn SomeInterface` instead of `Interface`, and this trickery is just to try to prevent people from writing other things in that position.
Hmmmm. Still not convinced you should ever need SameTypeAs, then.
I think what you probably want is T: MojomInterface and T: DynMojomInterface, with MojomInterface having `type Dyn: DynMojomInterface` (and autogenerated with the macro). I think that would anyway be a more easier to understand refactor, docs wise.
Then Remote<T> can have most methods be T: DynMojomInterace, and where you need to ensure the types are the same with Receiver, just use Receiver<T>.
It's a bit hard for me to be sure without looking at something end-to-end, but I think `T: MojomInterface + SameTypeAs<T::DynTy>` is much more conveniently represented as DynMojomInterface
You're right, I think the trick was realizing that you normally can't compare traits for equality (hence the `SameTypeAs` trick), but you _can_ do that for associated items. So we can write the comparison as `StateTy: MojomInterface<DynTy = T::DynTy>` and avoid all the boilerplate that comes from the new trait. Thanks!
Related details
- Daniel Cheng
- Manish Goregaokar
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manish Goregaokar (Gerrit)
Manish Goregaokar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Manish Goregaokar
| Code-Review | +1 |
1 comment
This looks much easier to understand.
I don't know enough about this setup to verify where we should be doing T: MojomInterface vs T: DynMojomInterface, someone should check that. But the model is much cleaner.
Related details
- Daniel Cheng
- Devon Loehr
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz voted and added 4 comments![]( "Open in Gerrit")
Votes added by Łukasz Anforowicz
| Code-Review | +1 |
4 comments
// rules and lack of support for multiple blanket implementations of a trait.Devon LoehrDisclaimer: I think that a macro is a reasonable, clean solution to all of this (and it gives us a centralized point of control where we can potentially tweak things in the future if needed). But for completeness, let me try to see if there are other solutions to the problems listed here.
IIUC the `.mojom` => `.rs` generator will generate `trait MathService`. Is it allowed / would it work to have a blanket impl **there**?:
```
impl<T> MojoService for T where T: MojoService {}
```
No; the problem is that we need to write
```
impl<T> MojomInterface for T where T: MathService {}
```So the trait we're `impl`ing is `MojomInterface`, which we didn't define. And of course, `T` is arbitrary, so we don't control that either.
Acknowledged
fn dispatch_response(message: Message, response_callback: Self::CallbackTy);I don't understand the `response_callback` parameter. The map of response_id-to-resonse_callback will be maintained by a `Remote`, right? If so, then should this take `&mut self` as the first parameter instead of taking `response_callback`? But maybe I just need to wait for an implementation...
Hopefully this will be clearer with the code (I only just realized we needed this myself). My intention is that the bindings will generate something like
```
CallbackTy = enum {
AddCallback(Box<dyn FnOnce(i32)>),
DivCallback(Box<dyn FnOnce(i32, i32)>),
}fn dispatch_response(message, callback) {
match (message_ID, callback) {
(ADD_ID, AddCallback(fun)) => fun(parse_add(message));
(DIV_ID, DivCallback(fun)) => fun(parse_div(message));
_ => panic!("Mismatched callback and message")
}
}
```The remote will just look up the stashed callback in its map, and pass it along. It can't actually deconstruct the callback type because this generic implementation doesn't know how many methods the associated interface will have.
Acknowledged
pending_responses: std::collections::HashMap<u32, T::CallbackTy>,Devon LoehrWould it simplify things if `pending_responses`:
- Was a field of the `mojom`-generated struct instead?
- Was split into multiple maps - one per method / per response callback type?
?
I'm not sure what `mojom`-generated struct you're talking about -- I think that for `Remote` we don't end up generating any structs (but we'll do some `impls` for `dyn Whatever`).
We could split it into multiple maps, but we'd have to do so dynamically, because `Remote<T>` doesn't know how many methods the associated interface has as long as `T` is generic.
Acknowledged
// alias for `bind` and `unbind`, but I think it's better that binding is
// explicit, since it's a stateful operation (not just converting data)I agree that `bind` and `unbind` sound better.
Related details
- Daniel Cheng
- Devon Loehr
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 8 comments![]( "Open in Gerrit")
//! function on the remote object, and pass a callback specifying how to handle
//! the response.Technically this is optional. Not sure if that's important to mention here though.
//! declare_mojom_state_object!(CountingMathService, MathService);I don't love this name.
//! Note that receivers can get receive messages while they are pending, but thoseNit: Remove "get" or "receive" here
// `MojomInterface` appropriately, and require uses to call it so the code is inusers?
type ResponseCallbackTy;In practice, this is a union of all callback types accepted by messages defined on this interface right?
/// This type represents one end of a Mojo pipe associated with a particularI think we want to be careful about using the word "associated" in these docs. I noticed a few other instances. Unfortunately "associated" already has a meaning in mojo.
I've seen the word "entangled" used for the two endpoints of a Mojo message pipe; the relationship between "message pipe" and "interface" is a bit murkier, since multiple interfaces can be multiplexed on the same message pipe (via the magic of associated interfaces); in the context of 'associated interfaces', I think the original one is the 'primary interface'.
// alias for `bind` and `unbind`, but I think it's better that binding is
// explicit, since it's a stateful operation (not just converting data)I agree that `bind` and `unbind` sound better.
+1
pub fn unbind(self) -> (PendingReceiver<StateTy::DynTy>, StateTy) {Honestly it's a bit of a dangerous method. I wonder if we should just avoid adding this for now; it's not very commonly used I think.
Related details
- Devon Loehr
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devon Loehr (Gerrit)
Devon Loehr added 7 comments![]( "Open in Gerrit")
@dcheng: Thanks for taking a look, please also note the naming scheme of Remotes/Receivers, where every type except `Receiver` takes a `dyn` type and `Receiver` takes a concrete type.
I'm still on the fence about whether it's cleaner for `Receiver` to take an (additional, useless) `dyn` type for consistency and searchability. I worry that it's obnoxious and will lead to people immediately aliasing the type to something even less searchable, which is probably what I'd do in that situation. I implemented it without the extra type, but it's not hard to add it if you still feel strongly that we should.
//! function on the remote object, and pass a callback specifying how to handle
//! the response.Technically this is optional. Not sure if that's important to mention here though.
Acknowledged
//! declare_mojom_state_object!(CountingMathService, MathService);I don't love this name.
Me neither; I've changed it several times. Tried once more, let me know what you think.
//! Note that receivers can get receive messages while they are pending, but thoseNit: Remove "get" or "receive" here
Done
// `MojomInterface` appropriately, and require uses to call it so the code is inDevon Loehrusers?
Done
/// This type represents one end of a Mojo pipe associated with a particularI think we want to be careful about using the word "associated" in these docs. I noticed a few other instances. Unfortunately "associated" already has a meaning in mojo.
I've seen the word "entangled" used for the two endpoints of a Mojo message pipe; the relationship between "message pipe" and "interface" is a bit murkier, since multiple interfaces can be multiplexed on the same message pipe (via the magic of associated interfaces); in the context of 'associated interfaces', I think the original one is the 'primary interface'.
Fair enough, I went through and reworded things (mostly replacing with `engtangled with` or `corresponding to`)
pub fn unbind(self) -> (PendingReceiver<StateTy::DynTy>, StateTy) {Honestly it's a bit of a dangerous method. I wonder if we should just avoid adding this for now; it's not very commonly used I think.
I want to have it here demonstrate its type signature, so I just made it private and left a note.
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
@dcheng: Thanks for taking a look, please also note the naming scheme of Remotes/Receivers, where every type except `Receiver` takes a `dyn` type and `Receiver` takes a concrete type.
I'm still on the fence about whether it's cleaner for `Receiver` to take an (additional, useless) `dyn` type for consistency and searchability. I worry that it's obnoxious and will lead to people immediately aliasing the type to something even less searchable, which is probably what I'd do in that situation. I implemented it without the extra type, but it's not hard to add it if you still feel strongly that we should.
1. How will people answer the question for "who listens to messages for this interface"?
2. I think we should explain the technical reason for this limitation in comments if we absolutely can't work around it.
Looks like the latest patchset might not be uploaded yet?
Related details
- Devon Loehr
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |