Issue #10605 generate unions from Typdefs inner type #12397

Scorpil · 2016-07-11T22:41:14Z

#11203 follow-up, rebased to current master.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #10605
There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-07-11T22:41:19Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/codegen/parser/WebIDL.py, components/script/dom/testbinding.rs, components/script/dom/webidls/TestBinding.webidl

highfive · 2016-07-11T22:41:20Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

emilio · 2016-07-11T22:45:02Z

Reviewed 3 of 4 files at r1.
Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/testbinding.rs, line 573 [r1] (raw file):

    fn FuncControlledMethodDisabled(&self) {}
    fn FuncControlledMethodEnabled(&self) {}
    fn DocumentOrTypedef(&self, _: Option<Option<DocumentOrTypedefTest >>) -> () {}

nit: No space before >>, and the return type can be ellided.

Comments from Reviewable

jdm · 2016-07-11T22:49:47Z

@Scorpil Do you use linux? The previous PR stalled after there was a bizarre reproducible build problem only on ubuntu.

Scorpil · 2016-07-11T22:53:10Z

@jdm I'm on mac, but trying to reproduce it in virtualbox. As you can see, that problem is still there, so this PR is still WIP.

emilio · 2016-07-11T22:53:38Z

I can try to reproduce, though I'm not on Ubuntu.

bors-servo · 2016-07-12T12:31:26Z

☔ The latest upstream changes (presumably #12353) made this pull request unmergeable. Please resolve the merge conflicts.

emilio · 2016-07-12T21:10:59Z

Ok, so I was able to reproduce it consistently.

I'm not an expert in how WebIDL.py works, so to fix it I did the obvious thing (#12418), and with that and this patch ~~I was able to build!~~ EDIT: I did something stupid, still doesn't build it seems, debugging...

Scorpil · 2016-07-12T21:40:53Z

@emilio, should i leave this ticket to you? We're doing same stuff in parallel now.

emilio · 2016-07-12T21:41:47Z

@Scorpil: Feel free to investigate, I'm not going to be able to dedicate it a lot of time to be fair.

Scorpil · 2016-07-12T21:42:51Z

@emilio ok, let me know if you have something.

emilio · 2016-07-12T21:53:45Z

I basically know what's happening, what I don't know is how to solve it.

So the reason we have our own UnionTypes.rs file, is because we generate that once all the other interfaces have been processed. The problem with the approach of this patch is that it relies on the order in which the files are being processed, that diverges between Linux and OSX.

On Linux, if you find a Blob field but you've not processed Blob.webidl yet (that's what unresolved type means), when you try to define a typedef for it, the parser bails out. On OSX it seems to be the case that the TestBinding file is processed after it, so it doesn't fail.

So I see two solutions:

Do nothing. Our bindings already do the correct thing, this is only sugar.
Figure out a way of specifying dependencies between WebIDL files, or doing that automatically. Seems doable but I'm not sure it's worth the effort.

Does that make sense?

KiChjang · 2016-07-12T23:06:00Z

Doing nothing is not an option. I don't think typing something like DocumentOrBlobOrBufferSourceOrFormDataOrURLSearchParamsOrUSVString is a good alternative to DocumentOrBodyInit, and it breaks expectations, i.e. when I see a WebIDL Document or BodyInit, I should expect to see a Rust enum of DocumentOrBodyInit, not the whole string of what BodyInit represents.

emilio · 2016-07-12T23:18:50Z

@KiChjang: What about generating a pub use instead (either on the same file, or on the UnionTypes.rs file? That would be easily doable, and solves the naming problem, sort of.

KiChjang · 2016-07-12T23:24:29Z

I believe that was tried before, but I think the problem was that none of the enum variants can be referenced that way, i.e.

pub use AOrBOrCOrD as AOrFoo;

// Does not work
let bar = AOrFoo::B(...);

emilio · 2016-07-12T23:28:30Z

Are you sure about that? I know it doesn't work with type A = B, but this seems to work just fine.

Scorpil · 2016-07-13T09:33:25Z

@emilio , @KiChjang well, we can fix the issue with this easy fix: Scorpil@49cadcd

It's not ideal, as it implicitly enforces file naming convention (if you rename TestBindings.rs to TestBindingsZ.rs it will break again), but it does the job now and will keep feature cross-platform until somebody implements proper file dependency logic.
What do you think?

Here's travis build btw: https://travis-ci.org/Scorpil/servo/jobs/144395789

Ms2ger · 2016-07-13T15:15:54Z

FWIW, distinct webidl files exist only as source code organization. Conceptually, they're all one big file where order does not matter.

Scorpil · 2016-07-13T17:32:59Z

@Ms2ger so we need some kind of "dynamic dispatch" here? I'll see what i can come up with.


        Issue #10605 generate unions from Typdefs inner type


        Sort input filelist to preserve expected definition order.


        Fix UnionType.complete bug.

KiChjang · 2016-07-14T22:27:56Z

r? @Ms2ger

emilio · 2016-07-14T22:29:28Z

The problem with sorting the list IMO is that even more unexpected behavior may appear. For example you can use a union with Blob in the Document idl file, but not the opposite...

emilio · 2016-07-14T22:32:29Z

Hadn't seen the last patch actually, but if you change the webidl parser, can you ensure:

- ./mach test-webidl succeeds
- You write a test for the behavior you're changing?

That would make the attempt clearer.

Scorpil · 2016-07-14T22:32:57Z

It wasn't related to fileorder after all. Well, maybe it was, but that was not the root of the problem. The bug was in 'complete' method for UnionType that:

Didn't enforce TypedefType completeness
Incorrectly evaluated nested union types

Scorpil · 2016-07-14T22:38:47Z

@emilio just tried test-webidl - passed.
You're right, I'll write some tests for this tomorrow or over the weekend.

KiChjang · 2016-07-14T22:52:29Z

Travis is failing:

$ ./mach test-unit

   Compiling util_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/util)

   Compiling profile_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/profile)

   Compiling style_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/style)

   Compiling net_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/net)

   Compiling net_traits_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/net_traits)

   Compiling gfx_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/gfx)

   Compiling script v0.0.1 (file:///home/travis/build/servo/servo/components/script)

   Compiling layout_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/layout)

