Observable validation logic

[gibson042]

It's a bit strange that e.g. GetAmountOptions rejects an invalid roundingMode value such as "foo" before even attempting to [[Get]] unit, but does not reject an invalid numeric fractionDigits such as 4.2 until after [[Get]]ing all options fields and verifying absence of a non-undefined value for significantDigits. I think it would make more sense (and be more aligned with recent additions such as Temporal) to perform isolated validation atomically with each [[Get]], leaving the door open for future consolidation that extends/wraps/post-processes GetOption similar to GetNumberOption, DefaultNumberOption, ToIntegerIfIntegral, and GetRoundingIncrementOption:

@@ GetAmountOptions (
           1. Let _opts_ be ? GetOptionsObject(_opts_).
           1. Let _fractionDigits_ be ? GetOption(_opts_, *"fractionDigits"*, ~number~, ~empty~, *undefined*).
+          1. If _fractionDigits_ is not *undefined* and not an integral Number, throw a *RangeError* exception.
           1. Let _roundingMode_ be ? GetOption(_opts_, *"roundingMode"*, ~string~, « *"ceil"*, *"floor"*, *"expand"*, *"trunc"*, *"halfCeil"*, *"halfFloor"*, *"halfExpand"*, *"halfTrunc"*, *"halfEven"* », *"halfEven"*).
           1. Let _significantDigits_ be ? GetOption(_opts_, *"significantDigits"*, ~number~, ~empty~, *undefined*).
+          1. If _significantDigits_ is not *undefined* and not a positive integral Number, throw a *RangeError* exception.
           1. Let _unit_ be ? GetOption(_opts_, *"unit"*, ~string~, ~empty~, *undefined*).
-          1. If _fractionDigits_ is not *undefined*, then
-            1. If _significantDigits_ is not *undefined*, throw a *RangeError* exception.
-            1. If _fractionDigits_ is not an integral Number, throw a *RangeError* exception.
-          1. Else if _significantDigits_ is not *undefined*, then
-            1. If _significantDigits_ is not an integral Number, throw a *RangeError* exception.
-            1. If _significantDigits_ &lt; *1*<sub>𝔽</sub>, throw a *RangeError* exception.
           1. If _unit_ is the empty String, throw a *RangeError* exception.
+          1. If _fractionDigits_ is not *undefined* and _significantDigits_ is not *undefined*, throw a *TypeError* exception.
           1. Return the Record { [[FractionDigits]]: _fractionDigits_, [[RoundingMode]]: _roundingMode_, [[SignificantDigits]]: _significantDigits_, [[Unit]]: _unit_ }.
@@ GetAmountConvertToOptions (
           1. Let _opts_ be ? GetOptionsObject(_opts_).
-          1. Let _minFractionDigits_ be ? GetOption(_opts_, *"minimumfractionDigits"*, ~number~, ~empty~, *undefined*).
+          1. Let _minFractionDigits_ be ? GetOption(_opts_, *"minimumFractionDigits"*, ~number~, ~empty~, *undefined*).
+          1. If _minFractionDigits_ is not *undefined* and not an integral Number, throw a *RangeError* exception.
-          1. Let _maxFractionDigits_ be ? GetOption(_opts_, *"maximumfractionDigits"*, ~number~, ~empty~, *undefined*).
+          1. Let _maxFractionDigits_ be ? GetOption(_opts_, *"maximumFractionDigits"*, ~number~, ~empty~, *undefined*).
+          1. If _maxFractionDigits_ is not *undefined* and not an integral Number, throw a *RangeError* exception.
           1. Let _roundingMode_ be ? GetOption(_opts_, *"roundingMode"*, ~string~, « *"ceil"*, *"floor"*, *"expand"*, *"trunc"*, *"halfCeil"*, *"halfFloor"*, *"halfExpand"*, *"halfTrunc"*, *"halfEven"* », *"halfEven"*).
           1. Let _roundingPriority_ be ? GetOption(_opts_, *"roundingPriority"*, ~string~, ~empty~, *undefined*).
           1. Let _minSignificantDigits_ be ? GetOption(_opts_, *"minimumSignificantDigits"*, ~number~, ~empty~, *undefined*).
+          1. If _minSignificantDigits_ is not *undefined* and not an integral Number, throw a *RangeError* exception.
           1. Let _maxSignificantDigits_ be ? GetOption(_opts_, *"maximumSignificantDigits"*, ~number~, ~empty~, *undefined*).
+          1. If _maxSignificantDigits_ is not *undefined* and not an integral Number, throw a *RangeError* exception.
-          1. Let _significantDigits_ be ? GetOption(_opts_, *"maximumSignificantDigits"*, ~number~, ~empty~, *undefined*).
           1. Let _unit_ be ? GetOption(_opts_, *"unit"*, ~string~, ~empty~, *undefined*).
-          1. If _minFractionDigits_ is not *undefined* and not an integral Number, throw a *RangeError* exception.
-          1. If _maxFractionDigits_ is not *undefined* and not an integral Number, throw a *RangeError* exception.
-          1. If _minSignificantDigits_ is not *undefined* and not an integral Number, throw a *RangeError* exception.
-          1. If _maxSignificantDigits_ is not *undefined* and not an integral Number, throw a *RangeError* exception.
           1. If _unit_ is the empty String, throw a *RangeError* exception.
-          1. Return the Record { [[MinimumFractionDigits]]: _minFractionDigits_, [[MaximumFractionDigits]]: _maxFractionDigits_, [[MinimumSignificantDigits]]: _minSignificantDigits_, [[MaximumSignificantDigits]]: _maxSignificantDigits_, [[RoundingMode]]: _roundingMode_, [[RoundingPriority]]: _roundingPriority_, [[SignificantDigits]]: _significantDigits_, [[Unit]]: _unit_ }.
+          1. Return the Record { [[MinimumFractionDigits]]: _minFractionDigits_, [[MaximumFractionDigits]]: _maxFractionDigits_, [[MinimumSignificantDigits]]: _minSignificantDigits_, [[MaximumSignificantDigits]]: _maxSignificantDigits_, [[RoundingMode]]: _roundingMode_, [[RoundingPriority]]: _roundingPriority_, [[Unit]]: _unit_ }.

(note also the alias/property key spelling corrections, removal of [[SignificantDigits]], and replacement of RangeError with TypeError to indicate conflicting options, aligning with e.g. CreateDateTimeFormat and SetNumberFormatDigitOptions)

Further, options should be read in alphabetical order, reording GetAmountConvertToOptions like ["maximumFractionDigits", "maximumSignificantDigits", "minimumFractionDigits", "minimumSignificantDigits", "roundingMode", "roundingPriority"].

[ljharb]

The general approach we've tried to take is usually either a) read everything and b) validate everything, or to read and validate one thing at a time.

The former is, I think, what most parts of the language do, and because it includes unconditional gets, is the most consistent and predictable.

[gibson042]

As I mentioned above, the most recent relevant changes are in Temporal, which leans heavily into the latter approach as seen in e.g. GetDifferenceSettings and ToPartialDurationRecord (among many others; search for "perform independent validation in alphabetical order"). An earlier iteration of Temporal actually had a SnapshotOwnProperties operation for consuming input bags up front such that later reads were unobservable, but that was intentionally removed by tc39/proposal-temporal@b889242 along with custom calendars and time zones in tc39/proposal-temporal#2925 .

Many existing algorithms already work that way, too—ArrayBuffer and SharedArrayBuffer validate length via ToIndex before looking for options.maxByteLength, ToPropertyDescriptor validates that a get value is undefined or callable before looking for set, Error.prototype.toString/RegExp.prototype.toString stringify a non-undefined O.name/R.source before looking for O.message/R.flags (respectively), and even RegExp.prototype [ %Symbol.replace% ] validates result.length before reading result[0] and converts that to a string before reading result.index. The fact is, <emu-alg> and our conventions pretty much encourage such an approach and it takes intentional effort to do otherwise.

Further, implementers were and presumably still are enthusiastic about eager validation, because it supports use of simpler "make invalid states unrepresentable" data structures that are more ergonomic (especially in Rust), smaller in memory (e.g., through use of enum and small integer types rather than string and float64), and faster to initialize (because there's no need to hold temporary values during a "read everything" phase).

Bottom line: I don't think trying to swim upstream on this is worthwhile. Both patterns are coherent, comprehensible, and testable, but eager validation requires less effort for both specifiers and implementers and potentially also fosters marginally better end user experiences.

[sffc]

read-validate-read-validate or read-read-validate-validate are both okay from a library point of view; what to avoid is interleaving "interesting" spec logic with reading fields. Using the value of a previously read field to decide whether to read or how to validate a future field is an example of "interesting" logic that makes it slightly harder for library authors.

[ljharb]

I definitely agree that either answer is acceptable, I just have a preference for the latter :-)