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

ByteString isn't supported in a union #12527

Closed
jdm opened this issue Jul 20, 2016 · 4 comments
Closed

ByteString isn't supported in a union #12527

jdm opened this issue Jul 20, 2016 · 4 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jul 20, 2016

Blocks #11897.

malisa@malisa-ThinkPad-T450s:~/git_repos/malisa-servo/servo$ ./mach build -d
   Compiling script v0.0.1 (file:///home/malisa/git_repos/malisa-servo/servo/components/script)
/home/malisa/git_repos/malisa-servo/servo/target/debug/build/script-daadca0f01f72a7e/out/UnionTypes.rs:840:32: 840:51 error: wrong number of type arguments: expected 1, found 0 [E0243]
/home/malisa/git_repos/malisa-servo/servo/target/debug/build/script-daadca0f01f72a7e/out/UnionTypes.rs:840     ByteStringSequenceSequence(Vec</*ByteString*/>),
                                                                                                                                          ^~~~~~~~~~~~~~~~~~~
/home/malisa/git_repos/malisa-servo/servo/components/script/dom/bindings/mod.rs:176:9: 176:62 note: in this expansion of include!
/home/malisa/git_repos/malisa-servo/servo/target/debug/build/script-daadca0f01f72a7e/out/UnionTypes.rs:840:32: 840:51 help: run `rustc --explain E0243` to see a detailed explanation
/home/malisa/git_repos/malisa-servo/servo/target/debug/build/script-daadca0f01f72a7e/out/UnionTypes.rs:886:111: 886:130 error: wrong number of type arguments: expected 1, found 0 [E0243]
/home/malisa/git_repos/malisa-servo/servo/target/debug/build/script-daadca0f01f72a7e/out/UnionTypes.rs:886     unsafe fn TryConvertToByteStringSequenceSequence(cx: *mut JSContext, value: HandleValue) -> Result<Option<Vec</*ByteString*/>>, ()> {
                                                                                                                                                                                                                         ^~~~~~~~~~~~~~~~~~~
/home/malisa/git_repos/malisa-servo/servo/components/script/dom/bindings/mod.rs:176:9: 176:62 note: in this expansion of include!
/home/malisa/git_repos/malisa-servo/servo/target/debug/build/script-daadca0f01f72a7e/out/UnionTypes.rs:886:111: 886:130 help: run `rustc --explain E0243` to see a detailed explanation
error: aborting due to 2 previous errors
error: Could not compile `script`.

To learn more, run the command again with --verbose.
Build completed in 0:00:17
@jdm jdm changed the title `sequence<sequence<ByteString>>` isn't supported `sequence<sequence<ByteString>>` isn't supported in a union Jul 20, 2016
@jdm jdm changed the title `sequence<sequence<ByteString>>` isn't supported in a union ByteString isn't supported in a union Jul 20, 2016
@jdm
Copy link
Member Author

@jdm jdm commented Jul 20, 2016

It goes into the else codepath in getUnionTypeTemplateVars that returns the commented-out type name. This should be a straightforward fix to add a branch like the USVString one. We should add a use of this to TestBinding.webidl and testbinding.rs to verify that it does the right thing.

Code: components/script/dom/bindings/codegen/CodegenRust.py

If the changes to TestBinding.webidl and testbinding.rs compile, that's good enough for a PR.

@highfive
Copy link

@highfive highfive commented Jul 20, 2016

@highfive
Copy link

@highfive highfive commented Jul 20, 2016

Please make a comment here if you intend to work on this issue. Thank you!

@malisas
Copy link
Contributor

@malisas malisas commented Jul 20, 2016

I'll work on this.

@jdm jdm added the C-assigned label Jul 20, 2016
@malisas malisas mentioned this issue Jul 21, 2016
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Jul 22, 2016
Union types now allow ByteString

<!-- Please describe your changes on the following line: -->
This PR addresses issue #12527 to allow ByteStrings in union types.
---
<!-- 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 #12527  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12546)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 22, 2016
Union types now allow ByteString

<!-- Please describe your changes on the following line: -->
This PR addresses issue #12527 to allow ByteStrings in union types.
---
<!-- 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 #12527  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12546)
<!-- Reviewable:end -->
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.

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