Skip to content

Design issues of FluentValue & FluentType #396

@maeldroem

Description

@maeldroem

What's the issue with FluentValue::Custom?

Currently FluentValue::Custom, introduced by #153, takes Box<dyn FluentType + Send>, this makes the variant !Sync, which propagates to the FluentValue enum, and up to FluentArgs, Scope and many more types.

This is an issue as it makes most translation operations (like Loader::lookup_with_args() from fluent-templates) bound to a single thread, when there's no actual restriction justifying that.

Approaches to the custom-type problem

From what I could see from the source code, there's currently no "real" support for custom values. In the case of an argument, the value is just turned into a string. If matched with a selector, it will always return false.

In this case, since we know that operations deal with comparing strings when using custom values, there's multiple approaches we can take.

  1. Generics (static dispatch) — Instead of using trait objects (dynamic dispatch), we can change the definition of the Custom variant to take a new generic C that implements FluentType, that way only specific operations that strictly need Send/Sync will limit the use of FluentValue containing a non-Send/Sync custom type, compared to the current state where any Send/Sync operations are restricted.
  2. Add + Sync — Most types implement both Send and Sync and adding this additional trait bound would (re-)allow multithreaded translation operations. The downside to this approach is that non-Sync types would no longer be supported. But since non-Sync types are quite rare, it could be possible to implement this approach without much risk.
  3. Wrap the contents of the Custom variant with SyncWrapper — While I'm not super familiar with how SyncWrapper works, this is an approach to consider.

I would be willing to do a PR but want to ensure what kind of approach is more fitting for that issue.

What's the issue with FluentType?

First, it describes duplicate(), and from how it's implemented and described, it feels like it's just a re-implementation of the standard Clone. For reference, there was this small discussion on the subject in the original PR, but I'm not familiar with the origins of the use for Any (and derived traits like AnyEq and the discussed AnyClone) in this crate, so there may be a justification for that. We should check if this is actually the case.

Second, it describes as_string() and as_string_threadsafe(). While I'm not certain of the origin of the need for such methods, I believe it should be separated and put in a more independent trait, e.g. IntlToString, FluentFormattable?

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions