Polish UnicodeSet builder API

[robertbastian]

cc @skius

• How should it interact with providers? Store the provider or load everything (even things we might not need) upfront
  • Storing the provider adds a lifetime to the builder object
• Should parse be a free function

[skius]

Context: The reason why our usual load-data-in-constructor pattern doesn't work here is that currently, the internal functions I'm using (load_for_ecma262_unstable, load_general_category, ..) accept the DataProvider and not the payload.

[skius]

We also need to decide how "overescaped" inputs should be handled, i.e., the kinds of inputs that exemplar characters is dealing with.

Thought: I believe allowing users to supply their own iterators would solve this issue with minimal effort on their and our side

[skius]

An alternative API to the free parse function:

let builder = UnicodeSetBuilder::new(provider);
let set = builder.parse(source, options)?;

Furthermore, the transliterator would benefit from a function that either takes an Iterator, or returns how much of the source string was actually parsed. That allows the transliterator parser to hand over to the UnicodeSet parser as soon as it reaches a [, and then continue from where the UnicodeSet parser left off after a successful UnicodeSet parse.

[sffc]

Another option (not necessarily recommending it) would be to do like we do in TimeZoneFormatter, where we have a bunch of Option<DataPayload<_>> for the different keys we may or may not need, and then a bunch of little load functions that fill in each key. In our case, the parse function could fail with a list of keys that need to be loaded or something like that. We could also have a parse function that takes &mut self and fills in the payloads as needed.

[skius]

That sounds a bit like a caching provider, or is a caching layer too complex if each provider individually is "optional"? (I'm not too sure, but I feel like such an implementation might need a type-based map (eg BidiControlV1Marker -> Option<DataPayload<BidiControlV1Marker>>))

[sffc]

I'd characterize it less as "caching" and more as "lazy loading" but you're correct.

[skius]

If we had a struct LazyProvider<P: DataProvider<M: DataMarker>> that impls DataProvider<M> for LazyProvider<P> that does the lazy-loading/caching you describe, we could keep all APIs the same but would avoid multiple calls to the underlying (potentially expensive) DataProvider::load function.

Do you think that's more expensive than TimeZoneFormatter's explicit approach? I feel like it just boils down to being able to generically replace TimeZoneDataPayloads

[sffc]

Discuss with:

• @younies
• @skius
• @sffc
• @robertbastian
• @eggrobin

Optional:

• @echeran
• @Manishearth

[sffc]

• @sffc - A few ways to interact with providers:

1. Eagerly load all of the property data when constructing a UnicodeSet Parser
2. Have slots in the UnicodeSet parser that are lazily loaded / cached
3. Always load the properties on demand when they are needed

• @younies - Can we give users the option?
• @robertbastian - It only really matters for postcard
• @Manishearth - We should consider a parser factory
• @sffc - Maybe a trait that providers can implement which is used during parsing
• @Manishearth - We don't normally hit the data provider in the terminal function
• @sffc - How about we make a new all-inclusive type in icu_properties that looks something like

pub struct PropertyNameToSetMapper { ... }
impl PropertyNameToSetMapper {
    pub fn try_new_for_ecma262(...) -> Self { ... }
    pub fn try_new_all(...) -> Self { ... }

    pub fn load_for_property(&self, name: &str) -> CodePointSetDataBorrowed { ... }
    pub fn load_for_property_with_value(&self, name: &str, value: &str) -> CodePointSetDataBorrowed { ... }
    // also strict/loose versions of those functions
}

• @skius - It should include strict/loose versions
• @Manishearth - It may as well still be a trait
• @sffc - Then the functions need to be fallible; is that okay?
• @robertbastian - Yeah, it can return an error type specified by the trait

pub trait PropertyNameToSetMapperLike {
    type Error: Debug;
    fn load_for_property(&self, name: &str) -> Result<CodePointSetDataBorrowed, Error> { ... }
    fn load_for_property_with_value(&self, name: &str, value: &str) -> Result<CodePointSetDataBorrowed, Error> { ... }
   // with the strict and loose
}

• @Manishearth - There's the special case for script and general category.
• @sffc - There are two ways for this: either the implementation of the trait needs to figure out how to resolve the script / general category, or UnicodeSet needs to do that.
• @robertbastian - Should strict/loose be separate methods or a flag?
• @Manishearth - Whether to call strict/loose is up to the UnicodeSet impl, not the data. We normally use methods.
• @skius - Ecma 262 needs strict
• @eggrobin - UTS needs loose

LGTM: @Manishearth @sffc @robertbastian @skius @eggrobin

Next: free function?

• @skius - The only thing in the options bag is how to handle the $ anchor. It is either the anchor or the FFFF character.
• @eggrobin - FFFF sounds like an ICU-specific implementation behavior that made it into the spec.
• @skius - CLDR makes use of $ in UnicodeSet.
• @robertbastian - Maybe we need the options bag to distinguish ICU behavior from spec behavior.
• @eggrobin - We should avoid standardizing the different dialects of UnicodeSet
• @skius - I feel like the consensus is that the spec should eventually reflect the ICU behavior, like comments that have been made on the Transliterator side
• @sffc - It sounds like what people are saying is that we just implement a single parser without options that tries to align with ICU behavior. Is there a diff between C and J?
• @eggrobin - They are supposed to be in alignment.
• @robertbastian - So we don't need Options.
• @sffc - Just to verify, the trait takes &self and not &mut self right?
• @robertbastian - Seems like it could be &mut self and the caching is handled by the UnicodeSetParser
• @Manishearth - The caching is often handled internally by the thing that is owning the data
• @sffc - I prefer &self because the PropertyNameToSetMapper should be a generally useful thing outside of the UnicodeSetParser
• @sffc - I prefer having UnicodeSetParser anyway. Free functions are weird.
• @robertbastian - Should it be called CodePointInversionListAndStringListParser?
• @sffc - No, I think it should be UnicodeSetParser but it could have parse_to_cpilsl. For now I think parse is fine. We can add more function overloads as needed in future releases.

LGTM: @Manishearth @skius @younies @robertbastian @eggrobin @sffc

Any additional feedback on this, @echeran?