Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add explicit cases to PluralCategory #5426

Closed
wants to merge 1 commit into from

Conversation

robertbastian
Copy link
Member

Explicit 0 and 1 cases are required by patterns in multiple components.

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make it a separate enum. While it would be much easier and cleaner in the short term to extend the existing enum as you did here, there are several reasons why it is wrong to do so:

  1. CLDR defines the plural categories as being zero, one, two, few, many, other. Explicit0 and Explicit1 are not part of the definition.
  2. ECMA-402 likewise defines a plural category as being one of the six.
  3. PluralRules should never return Explicit0 or Explicit1 as a category. It can match on it, but it should never return it, because it is not the plural category of the number.
  4. Even if you don't believe me on philosophical points 1-3, a technical reason why we shouldn't extend the enum is that the enum is exhaustive. We made a decision in 1.0 that the enum would be exhaustive and we would deprecate it and introduce a new enum if CLDR ever introduced new plural categories. However, with the explicit values, CLDR absolutely can and will add new explicit string values, so the enum definitely cannot be exhaustive.

@robertbastian robertbastian added the discuss-priority Discuss at the next ICU4X meeting label Aug 22, 2024
@sffc
Copy link
Member

sffc commented Aug 22, 2024

  • @robertbastian worried if we don't put in the enum, every caller of PluralElements you will forget to check for the specific cases (if you only handle general cases).
  • @sffc You should have a new enum:
enum PluralCategoryExtended {
    PluralCategory(PluralCategory),
    Explicit0,
    Explicit1,
}

impl PluralRules {
    pub fn select_from_plural_elements<T>(&self, op: PluralOperands, choices: &PluralElements<T>) -> T {
        let category = self.category_for(op);

        if op.is_exactly_one() {
            if let Some(value) = choices.get_explicit1() {
                return value;
            }
        }
        // same for zero
        
        
        choices.get(category)    
    }
    
    /*
    pub fn select_with_explicit(&self, op: PluralOperands) -> (Option<Explicit>, PluralCategory) {
        let category = self.category_for(op);

        if op.is_exactly_one() {
            if let Some(value) = choices.get_explicit1() {
                return (Some(Explicit::One), category);
            }
        }
        // same for zero
        
        (None, category)  
        // ...
    }
    */
}

// in icu::plurals
pub struct PluralElements<'a, T: ?Sized> {
    pub other: &'a T,
    pub zero: Option<&'a T>,
    // ...
}

// in icu::plurals::provider
impl PluralElementsV1<T> {
    pub fn get(&self, op: PluralOperands, rules: &PluralRules) -> &T {
        let category = rules.category_for(op);

        if op.is_exactly_zero() {
            if let Some(value) = self.get_explicit0() {
                return value;
            }
        }
        if op.is_exactly_one() {
            if let Some(value) = self.get_explicit1() {
                return value;
            }
        }
        
        
        self.get(category)   
    }
    
    pub fn new(builder: PluralElements<T>) -> Self where T: PartialEq {}
}
  • @robertbastian I've been working with a new struct named PluralElements; could that be the argument?
  • @robertbastian PluralCategoryExtended would need to be repr(u8)
  • @Manishearth We could make that the public interface and have a repr(u8) internal enum
  • @robertbastian For now it can be private
  • @younies So do we change call sites from PluralCategory to PluralCategoryExtended?
  • @robertbastian We need to change call sites so pass PluralOperands and PluralRules instead of a preresolved PluralCategory
  • @younies I believe in CurrencyCompact, you fallback for other if other doesn't exist. Then you go to the display name
  • @robertbastian - If other doesn't exist, none of the cases exist and you cannot construct a PluralElements
  • @robertbastian for current constructors for PluralElements, you pass in Options for everything
  • @sffc We should make the constructor be non-exhaustive so we can add more explicit cases
  • @robertbastian That's not defined in CLDR, leads to less optimization if it's non exhaustive. This is going to be used everywhere, we should really optimize
  • @sffc May not be the case in the future. It should be just as easy to support 2 as we could support 5 cases. I think we can cap it at 4 bits (16 variants)
  • @sffc Or maybe PluralElementsV1 should just be the VarULE of PluralElements?
  • @robertbastian Is it okay for PluralElementsV1 to deduplicate and not round-trip?
  • @Manishearth Equality?
  • @robertbastian We can write custom Eq impls.
  • @Manishearth - then yes
  • @sffc Should it be called PluralElementsBorrowed?
  • @robertbastian I don't think there should be an owned version because it is just a plural arguments bag.

Decision:

  • Create icu::plurals::PluralElements, a non-exhaustive struct with a bunch of Option fields
  • PluralRules gets a function that processes PluralElements
  • Create icu::plurals::provider::PluralElementsV1, a data struct that represents the same data in a more compact way, with private fields
  • We will migrate PluralElementsV1 to Better abstractions for splitting lengths out from VarZeroVec #5378. Unclear whether we will make PluralElementsV1 itself a VarULE or make a separate VarULE.
  • PluralElementsV1 gets a function that takes a &PluralRules and could have a more optimized algorithm, or could internally convert to PluralElements

LGTM: @robertbastian @sffc

robertbastian added a commit that referenced this pull request Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss-priority Discuss at the next ICU4X meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants