Repository navigation
fix(javascript): emit no event operation for a missing listener argument #8663
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
041a506
f1d30fe
4d36250
b6917aa
9617f1a
cb36de8
e9e604a
0bd6c11
c964e98
87b7be8
c9427ea
f038fdc
47e3b9b
ad13261
94dc27a
98316a1
c719fb1
88d5732
514a306
9a7b483
3c3684c
65d74b0
b6cc762
5944b69
676c01d
25cc26b
95cd68c
e118fc9
ba119f3
6b18b70
9b39943
09001af
be61d5c
048274c
486aade
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import { | |
| import { | ||
| resolveSemanticBindingState, | ||
| semanticResolutionBlocked, | ||
| semanticStableInitializer, | ||
| type JavaScriptSemanticAnalysisState, | ||
| type JavaScriptSemanticBindingState, | ||
| } from "./javascriptSemanticState.js"; | ||
|
|
@@ -23,6 +24,7 @@ import { traverseJavaScriptValueAst } from "./javascriptSemanticTraversal.js"; | |
| import { isUnmodifiedGlobalReference } from "./javascriptSemanticAmbientWrites.js"; | ||
| import { range } from "./javascriptStaticAnalysisHelpers.js"; | ||
| import { | ||
| builtinDataEffectCallablePath, | ||
| builtinDataEffectMethod, | ||
| dataEffectArgumentBindingId, | ||
| dataEffectLiteralString, | ||
|
|
@@ -161,6 +163,57 @@ const rejectsEventListener = (listener: t.Node): boolean => { | |
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * Node's `events` module, whose `EventEmitter` validates every listener | ||
| * argument before it registers or removes anything. | ||
| */ | ||
| const NODE_EVENTS_SPECIFIERS = ["events", "node:events"] as const; | ||
|
|
||
| /** | ||
| * Report whether a constructor reference resolves to Node's `EventEmitter`. | ||
| * Named and default imports, destructured requires, and member requires all | ||
| * denote that class. | ||
| */ | ||
| const isNodeEventEmitterConstructor = ( | ||
| node: t.Node, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => { | ||
| const value = unwrapJavaScriptExpression(node).node; | ||
| if (!t.isNewExpression(value)) return false; | ||
| const selected = builtinDataEffectCallablePath( | ||
| value.callee, | ||
| state, | ||
| NODE_EVENTS_SPECIFIERS, | ||
| ); | ||
| // An empty selected path is the module's default export, which `events` | ||
| // defines as `EventEmitter`. | ||
| return ( | ||
| selected !== null && | ||
| (selected.length === 0 || selected.at(-1) === "EventEmitter") | ||
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * Report whether the receiver is a value this module constructs from Node's | ||
| * `EventEmitter`. Only that class is bound by `checkListener`; other | ||
| * EventEmitter-style APIs define one-argument forms, such as Socket.IO's | ||
| * `socket.off("x")`, which removes every listener for the event. A receiver | ||
| * that is not proven this way keeps its operation as an unresolved candidate. | ||
| */ | ||
| const nodeEventEmitterReceiver = ( | ||
| node: t.Expression | t.Super, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => { | ||
| if (!t.isIdentifier(node)) return false; | ||
| const binding = resolveSemanticBindingState(state, node, node.name); | ||
| const initializer = semanticStableInitializer(binding, state); | ||
| return ( | ||
| initializer !== undefined && | ||
| initializer.projection.length === 0 && | ||
| isNodeEventEmitterConstructor(initializer.node, state) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For AGENTS.md reference: AGENTS.md:L21-L21 Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid finding, and it exposes the same mistake in a different place: I proved where the receiver came from and treated that as proof of which definition answers the call.
const e = new EventEmitter();
e.on = (name, fn = () => {}) => EventEmitter.prototype.on.call(e, name, fn);
EventEmitter.prototype.on = function (name, fn = () => {}) { return this; };Both register The write check uses the same Validation: three cases in Separately, this head now merges current |
||
| ); | ||
| }; | ||
|
|
||
| const collectEvent = ( | ||
| node: t.CallExpression | t.OptionalCallExpression, | ||
| ownerCallableId: string | null, | ||
|
|
@@ -177,9 +230,19 @@ const collectEvent = ( | |
| if (method === undefined) return; | ||
| const kind = eventKind(method); | ||
| const eventArgument = node.arguments[0]; | ||
| const listenerArgument = EVENT_LISTENER_METHODS.includes(method) | ||
| ? node.arguments[1] | ||
| : undefined; | ||
| const listenerMethod = EVENT_LISTENER_METHODS.includes(method); | ||
| const listenerArgument = listenerMethod ? node.arguments[1] : undefined; | ||
| // A listener method called without a listener argument passes `undefined` to | ||
| // Node's `checkListener`, which throws before it registers or removes | ||
| // anything. Only a spread argument can still reach that position, and only a | ||
| // receiver proven to be Node's `EventEmitter` is held to that contract. | ||
| if ( | ||
| listenerMethod && | ||
| listenerArgument === undefined && | ||
| !node.arguments.some((argument) => t.isSpreadElement(argument)) && | ||
| nodeEventEmitterReceiver(member.object, state) | ||
| ) | ||
| return; | ||
|
greptile-apps[bot] marked this conversation as resolved.
Resolved
Comment on lines
+682
to
+687
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For an EventEmitter subclass defining AGENTS.md reference: AGENTS.md:L21-L21 Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed on the same root cause as the other thread, and the subclass case is now a regression test. The guard keyed on the property name, so it applied Node's I also kept the uncertainty explicit rather than resolving it toward absence: a receiver that cannot be proven keeps its operation as an unresolved candidate, matching the behavior before this PR. Measured on Node 22.23.3 — the subclass defaults the listener, registers |
||
| // A leading spread shifts the runtime argument positions, so argument 1 is not | ||
| // provably the listener: `e.on(...["x", fn], 1)` registers `fn` and ignores the | ||
| // trailing `1`. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
import A from "events"; import B from "node:events", both bindings reference the same mutable constructor. AfterA.EventEmitter = class extends A { on(name, fn = () => {}) { return super.on(name, fn); } },const e = new B.EventEmitter(); e.on("ready")registers successfully (verified on Node 24.15.0). However,semanticMutableBuiltinOriginexcludeseventsfromMUTABLE_BUILTIN_DEFAULTS, so the write never enters shared module effects and this lookup throughBstill reports["EventEmitter"]. The prototype scan also ignores the constructor-slot write, allowing the new guard to discard the real registration. Register the shared mutable identity at the builtin-mutation owner before using this resolution to prove absence.AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid. Both specifiers resolve to one mutable module at runtime, so the proof cannot be scoped to the binding the construction happens to use.
Writes are now matched by what they name rather than by which binding owns them: any write whose path selects
EventEmitterwithdraws the proof, whichever import performed it, and the module records are read the same way for the inlinerequireform.Validation:
first.EventEmitter = class extends first.EventEmitter { ... }followed bynew second.EventEmitter()keeps the registration. Measured on Node 22.22.2:e.on("ready")does not throw afterwards. Reverting the class-surface check fails the new case.