Fix return type of WebIDL indexing getters

[j-devel]

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.

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]

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?

[j-devel]

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

[alexcrichton]

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]

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]

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

[j-devel]

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

wasm-bindgen/crates/backend/src/codegen.rs

Lines 1006 to 1012 in 74812bd

	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}

[alexcrichton]

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]

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

[alexcrichton]

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.