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

Throw TypeError for all input validation #589

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

a-sully
Copy link
Contributor

@a-sully a-sully commented Feb 28, 2024

This PR removes all uses of DataError from the spec in favor of using TypeError for all input validation

Notably, all validation checks and output shape calculations related to operand axes/ranks/dimensions/shapes are treated as TypeError with the rationale that axes/ranks/dimensions/shapes do not constitute an operand's data, but its type (as C++ does, for example)

Fixes #583


Preview | Diff

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

@a-sully , thanks for making this PR!

index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@a-sully
Copy link
Contributor Author

a-sully commented Feb 28, 2024

We discussed this topic internally, kicked off by @inexorabletash's observation that RangeError is very rarely used in web specs...

Taking a step back, we should express as many of these constraints in the IDL (as opposed to the algorithm text) as possible. That's what #562 is all about. But many of these constraints are not (yet) expressible via WebIDL.

That leads to the question of what might a future WebIDL do if it added support for e.g. range validation, default non-empty sequences, etc? Ideally we could anticipate this and throw the appropriate errors for WebNN...

...Given that even the [EnforceRange] IDL extended attribute results in a TypeError (crazy! 🤯 Thanks @reillyeon for the pointer) I think that's a strong argument that these future WebIDL features would also use TypeError, and we should just throw TypeError for all synchronous input validation in WebNN

Thoughts?

@a-sully
Copy link
Contributor Author

a-sully commented Feb 29, 2024

Apologies for the force-push (there were several conflicts) but the diff is just s/RangeError/TypeError

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @a-sully !

@huningxin huningxin requested a review from fdwr March 1, 2024 00:25
@huningxin
Copy link
Contributor

@a-sully , you may want to update your first comment where

A few instances use RangeError - for example validating that the value of an options dictionary member is not zero.

it may not be true in the latest commit.

@a-sully a-sully changed the title Throw TypeError for most all input validation Throw TypeError for all input validation Mar 1, 2024
@a-sully
Copy link
Contributor Author

a-sully commented Mar 1, 2024

Good catch @huningxin. I've updated the PR description and title. Please feel free to merge at your convenience

Thanks all for the reviews!

@a-sully
Copy link
Contributor Author

a-sully commented Mar 1, 2024

Just kidding, I just noticed that @fdwr hasn't looked at this yet. Dwayne, PTAL and merge at your convenience, if it looks good :)

@fdwr
Copy link
Collaborator

fdwr commented Mar 1, 2024

For cases like these...

...wouldn't they best be described as RangeErrors? I agree it's not about the data (so DataError "Provided data is inadequate." doesn't make so much sense), but it's really not about the data type either.

One good DataError case is:

@fdwr
Copy link
Collaborator

fdwr commented Mar 1, 2024

(read Joshua's comment above)

🤔 Well I still think RangeError semantically makes much sense for cases like passing an axis parameter that is out of range for the rank. For [EnforceRange], TypeError actually makes sense to me too because it's specifically about data type conversion (from a double to a unsigned long). Though, I'm content to accept this change either way because the current [DataError] doesn't really make sense either, and the WebIDL spec here says "Use TypeError for invalid arguments". 🤷‍♂️

we should express as many of these constraints in the IDL (as opposed to the algorithm text) as possible

👍

@fdwr fdwr merged commit ca0b903 into webmachinelearning:main Mar 1, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 1, 2024
SHA: ca0b903
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@a-sully a-sully deleted the fix-583 branch March 4, 2024 19:34
aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 20, 2024
The spec had been updated to throw TypeError for failed input
validations: webmachinelearning/webnn#589.

This CL replaces the last remaining DataErrors in lstm implementation and triangular wpt.

Bug: 40206287
Change-Id: I73952ce5b407f2df1d9d3b22c0086638d092e811
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5632940
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Austin Sullivan <[email protected]>
Auto-Submit: Jiewei Qian <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317381}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 20, 2024
The spec had been updated to throw TypeError for failed input
validations: webmachinelearning/webnn#589.

This CL replaces the last remaining DataErrors in lstm implementation and triangular wpt.

Bug: 40206287
Change-Id: I73952ce5b407f2df1d9d3b22c0086638d092e811
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5632940
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Austin Sullivan <[email protected]>
Auto-Submit: Jiewei Qian <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317381}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 20, 2024
The spec had been updated to throw TypeError for failed input
validations: webmachinelearning/webnn#589.

