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

Check inherited interfaces exist #10240

Merged

Conversation

Projects
None yet
5 participants
@lukebjerring
Copy link
Contributor

lukebjerring commented Mar 29, 2018

Ensures that B exists if you add_[untested_]idl('interface A : B {};')
See https://www.w3.org/TR/WebIDL-1/#idl-interfaces for definition of interface inheritance.

Fixes #10214

Note:
Side-stepping the Cannot read property 'has_extended_attribute' of undefined message at its source would involve non-null checks in several places (e.g. l1318, l1524, ...).
This change is consistent with the other early existence checks, and is a fail-fast (before testing each member), which is likely to change the number of executed tests for any IDL tests which have undefined inherits.


This change is Reviewable

@lukebjerring lukebjerring requested a review from foolip Mar 29, 2018

@wpt-pr-bot wpt-pr-bot added the infra label Mar 29, 2018

@wpt-pr-bot wpt-pr-bot requested review from ayg, gsnedders, jgraham and tobie Mar 29, 2018

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Mar 29, 2018

#9853 is an example of test output which, before this change, wasn't obvious which missing interface caused the problem.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 29, 2018

Build PASSED

Started: 2018-03-29 17:57:53
Finished: 2018-03-29 18:04:33

View more information about this build on:

@foolip

foolip approved these changes Mar 29, 2018

Copy link
Contributor

foolip left a comment

More tests for idlharness.js, yay! LGTM % nit.

@@ -682,6 +683,16 @@ IdlArray.prototype.test = function()
}
this["includes"] = {};

// Assert B defined for A : B
for (var member of Array.from(Object.values(this.members)).filter(m => m.base)) {

This comment has been minimized.

Copy link
@foolip

foolip Mar 29, 2018

Contributor

Object.values already returns a vanilla Array, so the Array.from ought not be needed.

@@ -613,9 +612,11 @@ IdlArray.prototype.assert_throws = function(error, idlArrayFunc)
error = error.message;
}
if (e.message !== error) {
throw new IdlHarnessError(`${idlArrayFunc} threw ${e}, not the expected IdlHarnessError`);
throw new IdlHarnessError(`${idlArrayFunc} threw "${e}", not the expected IdlHarnessError "${error}"`);

This comment has been minimized.

Copy link
@foolip

foolip Mar 29, 2018

Contributor

Can you mention this drive-by fixing in the commit message when merging? Changes themselves look good!

@lukebjerring lukebjerring merged commit 3adbaa2 into web-platform-tests:master Mar 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lukebjerring lukebjerring deleted the lukebjerring:idlharness-base-interfaces branch Mar 29, 2018

@lukebjerring lukebjerring restored the lukebjerring:idlharness-base-interfaces branch Mar 30, 2018

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 30, 2018

lukebjerring added a commit to lukebjerring/wpt that referenced this pull request Mar 30, 2018

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 30, 2018

Since reverting this won't break anything and a fix would require review, I'll go ahead and revert. This would be the "Breakage in browser repos like Chromium which import wpt" case in #9953.

foolip added a commit that referenced this pull request Mar 30, 2018

Revert "Check inherited interfaces exist (#10240)"
This has broken idlharness.js tests with dictionary inheritance:
https://bugs.chromium.org/p/chromium/issues/detail?id=827367

This reverts commit 3adbaa2.

foolip added a commit that referenced this pull request Mar 30, 2018

@inexorabletash

This comment has been minimized.

Copy link
Contributor

inexorabletash commented Mar 30, 2018

+1 to revert.

FWIW, I did some testing with a local fix:

         if (!(lhs in this.members)) throw new IdlHarnessError(`${lhs} inherits ${rhs}, but ${lhs} is undefined.`);
-        if (!(this.members[lhs] instanceof IdlInterface)) throw new IdlHarnessError(`${lhs} inherits ${rhs}, but ${lhs} is not an interface.`);
         if (!(rhs in this.members)) throw new IdlHarnessError(`${lhs} inherits ${rhs}, but ${rhs} is undefined.`);
-        if (!(this.members[rhs] instanceof IdlInterface)) throw new IdlHarnessError(`${lhs} inherits ${rhs}, but ${rhs} is not an interface.`);
+        let lhs_is_interface = this.members[lhs] instanceof IdlInterface;
+        let rhs_is_interface = this.members[rhs] instanceof IdlInterface;
+        if (rhs_is_interface != lhs_is_interface) {
+            if (!lhs_is_interface) throw new IdlHarnessError(`${lhs} inherits ${rhs}, but ${lhs} is not an interface.`);
+            if (!rhs_is_interface) throw new IdlHarnessError(`${lhs} inherits ${rhs}, but ${rhs} is not an interface.`);
+        }

... and there are a handful of interface tests that do have LHS with an undefined RHS (e.g. object "GamepadEventInit inherits EventInit, but EventInit is undefined.") so when a corrected version relands we will still see some real failures. (Which is a good thing!)

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 30, 2018

Landing this change in Chromium and exporting might be easier, tells us right away which tests will be affected.

@lukebjerring

This comment has been minimized.

Copy link
Contributor Author

lukebjerring commented Apr 3, 2018

I'll take the local fix and go via Chromium.

P.S. didn't know github supported diffs. Cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.