Skip to content
This repository was archived by the owner on Apr 25, 2025. It is now read-only.

[js-api] Tidy up rethrow identity test #256

Merged
aheejin merged 6 commits into from
Feb 23, 2023
Merged

[js-api] Tidy up rethrow identity test #256

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f1bdc73
[js-api] Tidy up rethrow identity test
aheejin Feb 17, 2023
ffd9918
Update test/js-api/exception/identity.tentative.any.js
aheejin Feb 17, 2023
c055b01
Comments on functions
aheejin Feb 17, 2023
968c69e
Typo
aheejin Feb 17, 2023
5e866d2
Delete payload comparison
aheejin Feb 17, 2023
5c8dea4
Merge branch 'main' into identity_test2
aheejin Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 31 additions & 22 deletions test/js-api/exception/identity.tentative.any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,59 +3,68 @@
// META: script=/wasm/jsapi/wasm-module-builder.js

test(() => {
const tag = new WebAssembly.Tag({ parameters: ["i32"] });
const exn = new WebAssembly.Exception(tag, [42]);
const exn_same_payload = new WebAssembly.Exception(tag, [42]);
const exn_diff_payload = new WebAssembly.Exception(tag, [53]);

const builder = new WasmModuleBuilder();
const jsfuncIndex = builder.addImport("module", "jsfunc", kSig_v_v);
const tagIndex = builder.addImportedTag("module", "tag", kSig_v_i);

// Tag defined in JavaScript and imported into Wasm
const jsTag = new WebAssembly.Tag({ parameters: ["i32"] });
const jsTagIndex = builder.addImportedTag("module", "jsTag", kSig_v_i)
const jsTagExn = new WebAssembly.Exception(jsTag, [42]);
const jsTagExnSamePayload = new WebAssembly.Exception(jsTag, [42]);
const jsTagExnDiffPayload = new WebAssembly.Exception(jsTag, [53]);
const throwJSTagExnIndex = builder.addImport("module", "throwJSTagExn", kSig_v_v);

const imports = {
module: {
jsfunc: function() { throw exn; },
tag: tag
throwJSTagExn: function() { throw jsTagExn; },
jsTag: jsTag
}
};

builder
.addFunction("catch_rethrow", kSig_v_v)
.addFunction("catch_js_tag_rethrow", kSig_v_v)
.addBody([
kExprTry, kWasmStmt,
kExprCallFunction, jsfuncIndex,
kExprCatch, tagIndex,
kExprCallFunction, throwJSTagExnIndex,
kExprCatch, jsTagIndex,
kExprDrop,
kExprRethrow, 0x00,
kExprEnd
])
.exportFunc();

builder
.addFunction("catch_all_rethrow", kSig_v_v)
.addFunction("catch_all_js_tag_rethrow", kSig_v_v)
.addBody([
kExprTry, kWasmStmt,
kExprCallFunction, jsfuncIndex,
kExprCallFunction, throwJSTagExnIndex,
kExprCatchAll,
kExprRethrow, 0x00,
kExprEnd
])
.exportFunc();

const buffer = builder.toBuffer();

// The exception object's identity should be preserved throw 'rethrow's in
// Wasm code. Do tests with a tag defined in JS.
Copy link
Contributor

@Ms2ger Ms2ger Feb 17, 2023

Choose a reason for hiding this comment

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

I'm finding it hard to follow the first sentence

Copy link
Member Author

@aheejin aheejin Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I committed @dschuff's suggestion: #256 (comment) Is this clearer?

WebAssembly.instantiate(buffer, imports).then(result => {
try {
result.instance.exports.catch_rethrow();
result.instance.exports.catch_js_tag_rethrow();
} catch (e) {
assert_equals(e, exn);
assert_not_equals(e, exn_same_payload);
assert_not_equals(e, exn_diff_payload);
assert_equals(e, jsTagExn);
assert_equals(e.getArg(jsTag, 0), jsTagExn.getArg(jsTag, 0));
Copy link
Contributor

@Ms2ger Ms2ger Feb 17, 2023

Choose a reason for hiding this comment

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

I don't see the value of this assertion; you already know they're the same object, so checking that the calling the same method on the same object returns the same result isn't very useful.

It might be interesting (if you hadn't planned that yet) to have a test where JS throws an exception (as you do here), and wasm catches it, but then returns the payload rather than using rethrow. Also one where wasm uses throw to create a new exception with the same payload.

Copy link
Member

@dschuff dschuff Feb 17, 2023

Choose a reason for hiding this comment

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

Those seem like good ideas; the latter one could check both that the payload is the same and that the exception object is different.

Copy link
Member Author

@aheejin aheejin Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@Ms2ger

I don't see the value of this assertion; you already know they're the same object, so checking that the calling the same method on the same object returns the same result isn't very useful.

The checking of the payload was suggested in #253 (comment), but I agree it is not strictly necessary. I can delete it. @dschuff Are you OK with that?

It might be interesting (if you hadn't planned that yet) to have a test where JS throws an exception (as you do here), and wasm catches it, but then returns the payload rather than using rethrow. Also one where wasm uses throw to create a new exception with the same payload.

Given that this PR intended only for cosmetic changes, do you mind if I do these in a follow-up?

Copy link
Member

@dschuff dschuff Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah if we're going to have the object equality check, I agree we don't need the payload check. Followup change for more tests is fine with me.

// Even if they have the same payload, they are different objects, so they
// shouldn't compare equal.
assert_not_equals(e, jsTagExnSamePayload);
assert_not_equals(e, jsTagExnDiffPayload);
}
try {
result.instance.exports.catch_all_rethrow();
result.instance.exports.catch_all_js_tag_rethrow();
} catch (e) {
assert_equals(e, exn);
assert_not_equals(e, exn_same_payload);
assert_not_equals(e, exn_diff_payload);
assert_equals(e, jsTagExn);
assert_equals(e.getArg(jsTag, 0), jsTagExn.getArg(jsTag, 0));
assert_not_equals(e, jsTagExnSamePayload);
assert_not_equals(e, jsTagExnDiffPayload);
}
});
}, "Identity check");
4 changes: 2 additions & 2 deletions test/js-api/wasm-module-builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,9 +754,9 @@ class WasmModuleBuilder {

addTag(type) {
let type_index = (typeof type) == "number" ? type : this.addType(type);
let except_index = this.tags.length + this.num_imported_tags;
let tag_index = this.tags.length + this.num_imported_tags;
this.tags.push(type_index);
return except_index;
return tag_index;
}

addFunction(name, type) {
Expand Down