-
Notifications
You must be signed in to change notification settings - Fork 109
Description
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.
- Generics (static dispatch) — Instead of using trait objects (dynamic dispatch), we can change the definition of the
Customvariant to take a new genericCthat implementsFluentType, that way only specific operations that strictly needSend/Syncwill limit the use ofFluentValuecontaining a non-Send/Synccustom type, compared to the current state where anySend/Syncoperations are restricted. - Add
+ Sync— Most types implement bothSendandSyncand adding this additional trait bound would (re-)allow multithreaded translation operations. The downside to this approach is that non-Synctypes would no longer be supported. But since non-Synctypes are quite rare, it could be possible to implement this approach without much risk. - Wrap the contents of the
Customvariant withSyncWrapper— While I'm not super familiar with howSyncWrapperworks, 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?