Cleanup `get_option` and calls to the functioncar (#3355)

boa_engine/src/builtins/intl/collator/mod.rs

@@ -34,7 +34,7 @@ use crate::{
 
 use super::{
     locale::{canonicalize_locale_list, resolve_locale, supported_locales, validate_extension},
-    options::{coerce_options_to_object, IntlOptions, LocaleMatcher},
+    options::{coerce_options_to_object, IntlOptions},
     Service,
 };
 
@@ -241,31 +241,28 @@ impl BuiltInConstructor for Collator {
         //     a. Let localeData be %Collator%.[[SortLocaleData]].
         // 6. Else,
         //     a. Let localeData be %Collator%.[[SearchLocaleData]].
-        let usage =
+        let usage = get_option(&options, utf16!("usage"), context)?.unwrap_or_default();
-            get_option::<Usage>(&options, utf16!("usage"), false, context)?.unwrap_or_default();
 
         // 7. Let opt be a new Record.
         // 8. Let matcher be ? GetOption(options, "localeMatcher", string, « "lookup", "best fit" », "best fit").
         // 9. Set opt.[[localeMatcher]] to matcher.
-        let matcher =
+        let matcher = get_option(&options, utf16!("localeMatcher"), context)?.unwrap_or_default();
-            get_option::<LocaleMatcher>(&options, utf16!("localeMatcher"), false, context)?
-                .unwrap_or_default();
 
         // 10. Let collation be ? GetOption(options, "collation", string, empty, undefined).
         // 11. If collation is not undefined, then
         //     a. If collation does not match the Unicode Locale Identifier type nonterminal, throw a RangeError exception.
         // 12. Set opt.[[co]] to collation.
-        let collation = get_option::<Value>(&options, utf16!("collation"), false, context)?;
+        let collation = get_option(&options, utf16!("collation"), context)?;
 
         // 13. Let numeric be ? GetOption(options, "numeric", boolean, empty, undefined).
         // 14. If numeric is not undefined, then
         //     a. Let numeric be ! ToString(numeric).
         // 15. Set opt.[[kn]] to numeric.
-        let numeric = get_option::<bool>(&options, utf16!("numeric"), false, context)?;
+        let numeric = get_option(&options, utf16!("numeric"), context)?;
 
         // 16. Let caseFirst be ? GetOption(options, "caseFirst", string, « "upper", "lower", "false" », undefined).
         // 17. Set opt.[[kf]] to caseFirst.
-        let case_first = get_option::<CaseFirst>(&options, utf16!("caseFirst"), false, context)?;
+        let case_first = get_option(&options, utf16!("caseFirst"), context)?;
 
         let mut intl_options = IntlOptions {
             matcher,
@@ -314,8 +311,7 @@ impl BuiltInConstructor for Collator {
 
         // 26. Let sensitivity be ? GetOption(options, "sensitivity", string, « "base", "accent", "case", "variant" », undefined).
         // 28. Set collator.[[Sensitivity]] to sensitivity.
-        let sensitivity =
+        let sensitivity = get_option(&options, utf16!("sensitivity"), context)?
-            get_option::<Sensitivity>(&options, utf16!("sensitivity"), false, context)?
             // 27. If sensitivity is undefined, then
             //     a. If usage is "sort", then
             //         i. Let sensitivity be "variant".
@@ -327,9 +323,8 @@ impl BuiltInConstructor for Collator {
 
         // 29. Let ignorePunctuation be ? GetOption(options, "ignorePunctuation", boolean, empty, false).
         // 30. Set collator.[[IgnorePunctuation]] to ignorePunctuation.
-        let ignore_punctuation =
+        let ignore_punctuation: bool =
-            get_option::<bool>(&options, utf16!("ignorePunctuation"), false, context)?
+            get_option(&options, utf16!("ignorePunctuation"), context)?.unwrap_or_default();
-                .unwrap_or_default();
 
         let (strength, case_level) = sensitivity.map(Sensitivity::to_collator_options).unzip();
 

boa_engine/src/builtins/intl/list_format/mod.rs

@@ -22,7 +22,7 @@ use crate::{
 
 use super::{
     locale::{canonicalize_locale_list, resolve_locale, supported_locales},
-    options::{IntlOptions, LocaleMatcher},
+    options::IntlOptions,
     Service,
 };
 
@@ -111,9 +111,7 @@ impl BuiltInConstructor for ListFormat {
 
         // 5. Let opt be a new Record.
         // 6. Let matcher be ? GetOption(options, "localeMatcher", string, « "lookup", "best fit" », "best fit").
-        let matcher =
+        let matcher = get_option(&options, utf16!("localeMatcher"), context)?.unwrap_or_default();
-            get_option::<LocaleMatcher>(&options, utf16!("localeMatcher"), false, context)?
-                .unwrap_or_default();
 
         // 7. Set opt.[[localeMatcher]] to matcher.
         // 8. Let localeData be %ListFormat%.[[LocaleData]].
@@ -130,13 +128,11 @@ impl BuiltInConstructor for ListFormat {
 
         // 11. Let type be ? GetOption(options, "type", string, « "conjunction", "disjunction", "unit" », "conjunction").
         // 12. Set listFormat.[[Type]] to type.
-        let typ = get_option::<ListFormatType>(&options, utf16!("type"), false, context)?
+        let typ = get_option(&options, utf16!("type"), context)?.unwrap_or_default();
-            .unwrap_or_default();
 
         // 13. Let style be ? GetOption(options, "style", string, « "long", "short", "narrow" », "long").
         // 14. Set listFormat.[[Style]] to style.
-        let style = get_option::<ListLength>(&options, utf16!("style"), false, context)?
+        let style = get_option(&options, utf16!("style"), context)?.unwrap_or(ListLength::Wide);
-            .unwrap_or(ListLength::Wide);
 
         // 15. Let dataLocale be r.[[dataLocale]].
         // 16. Let dataLocaleData be localeData.[[<dataLocale>]].

boa_engine/src/builtins/intl/locale/mod.rs

@@ -7,9 +7,7 @@ use boa_profiler::Profiler;
 use icu_collator::CaseFirst;
 use icu_datetime::options::preferences::HourCycle;
 use icu_locid::{
-    extensions::unicode::Value,
+    extensions::unicode::Value, extensions_unicode_key as key, extensions_unicode_value as value,
-    extensions_unicode_key as key, extensions_unicode_value as value,
-    subtags::{Language, Region, Script},
 };
 
 #[cfg(test)]
@@ -237,27 +235,18 @@ impl BuiltInConstructor for Locale {
             // 3. If ! IsStructurallyValidLanguageTag(tag) is false, throw a RangeError exception.
             // 4. Let language be ? GetOption(options, "language", string, empty, undefined).
             // 5. If language is not undefined, then
-            let language = get_option::<JsString>(options, utf16!("language"), false, context)?
             //    a. If language does not match the unicode_language_subtag production, throw a RangeError exception.
-                .map(|s| s.to_std_string_escaped().parse::<Language>())
+            let language = get_option(options, utf16!("language"), context)?;
-                .transpose()
-                .map_err(|e| JsNativeError::range().with_message(e.to_string()))?;
 
             // 6. Let script be ? GetOption(options, "script", string, empty, undefined).
             // 7. If script is not undefined, then
-            let script = get_option::<JsString>(options, utf16!("script"), false, context)?
-                .map(|s| s.to_std_string_escaped().parse::<Script>())
-                .transpose()
             //    a. If script does not match the unicode_script_subtag production, throw a RangeError exception.
-                .map_err(|e| JsNativeError::range().with_message(e.to_string()))?;
+            let script = get_option(options, utf16!("script"), context)?;
 
             // 8. Let region be ? GetOption(options, "region", string, empty, undefined).
             // 9. If region is not undefined, then
-            let region = get_option::<JsString>(options, utf16!("region"), false, context)?
-                .map(|s| s.to_std_string_escaped().parse::<Region>())
-                .transpose()
             //    a. If region does not match the unicode_region_subtag production, throw a RangeError exception.
-                .map_err(|e| JsNativeError::range().with_message(e.to_string()))?;
+            let region = get_option(options, utf16!("region"), context)?;
 
             // 10. Set tag to ! CanonicalizeUnicodeLocaleId(tag).
             context.icu().locale_canonicalizer().canonicalize(&mut tag);
@@ -299,42 +288,36 @@ impl BuiltInConstructor for Locale {
         // 14. If calendar is not undefined, then
         // 15. Set opt.[[ca]] to calendar.
         //     a. If calendar does not match the Unicode Locale Identifier type nonterminal, throw a RangeError exception.
-        let ca = get_option::<Value>(options, utf16!("calendar"), false, context)?;
+        let ca = get_option(options, utf16!("calendar"), context)?;
 
         // 16. Let collation be ? GetOption(options, "collation", string, empty, undefined).
         // 17. If collation is not undefined, then
         // 18. Set opt.[[co]] to collation.
         //     a. If collation does not match the Unicode Locale Identifier type nonterminal, throw a RangeError exception.
-        let co = get_option::<Value>(options, utf16!("collation"), false, context)?;
+        let co = get_option(options, utf16!("collation"), context)?;
 
         // 19. Let hc be ? GetOption(options, "hourCycle", string, « "h11", "h12", "h23", "h24" », undefined).
         // 20. Set opt.[[hc]] to hc.
-        let hc =
+        let hc = get_option(options, utf16!("hourCycle"), context)?.map(|hc| match hc {
-            get_option::<HourCycle>(options, utf16!("hourCycle"), false, context)?.map(
-                |hc| match hc {
             HourCycle::H24 => value!("h24"),
             HourCycle::H23 => value!("h23"),
             HourCycle::H12 => value!("h12"),
             HourCycle::H11 => value!("h11"),
-                },
+        });
-            );
 
         // 21. Let kf be ? GetOption(options, "caseFirst", string, « "upper", "lower", "false" », undefined).
         // 22. Set opt.[[kf]] to kf.
-        let kf =
+        let kf = get_option(options, utf16!("caseFirst"), context)?.map(|kf| match kf {
-            get_option::<CaseFirst>(options, utf16!("caseFirst"), false, context)?.map(
-                |kf| match kf {
             CaseFirst::UpperFirst => value!("upper"),
             CaseFirst::LowerFirst => value!("lower"),
             CaseFirst::Off => value!("false"),
             _ => unreachable!(),
-                },
+        });
-            );
 
         // 23. Let kn be ? GetOption(options, "numeric", boolean, empty, undefined).
         // 24. If kn is not undefined, set kn to ! ToString(kn).
         // 25. Set opt.[[kn]] to kn.
-        let kn = get_option::<bool>(options, utf16!("numeric"), false, context)?.map(|b| {
+        let kn = get_option(options, utf16!("numeric"), context)?.map(|b| {
             if b {
                 value!("true")
             } else {
@@ -346,7 +329,7 @@ impl BuiltInConstructor for Locale {
         // 27. If numberingSystem is not undefined, then
         // 28. Set opt.[[nu]] to numberingSystem.
         //     a. If numberingSystem does not match the Unicode Locale Identifier type nonterminal, throw a RangeError exception.
-        let nu = get_option::<Value>(options, utf16!("numberingSystem"), false, context)?;
+        let nu = get_option(options, utf16!("numberingSystem"), context)?;
 
         // 29. Let r be ! ApplyUnicodeExtensionToTag(tag, opt, relevantExtensionKeys).
         // 30. Set locale.[[Locale]] to r.[[locale]].

boa_engine/src/builtins/intl/locale/options.rs

@@ -1,21 +1,51 @@
-use icu_locid::extensions::unicode::Value;
+use icu_locid::{
+    extensions::unicode::Value,
+    subtags::{Language, Region, Script},
+};
 
 use crate::{builtins::options::OptionType, Context, JsNativeError};
 
 impl OptionType for Value {
     fn from_value(value: crate::JsValue, context: &mut Context<'_>) -> crate::JsResult<Self> {
-        let val = value
+        let val = value.to_string(context)?.to_std_string_escaped();
+
+        if val.len() < 3 {
+            return Err(JsNativeError::range()
+                .with_message("nonterminal `type` must be at least 3 characters long")
+                .into());
+        }
+
+        val.parse::<Self>()
+            .map_err(|e| JsNativeError::range().with_message(e.to_string()).into())
+    }
+}
+
+impl OptionType for Language {
+    fn from_value(value: crate::JsValue, context: &mut Context<'_>) -> crate::JsResult<Self> {
+        value
             .to_string(context)?
             .to_std_string_escaped()
             .parse::<Self>()
-            .map_err(|e| JsNativeError::range().with_message(e.to_string()))?;
+            .map_err(|e| JsNativeError::range().with_message(e.to_string()).into())
+    }
+}
 
-        if val.as_tinystr_slice().is_empty() {
+impl OptionType for Script {
-            return Err(JsNativeError::range()
+    fn from_value(value: crate::JsValue, context: &mut Context<'_>) -> crate::JsResult<Self> {
-                .with_message("Unicode Locale Identifier `type` cannot be empty")
+        value
-                .into());
+            .to_string(context)?
+            .to_std_string_escaped()
+            .parse::<Self>()
+            .map_err(|e| JsNativeError::range().with_message(e.to_string()).into())
+    }
 }
 
-        Ok(val)
+impl OptionType for Region {
+    fn from_value(value: crate::JsValue, context: &mut Context<'_>) -> crate::JsResult<Self> {
+        value
+            .to_string(context)?
+            .to_std_string_escaped()
+            .parse::<Self>()
+            .map_err(|e| JsNativeError::range().with_message(e.to_string()).into())
     }
 }

boa_engine/src/builtins/intl/locale/utils.rs

@@ -546,8 +546,7 @@ where
     let options = coerce_options_to_object(options, context)?;
 
     // 2. Let matcher be ? GetOption(options, "localeMatcher", string, « "lookup", "best fit" », "best fit").
-    let matcher = get_option::<LocaleMatcher>(&options, utf16!("localeMatcher"), false, context)?
+    let matcher = get_option(&options, utf16!("localeMatcher"), context)?.unwrap_or_default();
-        .unwrap_or_default();
 
     let elements = match matcher {
         // 4. Else,

boa_engine/src/builtins/intl/number_format/utils.rs

@@ -44,7 +44,7 @@ pub(crate) fn get_digit_format_options(
 
     // 7. Let roundingPriority be ? GetOption(options, "roundingPriority", string, « "auto", "morePrecision", "lessPrecision" », "auto").
     let mut rounding_priority =
-        get_option(options, utf16!("roundingPriority"), false, context)?.unwrap_or_default();
+        get_option(options, utf16!("roundingPriority"), context)?.unwrap_or_default();
 
     // 8. Let roundingIncrement be ? GetNumberOption(options, "roundingIncrement", 1, 5000, 1).
     // 9. If roundingIncrement is not in « 1, 2, 5, 10, 20, 25, 50, 100, 200, 250, 500, 1000, 2000, 2500, 5000 », throw a RangeError exception.
@@ -58,12 +58,11 @@ pub(crate) fn get_digit_format_options(
     }
 
     // 10. Let roundingMode be ? GetOption(options, "roundingMode", string, « "ceil", "floor", "expand", "trunc", "halfCeil", "halfFloor", "halfExpand", "halfTrunc", "halfEven" », "halfExpand").
-    let rounding_mode =
+    let rounding_mode = get_option(options, utf16!("roundingMode"), context)?.unwrap_or_default();
-        get_option(options, utf16!("roundingMode"), false, context)?.unwrap_or_default();
 
     // 11. Let trailingZeroDisplay be ? GetOption(options, "trailingZeroDisplay", string, « "auto", "stripIfInteger" », "auto").
     let trailing_zero_display =
-        get_option(options, utf16!("trailingZeroDisplay"), false, context)?.unwrap_or_default();
+        get_option(options, utf16!("trailingZeroDisplay"), context)?.unwrap_or_default();
 
     // 12. NOTE: All fields required by SetNumberFormatDigitOptions have now been read from options. The remainder of this AO interprets the options and may throw exceptions.
 

boa_engine/src/builtins/intl/plural_rules/mod.rs

@@ -29,7 +29,7 @@ use super::{
         f64_to_formatted_fixed_decimal, get_digit_format_options, DigitFormatOptions, Extrema,
         Notation,
     },
-    options::{coerce_options_to_object, IntlOptions, LocaleMatcher},
+    options::{coerce_options_to_object, IntlOptions},
     Service,
 };
 
@@ -113,14 +113,12 @@ impl BuiltInConstructor for PluralRules {
         // 3. Let opt be a new Record.
         // 4. Let matcher be ? GetOption(options, "localeMatcher", string, « "lookup", "best fit" », "best fit").
         // 5. Set opt.[[localeMatcher]] to matcher.
-        let matcher =
+        let matcher = get_option(&options, utf16!("localeMatcher"), context)?.unwrap_or_default();
-            get_option::<LocaleMatcher>(&options, utf16!("localeMatcher"), false, context)?
-                .unwrap_or_default();
 
         // 6. Let t be ? GetOption(options, "type", string, « "cardinal", "ordinal" », "cardinal").
         // 7. Set pluralRules.[[Type]] to t.
-        let rule_type = get_option::<PluralRuleType>(&options, utf16!("type"), false, context)?
+        let rule_type =
-            .unwrap_or(PluralRuleType::Cardinal);
+            get_option(&options, utf16!("type"), context)?.unwrap_or(PluralRuleType::Cardinal);
 
         // 8. Perform ? SetNumberFormatDigitOptions(pluralRules, options, +0𝔽, 3𝔽, "standard").
         let format_options = get_digit_format_options(&options, 0, 3, Notation::Standard, context)?;

boa_engine/src/builtins/intl/segmenter/mod.rs

@@ -32,7 +32,7 @@ pub(crate) use segments::*;
 
 use super::{
     locale::{canonicalize_locale_list, resolve_locale, supported_locales},
-    options::{IntlOptions, LocaleMatcher},
+    options::IntlOptions,
     Service,
 };
 
@@ -133,9 +133,7 @@ impl BuiltInConstructor for Segmenter {
 
         // 6. Let opt be a new Record.
         // 7. Let matcher be ? GetOption(options, "localeMatcher", string, « "lookup", "best fit" », "best fit").
-        let matcher =
+        let matcher = get_option(&options, utf16!("localeMatcher"), context)?.unwrap_or_default();
-            get_option::<LocaleMatcher>(&options, utf16!("localeMatcher"), false, context)?
-                .unwrap_or_default();
 
         // 8. Set opt.[[localeMatcher]] to matcher.
         // 9. Let localeData be %Segmenter%.[[LocaleData]].
@@ -151,9 +149,7 @@ impl BuiltInConstructor for Segmenter {
         );
 
         // 12. Let granularity be ? GetOption(options, "granularity", string, « "grapheme", "word", "sentence" », "grapheme").
-        let granularity =
+        let granularity = get_option(&options, utf16!("granularity"), context)?.unwrap_or_default();
-            get_option::<Granularity>(&options, utf16!("granularity"), false, context)?
-                .unwrap_or_default();
         // 13. Set segmenter.[[SegmenterGranularity]] to granularity.
 
         let native = match granularity {

boa_engine/src/builtins/options.rs

@@ -38,8 +38,9 @@ where
 /// Extracts the value of the property named `property` from the provided `options` object,
 /// converts it to the required `type` and checks whether it is one of a `List` of allowed
 /// `values`. If `values` is undefined, there is no fixed set of values and any is permitted.
-/// If the value is `undefined`, `required` determines if the function should return `None` or
+/// If the value is `undefined`, `required` would technically determine if the function should
-/// an `Err`. Use [`Option::unwrap_or`] and friends to manage the default value.
+/// return `None` or an `Err`, but handling this on the caller's side using [`Option::ok_or_else`]
+/// should provide better context for error messages.
 ///
 /// This is a safer alternative to `GetOption`, which tries to parse from the
 /// provided property a valid variant of the provided type `T`. It doesn't accept
@@ -50,7 +51,6 @@ where
 pub(crate) fn get_option<T: OptionType>(
     options: &JsObject,
     property: &[u16],
-    required: bool,
     context: &mut Context<'_>,
 ) -> JsResult<Option<T>> {
     // 1. Let value be ? Get(options, property).
@@ -58,15 +58,9 @@ pub(crate) fn get_option<T: OptionType>(
 
     // 2. If value is undefined, then
     if value.is_undefined() {
-        return if required {
         // a. If default is required, throw a RangeError exception.
-            Err(JsNativeError::range()
-                .with_message("GetOption: option value cannot be undefined")
-                .into())
-        } else {
         // b. Return default.
-            Ok(None)
+        return Ok(None);
-        };
     }
 
     // The steps 3 to 7 must be made for each `OptionType`.