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

new constructors for WebIDL bindings introduce undefined behavior by not setting default values #3937

Open
pablosichert opened this issue Apr 25, 2024 · 2 comments
Labels

Comments

@pablosichert
Copy link

pablosichert commented Apr 25, 2024

Describe the Bug

The definitions for WebIDL dictionaries contain information if fields are optional or required by prefixing them with required or marking the type ?. However, there's a third option: fields can be neither required nor ?, but have a default value after =.

Currently, code generation for constructor bindings does not set default values in the body of the new function. Therefore, creating a dictionary with new() and immediately reading a field that should have a default value is undefined behavior.

Steps to Reproduce

E.g. for the first dictionary declaration I could find going alphabetically in the enabled WebIDLs that uses default values:

dictionary AnalyserOptions : AudioNodeOptions {
unsigned long fftSize = 2048;
double maxDecibels = -30;
double minDecibels = -100;
double smoothingTimeConstant = 0.8;
};

use web_sys::{AnalyserOptions, AnalyserOptionsGetters as _};
let analyzer_options = AnalyserOptions::new();
let fft_size = analyzer_options.fft_size(); // Uncaught Error: expected a number argument, found undefined
println!("{}", fft_size);

This example is using the getters as implemented in #3933, but the same case can be constructed by many Web APIs that take a dictionary with properties containing default values as argument.

Expected Behavior

2048

Actual Behavior

imported JS function that was not marked as `catch` threw an error: expected a number argument, found undefined

Additional Context

The faulty generated code for AnalyserOptions::new that does not set default values:

impl AnalyserOptions {
#[doc = "Construct a new `AnalyserOptions`."]
#[doc = ""]
#[doc = "*This API requires the following crate features to be activated: `AnalyserOptions`*"]
pub fn new() -> Self {
#[allow(unused_mut)]
let mut ret: Self = ::wasm_bindgen::JsCast::unchecked_into(::js_sys::Object::new());
ret
}

I discovered this by working on WebIDL dictionary getters in #3933 and thinking about the implications of unsetting dictionary values in #3921 (comment).

Not allowing to safely read properties that are known to exist on a dictionary would require using Option in return values of property getters and require to be unnecessarily defensive when reading them.

@MOZGIII
Copy link

MOZGIII commented Apr 25, 2024

It is not really an undefined behavior - they just create an empty object with no fields assigned. This is valid, and the defaults are applied at the receiver end.

@pablosichert
Copy link
Author

pablosichert commented Apr 25, 2024

@MOZGIII I don't think that's the spirit of the WebIDL dictionary definitions. In that case having the concept of a ? marker would be superfluous since every field is optional.

Also, WebIDL APIs that return dictionaries always have the properties set that are not marked with ?.

Initializing these fields properly would make the consumer side much more streamlined.

If, for some reason, you are not interested in initializing the fields fully, you could still manually call JsCast::unchecked_into<MyDictionary>(Object::new()), but that should not make the consumer side unneccesarily convoluted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants