Web IDL: Check overloaded operations and redefined members #347
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This completes the Web IDL study tool with checks to report on invalid overloaded operations and duplicated members within interface definitions but also across partials, mixins and partial mixins. Two new types of anomalies get reported: `overloaded` and `redefinedMember`. As opposed to the test logic in Webref, no actual merge is being performed. Definitions are rather compared one by one, which makes it possible to report more fine-grained anomaly messages and in particular target the spec that likely needs fixing. Typically, when a partial dfn introduces a conflict with a base dfn, it seems logic to blame the partial dfn. Similarly, when a dfn introduces a conflict with one of its mixins, it seems logic to blame the dfn, since the dfn includes the mixin and not the other way round. Only cases when multiple specs need to be blamed is when two specs create overloads in partial dfns (no a priori reason to consider that a partial has priority over another partial). Note that the code will create multiple similar anomalies when the same method is overloaded in more than 2 partial dfns and in more than 2 specs. That is, theoretically, report could contain only one anomaly for the overloaded operation, linked to all specs that define the overload, but in practice report will contain one anomaly per pair of specs. That's not wrong per se, just not optimized. Such situations should not happen much in practice, if at all, unless the IDL starts being very clunky. This update also rewrites tests to check on the number of anomalies first and report a dump of the report or anomaly when assertion fails. This is to ease debugging when a test fails.
tidoust
force-pushed
the
study-webidl-overloaded
branch
from
ddd07ea
to
808e79f
Compare
dontcallmedom
requested changes
Apr 14, 2023
| @@ -285,6 +344,87 @@ async function studyWebIdl(edResults, curatedResults) { | |||
| } | |||
| } | |||
|
|
|||
| function checkMembers(target, source) { | |||
| const selfCheck = !source; | |||
| source = source ?? target; | |||
There was a problem hiding this comment.
maybe it would be clearer to have:
source = source ?? target;
const selfCheck = source === target;There was a problem hiding this comment.
or even
function checkMembers(target, source = target) {
const selfCheck = source === target;There was a problem hiding this comment.
I used the first approach. Second approach would not catch calls such as checkMembers(obj, null) as documented on MDN, which I always find confusing.
See #347 (comment) for context Update also improves readability of `selfCheck` and fixes check on mixins' length.
New more specific assert methods defined in particular. One to check the number of anomalies received, and one to check that an anomaly object "matches" the expected anomaly object, in the sense that it contains the expected properties with "matching" values for these properties. The lodash library is no longer needed as a result.
dontcallmedom
pushed a commit
that referenced
this pull request
Apr 14, 2023
See #347 (comment) for context Update also improves readability of `selfCheck` and fixes check on mixins' length.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This completes the Web IDL study tool with checks to report on invalid overloaded operations and duplicated members within interface definitions but also across partials, mixins and partial mixins. Two new types of anomalies get reported:
overloadedandredefinedMember.As opposed to the test logic in Webref, no actual merge is being performed. Definitions are rather compared one by one, which makes it possible to report more fine-grained anomaly messages and in particular to target the spec that likely needs fixing.
Typically, when a partial dfn introduces a conflict with a base dfn, it seems logic to blame the partial dfn. Similarly, when a dfn introduces a conflict with one of its mixins, it seems logic to blame the dfn, since the dfn includes the mixin and not the other way round.
Only cases when multiple specs need to be blamed is when two specs create overloads in partial dfns (no a priori reason to consider that a partial has priority over another partial).
Note that the code will create multiple similar anomalies when the same method is overloaded in more than 2 partial dfns and in more than 2 specs. That is, theoretically, report could contain only one anomaly for the overloaded operation, linked to all specs that define the overload, but in practice report will contain one anomaly per pair of specs. That's not wrong per se, just not optimized. Such situations should not happen much in practice, if at all, unless the IDL starts being very clunky.
This update also rewrites tests to check on the number of anomalies first and report a dump of the report or anomaly when assertion fails. This is to ease debugging when a test fails.
Additional notes:
I haven't tried the code on actual crawl results yet, so I wouldn't be surprised if I missed a few cases here and there. Hopefully, the logic is sound :)Code seems to yield consistent results on Webref non curated data, haven't tried yet with curated data.Code tested on curated Webref data. It reports 3incompatiblePartialIdlExposureanomalies for MSE v2 (known spec issues, base interfaces need to be exposed inDedicatedWorker), which actually should have been detected by Webref's tests... It also reports a couple ofunexpectedEventHandleranomalies (already reported as errors against the specs), and a number of enum anomalies (case or single value) that we will probably want to ignore in Webref.