From b0302b6d0de9c08f7447945d59d7a202712c964a Mon Sep 17 00:00:00 2001 From: Francois Daoust Date: Tue, 10 Sep 2024 22:38:49 +0200 Subject: [PATCH 1/3] Web IDL study: detect event handlers with no matching even When an interface defines an event handler, there should always be an event named after the event handler that targets the interface. The new `noEvent` anomaly detects situations where the event is missing. The analysis reports the two anomalies mentioned in: https://github.com/w3c/webref/issues/1216#issuecomment-2068997553 (The analysis reports additional anomalies when run on the raw extracts. That's normal and due to missing event targets that get added during curation) --- src/lib/study-webidl.js | 57 ++++++++++++++++++++++--- src/lib/study.js | 5 +++ test/study-webidl.js | 94 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 147 insertions(+), 9 deletions(-) diff --git a/src/lib/study-webidl.js b/src/lib/study-webidl.js index eac282cf..848fdd3d 100644 --- a/src/lib/study-webidl.js +++ b/src/lib/study-webidl.js @@ -20,6 +20,7 @@ */ import * as WebIDL2 from 'webidl2'; +import { getInterfaceTreeInfo } from 'reffy'; const getSpecs = list => [...new Set(list.map(({ spec }) => spec))]; const specName = spec => spec.shortname ?? spec.url; @@ -174,6 +175,15 @@ function studyWebIdl (specs, { crawledResults = [], curatedResults = [] } = {}) const usedTypes = {}; // Index of types used in the IDL const usedExtAttrs = {}; // Index of extended attributes + // We need to run the analysis on all specs, even if caller is only + // interested in a few of them, because types may be defined in specs that + // the caller is not interested in. + const allSpecs = (crawledResults.length > 0) ? crawledResults : specs; + const allEvents = allSpecs + .filter(spec => Array.isArray(spec.events)) + .map(spec => spec.events.map(e => Object.assign({ spec }, e))) + .flat(); + // Record an anomaly for the given spec(s), // provided we are indeed interested in the results function recordAnomaly (spec, name, message) { @@ -193,17 +203,54 @@ function studyWebIdl (specs, { crawledResults = [], curatedResults = [] } = {}) function inheritsFrom (iface, ancestor) { if (!iface.inheritance) return false; if (iface.inheritance === ancestor) return true; + if (!dfns[iface.inheritance]) return false; const parentInterface = dfns[iface.inheritance].find(({ idl }) => !idl.partial)?.idl; if (!parentInterface) return false; return inheritsFrom(parentInterface, ancestor); } + function getBubblingPath(iface) { + const interfaces = Object.values(dfns) + .map(dfn => dfn.find(d => d.idl.type === 'interface')) + .filter(dfn => !!dfn) + .map(dfn => dfn.idl); + const { bubblingPath } = getInterfaceTreeInfo(iface, interfaces); + return bubblingPath; + } + // TODO: a full check of event handlers would require: // - checking if the interface doesn't include a mixin with eventhandlers function checkEventHandlers (spec, memberHolder, iface = memberHolder) { - const eventHandler = memberHolder.members.find(m => m?.name?.startsWith('on') && m.type === 'attribute' && m.idlType?.idlType === 'EventHandler'); - if (eventHandler && !inheritsFrom(iface, 'EventTarget')) { - recordAnomaly(spec, 'unexpectedEventHandler', `The interface \`${iface.name}\` defines an event handler \`${eventHandler.name}\` but does not inherit from \`EventTarget\``); + const eventHandlers = memberHolder.members.filter(m => m?.name?.startsWith('on') && m.type === 'attribute' && m.idlType?.idlType === 'EventHandler'); + if (eventHandlers.length > 0 && !inheritsFrom(iface, 'EventTarget')) { + recordAnomaly(spec, 'unexpectedEventHandler', `The interface \`${iface.name}\` defines an event handler \`${eventHandlers[0].name}\` but does not inherit from \`EventTarget\``); + } + + for (const eventHandler of eventHandlers) { + const eventType = eventHandler.name.substring(2); + const events = allEvents.filter(e => e.type === eventType); + let event = events.find(e => e.targets?.find(target => + target === iface.name)); + if (!event) { + // No event found with the same type that targets the interface + // directly, but one the events may target an interface that + // inherits from the interface, or may bubble on the interface. + event = events.find(e => e.targets?.find(target => { + const inheritanceFound = dfns[target]?.find(dfn => + dfn.idl.type === 'interface' && + inheritsFrom(dfn.idl, iface.name)); + if (inheritanceFound) return true; + if (!e.bubbles) return false; + const bubblingPath = getBubblingPath(target); + if (!bubblingPath) return false; + return bubblingPath.find(bubblingIface => + iface.name === bubblingIface || + inheritsFrom(iface, bubblingIface)); + })); + } + if (!event) { + recordAnomaly(spec, 'noEvent', `The interface \`${iface.name}\` defines an event handler \`${eventHandler.name}\` but no event named "${eventType}" targets it`); + } } } @@ -384,10 +431,6 @@ function studyWebIdl (specs, { crawledResults = [], curatedResults = [] } = {}) } } - // We need to run the analysis on all specs, even if caller is only - // interested in a few of them, because types may be defined in specs that - // the caller is not interested in. - const allSpecs = (crawledResults.length > 0) ? crawledResults : specs; allSpecs // We're only interested in specs that define Web IDL content .filter(spec => !!spec.idl) diff --git a/src/lib/study.js b/src/lib/study.js index ce52bf09..6a856bee 100644 --- a/src/lib/study.js +++ b/src/lib/study.js @@ -149,6 +149,11 @@ const anomalyGroups = [ guidance: 'See the [`[Exposed]`](https://webidl.spec.whatwg.org/#Exposed) extended attribute section in Web IDL for requirements.' }, { name: 'invalid', title: 'Invalid Web IDL' }, + { + name: 'noEvent', + title: 'Suspicious event handlers', + description: 'The following suspicious event handlers were found' + }, { name: 'noExposure', title: 'Missing `[Exposed]` attributes' }, { name: 'noOriginalDefinition', title: 'Missing base interfaces' }, { name: 'overloaded', title: 'Invalid overloaded operations' }, diff --git a/test/study-webidl.js b/test/study-webidl.js index 0d3b3e7c..e2c3fb58 100644 --- a/test/study-webidl.js +++ b/test/study-webidl.js @@ -148,7 +148,7 @@ interface WhereIAm {}; }); }); - it('reports no anomaly for valid EventHandler attributes definitions', () => { + it('reports EventHandler attributes with no matching events', () => { const report = analyzeIdl(` [Exposed=*] interface Event {}; @@ -163,6 +163,34 @@ interface Carlos : EventTarget { [Exposed=*] interface EventTarget {}; `); + assertNbAnomalies(report, 1); + assertAnomaly(report, 0, { + name: 'noEvent', + message: 'The interface `Carlos` defines an event handler `onbigbisous` but no event named "bigbisous" targets it' + }); + }); + + it('reports no anomaly for valid EventHandler attributes definitions', () => { + const crawlResult = toCrawlResult(` +[Exposed=*] +interface Event {}; +[LegacyTreatNonObjectAsNull] +callback EventHandlerNonNull = any (Event event); +typedef EventHandlerNonNull? EventHandler; + +[Global=Window,Exposed=*] +interface Carlos : EventTarget { + attribute EventHandler onbigbisous; +}; +[Exposed=*] +interface EventTarget {}; +`); + crawlResult[0].events = [{ + type: 'bigbisous', + interface: 'Event', + targets: ['Carlos'] + }]; + const report = study(crawlResult); assertNbAnomalies(report, 0); }); @@ -179,11 +207,73 @@ interface Carlos { attribute EventHandler onbigbisous; }; `); - assertNbAnomalies(report, 1); + assertNbAnomalies(report, 2); assertAnomaly(report, 0, { name: 'unexpectedEventHandler', message: 'The interface `Carlos` defines an event handler `onbigbisous` but does not inherit from `EventTarget`' }); + assertAnomaly(report, 1, { name: 'noEvent' }); + }); + + it('follows the inheritance chain to assess event targets', () => { + const crawlResult = toCrawlResult(` +[Exposed=*] +interface Event {}; +[LegacyTreatNonObjectAsNull] +callback EventHandlerNonNull = any (Event event); +typedef EventHandlerNonNull? EventHandler; + +[Global=Window,Exposed=*] +interface Singer : EventTarget { + attribute EventHandler onbigbisous; +}; + +[Global=Window,Exposed=*] +interface Carlos : Singer {}; +[Exposed=*] +interface EventTarget {}; +`); + crawlResult[0].events = [{ + type: 'bigbisous', + interface: 'Event', + targets: [ + 'Carlos' + ] + }]; + const report = study(crawlResult); + assertNbAnomalies(report, 0); + }); + + it('follows the bubbling path to assess event targets', () => { + const crawlResult = toCrawlResult(` +[Exposed=*] +interface Event {}; +[LegacyTreatNonObjectAsNull] +callback EventHandlerNonNull = any (Event event); +typedef EventHandlerNonNull? EventHandler; + +[Global=Window,Exposed=*] +interface IDBDatabase : EventTarget { + attribute EventHandler onabort; +}; +[Global=Window,Exposed=*] +interface IDBTransaction : EventTarget { + attribute EventHandler onabort; +}; +[Global=Window,Exposed=*] +interface MyIndexedDB : IDBDatabase { +}; + +[Exposed=*] +interface EventTarget {};`); + crawlResult[0].events = [{ + type: 'abort', + interface: 'Event', + targets: ['IDBTransaction'], + bubbles: true + }]; + const report = study(crawlResult); + assertNbAnomalies(report, 0); }); it('detects incompatible partial exposure issues', () => { From d9ddf90b81a8664485f63a167290b077ad002bd1 Mon Sep 17 00:00:00 2001 From: Francois Daoust Date: Wed, 11 Sep 2024 12:17:37 +0200 Subject: [PATCH 2/3] Improve noEvent title and description --- src/lib/study.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/study.js b/src/lib/study.js index 6a856bee..3ff86098 100644 --- a/src/lib/study.js +++ b/src/lib/study.js @@ -151,8 +151,8 @@ const anomalyGroups = [ { name: 'invalid', title: 'Invalid Web IDL' }, { name: 'noEvent', - title: 'Suspicious event handlers', - description: 'The following suspicious event handlers were found' + title: 'Event handlers with no matching events', + description: 'No matching events were found for the following event handlers' }, { name: 'noExposure', title: 'Missing `[Exposed]` attributes' }, { name: 'noOriginalDefinition', title: 'Missing base interfaces' }, From 32ac8b1881b9e29bae350f4a676583c304ff5df2 Mon Sep 17 00:00:00 2001 From: Francois Daoust Date: Wed, 11 Sep 2024 12:22:51 +0200 Subject: [PATCH 3/3] Factor our the base IDL needed to define event handlers --- test/study-webidl.js | 70 ++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 51 deletions(-) diff --git a/test/study-webidl.js b/test/study-webidl.js index e2c3fb58..b970bcb6 100644 --- a/test/study-webidl.js +++ b/test/study-webidl.js @@ -7,6 +7,16 @@ import study from '../src/lib/study-webidl.js'; import { assertNbAnomalies, assertAnomaly } from './util.js'; +const baseEventIdl = ` +[Exposed=*] +interface Event {}; +[LegacyTreatNonObjectAsNull] +callback EventHandlerNonNull = any (Event event); +typedef EventHandlerNonNull? EventHandler; +[Exposed=*] +interface EventTarget {}; +`; + describe('The Web IDL analyser', () => { const specUrl = 'https://www.w3.org/TR/spec'; const specUrl2 = 'https://www.w3.org/TR/spec2'; @@ -149,19 +159,11 @@ interface WhereIAm {}; }); it('reports EventHandler attributes with no matching events', () => { - const report = analyzeIdl(` -[Exposed=*] -interface Event {}; -[LegacyTreatNonObjectAsNull] -callback EventHandlerNonNull = any (Event event); -typedef EventHandlerNonNull? EventHandler; - + const report = analyzeIdl(baseEventIdl + ` [Global=Window,Exposed=*] interface Carlos : EventTarget { attribute EventHandler onbigbisous; }; -[Exposed=*] -interface EventTarget {}; `); assertNbAnomalies(report, 1); assertAnomaly(report, 0, { @@ -171,20 +173,11 @@ interface EventTarget {}; }); it('reports no anomaly for valid EventHandler attributes definitions', () => { - const crawlResult = toCrawlResult(` -[Exposed=*] -interface Event {}; -[LegacyTreatNonObjectAsNull] -callback EventHandlerNonNull = any (Event event); -typedef EventHandlerNonNull? EventHandler; - + const crawlResult = toCrawlResult(baseEventIdl +` [Global=Window,Exposed=*] interface Carlos : EventTarget { attribute EventHandler onbigbisous; -}; -[Exposed=*] -interface EventTarget {}; -`); +};`); crawlResult[0].events = [{ type: 'bigbisous', interface: 'Event', @@ -195,18 +188,11 @@ interface EventTarget {}; }); it('detects unexpected EventHandler attributes', () => { - const report = analyzeIdl(` -[Exposed=*] -interface Event {}; -[LegacyTreatNonObjectAsNull] -callback EventHandlerNonNull = any (Event event); -typedef EventHandlerNonNull? EventHandler; - + const report = analyzeIdl(baseEventIdl + ` [Global=Window,Exposed=*] interface Carlos { attribute EventHandler onbigbisous; -}; -`); +};`); assertNbAnomalies(report, 2); assertAnomaly(report, 0, { name: 'unexpectedEventHandler', @@ -216,23 +202,14 @@ interface Carlos { }); it('follows the inheritance chain to assess event targets', () => { - const crawlResult = toCrawlResult(` -[Exposed=*] -interface Event {}; -[LegacyTreatNonObjectAsNull] -callback EventHandlerNonNull = any (Event event); -typedef EventHandlerNonNull? EventHandler; - + const crawlResult = toCrawlResult(baseEventIdl + ` [Global=Window,Exposed=*] interface Singer : EventTarget { attribute EventHandler onbigbisous; }; [Global=Window,Exposed=*] -interface Carlos : Singer {}; -[Exposed=*] -interface EventTarget {}; -`); +interface Carlos : Singer {};`); crawlResult[0].events = [{ type: 'bigbisous', interface: 'Event', @@ -245,13 +222,7 @@ interface EventTarget {}; }); it('follows the bubbling path to assess event targets', () => { - const crawlResult = toCrawlResult(` -[Exposed=*] -interface Event {}; -[LegacyTreatNonObjectAsNull] -callback EventHandlerNonNull = any (Event event); -typedef EventHandlerNonNull? EventHandler; - + const crawlResult = toCrawlResult(baseEventIdl + ` [Global=Window,Exposed=*] interface IDBDatabase : EventTarget { attribute EventHandler onabort; @@ -262,10 +233,7 @@ interface IDBTransaction : EventTarget { }; [Global=Window,Exposed=*] interface MyIndexedDB : IDBDatabase { -}; - -[Exposed=*] -interface EventTarget {};`); +};`); crawlResult[0].events = [{ type: 'abort', interface: 'Event',