Use web spec best practices

[zolkis]

Originally posted by @inexorabletash in #446 (comment)

Drive by comment: the first step here (If a or b is not an instance of MLOperand, then throw a "TypeError"...) is unnecessary, and I see this repeated throughout the newly introduced algorithms for methods ("operations" in Web IDL). The Web IDL bindings already take care validating input types and throwing TypeError.

Further, even in cases where an explicit error must be thrown, the intrinsic type TypeError should be thrown, not a DOMException of type "TypeError" (which is not present in https://webidl.spec.whatwg.org/#idl-DOMException-error-names)

(Issue created to document this change).

[inexorabletash]

Another example:

in constant() - If type is undefined, let type be "float32". - this is handled automatically by having the default in the Web IDL.

[inexorabletash]

Also (and this is not new), dictionary members are more normally references in prose using the infra ordered map syntax: options["foo"]

By the time an algorithm "sees" the dictionary, it's gone through the bindings layer and is an ordered map - https://webidl.spec.whatwg.org/#idl-dictionaries

[inexorabletash]

throw a "TypeError" simple exception

Just write: throw a TypeError.

In bikeshed markdown:

... [=exception/throw=] a {{TypeError}}.

[inexorabletash]

And (this new is in PR 446) the entire "check linear options" algorithm is unnecessary; the dictionary bindings for ECMAScript take care of all of that, including default values.

[inexorabletash]

(apologies for random feedback; I may have more time later this week to do a more thorough pass)

[inexorabletash]

Not new: In "create MLActivation" steps

If name is undefined or null, throw a "TypeError" and abort these steps.

It looks like all call sites are internal to the spec and so this could just be an assert.

And this is more off topic, but: none of the call sites for "create MLActivation" actually pass builder - that should be fixed at the call sites. I also don't see any of the call sites passing init-steps but maybe I'm missing something.

[inexorabletash]

Not new: in input(name, descriptor):

If name is undefined or an empty string, then throw a TypeError and stop.

name is defined as DOMString; passing literal undefined from script will yield the string "undefined". So this can be reduced to just a test for an empty string.

(This is a gotcha for web developers since an error would be better, but that's how the Web IDL bindings work.)

[inexorabletash]

Not new: in conv2d(input, filter, options)

If options is undefined, let options be an empty object.

The options argument is defined as optional MLConv2dOptions options = {} so it will always be present.

If options.padding is undefined, ...

It's more correct to write:

If options["padding"] exists, ...

in Bikeshed markdown:

If |options|[{{MLConv2dOptions/"padding"}}] [=map/exists=] ...

[inexorabletash]

Not new:

Let ... be the first argument.
Let ... be the second argument.

This is unnecessary, arguments are named. I see 5 uses of "first argument", and 2 of "second argument". Just drop these.

[inexorabletash]

Not new: Similar to a nitpick above, for "create MLOperand", the second step is:

If desc is not an object that implements MLOperandDescriptor ...

Since this is called internally with this everywhere, the validation is unnecessary.

Also, total nitpick, I noticed that some of the call sites are phrased as:

... the result of invoking the create MLOperand steps given this ...

and some are:

... the result of invoking the create MLOperand steps with this ...

This applies to other invocations as well ("with" vs. "given") - ideally the phrasing would be consistent, but there's not consistency in phrasing across specs so either is fine.

[inexorabletash]

Optional style suggestion:

If you rename the algorithms from (for example) "create MLOperand" to "create an MLOperand" (which is better grammar anyway), then you can use simplified phrasing when invoking algorithms.

Before:

Let operand be the result of invoking the create MLOperand steps given this and descriptor.

After:

Let operand be the result of creating an MLOperand given this and descriptor.

Note that Bikeshed is smart enough to link "creating an MLOperand" to a definition named "create an MLOperand". More and more specs are using the shorter form - less text is generally more readable.

[inexorabletash]

Not new: There are a couple checks for "... is not an an object that implements MLOperandDescriptor ..." - in both cases, the value is supplied from an internal slot or explicitly passed via the algorithm, so this validation seems unnecessary.

[inexorabletash]

New (mostly) in 446: When initializing lists:

... set it to [0, 0, 0, 0].

Technically at this point the Web IDL sequence has become a Infra list and should be initialized using this literal syntax:

... set it to « 0, 0, 0, 0 ».

Super nitpicky but it can make it clearer to implementers that by this point we're talking about abstract types, not a Javascript Array.

[inexorabletash]

Meta: if reporting style issues that I don't think should hold up PR 446 here is working for you, I'm happy to keep doing so. But I'm also happy to leave comments directly on the PR, file separate issues, etc.

[inexorabletash]

Not new: There are a few places where a record is being iterated, with:

For each key -> value of ...

This should use a proper right arrow (→) instead of the digraph, and can be written in markdown as:

[=map/For each=] |key| → |value| of ...

Or use a literal → in the source if you don't mind non-ASCII.

[inexorabletash]

Not new: Many places use "... throw ... and stop." or "... throw ... and abort these steps."

Per https://infra.spec.whatwg.org/#algorithm-control-flow this isn't necessary - throwing terminates the algorithm.

I know it's tempting to be explicit, but shorter is usually better!

[zolkis]

Thanks a lot for all these suggestions, they are much appreciated!
Listing them in this issue makes sense, at least we have all suggestions in one place for future reference. Actually we should move the other similar suggestions here, too.

So far I applied all of these, except for:

• the dict members vs ordered map syntax (let's discuss this in a call, as I find it easier/more concise to write and read the dict members syntax);
• and removing the "and stop" clauses, which I will do later (just too much and mostly manual work). Will need less busy time with lots of caffein motivators. :)

[zolkis]

Originally posted by @inexorabletash in #446 (comment)

Regarding using TypeError vs. a DataError DOMException:

1. If you mix and match exception types, the order of the checks becomes important. From experience (e.g. with https://w3c.github.io/IndexedDB/#add-or-put) this has some benefit (e.g. developers can ask a more specific question on StackOverflow) but in practice it turns into an implementation burden - implementations must align on the sequence of input validation steps exactly.
2. For modern specs where we expect non-web-browser implementations to exist (e.g. for Node.js) then the preference is to use JS-intrinsic types (TypeError, RangeError) everywhere, rather than DOMException. See https://fetch.spec.whatwg.org/ https://encoding.spec.whatwg.org/ etc.

Either way, you'll want Web Platform Tests that exercise each failure path and validate the correct exception type is thrown.

[inexorabletash]

the dict members vs ordered map syntax

Yeah, we can discuss. But queuing up thoughts, it helps to distinguish when you're referencing an interface member - usually only via named slots - vs. a dictionary member. And when things get wordy like: options["bias"].[[descriptor]]["dimensions"] it usually helps to use a temporary, e.g "Let bias be options["bias"]"

[inexorabletash]

Some more things that can be linkified to make the meaning clear:

• ... be the size of ... → [=list/size=]
  • And so far as I can tell, all of the places that use .length are referring to the size of a list (what a sequence<> turns into) and should use [=list/size=] instead.
• link enum members when you can, e.g. "nchw" as {{MLInputOperandLayout/"nchw"}} if possible
• link rank everywhere if you can, e.g. in a number between 0 and the rank of input

[inexorabletash]

reshape() is defined as:

 MLOperand reshape(MLOperand input, sequence<unsigned long?> newShape);

The steps have:

If newShape is a scalar number, set outputShape to « 1 ».
Otherwise, if newShape is an array of unsigned long:

That won't apply - newShape will always be a list, where the items are unsigned long or null.

EDIT to add:

Same thing applies to transpose() and permutation; this definition:

  sequence<unsigned long> permutation;

...means this is unnecessary:

If options.permutation is not a sequence of unsigned long, then throw a TypeError and stop.

And constant() has:

 MLOperand constant(double value, optional MLOperandType type = "float32");

... which means neither of these are needed:

If value is not a number, then throw a TypeError and stop.
Otherwise, if type is not one of MLOperandType, then throw a TypeError and stop.

gru() is defined as:

  MLRecurrentNetworkDirection direction = "forward";
  MLGruWeightLayout layout = "zrn";

... which means these are not necessary:

If options.direction is not one of MLRecurrentNetworkDirection, then throw a TypeError and stop.
If options.layout is not one of MLGruWeightLayout, then throw a TypeError and stop.
... and ditto for gruCell().

There are similar unnecessary enum value checks throughout that can all be removed.

(The whole point of Web IDL is that these checks are taken care of for you automatically!)

[inexorabletash]

In the "create an MLOperand", "copy an MLOperand", and "create an MLActivation" steps there are lines:

Let thing be a new object.

This should reference the specific type, e.g. "new MLOperand"

Right now, the term "object" is linking to FileAPI, which is probably not desired. It's worth looking at the Terms defined by reference section Bikeshed generates for you, to ensure you're getting the dependencies you expect (e.g. you probably aren't meaning to use CSS4's definition for "number")

[inexorabletash]

I lied - I put up a PR. But I'm about to go out-of-office so I'll let the editors make use of it or wait until January.

Happy Solstice, everyone!

[inexorabletash]

My skim of this issue - looking to see if we can close it yet - turns up these potentially unresolved issues:

• dict member syntax (dotted properties vs ordered map syntax) - IMHO not worth pursuing
• "register" used instead of "set" - covered in Update the programming model section #549#issuecomment-1921901735
• discussion of "underlying platform" simplification, covered in Improve "underlying platform" references in spec #462
• use of "partial interface" - Revisiting use of partial interfaces #579
• multiple methods defined with a single algorithm-steps - Style: Make grouped algorithm steps consistent #576

Otherwise I think @zolkis and I have tackled it all, and so we can close this out now or soon.

[inexorabletash]

Closing out now that outstanding issues are tracked elsewhere.