Groups keyboard shortcuts have been updated
Dismiss
See shortcuts

Re: possible use of a custom type instead of fidl type

3 views
Skip to first unread message

Filip Filmar

unread,
Sep 14, 2021, 3:58:02 PM9/14/21
to Mukesh Agrawal, ui-inp...@fuchsia.dev
Hi Mukesh! Thanks for the proposal.

I think I agree this is likely the best we can do today to ensure we do not mis-initialize FIDL in our Rust code.

That being said, I am not super thrilled by:
  1. The increase of the level of indirection by one, we have quite a lot of types as it is.
  2. The lack of similar validation logic in other FIDL bindings: this means we can still get malformed FIDL
  3. Nothing really preventing someone from not using the intermediate types even if they exist.
  4. Unclear how much it would save us from future bugs.  How many issues did we have around this?
On the upside, (1) this would be a straightforward change to make, and we have prior art with, I think, KeyEvent.  Perhaps keep it in the pocket until the first time we have a bug due to FIDL initialization?

If there were additional benefits to doing this instead just validation, I'd be more in favor.

And If I could get a pony around this, it would be a custom validation hook in FIDL that we could hook up to FIDL itself without the need to use an intermediary type.

HTH,
F


On Tue, Sep 14, 2021 at 12:44 PM Mukesh Agrawal <qui...@google.com> wrote:
Context: +Filip Filmar and I have talked about FIDL types vs. implementation-specific types before. This email describes a case where I think using a FIDL type within an implementation makes the code harder to maintain.

Right now, we have ad-hoc validation code to ensure that KeyEvents emitted by the IME service a) contain a timestamp, and b) contain either a Key or a KeyMeaning. We have to do this validation ad-hoc because a) we use the KeyEvent FIDL type internally in IME service, and b) the FIDL type uses tables, which makes `timestamp`, `key`, and `key_meaning` Option-al in the Rust bindings.

If we used a custom type instead, we could get static checking in most of the implementation, and idiomatic validation when receiving the FIDL type. See details below.

Filip, WDYT?

use fidl_fuchsia... as FidlKeyboardEvent;
use fidl_fuchsia... as FidlKeyMeaning;
use fidl_fuchsia... as FidlKey;

enum KeyData {
  // Only the key is available. This might be, e.g., because
  // the data is from a physical keyboard, and no keyboard
  // layout has been applied.
  KeyOnly(FidlKey),

  // Only the meaning is available. This might be, e.g., because
  // the data is from a virtual keyboard, and no reverse layout
  // has been applied.
  MeaningOnly(FidlKeyMeaning),

  // Both the key and its meaning are available.
  KeyAndMeaning(FidlKey, FidlKeyMeaning),
}

struct KeyboardEvent {
  timestamp: i32,
  key_data: KeyData,
  // ... other fields omitted
}

// Note: requires Rust 1.4.1. If we're using an older version, then 
// `impl std::convert::Into<FidlKeyboardEvent> for KeyboardEvent`
// instead.
impl std::convert::From<KeyboardEvent> for FidlKeyboardEvent {
  fn from(evt: KeyboardEvent) -> FidlKeyboardEvent {
    let (key, key_meaning) = match evt.key_data {
      KeyData::KeyOnly(k) => (Some(k), None),
      KeyData::MeaningOnly(m) => (None, Some(m)),
      KeyData::KeyAndMeaning(k, m) => (Some(k), Some(m)),
    };
    FidlKeyboardEvent {
      timestamp: Some(evt.timestamp),
      key,
      key_meaning,
      // ... other fields omitted
    }
  }
}

impl std::convert::TryFrom<FidlKeyboardEvent> for KeyboardEvent {
  fn try_from(evt: FidlKeyboardEvent) ->
      Result<KeyboardEvent, Error> {
    let key_data = match (evt.key, evt.key_meaning) {
      (Some(k), Some(m)) => KeyData::KeyAndMeaning(k, m),
      (Some(k), None) => KeyData::KeyOnly(k),
      (None, Some(m)) => KeyData::MeaningOnly(m),
      (None, None) => format_err!("no key or meaning")?,
    };
    KeyboardEvent {
      timestamp: evt.timestamp,
      key_data,
      // ... other fields omitted
    }
  }
}

