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

Fix return type of WebIDL indexing getters #1789

Merged
merged 9 commits into from
Oct 4, 2019

Conversation

j-devel
Copy link
Contributor

@j-devel j-devel commented Sep 25, 2019

This PR is to update the return signatures of WebIDL indexing getters. All indexing getters will return optional values unless marked by [Throws] in their WebIDL definitions.

I summarize the impact of this change in the list below. As a result, 14 indexing getters with originally unwrapped return type T (non-highlighted in 3rd column) will return Option<T> instead.

table-before-after-change-clean

Since this is a breaking change, I am including some commits that deal with affected examples as well. It turned out that I found only the todomvc example (50fbb5c).

Motivation

In #1756, we have observed the situations where indexing getters (such as __widl_f_get_DOMStringMap and __widl_f_get_CSSStyleDeclaration) can return undefined that breaks our assumptions based on their WebIDL definitions, leading to unrecoverable runtime exceptions. Following @Pauan 's advice, this PR is aimed at fixing the problem by updating the return signatures of indexing getters.

@alexcrichton
Copy link
Contributor

Thanks for the PR! It's been awhile since I looked at this, so can you describe in the commits/PR message what this is doing?

crates/webidl/src/util.rs Outdated Show resolved Hide resolved
@j-devel
Copy link
Contributor Author

j-devel commented Sep 30, 2019

@alexcrichton I have updated the description of PR/commits.

@j-devel j-devel marked this pull request as ready for review September 30, 2019 05:05
@alexcrichton
Copy link
Contributor

Thanks for this!

The impact here seems pretty minimal actually so I think it might be reasonable for us to consider doing this in a point release, despite it being a breaking change.

In terms of implementation though, could this be done not by looking for "Option<" but instead by updating the webidl signatures themselves? It seems like we should basically be able to say that all indexing getters should return optional values (or otherwise be marked with catch)?

@j-devel
Copy link
Contributor Author

j-devel commented Oct 2, 2019

Thanks for the feedback!

could this be done not by looking for "Option<" but instead by updating the webidl signatures themselves?

Yes, and I have arrived at a much more compact solution: 53b0cd0

It seems like we should basically be able to say that all indexing getters should return optional values (or otherwise be marked with catch)?

Precisely. I have updated the PR's description and implementation comments.

@alexcrichton
Copy link
Contributor

Great! Mind doing an analysis again to produce a list of which APIs are changing?

@j-devel
Copy link
Contributor Author

j-devel commented Oct 3, 2019

Mind doing an analysis again to produce a list of which APIs are changing?

I have done an analysis as follows:

1) set up API signature dumping
Before the let invocation line in

let invocation = quote! {
#(#attrs)*
#[allow(bad_style)]
#[doc = #doc_comment]
#[allow(clippy::all)]
#vis fn #rust_name(#me #(#arguments),*) #ret {

add

        {
            let js_fn_name = format!("{}", &self.shim);
            let rust_fn = quote! { #vis fn #rust_name(#me #(#arguments),*) };
            let rust_fn_ret = quote! { #ret };
            println!("analysis: | {} | {} | {} |", js_fn_name, rust_fn, rust_fn_ret);
        }

2) collect API signature dumps
2.1) dump signatures to ,log--before

$ cd crates/web-sys
$ cargo test --target wasm32-unknown-unknown html_element --all-features
$ grep '^analysis: ' ../../target/wasm32-unknown-unknown/debug/build/web-sys-b50e021c48dde6fa/output > ,log--before

2.2) apply 53b0cd0 and 46dd0aa
2.3) dump signatures to ,log--after

$ cargo test --target wasm32-unknown-unknown html_element --all-features
$ grep '^analysis: ' ../../target/wasm32-unknown-unknown/debug/build/web-sys-b50e021c48dde6fa/output > ,log--after

3) print the changing APIs

$ git diff --no-index --unified=0 ,log--{before,after}

web-sys debug paths

@alexcrichton
Copy link
Contributor

Thanks for this!

@fitzgen how do you feel landing this in a point release? While strictly a breaking change it seems like it's sort of hard to use right without this

@fitzgen
Copy link
Member

fitzgen commented Oct 3, 2019

I feel OK about it, given that these are pretty niche APIs. It seems like it is worth the trade off.

@alexcrichton
Copy link
Contributor

Ok, let's go ahead and merge this then. If issues come up we can yank, revert, and publish a new version, and then schedule it for the next breaking wasm-bindgen release.

@alexcrichton alexcrichton merged commit 0e3b696 into rustwasm:master Oct 4, 2019
@j-devel j-devel deleted the fix-indexing-getter-rets branch October 4, 2019 01:41
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.

3 participants