This CL replaces the last remaining DataErrors in lstm implementation and triangular wpt.

Bug: 40206287
Change-Id: I73952ce5b407f2df1d9d3b22c0086638d092e811
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5632940
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Austin Sullivan <[email protected]>
Auto-Submit: Jiewei Qian <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317381}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 27, 2024
… a=testonly

Automatic update from web-platform-tests
webnn: replace DataError with TypeError

The spec had been updated to throw TypeError for failed input
validations: webmachinelearning/webnn#589.

This CL replaces the last remaining DataErrors in lstm implementation and triangular wpt.

Bug: 40206287
Change-Id: I73952ce5b407f2df1d9d3b22c0086638d092e811
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5632940
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Austin Sullivan <[email protected]>
Auto-Submit: Jiewei Qian <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317381}

--

wpt-commits: b03730d3049c601dbd807c50643f3f38ff9a0add
wpt-pr: 46845
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Jun 27, 2024
… a=testonly

Automatic update from web-platform-tests
webnn: replace DataError with TypeError

The spec had been updated to throw TypeError for failed input
validations: webmachinelearning/webnn#589.

This CL replaces the last remaining DataErrors in lstm implementation and triangular wpt.

Bug: 40206287
Change-Id: I73952ce5b407f2df1d9d3b22c0086638d092e811
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5632940
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Austin Sullivan <[email protected]>
Auto-Submit: Jiewei Qian <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317381}

--

wpt-commits: b03730d3049c601dbd807c50643f3f38ff9a0add
wpt-pr: 46845
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jun 28, 2024
… a=testonly

Automatic update from web-platform-tests
webnn: replace DataError with TypeError

The spec had been updated to throw TypeError for failed input
validations: webmachinelearning/webnn#589.

This CL replaces the last remaining DataErrors in lstm implementation and triangular wpt.

Bug: 40206287
Change-Id: I73952ce5b407f2df1d9d3b22c0086638d092e811
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5632940
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Austin Sullivan <[email protected]>
Auto-Submit: Jiewei Qian <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317381}

--

wpt-commits: b03730d3049c601dbd807c50643f3f38ff9a0add
wpt-pr: 46845
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 1, 2024
… a=testonly

Automatic update from web-platform-tests
webnn: replace DataError with TypeError

The spec had been updated to throw TypeError for failed input
validations: webmachinelearning/webnn#589.

This CL replaces the last remaining DataErrors in lstm implementation and triangular wpt.

Bug: 40206287
Change-Id: I73952ce5b407f2df1d9d3b22c0086638d092e811
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5632940
Commit-Queue: Austin Sullivan <asullychromium.org>
Reviewed-by: Austin Sullivan <asullychromium.org>
Auto-Submit: Jiewei Qian <qjwchromium.org>
Cr-Commit-Position: refs/heads/main{#1317381}

--

wpt-commits: b03730d3049c601dbd807c50643f3f38ff9a0add
wpt-pr: 46845

UltraBlame original commit: 5e7160c2c0eae28706bcdec3b49e789ec48e360e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 1, 2024
… a=testonly

Automatic update from web-platform-tests
webnn: replace DataError with TypeError

The spec had been updated to throw TypeError for failed input
validations: webmachinelearning/webnn#589.

This CL replaces the last remaining DataErrors in lstm implementation and triangular wpt.

Bug: 40206287
Change-Id: I73952ce5b407f2df1d9d3b22c0086638d092e811
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5632940
Commit-Queue: Austin Sullivan <asullychromium.org>
Reviewed-by: Austin Sullivan <asullychromium.org>
Auto-Submit: Jiewei Qian <qjwchromium.org>
Cr-Commit-Position: refs/heads/main{#1317381}

--

wpt-commits: b03730d3049c601dbd807c50643f3f38ff9a0add
wpt-pr: 46845

UltraBlame original commit: 5e7160c2c0eae28706bcdec3b49e789ec48e360e
sadym-chromium pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 18, 2024
The spec had been updated to throw TypeError for failed input
validations: webmachinelearning/webnn#589.

This CL replaces the last remaining DataErrors in lstm implementation and triangular wpt.

Bug: 40206287
Change-Id: I73952ce5b407f2df1d9d3b22c0086638d092e811
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5632940
Commit-Queue: Austin Sullivan <[email protected]>
Reviewed-by: Austin Sullivan <[email protected]>
Auto-Submit: Jiewei Qian <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1317381}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider throwing TypeError for invalid input
3 participants