From dc3af91a07d341f1c4e6cc3b1774daaa03f02e12 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Tue, 22 Sep 2026 17:14:08 +0900 Subject: [PATCH 01/11] Make outbox-listener-delivery-required path-aware The rule decided whether a listener delivers a posted activity by scanning its source as flat text and re-matching names with regexes. That representation could not express whether a callback's result is awaited or returned, whether an identifier is a call or a reference, or which scope a name resolves in, so patching individual false positives and false negatives kept reproducing the same class of bug. Move the decision onto the AST instead: - Keep collecting reachable statements (if/else, try/catch/finally, switch, loops), but fix unconditional exits (e.g. `if (true) return;`) not propagating past a single nesting level. - Resolve a call's callee to the function it actually invokes by following variable bindings, reusing the same resolution already used for the listener argument itself, instead of regex-matching a helper's name. This correctly handles a helper called by reference, held in an object literal, or calling a sibling helper, and stops a member call like someService.deliver() from being confused with an unrelated local `deliver`. - Fold in an anonymous callback's body when the call it's passed to is awaited or returned, and an IIFE's body unconditionally, since both fall out of the same call-resolution step. - Keep the existing text-based pattern matching only for recognizing the delivery method name itself (aliases, destructuring, bracket/template notation), now run over the resolved reachable text. Add regression tests for the false positives and negatives dahlia and CodeRabbit found in the previous approach, update the rule's documentation to describe reachability, and revise the changelog fragment to describe the cases the rule now catches instead of calling this a bug fix, per review. https://github.com/fedify-dev/fedify/issues/900 https://github.com/fedify-dev/fedify/pull/1040#pullrequestreview-5274887580 Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 13 + changes.d/lint/outbox-listener-path-aware.md | 11 + docs/manual/lint.md | 41 +- .../outbox-listener-delivery-required.ts | 427 +++++++++++++- .../outbox-listener-delivery-required.test.ts | 545 ++++++++++++++++++ 5 files changed, 1029 insertions(+), 8 deletions(-) create mode 100644 changes.d/lint/outbox-listener-path-aware.md diff --git a/CHANGES.md b/CHANGES.md index f9df4528f..fad2a6e7b 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -272,7 +272,20 @@ To be released. when an actor dispatcher's return value does not include a `preferredUsername` property. [[#895], [#1022] by Jae-Hyuk-Jang\] + - Changed `outbox-listener-delivery-required` (`@fedify/lint`) to decide + whether a `ctx.sendActivity()`/`forwardActivity()` call actually runs, + instead of scanning the listener's source as a flat block of text. It + now reports a listener whose only delivery call sits behind a dead + branch, after an unconditional `return`/`throw`, or inside a local + helper function that is never actually called. A helper that is + called still counts, however it's referenced — by name, passed by + reference to another function, or reached through a local object + literal — and so does an inline callback whose result is awaited or + returned, such as `await Promise.all(recipients.map(...))`. + [[#900]] + [#895]: https://github.com/fedify-dev/fedify/issues/895 +[#900]: https://github.com/fedify-dev/fedify/issues/900 [#1022]: https://github.com/fedify-dev/fedify/pull/1022 ### @fedify/mysql diff --git a/changes.d/lint/outbox-listener-path-aware.md b/changes.d/lint/outbox-listener-path-aware.md new file mode 100644 index 000000000..0e4af16a1 --- /dev/null +++ b/changes.d/lint/outbox-listener-path-aware.md @@ -0,0 +1,11 @@ + - Changed `outbox-listener-delivery-required` (`@fedify/lint`) to decide + whether a `ctx.sendActivity()`/`forwardActivity()` call actually runs, + instead of scanning the listener's source as a flat block of text. It + now reports a listener whose only delivery call sits behind a dead + branch, after an unconditional `return`/`throw`, or inside a local + helper function that is never actually called. A helper that is + called still counts, however it's referenced — by name, passed by + reference to another function, or reached through a local object + literal — and so does an inline callback whose result is awaited or + returned, such as `await Promise.all(recipients.map(...))`. + [[#900]] diff --git a/docs/manual/lint.md b/docs/manual/lint.md index 4f9184c11..b5d7257e4 100644 --- a/docs/manual/lint.md +++ b/docs/manual/lint.md @@ -752,13 +752,21 @@ Warns when an outbox listener body does not deliver the posted activity with `ctx.sendActivity()` or `ctx.forwardActivity()`. **When this rule applies:** -You've registered an outbox listener with `setOutboxListeners()`, but the -listener body never calls either delivery method. +You've registered an outbox listener with `setOutboxListeners()`, but no +reachable path through the listener body calls either delivery method. The +rule follows the listener's own control flow (`if`/`else`, `try`/`catch`, +`switch`, loops) and resolves calls to local helper functions, so a delivery +call that sits in a dead branch, after an unconditional `return`, or inside a +helper that is declared but never actually called does not count. A helper +that *is* called does count, however it is referenced — by name, passed by +reference to another function, or reached through a local object literal — +and so does an inline callback whose result is awaited or returned, such as +`await Promise.all(recipients.map((r) => ctx.sendActivity(...)))`. **Why it matters:** Fedify does not federate client-to-server outbox posts automatically. If your application intends to deliver a posted activity, the listener must choose an -explicit delivery path. +explicit delivery path, and that path must actually run. ~~~~ typescript twoslash // @noErrors: 2345 @@ -773,6 +781,19 @@ federation console.log(ctx.identifier, activity.id?.href); }); +// ❌ Bad: The delivery call is unreachable dead code +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + if (activity.id == null) return; + return; + await ctx.sendActivity( + { identifier: ctx.identifier }, + "followers", + activity, + ); + }); + // ✅ Good: Listener federates explicitly federation .setOutboxListeners("/users/{identifier}/outbox") @@ -793,6 +814,20 @@ federation "followers", ); }); + +// ✅ Good: Delivery happens inside a helper that is actually called +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + async function deliver() { + await ctx.sendActivity( + { identifier: ctx.identifier }, + "followers", + activity, + ); + } + await deliver(); + }); ~~~~ ### `media-uploader-object-uri-required` diff --git a/packages/lint/src/rules/outbox-listener-delivery-required.ts b/packages/lint/src/rules/outbox-listener-delivery-required.ts index e6305fb57..692744796 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-required.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-required.ts @@ -52,6 +52,15 @@ type FunctionLikeNode = body: unknown; }); +const FUNCTION_NODE_TYPES = new Set([ + "FunctionDeclaration", + "FunctionExpression", + "ArrowFunctionExpression", +]); + +const isFunctionLikeNode = (node: Node): node is FunctionLikeNode => + FUNCTION_NODE_TYPES.has(node.type); + const getMemberPropertyName = (expr: Expression): string | null => { if (expr.type !== "MemberExpression") return null; const property = expr.property as Node; @@ -219,7 +228,15 @@ function buildContextExpressionPattern(contextName: string): string { .raw`(?:${boundedName}|\(\s*${boundedName}(?:\s+as\s+[^)]+)?\s*\))`; } -const resolveListenerReference = ( +/** + * Resolves an expression to the function it refers to: a direct function + * literal, a local variable bound to one, or a property of a local object + * literal bound to one (e.g. `handlers.deliver` where + * `const handlers = { deliver() {} }`). Used both to resolve a listener + * argument (`.on(Activity, handler)`) and to resolve what a call expression + * inside a listener actually invokes. + */ +const resolveFunctionBinding = ( expr: Expression, bindings: Map, seen = new Set(), @@ -237,7 +254,7 @@ const resolveListenerReference = ( return binding as FunctionLikeNode; } if (binding.type === "Identifier") { - return resolveListenerReference(binding, bindings, seen); + return resolveFunctionBinding(binding, bindings, seen); } return null; } @@ -273,11 +290,407 @@ const resolveListenerReference = ( return null; }; +// --------------------------------------------------------------------------- +// Reachability: which statements can actually run, following control flow +// (if/else, try/catch/finally, switch, loops) but never descending into a +// nested function's own body, and pruning dead code (a statically-falsy `if` +// branch, or anything after a statement that always returns/throws). +// --------------------------------------------------------------------------- + +const isStaticallyFalsy = (test: Expression): boolean => + test.type === "Literal" && !test.value; + +const isStaticallyTruthy = (test: Expression): boolean => + test.type === "Literal" && Boolean(test.value); + +/** + * Whether every path through this statement unconditionally returns or + * throws, meaning anything textually after it in the same statement list + * never runs. Deliberately conservative: when it can't prove that, it + * answers `false`, which keeps the following code counted as reachable + * (a missed dead-code case is safer than wrongly hiding live code). + */ +function alwaysExits(node: Node): boolean { + switch (node.type) { + case "ReturnStatement": + case "ThrowStatement": + return true; + + case "BlockStatement": + return node.body.some((statement) => alwaysExits(statement as Node)); + + case "IfStatement": { + const test = node.test as Expression; + if (isStaticallyFalsy(test)) { + return node.alternate != null && alwaysExits(node.alternate as Node); + } + if (isStaticallyTruthy(test)) { + return alwaysExits(node.consequent as Node); + } + if (node.alternate == null) return false; + return alwaysExits(node.consequent as Node) && + alwaysExits(node.alternate as Node); + } + + case "TryStatement": + // A `finally` that always exits dominates the whole statement. Beyond + // that, a `try` block can throw partway through and jump to `catch`, + // so proving more than this would need tracking which statements can + // throw -- stay conservative and say "not sure" instead. + return node.finalizer != null && alwaysExits(node.finalizer as Node); + + default: + return false; + } +} + +function collectReachableStatements(node: Node, out: Node[]): void { + switch (node.type) { + case "BlockStatement": + for (const statement of node.body) { + collectReachableStatements(statement as Node, out); + if (alwaysExits(statement as Node)) return; + } + return; + + case "IfStatement": { + const test = node.test as Expression; + if (!isStaticallyFalsy(test)) { + collectReachableStatements(node.consequent as Node, out); + } + if (node.alternate != null && !isStaticallyTruthy(test)) { + collectReachableStatements(node.alternate as Node, out); + } + return; + } + + case "TryStatement": + collectReachableStatements(node.block as Node, out); + if (node.handler != null) { + collectReachableStatements(node.handler.body as Node, out); + } + if (node.finalizer != null) { + collectReachableStatements(node.finalizer as Node, out); + } + return; + + case "SwitchStatement": + for (const switchCase of node.cases) { + for (const statement of switchCase.consequent) { + collectReachableStatements(statement as Node, out); + if (alwaysExits(statement as Node)) break; + } + } + return; + + case "WhileStatement": + case "DoWhileStatement": + case "ForStatement": + case "ForInStatement": + case "ForOfStatement": + collectReachableStatements(node.body as Node, out); + return; + + case "LabeledStatement": + case "WithStatement": + collectReachableStatements(node.body as Node, out); + return; + + default: + out.push(node); + return; + } +} + +// --------------------------------------------------------------------------- +// What a listener (or a helper's own body) resolves to when scanned for a +// delivery call: three independent mechanisms decide which nested function +// bodies are folded into the scan instead of being masked out. +// --------------------------------------------------------------------------- + +/** + * Collects plain-value references to identifiers: `deliver()`, + * `forEach(deliver)`, a shorthand `{ deliver }`, and so on. Skips positions + * that name something rather than reference a value -- a declaration's own + * `id`/params, and the non-computed `.property` of a member expression (so + * `someService.deliver()` never counts as a reference to an unrelated local + * `deliver`). + */ +function collectReferencedNames(node: unknown, out: Set): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectReferencedNames(item, out); + return; + } + if (!isNode(node)) return; + const n = node; + + if (n.type === "Identifier") { + out.add(n.name); + return; + } + if (n.type === "MemberExpression" && !n.computed) { + collectReferencedNames(n.object, out); + return; + } + if (n.type === "Property" && !n.computed) { + // `{ deliver: fn }` -- the key is a name, not a reference; only the + // value is (for shorthand `{ deliver }`, the value is the same name, + // so this still counts it). + collectReferencedNames(n.value, out); + return; + } + if (n.type === "VariableDeclarator") { + if ((n as VariableDeclarator).init != null) { + collectReferencedNames((n as VariableDeclarator).init, out); + } + return; + } + if (isFunctionLikeNode(n)) { + collectReferencedNames((n as { body: unknown }).body, out); + return; + } + + const record = n as unknown as Record; + for (const key in record) { + if (key === "parent") continue; + collectReferencedNames(record[key], out); + } +} + +/** + * Collects every named local helper in scope: a `function name() {}` + * declaration, or a `const name = function/arrow` binding. Object-literal + * properties are handled separately, through `resolveFunctionBinding`, + * since their name is only meaningful together with the object it lives on. + */ +function collectNamedHelpers( + node: unknown, + out: Map, +): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectNamedHelpers(item, out); + return; + } + if (!isNode(node)) return; + const n = node; + + if (n.type === "FunctionDeclaration") { + if (n.id?.name != null) out.set(n.id.name, n as FunctionLikeNode); + collectNamedHelpers((n as { body: unknown }).body, out); + return; + } + if ( + n.type === "FunctionExpression" || n.type === "ArrowFunctionExpression" + ) { + collectNamedHelpers((n as { body: unknown }).body, out); + return; + } + if (n.type === "VariableDeclarator") { + const decl = n as VariableDeclarator; + if (decl.id.type === "Identifier" && decl.init != null) { + const init = decl.init as Node; + if (isFunctionLikeNode(init)) out.set(decl.id.name, init); + } + if (decl.init != null) collectNamedHelpers(decl.init, out); + return; + } + + const record = n as unknown as Record; + for (const key in record) { + if (key === "parent") continue; + collectNamedHelpers(record[key], out); + } +} + +/** + * Resolves every call expression's callee against `bindings` (a local + * helper, or a property of a local object literal bound to one) and + * collects the functions those calls resolve to. This is also how a + * directly invoked function expression -- `(() => {...})()` -- gets found: + * `resolveFunctionBinding` returns a function literal callee as-is. + */ +function collectResolvedCallTargets( + node: unknown, + bindings: Map, + out: Set, +): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectResolvedCallTargets(item, bindings, out); + return; + } + if (!isNode(node)) return; + const n = node; + + if (n.type === "CallExpression") { + const resolved = resolveFunctionBinding(n.callee as Expression, bindings); + if (resolved != null) out.add(resolved); + } + + const record = n as unknown as Record; + for (const key in record) { + if (key === "parent") continue; + collectResolvedCallTargets(record[key], bindings, out); + } +} + +/** + * Collects anonymous function-literal arguments that are reachable because + * the call chain they are passed to is awaited or returned, e.g. the arrow + * function in `await Promise.all(recipients.map((inbox) => ...))`. Named + * references passed the same way (`recipients.map(deliver)`) don't need + * this: `collectReferencedNames` already finds them regardless of whether + * the result is awaited, matching how `array.forEach(deliver)` always + * invokes `deliver`. + */ +function collectConsumedCallbacks( + statements: readonly Node[], + impliedReturn: boolean, + out: Set, +): void { + const walkConsumed = (expr: unknown): void => { + if (expr == null || typeof expr !== "object" || Array.isArray(expr)) { + return; + } + if (!isNode(expr)) return; + if (isFunctionLikeNode(expr)) { + out.add(expr); + return; + } + if (expr.type === "CallExpression" || expr.type === "NewExpression") { + for (const arg of expr.arguments) walkConsumed(arg); + return; + } + }; + + for (const statement of statements) { + if (statement.type === "ReturnStatement") { + if (statement.argument != null) walkConsumed(statement.argument); + continue; + } + if ( + statement.type === "ExpressionStatement" && + statement.expression.type === "AwaitExpression" + ) { + walkConsumed(statement.expression.argument); + continue; + } + if (statement.type === "VariableDeclaration") { + for (const decl of statement.declarations) { + const init = (decl as VariableDeclarator).init as + | Node + | null + | undefined; + if (init?.type === "AwaitExpression") { + walkConsumed((init as { argument: unknown }).argument); + } + } + } + } + + if (impliedReturn && statements.length === 1) { + const [only] = statements; + if (only.type !== "BlockStatement") walkConsumed(only); + } +} + +/** + * Finds function literals directly nested in a reachable statement, without + * descending past them -- their own reachability is decided separately. + */ +function collectNestedFunctions( + node: unknown, + out: FunctionLikeNode[], +): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectNestedFunctions(item, out); + return; + } + if (!isNode(node)) return; + const n = node; + + if (isFunctionLikeNode(n)) { + out.push(n); + return; + } + + const record = n as unknown as Record; + for (const key in record) { + if (key === "parent") continue; + collectNestedFunctions(record[key], out); + } +} + +/** + * Builds the source text to scan for a delivery call: the reachable + * statements of `root`, with every nested function literal either folded in + * (its own reachable text spliced in place) or blanked out, depending on + * whether `used` says it is actually invoked. + */ +function collectDeliveryScanCode( + sourceCode: { getText(node: unknown): string }, + root: Node, + used: Set, + visited: Set, +): string { + if (visited.has(root)) return ""; + visited.add(root); + + const statements: Node[] = []; + collectReachableStatements(root, statements); + + const consumed = new Set(); + collectConsumedCallbacks( + statements, + root.type !== "BlockStatement", + consumed, + ); + + return statements + .map((statement) => { + let text = sourceCode.getText(statement); + const nested: FunctionLikeNode[] = []; + collectNestedFunctions(statement, nested); + for (const fn of nested) { + const fnText = sourceCode.getText(fn); + const replacement = used.has(fn) || consumed.has(fn) + ? collectDeliveryScanCode(sourceCode, fn.body as Node, used, visited) + : ""; + text = text.split(fnText).join( + replacement.length > 0 ? replacement : "()=>{}", + ); + } + return text; + }) + .join("\n"); +} + const listenerCallsDeliveryMethod = ( sourceCode: { getText(node: unknown): string }, listener: FunctionLikeNode, + bindings: Map, ): boolean => { - const code = stripCommentsAndStrings(sourceCode.getText(listener)); + const usedFunctions = new Set(); + const referencedNames = new Set(); + collectReferencedNames(listener.body, referencedNames); + const namedHelpers = new Map(); + collectNamedHelpers(listener.body, namedHelpers); + for (const [name, fn] of namedHelpers) { + if (referencedNames.has(name)) usedFunctions.add(fn); + } + collectResolvedCallTargets(listener.body, bindings, usedFunctions); + + const code = stripCommentsAndStrings( + collectDeliveryScanCode( + sourceCode, + listener.body as Node, + usedFunctions, + new Set(), + ), + ); const aliases = new Set(); const contextParam = unwrapContextParam( listener.params[0] as Node | undefined, @@ -377,11 +790,15 @@ function createRule( isNode(listener) && isFunction(listener as Expression) ? listener as FunctionLikeNode : isNode(listener) - ? resolveListenerReference(listener as Expression, bindings) + ? resolveFunctionBinding(listener as Expression, bindings) : null; if (resolvedListener == null) return; - if (listenerCallsDeliveryMethod(sourceCode, resolvedListener)) return; + if ( + listenerCallsDeliveryMethod(sourceCode, resolvedListener, bindings) + ) { + return; + } (context as { report: (arg: unknown) => void }).report({ node: resolvedListener, diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index 00533540a..f0cfc7a62 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -223,6 +223,289 @@ fakeFederation }), ); +test( + `${ruleName}: ✅ Good - delivery via a called nested helper`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + async function deliver() { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + await deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery inside a non-literal if branch`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + if (activity.id != null) { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery inside try/catch/finally`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + try { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } catch (error) { + console.error(error); + } finally { + console.log("done"); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery inside a switch case`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + switch (activity.constructor.name) { + case "Create": + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + break; + default: + console.log(ctx.identifier); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery inside a for-of loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + for (const inbox of [new URL("https://example.com/inbox")]) { + await ctx.sendActivity( + { identifier: ctx.identifier }, + inbox, + activity, + ); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited Promise.all(array.map(callback))`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const recipients = [new URL("https://example.com/inbox")]; + await Promise.all(recipients.map((inbox) => + ctx.sendActivity({ identifier: ctx.identifier }, inbox, activity) + )); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - returned Promise.all(array.map(callback))`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, (ctx, activity) => { + const recipients = [new URL("https://example.com/inbox")]; + return Promise.all(recipients.map((inbox) => + ctx.sendActivity({ identifier: ctx.identifier }, inbox, activity) + )); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited immediately invoked function expression`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + await (async () => { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + })(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - named helper passed by reference to forEach`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const deliver = (inbox) => + ctx.sendActivity({ identifier: ctx.identifier }, inbox, activity); + const inboxes = [new URL("https://example.com/inbox")]; + inboxes.forEach(deliver); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - dollar-prefixed helper name`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const $deliver = async () => { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + }; + await $deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - sibling helper calling a sibling helper`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + async function outer() { + await inner(); + } + async function inner() { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + await outer(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper held in an object literal`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const handlers = { + deliver: () => + ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ), + }; + await handlers.deliver(); + }); +`, + rule, + ruleName, + }), +); + test( `${ruleName}: ❌ Bad - missing delivery call`, lintTest({ @@ -427,3 +710,265 @@ federation "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", }), ); + +test( + `${ruleName}: ❌ Bad - unused nested delivery helper`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + async function deliver() { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + console.log(ctx.identifier, activity.id?.href); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery call behind if (false)`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + if (false) { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + console.log(ctx.identifier, activity.id?.href); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery call after unconditional return`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + console.log(ctx.identifier, activity.id?.href); + return; + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery call only inside an unawaited callback`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const recipients = [new URL("https://example.com/inbox")]; + recipients.map((inbox) => + ctx.sendActivity({ identifier: ctx.identifier }, inbox, activity) + ); + console.log(ctx.identifier, activity.id?.href); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery call in the dead branch of if (true)`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + if (true) { + console.log(ctx.identifier, activity.id?.href); + } else { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery call after if (true) return`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + if (true) { + return; + } + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery call after both if/else branches return`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + if (activity.id == null) { + return; + } else { + return; + } + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery call after a return in the same switch case`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + switch (activity.id) { + case null: + return; + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - helper mentioned only in a comment`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + function deliver() { + return ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + // call deliver() later + console.log(ctx.identifier); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - unrelated method sharing a local helper's name`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + function deliver() { + return ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + const someService = { deliver: async () => {} }; + await someService.deliver(); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); From acd31bd2ed6ec2b9360a02bc0b9014cabfeb6595 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Tue, 22 Sep 2026 17:23:19 +0900 Subject: [PATCH 02/11] Add PR credit to lint changelog fragment https://github.com/fedify-dev/fedify/pull/1050 Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 3 ++- changes.d/lint/outbox-listener-path-aware.md | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index fad2a6e7b..ef61a4147 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -282,11 +282,12 @@ To be released. reference to another function, or reached through a local object literal — and so does an inline callback whose result is awaited or returned, such as `await Promise.all(recipients.map(...))`. - [[#900]] + [[#900], [#1050] by Jae-Hyuk-Jang\] [#895]: https://github.com/fedify-dev/fedify/issues/895 [#900]: https://github.com/fedify-dev/fedify/issues/900 [#1022]: https://github.com/fedify-dev/fedify/pull/1022 +[#1050]: https://github.com/fedify-dev/fedify/pull/1050 ### @fedify/mysql diff --git a/changes.d/lint/outbox-listener-path-aware.md b/changes.d/lint/outbox-listener-path-aware.md index 0e4af16a1..e390749da 100644 --- a/changes.d/lint/outbox-listener-path-aware.md +++ b/changes.d/lint/outbox-listener-path-aware.md @@ -1,3 +1,8 @@ +--- +links: + '#1050': https://github.com/fedify-dev/fedify/pull/1050 + '#900': https://github.com/fedify-dev/fedify/issues/900 +--- - Changed `outbox-listener-delivery-required` (`@fedify/lint`) to decide whether a `ctx.sendActivity()`/`forwardActivity()` call actually runs, instead of scanning the listener's source as a flat block of text. It @@ -8,4 +13,4 @@ reference to another function, or reached through a local object literal — and so does an inline callback whose result is awaited or returned, such as `await Promise.all(recipients.map(...))`. - [[#900]] + [[#900], [#1050] by Jae-Hyuk-Jang] From 0f15842d4d3c1bd3df15a5e3577f86150f8d96af Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Tue, 22 Sep 2026 18:52:56 +0900 Subject: [PATCH 03/11] Address CodeRabbit review of the reachability rewrite - Treat break/continue as statement-list exits alongside return/throw, so a delivery call after one of them in the same switch case or loop body is still correctly excluded. - Find an await anywhere in a reachable expression, not just at the top of a bare expression statement or variable initializer, so `result = await Promise.all(...)` is recognized the same as `await Promise.all(...)`. - Rework how "used" functions are determined: instead of computing it once from the whole listener body, walk a worklist starting from the listener's own reachable statements, and only queue a function actually referenced or called from *another* function's reachable statements once that function is itself confirmed reachable. A call sitting in a dead branch, or an unrelated value reference, no longer marks a helper as used. - Resolve a bare identifier call against the current scope's own, correctly shadowed helper map before falling back to the whole-file bindings map, so a call resolves to the helper actually in scope even when another, same-named helper exists elsewhere in the listener. Add regression tests for each case, plus a shadowing test ordered so it only passes with the scope-aware resolution (an earlier version of it happened to pass either way, since the old whole-body scan picked the right function by coincidence of declaration order). https://github.com/fedify-dev/fedify/pull/1050#pullrequestreview-5275817202 Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 3 +- changes.d/lint/outbox-listener-path-aware.md | 3 +- .../outbox-listener-delivery-required.ts | 176 +++++++++++++----- .../outbox-listener-delivery-required.test.ts | 133 +++++++++++++ 4 files changed, 266 insertions(+), 49 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index ef61a4147..324489916 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -273,7 +273,8 @@ To be released. `preferredUsername` property. [[#895], [#1022] by Jae-Hyuk-Jang\] - Changed `outbox-listener-delivery-required` (`@fedify/lint`) to decide - whether a `ctx.sendActivity()`/`forwardActivity()` call actually runs, + whether a `ctx.sendActivity()`/`ctx.forwardActivity()` call actually + runs, instead of scanning the listener's source as a flat block of text. It now reports a listener whose only delivery call sits behind a dead branch, after an unconditional `return`/`throw`, or inside a local diff --git a/changes.d/lint/outbox-listener-path-aware.md b/changes.d/lint/outbox-listener-path-aware.md index e390749da..0228c96c8 100644 --- a/changes.d/lint/outbox-listener-path-aware.md +++ b/changes.d/lint/outbox-listener-path-aware.md @@ -4,7 +4,8 @@ links: '#900': https://github.com/fedify-dev/fedify/issues/900 --- - Changed `outbox-listener-delivery-required` (`@fedify/lint`) to decide - whether a `ctx.sendActivity()`/`forwardActivity()` call actually runs, + whether a `ctx.sendActivity()`/`ctx.forwardActivity()` call actually + runs, instead of scanning the listener's source as a flat block of text. It now reports a listener whose only delivery call sits behind a dead branch, after an unconditional `return`/`throw`, or inside a local diff --git a/packages/lint/src/rules/outbox-listener-delivery-required.ts b/packages/lint/src/rules/outbox-listener-delivery-required.ts index 692744796..b693315b1 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-required.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-required.ts @@ -314,6 +314,8 @@ function alwaysExits(node: Node): boolean { switch (node.type) { case "ReturnStatement": case "ThrowStatement": + case "BreakStatement": + case "ContinueStatement": return true; case "BlockStatement": @@ -447,7 +449,9 @@ function collectReferencedNames(node: unknown, out: Set): void { return; } if (isFunctionLikeNode(n)) { - collectReferencedNames((n as { body: unknown }).body, out); + // Stop at a nested function's own boundary: whether a name it + // references counts is decided separately, only once that function + // itself is found to be reachable. return; } @@ -459,10 +463,15 @@ function collectReferencedNames(node: unknown, out: Set): void { } /** - * Collects every named local helper in scope: a `function name() {}` - * declaration, or a `const name = function/arrow` binding. Object-literal - * properties are handled separately, through `resolveFunctionBinding`, - * since their name is only meaningful together with the object it lives on. + * Collects every named local helper declared directly in `node`'s own + * scope: a `function name() {}` declaration, or a `const name = + * function/arrow` binding. Does not descend into a found function's own + * body -- a helper nested inside another helper is only found once that + * outer helper is itself resolved as reachable, so it can be layered on + * top of (and correctly shadow) the outer scope's helpers of the same + * name. Object-literal properties are handled separately, through + * `resolveFunctionBinding`, since their name is only meaningful together + * with the object it lives on. */ function collectNamedHelpers( node: unknown, @@ -478,13 +487,11 @@ function collectNamedHelpers( if (n.type === "FunctionDeclaration") { if (n.id?.name != null) out.set(n.id.name, n as FunctionLikeNode); - collectNamedHelpers((n as { body: unknown }).body, out); return; } if ( n.type === "FunctionExpression" || n.type === "ArrowFunctionExpression" ) { - collectNamedHelpers((n as { body: unknown }).body, out); return; } if (n.type === "VariableDeclarator") { @@ -493,7 +500,6 @@ function collectNamedHelpers( const init = decl.init as Node; if (isFunctionLikeNode(init)) out.set(decl.id.name, init); } - if (decl.init != null) collectNamedHelpers(decl.init, out); return; } @@ -505,34 +511,49 @@ function collectNamedHelpers( } /** - * Resolves every call expression's callee against `bindings` (a local - * helper, or a property of a local object literal bound to one) and - * collects the functions those calls resolve to. This is also how a - * directly invoked function expression -- `(() => {...})()` -- gets found: - * `resolveFunctionBinding` returns a function literal callee as-is. + * Resolves every call expression's callee and collects the functions those + * calls resolve to. A bare identifier callee is resolved against `helpers` + * first -- the current scope's own, correctly shadowed helper map -- so a + * call to a shadowed name never resolves to some other, same-named + * function declared elsewhere. Anything `helpers` doesn't have (a member + * call such as `handlers.deliver()`, or a name that isn't a local helper + * at all) falls back to `bindings`, the whole-file map used to resolve a + * local object literal's properties. This is also how a directly invoked + * function expression -- `(() => {...})()` -- gets found: + * `resolveFunctionBinding` returns a function literal callee as-is. Stops + * at a nested function's own boundary, so a call that only happens inside + * some other, not-yet-reachable function doesn't count here -- it is + * found on its own once that function is resolved as reachable. */ function collectResolvedCallTargets( node: unknown, + helpers: ReadonlyMap, bindings: Map, out: Set, ): void { if (node == null || typeof node !== "object") return; if (Array.isArray(node)) { - for (const item of node) collectResolvedCallTargets(item, bindings, out); + for (const item of node) { + collectResolvedCallTargets(item, helpers, bindings, out); + } return; } if (!isNode(node)) return; const n = node; + if (isFunctionLikeNode(n)) return; if (n.type === "CallExpression") { - const resolved = resolveFunctionBinding(n.callee as Expression, bindings); + const callee = n.callee as Expression; + const resolved = callee.type === "Identifier" && helpers.has(callee.name) + ? helpers.get(callee.name)! + : resolveFunctionBinding(callee, bindings); if (resolved != null) out.add(resolved); } const record = n as unknown as Record; for (const key in record) { if (key === "parent") continue; - collectResolvedCallTargets(record[key], bindings, out); + collectResolvedCallTargets(record[key], helpers, bindings, out); } } @@ -565,16 +586,35 @@ function collectConsumedCallbacks( } }; + // Finds an `await` anywhere in `expr` -- not just at its top level, so + // `result = await Promise.all(...)` and similar wrapping still count -- + // and feeds what it awaits into `walkConsumed`. Stops at a nested + // function's own boundary. + const walkAwaitExpressions = (expr: unknown): void => { + if (expr == null || typeof expr !== "object") return; + if (Array.isArray(expr)) { + for (const item of expr) walkAwaitExpressions(item); + return; + } + if (!isNode(expr) || isFunctionLikeNode(expr)) return; + if (expr.type === "AwaitExpression") { + walkConsumed((expr as { argument: unknown }).argument); + return; + } + const record = expr as unknown as Record; + for (const key in record) { + if (key !== "parent") walkAwaitExpressions(record[key]); + } + }; + for (const statement of statements) { if (statement.type === "ReturnStatement") { + // A return value is consumed by definition -- no `await` needed. if (statement.argument != null) walkConsumed(statement.argument); continue; } - if ( - statement.type === "ExpressionStatement" && - statement.expression.type === "AwaitExpression" - ) { - walkConsumed(statement.expression.argument); + if (statement.type === "ExpressionStatement") { + walkAwaitExpressions(statement.expression); continue; } if (statement.type === "VariableDeclaration") { @@ -583,9 +623,7 @@ function collectConsumedCallbacks( | Node | null | undefined; - if (init?.type === "AwaitExpression") { - walkConsumed((init as { argument: unknown }).argument); - } + if (init != null) walkAwaitExpressions(init); } } } @@ -624,16 +662,76 @@ function collectNestedFunctions( } } +/** + * Computes the full set of function nodes that are actually reachable from + * `root`: `root` itself feeds a worklist, and each function it (or a + * function already on the worklist) references from *its own* reachable + * statements -- never from a dead branch or some other not-yet-reached + * function's body -- gets queued in turn. `outerHelpers` is layered fresh + * for each scope, so a helper declared at an inner scope shadows a + * same-named one further out instead of overwriting it globally, and a + * dead branch that merely mentions a helper's name never queues it. + */ +function computeUsedFunctions( + root: Node, + bindings: Map, +): Set { + const used = new Set(); + const visited = new Set(); + + const processScope = ( + scopeRoot: Node, + outerHelpers: ReadonlyMap, + ): void => { + if (visited.has(scopeRoot)) return; + visited.add(scopeRoot); + + const statements: Node[] = []; + collectReachableStatements(scopeRoot, statements); + + const helpers = new Map(outerHelpers); + for (const statement of statements) { + collectNamedHelpers(statement, helpers); + } + + const referencedNames = new Set(); + for (const statement of statements) { + collectReferencedNames(statement, referencedNames); + } + const reached = new Set(); + for (const [name, fn] of helpers) { + if (referencedNames.has(name)) reached.add(fn); + } + for (const statement of statements) { + collectResolvedCallTargets(statement, helpers, bindings, reached); + } + collectConsumedCallbacks( + statements, + scopeRoot.type !== "BlockStatement", + reached, + ); + + for (const fn of reached) { + used.add(fn); + processScope(fn.body as Node, helpers); + } + }; + + processScope(root, new Map()); + return used; +} + /** * Builds the source text to scan for a delivery call: the reachable - * statements of `root`, with every nested function literal either folded in - * (its own reachable text spliced in place) or blanked out, depending on - * whether `used` says it is actually invoked. + * statements of `root`, with every nested function literal either folded + * in (its own reachable text spliced in place, wherever that function's + * own declaration happens to live) or blanked out, depending on whether + * `used` (from `computeUsedFunctions`) says it is actually invoked. */ function collectDeliveryScanCode( sourceCode: { getText(node: unknown): string }, root: Node, - used: Set, + used: ReadonlySet, visited: Set, ): string { if (visited.has(root)) return ""; @@ -642,13 +740,6 @@ function collectDeliveryScanCode( const statements: Node[] = []; collectReachableStatements(root, statements); - const consumed = new Set(); - collectConsumedCallbacks( - statements, - root.type !== "BlockStatement", - consumed, - ); - return statements .map((statement) => { let text = sourceCode.getText(statement); @@ -656,7 +747,7 @@ function collectDeliveryScanCode( collectNestedFunctions(statement, nested); for (const fn of nested) { const fnText = sourceCode.getText(fn); - const replacement = used.has(fn) || consumed.has(fn) + const replacement = used.has(fn) ? collectDeliveryScanCode(sourceCode, fn.body as Node, used, visited) : ""; text = text.split(fnText).join( @@ -673,21 +764,12 @@ const listenerCallsDeliveryMethod = ( listener: FunctionLikeNode, bindings: Map, ): boolean => { - const usedFunctions = new Set(); - const referencedNames = new Set(); - collectReferencedNames(listener.body, referencedNames); - const namedHelpers = new Map(); - collectNamedHelpers(listener.body, namedHelpers); - for (const [name, fn] of namedHelpers) { - if (referencedNames.has(name)) usedFunctions.add(fn); - } - collectResolvedCallTargets(listener.body, bindings, usedFunctions); - + const used = computeUsedFunctions(listener.body as Node, bindings); const code = stripCommentsAndStrings( collectDeliveryScanCode( sourceCode, listener.body as Node, - usedFunctions, + used, new Set(), ), ); diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index f0cfc7a62..6a6632712 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -972,3 +972,136 @@ federation "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", }), ); + +test( + `${ruleName}: ❌ Bad - delivery call after break in a switch case`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + switch (activity.constructor.name) { + case "Create": + break; + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery call after continue in a loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + for (const inbox of [new URL("https://example.com/inbox")]) { + continue; + await ctx.sendActivity( + { identifier: ctx.identifier }, + inbox, + activity, + ); + } + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ✅ Good - awaited Promise.all assigned to a variable`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const recipients = [new URL("https://example.com/inbox")]; + let result; + result = await Promise.all(recipients.map((inbox) => + ctx.sendActivity({ identifier: ctx.identifier }, inbox, activity) + )); + return result; + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad - helper only called from a dead branch`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + function deliver() { + return ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + if (false) { + deliver(); + } + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ✅ Good - inner helper shadows a same-named outer helper`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + async function outer() { + function deliver() { + return ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); + } + await deliver(); + } + function deliver() { + console.log("outer deliver never actually delivers"); + } + await outer(); + }); +`, + rule, + ruleName, + }), +); From 16fd5ff5c1c78bbcf2d69be95484af3e3e14d9e4 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Wed, 23 Sep 2026 09:56:56 +0900 Subject: [PATCH 04/11] Reword around spaced em dashes in the lint docs and changelog CONTRIBUTING.md asks for an em dash without surrounding spaces in narrative text, and prefers avoiding them where a comma, colon, or a reworded sentence will do. Replace the em-dash-delimited aside in both the changelog fragment and the rule's manual entry with a colon-led list instead. https://github.com/fedify-dev/fedify/pull/1050#discussion_r4074433431 Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 8 ++++---- changes.d/lint/outbox-listener-path-aware.md | 8 ++++---- docs/manual/lint.md | 7 ++++--- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 324489916..37232f4d4 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -279,10 +279,10 @@ To be released. now reports a listener whose only delivery call sits behind a dead branch, after an unconditional `return`/`throw`, or inside a local helper function that is never actually called. A helper that is - called still counts, however it's referenced — by name, passed by - reference to another function, or reached through a local object - literal — and so does an inline callback whose result is awaited or - returned, such as `await Promise.all(recipients.map(...))`. + called still counts, regardless of how it's referenced: by name, + passed by reference to another function, or reached through a local + object literal. An inline callback whose result is awaited or + returned counts too, such as `await Promise.all(recipients.map(...))`. [[#900], [#1050] by Jae-Hyuk-Jang\] [#895]: https://github.com/fedify-dev/fedify/issues/895 diff --git a/changes.d/lint/outbox-listener-path-aware.md b/changes.d/lint/outbox-listener-path-aware.md index 0228c96c8..e3674f840 100644 --- a/changes.d/lint/outbox-listener-path-aware.md +++ b/changes.d/lint/outbox-listener-path-aware.md @@ -10,8 +10,8 @@ links: now reports a listener whose only delivery call sits behind a dead branch, after an unconditional `return`/`throw`, or inside a local helper function that is never actually called. A helper that is - called still counts, however it's referenced — by name, passed by - reference to another function, or reached through a local object - literal — and so does an inline callback whose result is awaited or - returned, such as `await Promise.all(recipients.map(...))`. + called still counts, regardless of how it's referenced: by name, + passed by reference to another function, or reached through a local + object literal. An inline callback whose result is awaited or + returned counts too, such as `await Promise.all(recipients.map(...))`. [[#900], [#1050] by Jae-Hyuk-Jang] diff --git a/docs/manual/lint.md b/docs/manual/lint.md index b5d7257e4..c5e097316 100644 --- a/docs/manual/lint.md +++ b/docs/manual/lint.md @@ -758,9 +758,10 @@ rule follows the listener's own control flow (`if`/`else`, `try`/`catch`, `switch`, loops) and resolves calls to local helper functions, so a delivery call that sits in a dead branch, after an unconditional `return`, or inside a helper that is declared but never actually called does not count. A helper -that *is* called does count, however it is referenced — by name, passed by -reference to another function, or reached through a local object literal — -and so does an inline callback whose result is awaited or returned, such as +that *is* called does count, regardless of how it is referenced: by name, +passed by reference to another function, or reached through a local object +literal. An inline callback whose result is awaited or returned counts +too, such as `await Promise.all(recipients.map((r) => ctx.sendActivity(...)))`. **Why it matters:** From 1a60aabe8c94543570802e4b0d18e4052fcb357f Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Wed, 23 Sep 2026 11:21:37 +0900 Subject: [PATCH 05/11] Replace text-matching splices with range-based ones text.split(fnText).join(...) matched by string content across the whole statement, so two function literals with byte-identical source text collided: replacing the first blanked out the second's text too, leaving nothing for the second replacement to find. Splice by each function's own range instead, applied from the end of the statement backward so an earlier replacement's length change never shifts a later one's still-unprocessed offset. Assisted-by: Claude Code:claude-sonnet-5 --- .../outbox-listener-delivery-required.ts | 43 +++++++++++++--- .../outbox-listener-delivery-required.test.ts | 50 +++++++++++++++++++ 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/packages/lint/src/rules/outbox-listener-delivery-required.ts b/packages/lint/src/rules/outbox-listener-delivery-required.ts index b693315b1..ca6ba0acc 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-required.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-required.ts @@ -721,12 +721,34 @@ function computeUsedFunctions( return used; } +/** + * A node's `[start, end)` character offsets into the whole source file. + * Both engines always populate this -- ESLint forces it on regardless of + * parser options, and Deno.lint exposes it the same way as every other + * child property (see the `for...in` note on why plain property access + * still works even though it's not an own enumerable property). + */ +function getRange(node: Node): readonly [number, number] { + return (node as unknown as { range: [number, number] }).range; +} + /** * Builds the source text to scan for a delivery call: the reachable * statements of `root`, with every nested function literal either folded * in (its own reachable text spliced in place, wherever that function's * own declaration happens to live) or blanked out, depending on whether * `used` (from `computeUsedFunctions`) says it is actually invoked. + * + * Splices each function by its own range rather than by matching its + * source text, and applies the splices from the end of the statement + * backward. That keeps two functions with byte-identical bodies (e.g. two + * object-literal methods that both merely call `ctx.sendActivity(...)`) + * from colliding: a text-based replacement would find and blank out both + * occurrences the first time either one is processed, since it matches by + * content everywhere in the statement rather than by which node is + * actually being replaced. Replacing from the end backward also means a + * later replacement's length change never shifts the still-unprocessed + * offsets of an earlier one. */ function collectDeliveryScanCode( sourceCode: { getText(node: unknown): string }, @@ -742,19 +764,26 @@ function collectDeliveryScanCode( return statements .map((statement) => { - let text = sourceCode.getText(statement); + const text = sourceCode.getText(statement); + const [statementStart] = getRange(statement); + const nested: FunctionLikeNode[] = []; collectNestedFunctions(statement, nested); - for (const fn of nested) { - const fnText = sourceCode.getText(fn); + const byDescendingStart = [...nested].sort((a, b) => + getRange(b)[0] - getRange(a)[0] + ); + + let result = text; + for (const fn of byDescendingStart) { + const [fnStart, fnEnd] = getRange(fn); const replacement = used.has(fn) ? collectDeliveryScanCode(sourceCode, fn.body as Node, used, visited) : ""; - text = text.split(fnText).join( - replacement.length > 0 ? replacement : "()=>{}", - ); + result = result.slice(0, fnStart - statementStart) + + (replacement.length > 0 ? replacement : "()=>{}") + + result.slice(fnEnd - statementStart); } - return text; + return result; }) .join("\n"); } diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index 6a6632712..65dd1ee67 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -1105,3 +1105,53 @@ federation ruleName, }), ); + +test( + `${ruleName}: ✅ Good - used helper has byte-identical text to an unused one`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const inbox = new URL("https://example.com/inbox"); + const handlers = { + unused: () => + ctx.sendActivity({ identifier: ctx.identifier }, inbox, activity), + used: () => + ctx.sendActivity({ identifier: ctx.identifier }, inbox, activity), + }; + await handlers.used(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad - unused helper has byte-identical text to a used one`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const inbox = new URL("https://example.com/inbox"); + const handlers = { + used: () => + ctx.sendActivity({ identifier: ctx.identifier }, inbox, activity), + unused: () => + ctx.sendActivity({ identifier: ctx.identifier }, inbox, activity), + }; + console.log("never actually calls a handler"); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); From e4c56f62ca2683b6e0c1be3243ef852fac6f7f6c Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Wed, 23 Sep 2026 14:27:24 +0900 Subject: [PATCH 06/11] Recognize more ways a callback gets consumed walkConsumed() only expanded through a call's own arguments, so a callback wrapped in an array literal, a spread, an object literal, or a chained call's receiver before reaching the awaited/returned expression was missed, e.g. await Promise.all([...a.map(cb), ...b.map(cb)]). Teach it to expand through those shapes too. forEach() always invokes its callback synchronously and never returns anything worth awaiting, so gating its callback behind an await/return check (as ExpressionStatement handling did) reported a listener that plainly delivers. Track forEach() separately from the await/return check; map()/filter()/etc. keep needing one, since their return value usually is meant to be consumed. Also comment the intentional false negative where merely mentioning a helper's name counts as using it, so it doesn't get "fixed" later and break passing a helper by reference. Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 6 +- changes.d/lint/outbox-listener-path-aware.md | 6 +- .../outbox-listener-delivery-required.ts | 111 ++++++++++++++++-- .../outbox-listener-delivery-required.test.ts | 81 +++++++++++++ 4 files changed, 195 insertions(+), 9 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 37232f4d4..a99125f69 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -282,7 +282,11 @@ To be released. called still counts, regardless of how it's referenced: by name, passed by reference to another function, or reached through a local object literal. An inline callback whose result is awaited or - returned counts too, such as `await Promise.all(recipients.map(...))`. + returned counts too, including one nested inside an array, a spread, + or an object literal, such as + `await Promise.all([...a.map(...), ...b.map(...)])`. A callback + passed to `forEach()` counts as well, since it always runs + synchronously regardless of what the call returns. [[#900], [#1050] by Jae-Hyuk-Jang\] [#895]: https://github.com/fedify-dev/fedify/issues/895 diff --git a/changes.d/lint/outbox-listener-path-aware.md b/changes.d/lint/outbox-listener-path-aware.md index e3674f840..f65c031a0 100644 --- a/changes.d/lint/outbox-listener-path-aware.md +++ b/changes.d/lint/outbox-listener-path-aware.md @@ -13,5 +13,9 @@ links: called still counts, regardless of how it's referenced: by name, passed by reference to another function, or reached through a local object literal. An inline callback whose result is awaited or - returned counts too, such as `await Promise.all(recipients.map(...))`. + returned counts too, including one nested inside an array, a spread, + or an object literal, such as + `await Promise.all([...a.map(...), ...b.map(...)])`. A callback + passed to `forEach()` counts as well, since it always runs + synchronously regardless of what the call returns. [[#900], [#1050] by Jae-Hyuk-Jang] diff --git a/packages/lint/src/rules/outbox-listener-delivery-required.ts b/packages/lint/src/rules/outbox-listener-delivery-required.ts index ca6ba0acc..328c96f50 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-required.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-required.ts @@ -43,6 +43,21 @@ const isChainedFromOutboxListeners = ( const DELIVERY_METHOD_NAMES = new Set(["sendActivity", "forwardActivity"]); +/** + * Iteration methods whose own return value is never meant to be consumed, + * so nothing is ever "forgotten" by not awaiting or collecting it -- the + * language spec guarantees each invokes its callback argument synchronously + * for every element regardless. `forEach` always returns `undefined`; a + * callback passed to it has run by the time the call completes whether or + * not anything looks at what the call returns. This is deliberately + * narrower than the full iteration protocol: `map`, `filter` and the rest + * return something that usually *is* meant to be consumed (an array of + * promises passed to `Promise.all`, for instance), so calling one of those + * without awaiting or returning the result stays a real thing to flag, not + * a rule limitation to work around. + */ +const SYNCHRONOUS_ITERATION_METHODS = new Set(["forEach"]); + type FunctionLikeNode = | FunctionNode | (Node & { @@ -558,19 +573,29 @@ function collectResolvedCallTargets( } /** - * Collects anonymous function-literal arguments that are reachable because - * the call chain they are passed to is awaited or returned, e.g. the arrow - * function in `await Promise.all(recipients.map((inbox) => ...))`. Named - * references passed the same way (`recipients.map(deliver)`) don't need - * this: `collectReferencedNames` already finds them regardless of whether - * the result is awaited, matching how `array.forEach(deliver)` always - * invokes `deliver`. + * Collects anonymous function-literal arguments that are reachable through + * either of two paths: the call chain they are passed to is awaited or + * returned (e.g. the arrow function in + * `await Promise.all(recipients.map((inbox) => ...))`, including when a + * collection literal sits between the call and the `await`, as in + * `await Promise.all([...a.map(cb), ...b.map(cb)])`), or the call + * receiving them is a method known to invoke its callback synchronously + * regardless of what happens to its own return value (`SYNCHRONOUS_ITERATION_METHODS`, + * e.g. `recipients.forEach((inbox) => ...)`). Named references passed + * either way (`recipients.map(deliver)`) don't need this: + * `collectReferencedNames` already finds them regardless of context. */ function collectConsumedCallbacks( statements: readonly Node[], impliedReturn: boolean, out: Set, ): void { + // Expands outward from a value known to be consumed (awaited, returned, + // or the argument of a synchronously invoked method) through the shapes + // that merely carry it along -- a call's own arguments, and the + // collection literals (`[...]`, `...spread`, `{...}`) commonly used to + // gather several such values before consuming them together -- until it + // finds the function literals actually being passed. const walkConsumed = (expr: unknown): void => { if (expr == null || typeof expr !== "object" || Array.isArray(expr)) { return; @@ -582,6 +607,37 @@ function collectConsumedCallbacks( } if (expr.type === "CallExpression" || expr.type === "NewExpression") { for (const arg of expr.arguments) walkConsumed(arg); + // A chained call's receiver carries the same value forward, e.g. + // `[a.map(cb)].flat()`: the array literal built from `a.map(cb)` is + // what `flat()` is called on, so it's still part of what ends up + // awaited or returned. + if ( + expr.type === "CallExpression" && + expr.callee.type === "MemberExpression" + ) { + walkConsumed(expr.callee.object); + } + return; + } + if (expr.type === "ArrayExpression") { + for (const element of expr.elements) { + if (element != null) walkConsumed(element); + } + return; + } + if (expr.type === "SpreadElement") { + walkConsumed((expr as { argument: unknown }).argument); + return; + } + if (expr.type === "ObjectExpression") { + for (const prop of expr.properties) { + if ( + isNode(prop) && prop.type === "Property" && + !(prop as { computed?: boolean }).computed + ) { + walkConsumed((prop as { value: unknown }).value); + } + } return; } }; @@ -607,7 +663,38 @@ function collectConsumedCallbacks( } }; + // Finds every call to a `SYNCHRONOUS_ITERATION_METHODS` method anywhere + // in `node` and feeds its arguments to `walkConsumed`, independent of + // whether the call's own result is ever awaited, returned, or used at + // all. Stops at a nested function's own boundary. + const walkSynchronousCallbacks = (node: unknown): void => { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) walkSynchronousCallbacks(item); + return; + } + if (!isNode(node) || isFunctionLikeNode(node)) return; + + if (node.type === "CallExpression") { + const callee = node.callee as Expression; + if (callee.type === "MemberExpression" && !callee.computed) { + const methodName = getMemberPropertyName(callee); + if ( + methodName != null && SYNCHRONOUS_ITERATION_METHODS.has(methodName) + ) { + for (const arg of node.arguments) walkConsumed(arg); + } + } + } + + const record = node as unknown as Record; + for (const key in record) { + if (key !== "parent") walkSynchronousCallbacks(record[key]); + } + }; + for (const statement of statements) { + walkSynchronousCallbacks(statement); if (statement.type === "ReturnStatement") { // A return value is consumed by definition -- no `await` needed. if (statement.argument != null) walkConsumed(statement.argument); @@ -698,6 +785,16 @@ function computeUsedFunctions( for (const statement of statements) { collectReferencedNames(statement, referencedNames); } + // A helper counts as reached as soon as its name is mentioned at all -- + // passed to `console.log`, stored in a variable, anything -- not only + // when it's actually invoked. That's what lets `recipients.map(deliver)` + // resolve `deliver` as used without this code having to know that `map` + // invokes its argument; telling a real invocation apart from merely + // holding a reference would need knowing which APIs call what they're + // given, which is more than this rule should carry. The cost is a + // narrow false negative -- a helper that's only logged or reassigned, + // never called, is not reported -- accepted deliberately, since missing + // a case here is the safe direction. Leave this as is. const reached = new Set(); for (const [name, fn] of helpers) { if (referencedNames.has(name)) reached.add(fn); diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index 65dd1ee67..6683f4616 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -1155,3 +1155,84 @@ federation "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", }), ); + +test( + `${ruleName}: ✅ Good - awaited callback nested inside an array literal and spreads`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + await Promise.all([ + ...inboxes.map((inbox) => ctx.sendActivity(sender, inbox, activity)), + ...others.map((inbox) => ctx.sendActivity(sender, inbox, activity)), + ]); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited callback nested inside an array literal and a chained call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + await Promise.all( + [inboxes.map((inbox) => ctx.sendActivity(sender, inbox, activity))] + .flat(), + ); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - callback passed to a bare forEach that is never awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + inboxes.forEach((inbox) => ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad - callback passed to a bare forEach that never delivers`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + inboxes.forEach((inbox) => { + console.log(inbox); + }); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); From 5aecfa949795c8d488629bff0d0e0ae770aace87 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Wed, 23 Sep 2026 17:03:57 +0900 Subject: [PATCH 07/11] Add a regression test for the object-literal case walkConsumed() already handles a callback nested inside an ObjectExpression, added alongside the array/spread fix, but nothing exercised that branch. Add a test, and confirm it fails when the branch is disabled. Assisted-by: Claude Code:claude-sonnet-5 --- .../outbox-listener-delivery-required.test.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index 6683f4616..759889fc5 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -1198,6 +1198,28 @@ federation }), ); +test( + `${ruleName}: ✅ Good - awaited callback nested inside an object literal property`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + await Promise.all( + Object.values({ + a: inboxes.map((inbox) => ctx.sendActivity(sender, inbox, activity)), + }), + ); + }); +`, + rule, + ruleName, + }), +); + test( `${ruleName}: ✅ Good - callback passed to a bare forEach that is never awaited`, lintTest({ From 82f2b5da957270341c39cb704e2741172086c94f Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Wed, 23 Sep 2026 17:32:02 +0900 Subject: [PATCH 08/11] Fix the object-literal test to actually await its promises Object.values({ a: inboxes.map(cb) }) returns an array holding the map() result as a single element, not the individual promises, so Promise.all() over it never awaited what ctx.sendActivity() returned. Wrap the map() result in its own Promise.all() so the fixture matches what its name says. Assisted-by: Claude Code:claude-sonnet-5 --- .../lint/src/tests/outbox-listener-delivery-required.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index 759889fc5..3e4e3ef10 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -1210,7 +1210,9 @@ federation const sender = { identifier: ctx.identifier }; await Promise.all( Object.values({ - a: inboxes.map((inbox) => ctx.sendActivity(sender, inbox, activity)), + a: Promise.all( + inboxes.map((inbox) => ctx.sendActivity(sender, inbox, activity)), + ), }), ); }); From 4626ac442fec45f46010b89d8e158b1e3ccf1a7f Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Wed, 23 Sep 2026 22:59:09 +0900 Subject: [PATCH 09/11] Count functions as used once their name appears Working out how a function value reaches a call site (an alias, a destructured property, a computed member access, a later assignment, an array element, a wrapper call) is open-ended, and each shape the rule did not recognize reported a listener that plainly delivers, where the old text scan accepted it. Showing that a name never appears anywhere that runs is not open-ended, so flip the default: a function held under a name (a declaration, a variable or its initializer, an assignment, an object property, a class method) is used as soon as that name is mentioned, however it is mentioned, and only a name that never appears leaves its functions dead. An assignment target is a write, not a mention. A function declaration hoists, so one written below an unconditional exit is still callable from the code above it. Keep it in the scan instead of pruning it with the dead code after the exit. Document the contract in the manual: the rule reports only when it can account for every delivery call it can see and show that each one does not run. https://github.com/fedify-dev/fedify/pull/1050#pullrequestreview-5289632641 Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 21 +- changes.d/lint/outbox-listener-path-aware.md | 21 +- docs/manual/lint.md | 49 ++- .../outbox-listener-delivery-required.ts | 232 ++++++++--- .../outbox-listener-delivery-required.test.ts | 390 ++++++++++++++++++ 5 files changed, 618 insertions(+), 95 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index a99125f69..1ccabd6c7 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -274,19 +274,14 @@ To be released. - Changed `outbox-listener-delivery-required` (`@fedify/lint`) to decide whether a `ctx.sendActivity()`/`ctx.forwardActivity()` call actually - runs, - instead of scanning the listener's source as a flat block of text. It - now reports a listener whose only delivery call sits behind a dead - branch, after an unconditional `return`/`throw`, or inside a local - helper function that is never actually called. A helper that is - called still counts, regardless of how it's referenced: by name, - passed by reference to another function, or reached through a local - object literal. An inline callback whose result is awaited or - returned counts too, including one nested inside an array, a spread, - or an object literal, such as - `await Promise.all([...a.map(...), ...b.map(...)])`. A callback - passed to `forEach()` counts as well, since it always runs - synchronously regardless of what the call returns. + runs, instead of scanning the listener's source as a flat block of text. + It now reports a listener whose only delivery calls sit behind a dead + branch, after an unconditional `return`/`throw`, inside a function that + is never used, or inside an inline callback whose result is dropped. + When it cannot tell whether a delivery call runs, it stays quiet: a + function held under a name counts as used as soon as that name is + mentioned, however it is passed around, and a callback that is awaited, + returned, or passed to `forEach()` counts too. [[#900], [#1050] by Jae-Hyuk-Jang\] [#895]: https://github.com/fedify-dev/fedify/issues/895 diff --git a/changes.d/lint/outbox-listener-path-aware.md b/changes.d/lint/outbox-listener-path-aware.md index f65c031a0..71af282c2 100644 --- a/changes.d/lint/outbox-listener-path-aware.md +++ b/changes.d/lint/outbox-listener-path-aware.md @@ -5,17 +5,12 @@ links: --- - Changed `outbox-listener-delivery-required` (`@fedify/lint`) to decide whether a `ctx.sendActivity()`/`ctx.forwardActivity()` call actually - runs, - instead of scanning the listener's source as a flat block of text. It - now reports a listener whose only delivery call sits behind a dead - branch, after an unconditional `return`/`throw`, or inside a local - helper function that is never actually called. A helper that is - called still counts, regardless of how it's referenced: by name, - passed by reference to another function, or reached through a local - object literal. An inline callback whose result is awaited or - returned counts too, including one nested inside an array, a spread, - or an object literal, such as - `await Promise.all([...a.map(...), ...b.map(...)])`. A callback - passed to `forEach()` counts as well, since it always runs - synchronously regardless of what the call returns. + runs, instead of scanning the listener's source as a flat block of text. + It now reports a listener whose only delivery calls sit behind a dead + branch, after an unconditional `return`/`throw`, inside a function that + is never used, or inside an inline callback whose result is dropped. + When it cannot tell whether a delivery call runs, it stays quiet: a + function held under a name counts as used as soon as that name is + mentioned, however it is passed around, and a callback that is awaited, + returned, or passed to `forEach()` counts too. [[#900], [#1050] by Jae-Hyuk-Jang] diff --git a/docs/manual/lint.md b/docs/manual/lint.md index c5e097316..b968928c6 100644 --- a/docs/manual/lint.md +++ b/docs/manual/lint.md @@ -752,17 +752,32 @@ Warns when an outbox listener body does not deliver the posted activity with `ctx.sendActivity()` or `ctx.forwardActivity()`. **When this rule applies:** -You've registered an outbox listener with `setOutboxListeners()`, but no -reachable path through the listener body calls either delivery method. The -rule follows the listener's own control flow (`if`/`else`, `try`/`catch`, -`switch`, loops) and resolves calls to local helper functions, so a delivery -call that sits in a dead branch, after an unconditional `return`, or inside a -helper that is declared but never actually called does not count. A helper -that *is* called does count, regardless of how it is referenced: by name, -passed by reference to another function, or reached through a local object -literal. An inline callback whose result is awaited or returned counts -too, such as -`await Promise.all(recipients.map((r) => ctx.sendActivity(...)))`. +You've registered an outbox listener with `setOutboxListeners()`, and the rule +can show that no path through the listener body calls either delivery method. +It follows the listener's own control flow (`if`/`else`, `try`/`catch`, +`switch`, loops), so a delivery call that sits in a dead branch, after an +unconditional `return`, in a function that is never used, or in an inline +callback whose result is dropped does not count. + +The rule reports only when it can account for every delivery call it can see +and show that each one does not run. When it cannot tell, it stays quiet: a +missed warning is the safe direction, while a warning on code that delivers is +not. In practice: + + - A function held under a name counts as used as soon as that name is + mentioned anywhere in code that runs, however it is mentioned: called, + passed to another function, aliased, destructured from an object, or + reached through an array or a wrapper call. The rule does not follow the + value any further, so a function that is only logged or stored, and never + called, is not reported. + - An inline callback counts when its result is awaited or returned, such as + `await Promise.all(recipients.map((r) => ctx.sendActivity(...)))`, even + when it sits inside an array or an object literal. A callback passed to + `forEach()` counts too, since `forEach()` always runs it. A callback + passed to any other call whose result is dropped, such as an unawaited + `recipients.map(...)`, does not. + - The rule reads only the listener body. A delivery call in a helper that + is declared outside the listener, or in another module, is not seen. **Why it matters:** Fedify does not federate client-to-server outbox posts automatically. If your @@ -829,6 +844,18 @@ federation } await deliver(); }); + +// ✅ Good: A helper reached through a destructured property still counts +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const handlers = { + deliver: () => + ctx.sendActivity({ identifier: ctx.identifier }, "followers", activity), + }; + const { deliver } = handlers; + await deliver(); + }); ~~~~ ### `media-uploader-object-uri-required` diff --git a/packages/lint/src/rules/outbox-listener-delivery-required.ts b/packages/lint/src/rules/outbox-listener-delivery-required.ts index 328c96f50..2c735e2a9 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-required.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-required.ts @@ -364,9 +364,18 @@ function alwaysExits(node: Node): boolean { function collectReachableStatements(node: Node, out: Node[]): void { switch (node.type) { case "BlockStatement": - for (const statement of node.body) { + for (const [index, statement] of node.body.entries()) { collectReachableStatements(statement as Node, out); - if (alwaysExits(statement as Node)) return; + if (alwaysExits(statement as Node)) { + // Function declarations hoist: one written below an exit is still + // callable from the code above it. + for (const rest of node.body.slice(index + 1)) { + if ((rest as Node).type === "FunctionDeclaration") { + out.push(rest as Node); + } + } + return; + } } return; @@ -423,15 +432,25 @@ function collectReachableStatements(node: Node, out: Node[]): void { // What a listener (or a helper's own body) resolves to when scanned for a // delivery call: three independent mechanisms decide which nested function // bodies are folded into the scan instead of being masked out. +// +// The rule reports only when none of them can account for a delivery call, +// so each one errs toward treating a function as used. Working out how a +// function value travels through arbitrary JavaScript (an alias, a +// destructured property, an array, a wrapper call) is open-ended, but +// showing that a name never appears anywhere that runs is not. A function +// held under a name is therefore used as soon as that name is mentioned, +// without tracing how it is then passed around. A missed warning is the +// safe direction; a warning on code that delivers is not. // --------------------------------------------------------------------------- /** * Collects plain-value references to identifiers: `deliver()`, * `forEach(deliver)`, a shorthand `{ deliver }`, and so on. Skips positions - * that name something rather than reference a value -- a declaration's own - * `id`/params, and the non-computed `.property` of a member expression (so + * that name something rather than reference a value: a declaration's own + * `id`/params, the non-computed `.property` of a member expression (so * `someService.deliver()` never counts as a reference to an unrelated local - * `deliver`). + * `deliver`), and the target of an assignment, which writes to a name + * instead of reading it. */ function collectReferencedNames(node: unknown, out: Set): void { if (node == null || typeof node !== "object") return; @@ -463,6 +482,28 @@ function collectReferencedNames(node: unknown, out: Set): void { } return; } + if (n.type === "ClassDeclaration" || n.type === "ClassExpression") { + // The class's own name is a declaration, not a mention of it. + collectReferencedNames(n.superClass, out); + collectReferencedNames(n.body, out); + return; + } + if ( + (n.type === "MethodDefinition" || n.type === "PropertyDefinition") && + !n.computed + ) { + // Same as an object literal's `Property`: the key names a member, and + // only what it holds can reference something. + collectReferencedNames(n.value, out); + return; + } + if (n.type === "AssignmentExpression" && n.operator === "=") { + // `x = fn` and `obj.x = fn` write to a name rather than mention it. + if (getAssignmentTargetName(n.left as Node) != null) { + collectReferencedNames(n.right, out); + return; + } + } if (isFunctionLikeNode(n)) { // Stop at a nested function's own boundary: whether a name it // references counts is decided separately, only once that function @@ -478,78 +519,139 @@ function collectReferencedNames(node: unknown, out: Set): void { } /** - * Collects every named local helper declared directly in `node`'s own - * scope: a `function name() {}` declaration, or a `const name = - * function/arrow` binding. Does not descend into a found function's own - * body -- a helper nested inside another helper is only found once that - * outer helper is itself resolved as reachable, so it can be layered on - * top of (and correctly shadow) the outer scope's helpers of the same - * name. Object-literal properties are handled separately, through - * `resolveFunctionBinding`, since their name is only meaningful together - * with the object it lives on. + * The name an assignment writes to: `x` for `x = ...`, and the root object + * for `obj.a.b = ...`. `null` for anything more exotic. + */ +function getAssignmentTargetName(target: Node): string | null { + let current: Node = target; + while (current.type === "MemberExpression") current = current.object as Node; + return current.type === "Identifier" ? current.name : null; +} + +/** Every identifier a declaration pattern binds (`a`, `{ a, b: c }`, `[a]`). */ +function collectBoundNames(pattern: unknown, out: string[]): void { + if (pattern == null || typeof pattern !== "object" || !isNode(pattern)) { + return; + } + const p = pattern as Node; + switch (p.type) { + case "Identifier": + out.push(p.name); + return; + case "AssignmentPattern": + collectBoundNames(p.left, out); + return; + case "RestElement": + collectBoundNames(p.argument, out); + return; + case "ArrayPattern": + for (const element of p.elements) collectBoundNames(element, out); + return; + case "ObjectPattern": + for (const prop of p.properties) { + collectBoundNames( + (prop as { value?: unknown; argument?: unknown }).value ?? + (prop as { argument?: unknown }).argument, + out, + ); + } + return; + } +} + +/** + * Collects the functions each name in `node`'s own scope holds, wherever + * they sit in what the name is bound to: `function name() {}`, + * `class Name {}`, the value of `const name = ...` or a later + * `name = ...` or `name.prop = ...`, including a function inside an object + * or array literal or passed through a call (`const deliver = once(fn)`). + * A function held under a name counts as used as soon as the name is + * mentioned, however it is mentioned, so this never has to work out how the + * name reaches the function. Does not descend into a found function's own + * body: a name bound inside it is only found once that function is itself + * resolved as reachable, so it can be layered on top of (and correctly + * shadow) the outer scope's names. */ -function collectNamedHelpers( +function collectFunctionsByName( node: unknown, - out: Map, + out: Map, ): void { if (node == null || typeof node !== "object") return; if (Array.isArray(node)) { - for (const item of node) collectNamedHelpers(item, out); + for (const item of node) collectFunctionsByName(item, out); return; } if (!isNode(node)) return; const n = node; + const bindTo = (names: string[], from: unknown): void => { + const functions: FunctionLikeNode[] = []; + collectNestedFunctions(from, functions); + if (functions.length < 1) return; + for (const name of names) { + out.set(name, [...(out.get(name) ?? []), ...functions]); + } + }; + if (n.type === "FunctionDeclaration") { - if (n.id?.name != null) out.set(n.id.name, n as FunctionLikeNode); + if (n.id?.name != null) bindTo([n.id.name], n); return; } - if ( - n.type === "FunctionExpression" || n.type === "ArrowFunctionExpression" - ) { + if (n.type === "ClassDeclaration") { + if (n.id?.name != null) bindTo([n.id.name], n); return; } + if (isFunctionLikeNode(n)) return; if (n.type === "VariableDeclarator") { const decl = n as VariableDeclarator; - if (decl.id.type === "Identifier" && decl.init != null) { - const init = decl.init as Node; - if (isFunctionLikeNode(init)) out.set(decl.id.name, init); + if (decl.init != null) { + const names: string[] = []; + collectBoundNames(decl.id, names); + bindTo(names, decl.init); + collectFunctionsByName(decl.init, out); } return; } + if (n.type === "AssignmentExpression") { + const name = getAssignmentTargetName(n.left as Node); + if (name != null) bindTo([name], n.right); + collectFunctionsByName(n.right, out); + return; + } const record = n as unknown as Record; for (const key in record) { if (key === "parent") continue; - collectNamedHelpers(record[key], out); + collectFunctionsByName(record[key], out); } } /** * Resolves every call expression's callee and collects the functions those - * calls resolve to. A bare identifier callee is resolved against `helpers` - * first -- the current scope's own, correctly shadowed helper map -- so a - * call to a shadowed name never resolves to some other, same-named - * function declared elsewhere. Anything `helpers` doesn't have (a member - * call such as `handlers.deliver()`, or a name that isn't a local helper - * at all) falls back to `bindings`, the whole-file map used to resolve a - * local object literal's properties. This is also how a directly invoked - * function expression -- `(() => {...})()` -- gets found: + * calls resolve to, for the callees that mentioning a name doesn't already + * cover. A bare identifier this scope holds functions under + * (`functionsByName`, correctly shadowed) is skipped: its mention reaches + * those functions, and resolving it again against the file-wide `bindings` + * could land on some other, same-named function declared elsewhere. + * Anything else, such as a member call like `handlers.deliver()` or a name + * that isn't held locally, is resolved against `bindings`, the whole-file + * map used to resolve a local object literal's properties. This is also how + * a directly invoked function expression, `(() => {...})()`, gets found: * `resolveFunctionBinding` returns a function literal callee as-is. Stops * at a nested function's own boundary, so a call that only happens inside - * some other, not-yet-reachable function doesn't count here -- it is - * found on its own once that function is resolved as reachable. + * some other, not-yet-reachable function doesn't count here. It is found + * on its own once that function is resolved as reachable. */ function collectResolvedCallTargets( node: unknown, - helpers: ReadonlyMap, + functionsByName: ReadonlyMap, bindings: Map, out: Set, ): void { if (node == null || typeof node !== "object") return; if (Array.isArray(node)) { for (const item of node) { - collectResolvedCallTargets(item, helpers, bindings, out); + collectResolvedCallTargets(item, functionsByName, bindings, out); } return; } @@ -559,8 +661,13 @@ function collectResolvedCallTargets( if (n.type === "CallExpression") { const callee = n.callee as Expression; - const resolved = callee.type === "Identifier" && helpers.has(callee.name) - ? helpers.get(callee.name)! + // A name this scope already holds functions under is reached through + // its mention, and must not fall through to the file-wide map, which + // could hold a different function of the same name. + const heldLocally = callee.type === "Identifier" && + functionsByName.has(callee.name); + const resolved = heldLocally + ? null : resolveFunctionBinding(callee, bindings); if (resolved != null) out.add(resolved); } @@ -568,7 +675,7 @@ function collectResolvedCallTargets( const record = n as unknown as Record; for (const key in record) { if (key === "parent") continue; - collectResolvedCallTargets(record[key], helpers, bindings, out); + collectResolvedCallTargets(record[key], functionsByName, bindings, out); } } @@ -754,10 +861,10 @@ function collectNestedFunctions( * `root`: `root` itself feeds a worklist, and each function it (or a * function already on the worklist) references from *its own* reachable * statements -- never from a dead branch or some other not-yet-reached - * function's body -- gets queued in turn. `outerHelpers` is layered fresh - * for each scope, so a helper declared at an inner scope shadows a + * function's body -- gets queued in turn. `outerFunctionsByName` is layered + * fresh for each scope, so a name bound at an inner scope shadows a * same-named one further out instead of overwriting it globally, and a - * dead branch that merely mentions a helper's name never queues it. + * dead branch that merely mentions a name never queues what it holds. */ function computeUsedFunctions( root: Node, @@ -768,7 +875,7 @@ function computeUsedFunctions( const processScope = ( scopeRoot: Node, - outerHelpers: ReadonlyMap, + outerFunctionsByName: ReadonlyMap, ): void => { if (visited.has(scopeRoot)) return; visited.add(scopeRoot); @@ -776,31 +883,40 @@ function computeUsedFunctions( const statements: Node[] = []; collectReachableStatements(scopeRoot, statements); - const helpers = new Map(outerHelpers); + const functionsHere = new Map(); for (const statement of statements) { - collectNamedHelpers(statement, helpers); + collectFunctionsByName(statement, functionsHere); + } + const functionsByName = new Map(outerFunctionsByName); + for (const [name, functions] of functionsHere) { + functionsByName.set(name, functions); } const referencedNames = new Set(); for (const statement of statements) { collectReferencedNames(statement, referencedNames); } - // A helper counts as reached as soon as its name is mentioned at all -- + // A function held under a name counts as reached as soon as that name + // is mentioned at all -- called, passed along, aliased, destructured, // passed to `console.log`, stored in a variable, anything -- not only // when it's actually invoked. That's what lets `recipients.map(deliver)` - // resolve `deliver` as used without this code having to know that `map` - // invokes its argument; telling a real invocation apart from merely - // holding a reference would need knowing which APIs call what they're - // given, which is more than this rule should carry. The cost is a - // narrow false negative -- a helper that's only logged or reassigned, - // never called, is not reported -- accepted deliberately, since missing - // a case here is the safe direction. Leave this as is. + // and `const { deliver } = handlers` resolve as used without this code + // having to know that `map` invokes its argument or how a destructured + // property gets from the object to the call. Telling a real invocation + // apart from merely holding a reference would need following every + // shape a function value can travel in and knowing which APIs call what + // they're given, which is more than this rule should carry, and any + // shape it missed would report code that delivers. The cost is a + // narrow false negative: a function that's only logged or reassigned, + // never called, is not reported. That is accepted deliberately, since + // missing a case here is the safe direction. Leave this as is. const reached = new Set(); - for (const [name, fn] of helpers) { - if (referencedNames.has(name)) reached.add(fn); + for (const [name, functions] of functionsByName) { + if (!referencedNames.has(name)) continue; + for (const fn of functions) reached.add(fn); } for (const statement of statements) { - collectResolvedCallTargets(statement, helpers, bindings, reached); + collectResolvedCallTargets(statement, functionsByName, bindings, reached); } collectConsumedCallbacks( statements, @@ -810,7 +926,7 @@ function computeUsedFunctions( for (const fn of reached) { used.add(fn); - processScope(fn.body as Node, helpers); + processScope(fn.body as Node, functionsByName); } }; diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index 3e4e3ef10..acb499af8 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -1260,3 +1260,393 @@ federation "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", }), ); + +test( + `${ruleName}: ✅ Good - helper reached through an alias of an object property`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = { + deliver: () => ctx.sendActivity(sender, inbox, activity), + }; + const alias = handlers.deliver; + await alias(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper called through a computed member access`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = { + deliver: () => ctx.sendActivity(sender, inbox, activity), + }; + await handlers["deliver"](); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper destructured from an object of helpers`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = { + deliver: () => ctx.sendActivity(sender, inbox, activity), + }; + const { deliver } = handlers; + await deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper assigned after its declaration`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + let deliver; + deliver = () => ctx.sendActivity(sender, inbox, activity); + await deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper declared below an unconditional return`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await deliver(); + return; + function deliver() { + return ctx.sendActivity(sender, inbox, activity); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper stored in an array`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = [() => ctx.sendActivity(sender, inbox, activity)]; + await handlers[0](); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper wrapped in a call before being bound`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const once = (fn) => fn; + const deliver = once(() => ctx.sendActivity(sender, inbox, activity)); + await deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - map result kept in a variable before Promise.all`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const promises = inboxes.map((target) => + ctx.sendActivity(sender, target, activity) + ); + await Promise.all(promises); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper assigned as a property after the object is created`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = {}; + handlers.deliver = () => ctx.sendActivity(sender, inbox, activity); + await handlers.deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper destructured straight from an object literal`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const { deliver } = { + deliver: () => ctx.sendActivity(sender, inbox, activity), + }; + await deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper defined as a class method`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + class Sender { + deliver() { + return ctx.sendActivity(sender, inbox, activity); + } + } + await new Sender().deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad - helper assigned to a variable but never called`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + let deliver; + deliver = () => ctx.sendActivity(sender, inbox, activity); + console.log("never called"); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - helper assigned as a property but never called`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = {}; + handlers.deliver = () => ctx.sendActivity(sender, inbox, activity); + console.log("never called"); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - map result kept in a variable but never used`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const promises = inboxes.map((target) => + ctx.sendActivity(sender, target, activity) + ); + console.log("never awaited"); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - helper declared below a return but never called`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + console.log("nothing below runs"); + return; + function deliver() { + return ctx.sendActivity(sender, inbox, activity); + } + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - nested helper never called by the helper that declares it`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function outer() { + function inner() { + return ctx.sendActivity(sender, inbox, activity); + } + console.log("never calls inner"); + } + await outer(); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - class whose methods are never used`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + class Sender { + deliver() { + return ctx.sendActivity(sender, inbox, activity); + } + } + console.log("never instantiated"); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); From e1f77d8c2712bb17fb27c6ee3dae8809ab299ce1 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Wed, 23 Sep 2026 23:06:53 +0900 Subject: [PATCH 10/11] Scan control-flow head expressions too The reachability walk collected the statements inside an if, switch, or loop but never the expressions in its head: an if test, a switch discriminant and case tests, a loop's init, test, update and right. A delivery call or a helper call written there was invisible, so a listener that runs its jobs with for (const job of jobs) await job(); was reported as not delivering, although it passed on main. A head runs whenever its statement does, whichever branch is taken, so collect it alongside the bodies and let the existing machinery handle what it finds, including an awaited callback. Collecting a head never revives the branch behind it: if (false) still hides its consequent, and the heads of statements after an unconditional exit are never reached. This covers https://github.com/fedify-dev/fedify/issues/1052. Assisted-by: Claude Code:claude-sonnet-5 --- .../outbox-listener-delivery-required.ts | 30 ++ .../outbox-listener-delivery-required.test.ts | 319 ++++++++++++++++++ 2 files changed, 349 insertions(+) diff --git a/packages/lint/src/rules/outbox-listener-delivery-required.ts b/packages/lint/src/rules/outbox-listener-delivery-required.ts index 2c735e2a9..7bf0bf099 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-required.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-required.ts @@ -310,6 +310,12 @@ const resolveFunctionBinding = ( // (if/else, try/catch/finally, switch, loops) but never descending into a // nested function's own body, and pruning dead code (a statically-falsy `if` // branch, or anything after a statement that always returns/throws). +// +// A control-flow statement's head expressions (an `if` test, a `switch` +// discriminant and case tests, a loop's `init`/`test`/`update`/`right`) run +// whenever the statement itself does, whichever branch is taken, so they are +// collected alongside the bodies. Collecting one never revives the branch +// behind it: `if (false)` still hides its consequent. // --------------------------------------------------------------------------- const isStaticallyFalsy = (test: Expression): boolean => @@ -381,6 +387,7 @@ function collectReachableStatements(node: Node, out: Node[]): void { case "IfStatement": { const test = node.test as Expression; + out.push(test); if (!isStaticallyFalsy(test)) { collectReachableStatements(node.consequent as Node, out); } @@ -401,7 +408,9 @@ function collectReachableStatements(node: Node, out: Node[]): void { return; case "SwitchStatement": + out.push(node.discriminant as Node); for (const switchCase of node.cases) { + if (switchCase.test != null) out.push(switchCase.test as Node); for (const statement of switchCase.consequent) { collectReachableStatements(statement as Node, out); if (alwaysExits(statement as Node)) break; @@ -411,14 +420,31 @@ function collectReachableStatements(node: Node, out: Node[]): void { case "WhileStatement": case "DoWhileStatement": + out.push(node.test as Node); + collectReachableStatements(node.body as Node, out); + return; + case "ForStatement": + for (const head of [node.init, node.test, node.update]) { + if (head != null) out.push(head as Node); + } + collectReachableStatements(node.body as Node, out); + return; + case "ForInStatement": case "ForOfStatement": + // Only `right` is evaluated as a value; `left` declares or assigns the + // loop variable. + out.push(node.right as Node); collectReachableStatements(node.body as Node, out); return; case "LabeledStatement": + collectReachableStatements(node.body as Node, out); + return; + case "WithStatement": + out.push(node.object as Node); collectReachableStatements(node.body as Node, out); return; @@ -819,7 +845,11 @@ function collectConsumedCallbacks( | undefined; if (init != null) walkAwaitExpressions(init); } + continue; } + // Anything else is a control-flow head expression, such as the + // `await ...` in `if (await ...)`. + walkAwaitExpressions(statement); } if (impliedReturn && statements.length === 1) { diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index acb499af8..6972fdaa1 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -1650,3 +1650,322 @@ federation "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", }), ); + +test( + `${ruleName}: ✅ Good - delivery call in an if test`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (await ctx.sendActivity(sender, inbox, activity)) { + console.log("sent"); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper call in a switch discriminant`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + return 1; + } + switch (await deliver()) { + case 1: + break; + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper call in a switch case test`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + return 1; + } + switch (1) { + case await deliver(): + break; + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper call in a while test`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + return 1; + } + while (await deliver() > 1) { + console.log("looping"); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper call in a do-while test`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + return 1; + } + do { + console.log("once"); + } while (await deliver() > 1); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery call in a for loop init`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + for (let sent = await ctx.sendActivity(sender, inbox, activity); false;) { + console.log(sent); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper call in a for loop test`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + return 1; + } + for (let i = 0; i < await deliver(); i++) { + console.log(i); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper call in a for loop update`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + return 1; + } + for (let i = 0; i < 1; i += await deliver()) { + console.log(i); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helpers held in an array and run by a for-of loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const jobs = [ + () => ctx.sendActivity(sender, inbox, activity), + () => ctx.sendActivity(sender, inbox, activity), + ]; + for (const job of jobs) await job(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited callback inside an if test`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (await Promise.all(inboxes.map((target) => + ctx.sendActivity(sender, target, activity) + ))) { + console.log("sent"); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad - unrelated helper called in an if test`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function check() { + return true; + } + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + } + if (await check()) { + console.log("checked"); + } + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - unrelated for-of head next to an unused helper`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => ctx.sendActivity(sender, inbox, activity); + for (const target of [inbox]) { + console.log(target); + } + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery call in the branch behind an if test that is scanned`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (await Promise.resolve(false)) { + return; + } + if (false) { + await ctx.sendActivity(sender, inbox, activity); + } + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +); From 3e862dcd2e94358ddaa3160ca98e6cf1776e3a73 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Thu, 24 Sep 2026 08:58:29 +0900 Subject: [PATCH 11/11] Count any inline callback as used The rule never checked whether a delivery call is awaited: a bare, voided, caught or stored ctx.sendActivity() call already passed. The only unawaited shape it reported was a dropped map(), and only because of how consumption detection was built, so queue.push(cb) and setTimeout(cb, 0) were the same accident from the other side. Drop the awaited-or-returned check and the forEach special case. A function held under a name is still used once the name is mentioned, and every other function literal now counts wherever it appears, since the rule cannot show that the receiving call never runs it. That also removes the call-target resolver, which no longer changes any result. An unawaited promise can still be cut off before the activity leaves, which is real on Cloudflare Workers, but this rule is the wrong place to check it: https://github.com/fedify-dev/fedify/issues/1057 tracks a rule for that. Say so in the manual, next to what the rule does and does not claim. https://github.com/fedify-dev/fedify/pull/1050#pullrequestreview-5293260047 Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 11 +- changes.d/lint/outbox-listener-path-aware.md | 11 +- docs/manual/lint.md | 19 +- .../outbox-listener-delivery-required.ts | 337 ++++-------------- .../outbox-listener-delivery-required.test.ts | 129 ++++++- 5 files changed, 219 insertions(+), 288 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 1ccabd6c7..3ed53b9ae 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -276,12 +276,11 @@ To be released. whether a `ctx.sendActivity()`/`ctx.forwardActivity()` call actually runs, instead of scanning the listener's source as a flat block of text. It now reports a listener whose only delivery calls sit behind a dead - branch, after an unconditional `return`/`throw`, inside a function that - is never used, or inside an inline callback whose result is dropped. - When it cannot tell whether a delivery call runs, it stays quiet: a - function held under a name counts as used as soon as that name is - mentioned, however it is passed around, and a callback that is awaited, - returned, or passed to `forEach()` counts too. + branch, after an unconditional `return`/`throw`, or inside a function + that is never used. When it cannot tell whether a delivery call runs, it + stays quiet: a function held under a name counts as used as soon as that + name is mentioned, however it is passed around, and an inline callback + counts wherever it is passed. [[#900], [#1050] by Jae-Hyuk-Jang\] [#895]: https://github.com/fedify-dev/fedify/issues/895 diff --git a/changes.d/lint/outbox-listener-path-aware.md b/changes.d/lint/outbox-listener-path-aware.md index 71af282c2..b54c302ba 100644 --- a/changes.d/lint/outbox-listener-path-aware.md +++ b/changes.d/lint/outbox-listener-path-aware.md @@ -7,10 +7,9 @@ links: whether a `ctx.sendActivity()`/`ctx.forwardActivity()` call actually runs, instead of scanning the listener's source as a flat block of text. It now reports a listener whose only delivery calls sit behind a dead - branch, after an unconditional `return`/`throw`, inside a function that - is never used, or inside an inline callback whose result is dropped. - When it cannot tell whether a delivery call runs, it stays quiet: a - function held under a name counts as used as soon as that name is - mentioned, however it is passed around, and a callback that is awaited, - returned, or passed to `forEach()` counts too. + branch, after an unconditional `return`/`throw`, or inside a function + that is never used. When it cannot tell whether a delivery call runs, it + stays quiet: a function held under a name counts as used as soon as that + name is mentioned, however it is passed around, and an inline callback + counts wherever it is passed. [[#900], [#1050] by Jae-Hyuk-Jang] diff --git a/docs/manual/lint.md b/docs/manual/lint.md index b968928c6..506219229 100644 --- a/docs/manual/lint.md +++ b/docs/manual/lint.md @@ -756,8 +756,7 @@ You've registered an outbox listener with `setOutboxListeners()`, and the rule can show that no path through the listener body calls either delivery method. It follows the listener's own control flow (`if`/`else`, `try`/`catch`, `switch`, loops), so a delivery call that sits in a dead branch, after an -unconditional `return`, in a function that is never used, or in an inline -callback whose result is dropped does not count. +unconditional `return`, or in a function that is never used does not count. The rule reports only when it can account for every delivery call it can see and show that each one does not run. When it cannot tell, it stays quiet: a @@ -770,12 +769,8 @@ not. In practice: reached through an array or a wrapper call. The rule does not follow the value any further, so a function that is only logged or stored, and never called, is not reported. - - An inline callback counts when its result is awaited or returned, such as - `await Promise.all(recipients.map((r) => ctx.sendActivity(...)))`, even - when it sits inside an array or an object literal. A callback passed to - `forEach()` counts too, since `forEach()` always runs it. A callback - passed to any other call whose result is dropped, such as an unawaited - `recipients.map(...)`, does not. + - An inline callback counts wherever it is passed, since the rule cannot show + that the receiving call never runs it. - The rule reads only the listener body. A delivery call in a helper that is declared outside the listener, or in another module, is not seen. @@ -784,6 +779,12 @@ Fedify does not federate client-to-server outbox posts automatically. If your application intends to deliver a posted activity, the listener must choose an explicit delivery path, and that path must actually run. +The rule checks that a delivery call exists and can run, not that the delivery +completes, so a listener it accepts is not guaranteed to federate. A delivery +call that is never awaited is not reported; +[#1057] tracks a rule for +that. + ~~~~ typescript twoslash // @noErrors: 2345 import { createFederation } from "@fedify/fedify"; @@ -858,6 +859,8 @@ federation }); ~~~~ +[#1057]: https://github.com/fedify-dev/fedify/issues/1057 + ### `media-uploader-object-uri-required` Warns when a `setMediaUploader()` callback returns a value that is not derived diff --git a/packages/lint/src/rules/outbox-listener-delivery-required.ts b/packages/lint/src/rules/outbox-listener-delivery-required.ts index 7bf0bf099..91d8f4459 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-required.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-required.ts @@ -43,21 +43,6 @@ const isChainedFromOutboxListeners = ( const DELIVERY_METHOD_NAMES = new Set(["sendActivity", "forwardActivity"]); -/** - * Iteration methods whose own return value is never meant to be consumed, - * so nothing is ever "forgotten" by not awaiting or collecting it -- the - * language spec guarantees each invokes its callback argument synchronously - * for every element regardless. `forEach` always returns `undefined`; a - * callback passed to it has run by the time the call completes whether or - * not anything looks at what the call returns. This is deliberately - * narrower than the full iteration protocol: `map`, `filter` and the rest - * return something that usually *is* meant to be consumed (an array of - * promises passed to `Promise.all`, for instance), so calling one of those - * without awaiting or returning the result stays a real thing to flag, not - * a rule limitation to work around. - */ -const SYNCHRONOUS_ITERATION_METHODS = new Set(["forEach"]); - type FunctionLikeNode = | FunctionNode | (Node & { @@ -247,9 +232,8 @@ function buildContextExpressionPattern(contextName: string): string { * Resolves an expression to the function it refers to: a direct function * literal, a local variable bound to one, or a property of a local object * literal bound to one (e.g. `handlers.deliver` where - * `const handlers = { deliver() {} }`). Used both to resolve a listener - * argument (`.on(Activity, handler)`) and to resolve what a call expression - * inside a listener actually invokes. + * `const handlers = { deliver() {} }`). Used to resolve a listener argument + * (`.on(Activity, handler)`). */ const resolveFunctionBinding = ( expr: Expression, @@ -456,17 +440,20 @@ function collectReachableStatements(node: Node, out: Node[]): void { // --------------------------------------------------------------------------- // What a listener (or a helper's own body) resolves to when scanned for a -// delivery call: three independent mechanisms decide which nested function -// bodies are folded into the scan instead of being masked out. +// delivery call: two rules decide which nested function bodies are folded +// into the scan instead of being masked out. // -// The rule reports only when none of them can account for a delivery call, -// so each one errs toward treating a function as used. Working out how a -// function value travels through arbitrary JavaScript (an alias, a -// destructured property, an array, a wrapper call) is open-ended, but -// showing that a name never appears anywhere that runs is not. A function -// held under a name is therefore used as soon as that name is mentioned, -// without tracing how it is then passed around. A missed warning is the -// safe direction; a warning on code that delivers is not. +// The rule reports only when neither can account for a delivery call, so +// both err toward treating a function as used. Working out how a function +// value travels through arbitrary JavaScript (an alias, a destructured +// property, an array, a wrapper call) is open-ended, and so is working out +// what a call does with a callback it receives. Showing that a name never +// appears anywhere that runs is not. So a function held under a name is +// used as soon as that name is mentioned, without tracing how it is then +// passed around, and any other function literal, such as a callback handed +// to a call, is used wherever it appears, since the rule cannot show that +// the receiving call never runs it. A missed warning is the safe direction; +// a warning on code that delivers is not. // --------------------------------------------------------------------------- /** @@ -586,17 +573,45 @@ function collectBoundNames(pattern: unknown, out: string[]): void { } /** - * Collects the functions each name in `node`'s own scope holds, wherever - * they sit in what the name is bound to: `function name() {}`, - * `class Name {}`, the value of `const name = ...` or a later - * `name = ...` or `name.prop = ...`, including a function inside an object - * or array literal or passed through a call (`const deliver = once(fn)`). - * A function held under a name counts as used as soon as the name is - * mentioned, however it is mentioned, so this never has to work out how the - * name reaches the function. Does not descend into a found function's own - * body: a name bound inside it is only found once that function is itself - * resolved as reachable, so it can be layered on top of (and correctly - * shadow) the outer scope's names. + * Finds the function literals a value holds itself: the value is the + * function, or it sits in an object or array literal, a conditional, or a + * class body. Stops at a call, so a function handed to one as an argument, + * or invoked by it, is not held by whatever the call's result is bound to. + * Those count on their own wherever they appear. + */ +function collectHeldFunctions(node: unknown, out: FunctionLikeNode[]): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectHeldFunctions(item, out); + return; + } + if (!isNode(node)) return; + const n = node; + + if (isFunctionLikeNode(n)) { + out.push(n); + return; + } + if (n.type === "CallExpression" || n.type === "NewExpression") return; + + const record = n as unknown as Record; + for (const key in record) { + if (key === "parent") continue; + collectHeldFunctions(record[key], out); + } +} + +/** + * Collects the functions each name in `node`'s own scope holds: `function + * name() {}`, `class Name {}`, and the value of `const name = ...` or a + * later `name = ...` or `name.prop = ...`, including a function inside an + * object or array literal (see `collectHeldFunctions`). A function held + * under a name counts as used as soon as the name is mentioned, however it + * is mentioned, so this never has to work out how the name reaches the + * function. Does not descend into a found function's own body: a name bound + * inside it is only found once that function is itself resolved as + * reachable, so it can be layered on top of (and correctly shadow) the outer + * scope's names. */ function collectFunctionsByName( node: unknown, @@ -612,7 +627,7 @@ function collectFunctionsByName( const bindTo = (names: string[], from: unknown): void => { const functions: FunctionLikeNode[] = []; - collectNestedFunctions(from, functions); + collectHeldFunctions(from, functions); if (functions.length < 1) return; for (const name of names) { out.set(name, [...(out.get(name) ?? []), ...functions]); @@ -652,212 +667,6 @@ function collectFunctionsByName( } } -/** - * Resolves every call expression's callee and collects the functions those - * calls resolve to, for the callees that mentioning a name doesn't already - * cover. A bare identifier this scope holds functions under - * (`functionsByName`, correctly shadowed) is skipped: its mention reaches - * those functions, and resolving it again against the file-wide `bindings` - * could land on some other, same-named function declared elsewhere. - * Anything else, such as a member call like `handlers.deliver()` or a name - * that isn't held locally, is resolved against `bindings`, the whole-file - * map used to resolve a local object literal's properties. This is also how - * a directly invoked function expression, `(() => {...})()`, gets found: - * `resolveFunctionBinding` returns a function literal callee as-is. Stops - * at a nested function's own boundary, so a call that only happens inside - * some other, not-yet-reachable function doesn't count here. It is found - * on its own once that function is resolved as reachable. - */ -function collectResolvedCallTargets( - node: unknown, - functionsByName: ReadonlyMap, - bindings: Map, - out: Set, -): void { - if (node == null || typeof node !== "object") return; - if (Array.isArray(node)) { - for (const item of node) { - collectResolvedCallTargets(item, functionsByName, bindings, out); - } - return; - } - if (!isNode(node)) return; - const n = node; - if (isFunctionLikeNode(n)) return; - - if (n.type === "CallExpression") { - const callee = n.callee as Expression; - // A name this scope already holds functions under is reached through - // its mention, and must not fall through to the file-wide map, which - // could hold a different function of the same name. - const heldLocally = callee.type === "Identifier" && - functionsByName.has(callee.name); - const resolved = heldLocally - ? null - : resolveFunctionBinding(callee, bindings); - if (resolved != null) out.add(resolved); - } - - const record = n as unknown as Record; - for (const key in record) { - if (key === "parent") continue; - collectResolvedCallTargets(record[key], functionsByName, bindings, out); - } -} - -/** - * Collects anonymous function-literal arguments that are reachable through - * either of two paths: the call chain they are passed to is awaited or - * returned (e.g. the arrow function in - * `await Promise.all(recipients.map((inbox) => ...))`, including when a - * collection literal sits between the call and the `await`, as in - * `await Promise.all([...a.map(cb), ...b.map(cb)])`), or the call - * receiving them is a method known to invoke its callback synchronously - * regardless of what happens to its own return value (`SYNCHRONOUS_ITERATION_METHODS`, - * e.g. `recipients.forEach((inbox) => ...)`). Named references passed - * either way (`recipients.map(deliver)`) don't need this: - * `collectReferencedNames` already finds them regardless of context. - */ -function collectConsumedCallbacks( - statements: readonly Node[], - impliedReturn: boolean, - out: Set, -): void { - // Expands outward from a value known to be consumed (awaited, returned, - // or the argument of a synchronously invoked method) through the shapes - // that merely carry it along -- a call's own arguments, and the - // collection literals (`[...]`, `...spread`, `{...}`) commonly used to - // gather several such values before consuming them together -- until it - // finds the function literals actually being passed. - const walkConsumed = (expr: unknown): void => { - if (expr == null || typeof expr !== "object" || Array.isArray(expr)) { - return; - } - if (!isNode(expr)) return; - if (isFunctionLikeNode(expr)) { - out.add(expr); - return; - } - if (expr.type === "CallExpression" || expr.type === "NewExpression") { - for (const arg of expr.arguments) walkConsumed(arg); - // A chained call's receiver carries the same value forward, e.g. - // `[a.map(cb)].flat()`: the array literal built from `a.map(cb)` is - // what `flat()` is called on, so it's still part of what ends up - // awaited or returned. - if ( - expr.type === "CallExpression" && - expr.callee.type === "MemberExpression" - ) { - walkConsumed(expr.callee.object); - } - return; - } - if (expr.type === "ArrayExpression") { - for (const element of expr.elements) { - if (element != null) walkConsumed(element); - } - return; - } - if (expr.type === "SpreadElement") { - walkConsumed((expr as { argument: unknown }).argument); - return; - } - if (expr.type === "ObjectExpression") { - for (const prop of expr.properties) { - if ( - isNode(prop) && prop.type === "Property" && - !(prop as { computed?: boolean }).computed - ) { - walkConsumed((prop as { value: unknown }).value); - } - } - return; - } - }; - - // Finds an `await` anywhere in `expr` -- not just at its top level, so - // `result = await Promise.all(...)` and similar wrapping still count -- - // and feeds what it awaits into `walkConsumed`. Stops at a nested - // function's own boundary. - const walkAwaitExpressions = (expr: unknown): void => { - if (expr == null || typeof expr !== "object") return; - if (Array.isArray(expr)) { - for (const item of expr) walkAwaitExpressions(item); - return; - } - if (!isNode(expr) || isFunctionLikeNode(expr)) return; - if (expr.type === "AwaitExpression") { - walkConsumed((expr as { argument: unknown }).argument); - return; - } - const record = expr as unknown as Record; - for (const key in record) { - if (key !== "parent") walkAwaitExpressions(record[key]); - } - }; - - // Finds every call to a `SYNCHRONOUS_ITERATION_METHODS` method anywhere - // in `node` and feeds its arguments to `walkConsumed`, independent of - // whether the call's own result is ever awaited, returned, or used at - // all. Stops at a nested function's own boundary. - const walkSynchronousCallbacks = (node: unknown): void => { - if (node == null || typeof node !== "object") return; - if (Array.isArray(node)) { - for (const item of node) walkSynchronousCallbacks(item); - return; - } - if (!isNode(node) || isFunctionLikeNode(node)) return; - - if (node.type === "CallExpression") { - const callee = node.callee as Expression; - if (callee.type === "MemberExpression" && !callee.computed) { - const methodName = getMemberPropertyName(callee); - if ( - methodName != null && SYNCHRONOUS_ITERATION_METHODS.has(methodName) - ) { - for (const arg of node.arguments) walkConsumed(arg); - } - } - } - - const record = node as unknown as Record; - for (const key in record) { - if (key !== "parent") walkSynchronousCallbacks(record[key]); - } - }; - - for (const statement of statements) { - walkSynchronousCallbacks(statement); - if (statement.type === "ReturnStatement") { - // A return value is consumed by definition -- no `await` needed. - if (statement.argument != null) walkConsumed(statement.argument); - continue; - } - if (statement.type === "ExpressionStatement") { - walkAwaitExpressions(statement.expression); - continue; - } - if (statement.type === "VariableDeclaration") { - for (const decl of statement.declarations) { - const init = (decl as VariableDeclarator).init as - | Node - | null - | undefined; - if (init != null) walkAwaitExpressions(init); - } - continue; - } - // Anything else is a control-flow head expression, such as the - // `await ...` in `if (await ...)`. - walkAwaitExpressions(statement); - } - - if (impliedReturn && statements.length === 1) { - const [only] = statements; - if (only.type !== "BlockStatement") walkConsumed(only); - } -} - /** * Finds function literals directly nested in a reachable statement, without * descending past them -- their own reachability is decided separately. @@ -889,17 +698,14 @@ function collectNestedFunctions( /** * Computes the full set of function nodes that are actually reachable from * `root`: `root` itself feeds a worklist, and each function it (or a - * function already on the worklist) references from *its own* reachable + * function already on the worklist) uses from *its own* reachable * statements -- never from a dead branch or some other not-yet-reached * function's body -- gets queued in turn. `outerFunctionsByName` is layered * fresh for each scope, so a name bound at an inner scope shadows a * same-named one further out instead of overwriting it globally, and a * dead branch that merely mentions a name never queues what it holds. */ -function computeUsedFunctions( - root: Node, - bindings: Map, -): Set { +function computeUsedFunctions(root: Node): Set { const used = new Set(); const visited = new Set(); @@ -945,14 +751,24 @@ function computeUsedFunctions( if (!referencedNames.has(name)) continue; for (const fn of functions) reached.add(fn); } + + // Every other function literal counts wherever it appears: a callback + // handed to `map`, `forEach`, `queue.push` or a call the rule has never + // heard of, an immediately invoked function, a returned closure. The + // rule cannot show that the receiving code never runs it, and it does + // not check whether the result is awaited: a delivery call that is + // never awaited is left alone too. Leave this as is. + const held = new Set(); + for (const functions of functionsHere.values()) { + for (const fn of functions) held.add(fn); + } for (const statement of statements) { - collectResolvedCallTargets(statement, functionsByName, bindings, reached); + const nested: FunctionLikeNode[] = []; + collectNestedFunctions(statement, nested); + for (const fn of nested) { + if (!held.has(fn)) reached.add(fn); + } } - collectConsumedCallbacks( - statements, - scopeRoot.type !== "BlockStatement", - reached, - ); for (const fn of reached) { used.add(fn); @@ -1034,9 +850,8 @@ function collectDeliveryScanCode( const listenerCallsDeliveryMethod = ( sourceCode: { getText(node: unknown): string }, listener: FunctionLikeNode, - bindings: Map, ): boolean => { - const used = computeUsedFunctions(listener.body as Node, bindings); + const used = computeUsedFunctions(listener.body as Node); const code = stripCommentsAndStrings( collectDeliveryScanCode( sourceCode, @@ -1148,9 +963,7 @@ function createRule( : null; if (resolvedListener == null) return; - if ( - listenerCallsDeliveryMethod(sourceCode, resolvedListener, bindings) - ) { + if (listenerCallsDeliveryMethod(sourceCode, resolvedListener)) { return; } diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index 6972fdaa1..2346eb150 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -789,7 +789,7 @@ federation ); test( - `${ruleName}: ❌ Bad - delivery call only inside an unawaited callback`, + `${ruleName}: ✅ Good - delivery call inside a callback whose result is dropped`, lintTest({ code: ` import { Activity } from "@fedify/vocab"; @@ -806,8 +806,6 @@ federation `, rule, ruleName, - expectedError: - "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", }), ); @@ -1550,7 +1548,7 @@ federation ); test( - `${ruleName}: ❌ Bad - map result kept in a variable but never used`, + `${ruleName}: ✅ Good - callback passed to map whose result is never used`, lintTest({ code: ` import { Activity } from "@fedify/vocab"; @@ -1568,8 +1566,6 @@ federation `, rule, ruleName, - expectedError: - "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", }), ); @@ -1969,3 +1965,124 @@ federation "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", }), ); + +test( + `${ruleName}: ✅ Good - callback passed to queue.push`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const queue = []; + queue.push(() => ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - callback passed to setTimeout`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + setTimeout(() => ctx.sendActivity(sender, inbox, activity), 0); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - callback passed to a then that is not awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + Promise.resolve().then(() => ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - callback inside an object passed to a call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = {}; + Object.assign(handlers, { + deliver: () => ctx.sendActivity(sender, inbox, activity), + }); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery call that is never awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad - callback passed to a call that never delivers`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const queue = []; + queue.push(() => console.log("no delivery in here")); + }); +`, + rule, + ruleName, + expectedError: + "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", + }), +);