/home/travis/build/servo/servo/components/script/dom/testbinding.rs:10:60: 10:81 error: unresolved import `dom::bindings::codegen::Bindings::TestBindingBinding::DocumentOrTypedefTest`. There is no `DocumentOrTypedefTest` in `dom::bindings::codegen::Bindings::TestBindingBinding`. Did you mean to use `documentOrTypedef`? [E0432]

/home/travis/build/servo/servo/components/script/dom/testbinding.rs:10 use dom::bindings::codegen::Bindings::TestBindingBinding::{DocumentOrTypedefTest, TestBindingMethods};

                                                                                                                                  ^~~~~~~~~~~~~~~~~~~~~

/home/travis/build/servo/servo/components/script/dom/testbinding.rs:10:60: 10:81 help: run `rustc --explain E0432` to see a detailed explanation

error: aborting due to previous error

error: Could not compile `script`.

To learn more, run the command again with --verbose.

The command "./mach test-unit" exited with 101.

39.96s$ ./mach test-compiletest

   Compiling compiletest_rs v0.1.3

   Compiling script v0.0.1 (file:///home/travis/build/servo/servo/components/script)

   Compiling compiletest_helper v0.0.1 (file:///home/travis/build/servo/servo/tests/compiletest/helper)

/home/travis/build/servo/servo/components/script/dom/testbinding.rs:10:60: 10:81 error: unresolved import `dom::bindings::codegen::Bindings::TestBindingBinding::DocumentOrTypedefTest`. There is no `DocumentOrTypedefTest` in `dom::bindings::codegen::Bindings::TestBindingBinding`. Did you mean to use `documentOrTypedef`? [E0432]

/home/travis/build/servo/servo/components/script/dom/testbinding.rs:10 use dom::bindings::codegen::Bindings::TestBindingBinding::{DocumentOrTypedefTest, TestBindingMethods};

                                                                                                                                  ^~~~~~~~~~~~~~~~~~~~~

/home/travis/build/servo/servo/components/script/dom/testbinding.rs:10:60: 10:81 help: run `rustc --explain E0432` to see a detailed explanation

error: aborting due to previous error

error: Could not compile `script`.

Scorpil · 2016-07-14T22:54:38Z

@KiChjang ugh, just saw it too... This is one strange bug. I'm going to fix it, again, over the next couple of days. You should probably remove awaiting review flair for now.

emilio · 2016-07-14T23:04:56Z

@Scorpil fwiw, is it again a linux-only bug?

Scorpil · 2016-07-14T23:15:11Z

@emilio yep.

bors-servo · 2016-07-21T20:29:34Z

☔ The latest upstream changes (presumably #12541) made this pull request unmergeable. Please resolve the merge conflicts.

KiChjang · 2016-08-01T22:57:59Z

@Scorpil Any updates on this?

Scorpil · 2016-08-03T12:29:46Z

@KiChjang unfortunately not. At this moment I'm actually struggling to compile servo under linux virtual machine. Getting some strange errors in mozjs compilation. Will try to have an actual update by the end of next week. Is it ok or too long, @KiChjang ?

jdm · 2016-08-03T12:38:58Z

You might try running ./mach clean (should only be necessary once) and then ./mach build -j1; those address two of the most common known issues with building mozjs on linux at the moment.

KiChjang · 2016-08-12T23:15:46Z

@Scorpil Are you planning on finishing this?

Scorpil · 2016-08-16T10:51:04Z

@KiChjang I'd like to, but looks like I don't have enough experience with servo to get to the root of this in my free time. Sorry, I'll try to help servo on some other ticket.

KiChjang · 2016-09-06T17:56:59Z

I've rebased this in my branch, and I found out that some new parts of the code was never run at all.

KiChjang · 2016-09-06T17:58:50Z

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

@@ -3860,6 +3878,14 @@ def get_match(name):
        else:
            arrayObject = None

+        typedefMemberTypes = filter(lambda t: isinstance(t, IDLTypedefType), memberTypes)
+        if len(typedefMemberTypes) > 0:


For example. typedefMemberTypes was always null, even if I change isinstance(t, IDLTypedefType) to t.isTypedef(), causing the if len(typedefMemberTypes) > 0: body to never be run.

KiChjang · 2016-09-06T18:09:44Z

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

+    for memberType in t.memberTypes:
+        if isinstance(memberType, IDLTypedefType):
+            return True
+    return False


This function here is pointless because none of the types passed in ever reaches the true branch condition.

KiChjang · 2016-09-06T18:23:06Z

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

@@ -3758,6 +3773,9 @@ def getUnionTypeTemplateVars(type, descriptorProvider):
    elif type.isPrimitive():
        name = type.name
        typeName = builtinNames[type.tag()]
+    elif isinstance(type, IDLTypedefType):
+        name = type.name
+        typeName = type.name


This branch was also never ran. None of the types passed in was ever an IDLTypedef or IDLTypedefType.

KiChjang · 2016-09-06T18:25:53Z

As it currently stands right now, the code in these commits do not work as intended, and as such this isn't valuable in keeping open.

highfive assigned Manishearth Jul 11, 2016

highfive added the S-awaiting-review label Jul 11, 2016

KiChjang mentioned this pull request Jul 11, 2016

generate unions from Typdefs inner type #11203

Closed

4 of 5 tasks complete

highfive added the S-needs-rebase label Jul 12, 2016

amarant and others added 2 commits Apr 14, 2016

Issue #10605 generate unions from Typdefs inner type

9dc4e4e

Sort input filelist to preserve expected definition order.

Loading status checks…

ddcc769

Scorpil force-pushed the Scorpil:typedefs branch from 6709c5b to ddcc769 Jul 14, 2016

Fix UnionType.complete bug.

Loading status checks…

4727eda

KiChjang removed the S-needs-rebase label Jul 14, 2016

highfive unassigned Manishearth Jul 14, 2016

highfive assigned Ms2ger Jul 14, 2016

emilio added S-needs-code-changes and removed S-awaiting-review labels Jul 14, 2016

frewsxcv mentioned this pull request Jul 17, 2016

Implement XMLHttpRequest.send(Document) #10604

Closed

highfive added the S-needs-rebase label Jul 21, 2016

KiChjang added the S-needs-new-owner label Aug 16, 2016

KiChjang mentioned this pull request Aug 30, 2016

Added FormData Iterable #13104

Closed

4 of 4 tasks complete

KiChjang reviewed Sep 6, 2016
View changes

KiChjang closed this Sep 6, 2016

servo / servo

Join GitHub today

Issue #10605 generate unions from Typdefs inner type #12397

Issue #10605 generate unions from Typdefs inner type #12397

Conversation

Scorpil commented Jul 11, 2016 • edited by larsbergstrom

highfive commented Jul 11, 2016

highfive commented Jul 11, 2016

emilio commented Jul 11, 2016

jdm commented Jul 11, 2016

Scorpil commented Jul 11, 2016

emilio commented Jul 11, 2016

bors-servo commented Jul 12, 2016

emilio commented Jul 12, 2016 • edited

Scorpil commented Jul 12, 2016

emilio commented Jul 12, 2016 • edited

Scorpil commented Jul 12, 2016

emilio commented Jul 12, 2016 • edited

KiChjang commented Jul 12, 2016

emilio commented Jul 12, 2016

KiChjang commented Jul 12, 2016

emilio commented Jul 12, 2016

Scorpil commented Jul 13, 2016

Ms2ger commented Jul 13, 2016

Scorpil commented Jul 13, 2016

KiChjang commented Jul 14, 2016

emilio commented Jul 14, 2016 • edited

emilio commented Jul 14, 2016

Scorpil commented Jul 14, 2016 • edited

Scorpil commented Jul 14, 2016

KiChjang commented Jul 14, 2016

Scorpil commented Jul 14, 2016

emilio commented Jul 14, 2016

Scorpil commented Jul 14, 2016

bors-servo commented Jul 21, 2016

KiChjang commented Aug 1, 2016

Scorpil commented Aug 3, 2016

jdm commented Aug 3, 2016

KiChjang commented Aug 12, 2016

Scorpil commented Aug 16, 2016

KiChjang commented Sep 6, 2016

This comment has been minimized.

KiChjang Sep 6, 2016

This comment has been minimized.

KiChjang Sep 6, 2016

This comment has been minimized.

KiChjang Sep 6, 2016 • edited

KiChjang commented Sep 6, 2016

Scorpil commented Jul 11, 2016 •

edited by larsbergstrom

emilio commented Jul 12, 2016 •

edited

emilio commented Jul 12, 2016 •

edited

emilio commented Jul 12, 2016 •

edited

emilio commented Jul 14, 2016 •

edited

Scorpil commented Jul 14, 2016 •

edited

KiChjang Sep 6, 2016 •

edited