Kevin Lindkvist

unread,
Sep 14, 2021, 4:26:23 PM9/14/21
to Filip Filmar, Mukesh Agrawal, ui-inp...@fuchsia.dev
The component framework does some pretty fancy stuff for translating to/from FIDL types: https://cs.opensource.google/fuchsia/fuchsia/+/main:src/sys/lib/cm_rust/src/lib.rs
 
Could be a bit much for your use cases, but figured you might be interested.  

--
All posts must follow the Fuchsia Code of Conduct https://fuchsia.dev/fuchsia-src/CODE_OF_CONDUCT or may be removed.
---
You received this message because you are subscribed to the Google Groups "ui-input-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ui-input-dev...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/ui-input-dev/CAGEh6bjgi7P76uuACM2ewNTRQRMbs-fB13uhVyqDh9uFXjB6Cw%40mail.gmail.com.

Mukesh Agrawal

unread,
Sep 14, 2021, 4:26:23 PM9/14/21
to Filip Filmar, ui-inp...@fuchsia.dev

Mukesh Agrawal

unread,
Sep 14, 2021, 5:15:56 PM9/14/21
to Filip Filmar, ui-inp...@fuchsia.dev
By "lack of similar validation in other FIDL bindings", do you mean bindings for other FIDL types used in ime_service, or do you mean bindings used by other components?
No rush to make a change like this, but I think we already had the first bug: fxb/83632.

Mukesh Agrawal

unread,
Sep 14, 2021, 5:18:52 PM9/14/21
to Kevin Lindkvist, Filip Filmar, ui-inp...@fuchsia.dev
Thanks! If we end up having native types for a lot of FIDL types, the pattern from CF could be a good one to follow.

Filip Filmar

unread,
Sep 14, 2021, 5:23:09 PM9/14/21
to Mukesh Agrawal, ui-inp...@fuchsia.dev
On Tue, Sep 14, 2021 at 2:15 PM Mukesh Agrawal <qui...@google.com> wrote:
By "lack of similar validation in other FIDL bindings", do you mean bindings for other FIDL types used in ime_service, or do you mean bindings used by other components?

I mean, if we implement this in Rust, there are still C++ and dart (and I guess technically go) bindings which don't have it.  So the impact of this validation is limited.

No rush to make a change like this, but I think we already had the first bug: fxb/83632.

Not sure if this was the only bug, but am fairly confident that the known bugs about this have been fixed.  I'd recommend we may want to go this route only if we find more of similar bugs.

F

Filip Filmar

unread,
Sep 14, 2021, 5:28:03 PM9/14/21
to Mukesh Agrawal, Kevin Lindkvist, ui-inp...@fuchsia.dev
On Tue, Sep 14, 2021 at 2:18 PM Mukesh Agrawal <qui...@google.com> wrote:
Thanks! If we end up having native types for a lot of FIDL types, the pattern from CF could be a good one to follow.

+1. 

CF has quite a strong use case too - they need to validate a lot of types. 

F

Mukesh Agrawal

unread,
Sep 14, 2021, 5:28:39 PM9/14/21
to Filip Filmar, ui-inp...@fuchsia.dev
On Tue, Sep 14, 2021 at 2:23 PM Filip Filmar <fm...@google.com> wrote:
I mean, if we implement this in Rust, there are still C++ and dart (and I guess technically go) bindings which don't have it.  So the impact of this validation is limited.

Right. To "really fix" this, we'd need to change the FIDL.

Where this approach is useful is when we have protocols that can't be changed (either too high a migration cost, or some more general implementations want greater flexibility), but where we want to isolate the impedance mismatch between what the protocol allows on the wire, and what our protocol spports.
 
Not sure if this was the only bug, but am fairly confident that the known bugs about this have been fixed.  I'd recommend we may want to go this route only if we find more of similar bugs.

SGTM.
Reply all
Reply to author
Forward
0 new messages