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

Support default values for WebIDL unions #21342

Closed
ferjm opened this issue Aug 3, 2018 · 3 comments
Closed

Support default values for WebIDL unions #21342

ferjm opened this issue Aug 3, 2018 · 3 comments

Comments

@ferjm
Copy link
Member

@ferjm ferjm commented Aug 3, 2018

Generating bindings for this dictionary https://webaudio.github.io/web-audio-api/#dictdef-audiocontextoptions breaks with:

--- stderr
Traceback (most recent call last):
  File "/Users/ferjm/dev/mozilla/servo/components/script/dom/bindings/codegen/pythonpath.py", line 61, in <module>
    main(sys.argv[1:])
  File "/Users/ferjm/dev/mozilla/servo/components/script/dom/bindings/codegen/pythonpath.py", line 52, in main
    execfile(script, frozenglobals)
  File "/Users/ferjm/dev/mozilla/servo/components/script/dom/bindings/codegen/BindingGen.py", line 54, in <module>
    main()
  File "/Users/ferjm/dev/mozilla/servo/components/script/dom/bindings/codegen/BindingGen.py", line 51, in main
    generate_binding_rs(config, outputPrefix, webIDLFile)
  File "/Users/ferjm/dev/mozilla/servo/components/script/dom/bindings/codegen/BindingGen.py", line 21, in generate_binding_rs
    module = CGBindingRoot(config, outputprefix, webidlfile).define()
  File "/Users/ferjm/dev/mozilla/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 6521, in __init__
    for d in dictionaries])
  File "/Users/ferjm/dev/mozilla/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 6182, in __init__
    for member in dictionary.members]
  File "/Users/ferjm/dev/mozilla/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 755, in getJSToNativeConversionInfo
    default = handleDefaultNull("None")
  File "/Users/ferjm/dev/mozilla/servo/components/script/dom/bindings/codegen/CodegenRust.py", line 677, in handleDefaultNull
    raise TypeError("Can't handle non-null default value here")
TypeError: Can't handle non-null default value here
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Nov 5, 2018

This may have been solved by #22084. Should add a test case in TestBinding.webidl to ensure this works correctly.

@ferjm
Copy link
Member Author

@ferjm ferjm commented Jan 9, 2019

I checked that #22084 does not solve this issue. I am still getting the TypeError: Can't handle non-null or non-empty sequence default value here error.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 9, 2019

Ah, so the default value for that particular field is "interactive". Here's the WebIDL in question:

dictionary AudioContextOptions {
  (AudioContextLatencyCategory or double) latencyHint = "interactive";
  float sampleRate;
};

enum AudioContextLatencyCategory {
    "balanced",
    "interactive",
    "playback"
};

And the codegen pukes at (AudioContextLatencyCategory or double) latencyHint = "interactive";, because "interactive" is not a default value that is null, nor is it an empty sequence (i.e. []).

The complexity in implementing default values in its complete form is that we need to ensure that the default value falls into the set of possible values as described by the field's type during codegen, e.g. in this case, we must ensure that "interactive" is a valid value to the type of (AudioContextLatencyCategory or double) whilst we're generating the rust bindings, and if it does not, we should fail with an appropriate error message during codegen.

@saschanaz saschanaz mentioned this issue Oct 17, 2019
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Oct 17, 2019
Support enum value as a union default value

<!-- Please describe your changes on the following line: -->

Didn't implement the actual latency thing because the relevant things are already marked as TODO.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21342

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Oct 17, 2019
Support enum value as a union default value

<!-- Please describe your changes on the following line: -->

Didn't implement the actual latency thing because the relevant things are already marked as TODO.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21342

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.