refactor: retire function-keyword in favor of arrow/method syntax per erights review - #474
Conversation
|
@kriscendobot Please shepherd and run the gauntlet. Please also move the design into a more permanent location for future reference and frame as documentation of house style. Ensure that it can be found from CLAUDE.md. Dispatch a gardener to reinforce this house style going forward. |
…cation Move designs/retire-function-keyword.md to docs/house-style/function-keyword.md and reframe from proposal prose to declarative house-style documentation (arrow/method syntax over function keyword, with rationale and exception categories). Add a House style section to AGENTS.md linking to docs/house-style/function-keyword.md so agents and contributors can find the rule from the root instructions file. Per kriskowal's review on PR #474 (comment 4776353908).
|
@kriskowal Addressing your comment (2026-06-23T06:25:14Z) on #474 in full: Design moved (ca341e5): CLAUDE.md linkage: AGENTS.md (which CI fix (same commit): The lint job was failing on
New HEAD: CI status: all non-lint checks were passing before this push. The lint failure was the typedoc errors above; those are now fixed. Gardener dispatch to reinforce this house style across role/juror files will follow separately from the liaison. |
Per kriskowal's directive on endojs/endo-but-for-bots#474 (2026-06-23): "Dispatch a gardener to reinforce this house style going forward." The rule (no `function` keyword in endo-family package sources; arrow and concise-method syntax instead) is now standing house style, codified in docs/house-style/function-keyword.md on the bot fork and linked from AGENTS.md. The seven legitimate-exception categories live in that doc. Landings: - skills/no-function-keyword/SKILL.md: new canonical skill. Names the rule, the four hardened-JavaScript hazards, the scope (endo-family package sources; the upstream doc carries the live exception catalogue), and per-role application discipline. - roles/builder/AGENT.md: skill listed; norm directs new code to arrow / concise-method syntax by default with inline reason comment on exception. - roles/fixer/AGENT.md: skill listed; same norm for follow-up commits, plus the directive framing for retire-function-keyword asks. - roles/cleaner/AGENT.md: skill listed; new test code follows the same rule with the inline-reason exception for fixtures that specifically exercise function-keyword behavior. - roles/jurors/purist/AGENT.md: skill listed; inquiry axis added to the primary surface (the four hazards align with the purist lens directly). - roles/jurors/warden/AGENT.md: skill listed as secondary overlap; the freeze-vs-harden boundary consequence is the warden's slice while the purist owns the introduction itself. - CLAUDE.md inventory: new skill listed. Stylist was deliberately left untouched: its remit is naming only, and the function-keyword rule is syntax shape, not naming. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per kriskowal's directive on endojs/endo-but-for-bots#474 (2026-06-23): "Dispatch a gardener to reinforce this house style going forward." The rule (no `function` keyword in endo-family package sources; arrow and concise-method syntax instead) is now standing house style, codified in docs/house-style/function-keyword.md on the bot fork and linked from AGENTS.md. The seven legitimate-exception categories live in that doc. Landings: - skills/no-function-keyword/SKILL.md: new canonical skill. Names the rule, the four hardened-JavaScript hazards, the scope (endo-family package sources; the upstream doc carries the live exception catalogue), and per-role application discipline. - roles/builder/AGENT.md: skill listed; norm directs new code to arrow / concise-method syntax by default with inline reason comment on exception. - roles/fixer/AGENT.md: skill listed; same norm for follow-up commits, plus the directive framing for retire-function-keyword asks. - roles/cleaner/AGENT.md: skill listed; new test code follows the same rule with the inline-reason exception for fixtures that specifically exercise function-keyword behavior. - roles/jurors/purist/AGENT.md: skill listed; inquiry axis added to the primary surface (the four hazards align with the purist lens directly). - roles/jurors/warden/AGENT.md: skill listed as secondary overlap; the freeze-vs-harden boundary consequence is the warden's slice while the purist owns the introduction itself. - CLAUDE.md inventory: new skill listed. Stylist was deliberately left untouched: its remit is naming only, and the function-keyword rule is syntax shape, not naming. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR refactors package source functions to avoid the function keyword in favor of arrow functions and concise method syntax, aligning with the hardened-JS safety guidance referenced in the PR description.
Changes:
- Converted many
functiondeclarations/expressions toconstarrow functions (and a few concise methods) across multiple packages. - Updated
AsyncLocalStorageprototype patching to use concise-method syntax while preserving method names for stack traces. - Added repository documentation for the “retire function keyword” house style and linked it from
AGENTS.md.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/zip/src/signature.js | Converts helper to arrow function for non-constructability/prototype safety. |
| packages/zip/src/format-writer.js | Rewrites writer helpers and exports to arrow functions/const exports. |
| packages/zip/src/format-reader.js | Rewrites reader helpers and exports to arrow functions/const exports. |
| packages/zip/src/crc32.js | Converts CRC table builder and exported function to arrow/const export. |
| packages/trampoline/src/trampoline.js | Converts exported trampolines to const arrow functions. |
| packages/ses/src/transforms.js | Converts getLineNumber helper to arrow function. |
| packages/ses/src/scope-constants.js | Converts isImmutableDataProperty helper to arrow function. |
| packages/ses/src/module-link.js | Converts validation helpers to arrow functions. |
| packages/ses/src/module-instance.js | Converts inner imports/execute functions to arrow functions. |
| packages/ses/src/intrinsics.js | Converts initProperty/initProperties/sampleGlobals to arrow functions. |
| packages/promise-kit/src/is-promise.js | Converts export to const arrow and hardens the binding. |
| packages/ocapn/src/syrup/js-representation.js | Converts encode/decode exports to const arrow functions. |
| packages/ocapn/src/syrup/encode.js | Converts internal writer helpers and export to arrow functions. |
| packages/ocapn/src/syrup/decode.js | Converts internal reader helpers and export to arrow/const export. |
| packages/ocapn/src/syrup/compare.js | Converts exported comparator to const arrow function. |
| packages/module-source/src/transform-analyze.js | Refactors analyzer factory to return an arrow function and converts helpers. |
| packages/module-source/src/babel-plugin.js | Converts makeModulePlugins to const arrow function. |
| packages/init/src/node-async-local-storage-patch.js | Replaces named function expressions with concise methods on a patches object. |
| packages/import-bundle/src/index.js | Converts importBundle export to const async arrow function. |
| packages/import-bundle/src/compartment-wrapper.js | Converts wrapInescapableCompartment export to const arrow function. |
| packages/immutable-arraybuffer/src/lib.js | Adjusts JSDoc placement and documents a TS narrowing workaround. |
| packages/harden/make-hardener.js | Converts inner enqueue helper to arrow function. |
| packages/eventual-send/src/postponed.js | Converts returned postponement handler to arrow function. |
| packages/evasive-transform/src/transform-comment.js | Converts evadeComment export to const arrow function. |
| packages/evasive-transform/src/transform-ast.js | Converts transformAst export to const arrow function. |
| packages/evasive-transform/src/parse-ast.js | Converts parseAst export to concise arrow with implicit return. |
| packages/evasive-transform/src/index.js | Converts exports to arrow functions and simplifies async wrapper. |
| packages/eslint-plugin/lib/rules/no-polymorphic-call.js | Converts helper to arrow function. |
| packages/eslint-plugin/lib/rules/no-assign-to-exported-let-var-or-function.js | Converts several inner helpers to arrow functions. |
| packages/eslint-plugin/lib/rules/harden-exports.js | Converts Program:exit visitor handler to arrow function. |
| packages/daemon/src/daemon.js | Converts formulateMarshalValue to const async arrow function. |
| packages/compartment-mapper/src/policy.js | Converts attenuator helpers to const async arrow functions. |
| packages/compartment-mapper/src/link.js | Refactors hook-maker to nested arrow functions. |
| packages/compartment-mapper/src/import-hook.js | Converts helpers and makeImportNowHookMaker export to const arrow functions. |
| packages/cli/src/prompt.js | Converts prompt export to const async arrow function. |
| packages/bundle-source/src/zip-base64.js | Converts bundleZipBase64 export to const async arrow function. |
| packages/bundle-source/src/script.js | Converts bundleScript export to const async arrow function. |
| packages/benchmark/src/benchmark.js | Converts local helpers to arrow functions (benchmark harness). |
| docs/house-style/function-keyword.md | Adds documentation defining the repository’s “no function keyword” house style and exceptions. |
| AGENTS.md | Links global house-style documentation for agent guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kriscendobot Please address copilot review feedback. |
- benchmark: fix expedtedTime typo -> expectedTime (local param rename) - module-source: makeModuleSourceAnalyzer returns a named createStaticRecord const (block body) so JSDoc/stack traces attach to the callable - eventual-send: name the postponed-operation arrow `postpone` to retain the prior function name in stack traces without the function keyword
|
Addressed the Copilot review feedback in
Verification: No shepherd needed — these are zero-behavior cosmetic edits. (Copilot also noted it couldn't run its full agentic pass for lack of an Actions runner; that's an infra note unrelated to these fixes.) |
|
@kriscendobot Please retcon the copilot review feedback. |
…jects (#542) Follow-up to #474. On #474, @kriscendobot offered to convert the `@endo/eslint-plugin` rule **visitor objects** to concise-method shorthand, and @erights asked for it in a follow-up PR ([comment](#474 (comment))): "Please do so in a follow-up PR." This is that follow-up. ## Why #474 could not do it The `object-shorthand` rule runs with `avoidQuotes: true` (inherited from `eslint-config-airbnb-base` via `@endo/style`). Quoted AST-selector visitor keys (`'Program:exit'(node) { ... }`) are exactly the case `avoidQuotes` exempts, so those handlers were left as non-shorthand properties in #474. ## What this does 1. **Scoped relaxation.** Adds an `overrides` entry to `packages/eslint-plugin/package.json`'s `eslintConfig`, scoped to `lib/rules/*.js`, setting `object-shorthand: ['error', 'always', { avoidQuotes: false }]`. This is a targeted relaxation for the rule-file visitor-object idiom, not a global flip. 2. **Applies the shorthand** to the now-eligible handler. Running `eslint .` in the package after the relaxation flags exactly one handler: the anonymous `'Program:exit': function () { ... }` in `lib/rules/harden-exports.js`. It becomes `'Program:exit'() { ... }`, and the `func-names` disable directive the conversion orphaned is removed. The `assert-fail-as-throw.js` `'Program:exit'` / `Program` handlers are **named** function expressions (`restoreAssertFailAsThrow`, `installAssertFailAsThrow`), which `object-shorthand` never converts (shorthand would drop the name), so they are correctly left untouched. Pure-shorthand conversion: no behavioral change. ## Validation - `yarn workspace @endo/eslint-plugin lint:eslint` — clean. - `yarn workspace @endo/eslint-plugin lint:types` (`tsc`) — clean. - `yarn workspace @endo/eslint-plugin test` — 137 passing. - `prettier --check` on both changed files — clean. - No `yarn.lock` change (config-only). Opened as a draft for the review gamut.
|
Relayed from the garden's automated agent (per @erights bullet 4 here: "agreed, not in this PR. Please start a separate PR with that change… Attn @kriskowal"). Before opening the separate harden exported function literals PR, a scope decision is needed (the diff shape depends on it):
Decision: (1) which base branch, and (2) narrow (just the two evasive-transform exports @erights flagged) vs. a repo-wide hardened-exports pass? Reply here and the garden will open the separate PR accordingly. |
Ferries the erights (Mark Miller) CHANGES_REQUESTED review responses from endojs/endo-but-for-bots#474 onto this PR, bringing the reviewed content to parity with the merged bots PR (final commit 470ccdfa): - Rewrite the house-style doc's hoisting-hazard and arrow/concise-method this-binding rationale; recommend concise-method generators by default; correct the sloppy-mode this-detection note. - Rename the eventual-send baseHandledPromise constructor to BaseHandledPromise (invoked with new; checks new.target). - Install AsyncLocalStorage patches via defineProperties(getOwnPropertyDescriptors(patches)) descriptor form. - Convert object-member generators (captp trapHost; daemon subscription and connection-number generators) to concise generator methods, pinning their return types. - Convert compartment-mapper assertModuleConfiguration to an arrow carrying the asserts narrowing via @overload; clarify the import-hook comment. - Cross-reference docs/house-style/function-keyword.md from each non-constructor site that deliberately keeps the function keyword. - Correct the init [[Set]] failure-mode comment. Lint and tests pass for all affected packages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ferries the erights (Mark Miller) CHANGES_REQUESTED review responses from endojs/endo-but-for-bots#474 onto this PR, bringing the reviewed content to parity with the merged bots PR (final commit 470ccdfa): - Rewrite the house-style doc's hoisting-hazard and arrow/concise-method this-binding rationale; recommend concise-method generators by default; correct the sloppy-mode this-detection note. - Rename the eventual-send baseHandledPromise constructor to BaseHandledPromise (invoked with new; checks new.target). - Install AsyncLocalStorage patches via defineProperties(getOwnPropertyDescriptors(patches)) descriptor form. - Convert object-member generators (captp trapHost; daemon subscription and connection-number generators) to concise generator methods, pinning their return types. - Convert compartment-mapper assertModuleConfiguration to an arrow carrying the asserts narrowing via @overload; clarify the import-hook comment. - Cross-reference docs/house-style/function-keyword.md from each non-constructor site that deliberately keeps the function keyword. - Correct the init [[Set]] failure-mode comment. Lint and tests pass for all affected packages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the house-style doc for arrow and concise-method syntax over the function keyword (rationale, hardened-JS hazards, exception categories) and link it from AGENTS.md. Reconstructs the doc from endojs/endo-but-for-bots#474.
Add the house-style doc for arrow and concise-method syntax over the function keyword (rationale, hardened-JS hazards, exception categories) and link it from AGENTS.md. Reconstructs the doc from endojs/endo-but-for-bots#474.
Add the house-style doc for arrow and concise-method syntax over the function keyword (rationale, hardened-JS hazards, exception categories) and link it from AGENTS.md. Reconstructs the doc from #474.
Apply Mark Miller's CHANGES_REQUESTED review (endojs/endo#3312 review 4575447499, mirrored as #474): - Rewrite the hoisting-hazards and arrow/concise-method this-binding rationale per his suggestion blocks (no TDZ on declarations; harden/freeze after a declaration does not prevent earlier mutation; arrow lexical this vs caller-this-sensitive concise methods). - Recommend concise-method generators/async generators by default; correct the false claim that function*/async function* is the only spelling. - Correct the sloppy-mode this-detection note: concise methods are caller-this-sensitive, not lexical. - Replace the function-keyword TypeScript-assertion exception: convert assertModuleConfiguration to an arrow (less-hazardous runtime) carrying the asserts narrowing via JSDoc @overload, so neither the function keyword nor a @ts-expect-error is needed; document preferring suppression over runtime hazards. - Apply his decision-question rewrite. - Clarify the import-hook getImportsFromRecord "annoying" comment (the dual record shape, orthogonal to the arrow/function-keyword spelling).
…eHandledPromise The function is a constructor (invoked with new; checks new.target), so it takes an initial-capital name per the house style. Renames the declaration and its four internal references, and updates the function-keyword.md reference. Addresses erights review on #474.
Replaces the four per-property assignments with defineProperties(prototype, getOwnPropertyDescriptors(patches)), which transfers each method via [[DefineOwnProperty]] rather than [[Set]]. Addresses erights review on #474.
…ion sites Adds a 'See docs/house-style/function-keyword.md' note at each non-constructor site that deliberately keeps the function keyword: the getThis sloppy-mode probe, the captp/ocapn module-init forward references, the eslint-plugin safeRequire forward reference, and the standalone generator sentinels in trampoline and ses. Addresses erights review on #474.
…ethods per erights review erights' CHANGES_REQUESTED review on #474 rejected deferring the object-member generator conversions to follow-up work and asked for them in this PR. Convert every naturally-object-member `function*`/`async function*` site to a concise generator method, dropping the `function` keyword while preserving behavior, name, and JSDoc typing: - captp/src/atomics.js: `trapHost` becomes a concise `async *trapHost()` method on a hardened object; the returned value still satisfies `TrapHost` and stays hardened. - daemon/src/{pet-sitter,pet-store,mail,directory,daemon}.js: the `followNameChanges`, `followIdNameChanges`, `followLocatorNameChanges`, and `followMessages` subscription generators are rewritten as concise generator methods on a single-property object literal and extracted, preserving each `const` binding, name, and `@type` annotation. - daemon/src/{daemon-node-powers,networks/tcp-netstring,web-server-node}.js: the `generateNumbers` connection-number counters likewise. Standalone top-level generator declarations (compartment-mapper's `enumerate`/`chooseModuleDescriptor`/`getParserGenerator`/infer-exports; ses's `loadWithoutErrorAnnotation`) and the intrinsic-prototype sentinels legitimately keep the keyword — forcing them into object literals would be worse style. The house-style doc's final section is rewritten to state the object-member generators are converted and to enumerate the standalone cases that keep the keyword, dropping the "follow-up tracked separately" deferral erights objected to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… erights review (#474) ## Summary Per erights's review on [#468 (comment 3439684004)](#468 (comment)), this PR retires `function`-keyword functions in favor of arrow and concise-method syntax across the package sources, except for the legitimate-exception categories enumerated in [`designs/retire-function-keyword.md`](https://github.com/endojs/endo-but-for-bots/blob/chore/retire-function-keyword/designs/retire-function-keyword.md). The motivating hazards per erights: 1. `function`-keyword functions have both `[[Construct]]` and `[[Call]]` behaviors (can be called with `new`). 2. They have an initial `prototype` property pointing at an irrelevant prototype object. 3. Because of that extra object, `freeze` is not equivalent to `harden`, leaving hazardous mutability behind. 4. Function-keyword *declarations* additionally have hoisting hazards in import cycles (TDZ-skipping). Arrow functions and concise methods have none of these properties; they are the desired default. The exceptions documented in the design file are kept and explained. ## Per-package conversions | Package | Sites converted | Notes | | --------------------- | --------------: | ----- | | promise-kit | 1 | `isPromise` | | harden | 1 | inner `enqueue` | | eventual-send | 1 | `postpone`; kept `baseHandledPromise` (documented constructor) | | trampoline | 2 | `syncTrampoline`, `asyncTrampoline`; kept `function* () {}` sentinel | | import-bundle | 2 | `importBundle`, `wrapInescapableCompartment`; kept `Compartment` (constructor) | | cli | 1 | `prompt` | | benchmark | 4 | `benchmark`, `assert`, `truthy`, `test` | | evasive-transform | 5 | all five exports | | init | 4 | rewrote `AsyncLocalStorage` patches as concise methods | | bundle-source | 2 | `bundleZipBase64`, `bundleScript`; demo files left intentional | | zip | 21 | all `function` declarations in `signature`, `crc32`, `format-reader`, `format-writer` | | ocapn (syrup) | 19 | top-level helpers + `decodeSyrup`, `encodeSyrup`, `peekTypeHint`, `makeSyrupWriter`, `compareUint8Arrays` | | module-source | 4 | `makeModulePlugins`, `createStaticRecord`, `curryImporter`, `makeImportExpr`; kept `ModuleSource`/`AbstractModuleSource` constructors and generators | | compartment-mapper | 6 | `attenuate*`, `getImportsFromRecord`, `makeImportNowHookMaker`, anonymous error throwers; kept generators and assertion fn | | daemon | 1 | `formulateMarshalValue` | | eslint-plugin | 8 | rule-helper inner functions and one visitor handler | | ses | 11 | `isImmutableDataProperty`, `getLineNumber`, `initProperty`/`initProperties`/`sampleGlobals`, `mayBe*`/`validate*` helpers in `module-link.js`, inner `imports`/`execute` in `module-instance.js` | Total: ~93 conversion sites across 17 packages. ## Legitimate exceptions kept Documented in detail in [`designs/retire-function-keyword.md`](https://github.com/endojs/endo-but-for-bots/blob/chore/retire-function-keyword/designs/retire-function-keyword.md). Summary: - **Constructor emulation**: `PseudoTypedArray` (immutable-arraybuffer), `baseHandledPromise` (eventual-send), `NewCompartment` / `Compartment` (import-bundle, ses), the SES inert-constructor pattern (`InertConstructor` for `Function`, `Date`, `RegExp`, `Error`, `Symbol`), `ModuleSource` and `AbstractModuleSource` (module-source). - **Generator and async-generator function expressions**: no arrow-function spelling exists; preserved in `trampoline`, `captp`, `compartment-mapper` (multiple), `daemon` (many), `stream`, `syrup-frame`, `netstring`, `module-source/src-xs`. - **Vendored / third-party-derived code**: `cjs-module-analyzer` (port of `es-module-lexer` with mutual recursion via hoisting), `test262-runner/test262` (tc39 test suite under separate license). - **Sloppy-mode `this` detection**: `function getThis() { return this; }` in `ses/src/assert-sloppy-mode.js`. Arrow `this` is lexical; the function-keyword `this` is what `SES_NO_SLOPPY` needs. - **TypeScript assertion functions**: `function assertX(...): asserts x is Y` cannot be expressed as a const arrow under the current TS checker; one site in `compartment-mapper/src/compartment-map.js`. - **Module-init forward references**: `convertValToSlot` / `convertSlotToVal` in `captp/src/captp.js`, `serializeAndSendMessage` in `ocapn/src/client/ocapn.js`, and `safeRequire` in `eslint-plugin/lib/rules/assert-fail-as-throw.js`. Each is referenced before its declaration during module init, which a `const` arrow cannot satisfy without restructuring the file. Restructuring is intentionally out of scope for this PR. - **Bundler runtime template literals**: `function observeImports` / `function wrapCjsFunctor` text inside `compartment-mapper/src/bundle-mjs.js` and `bundle-cjs.js`'s template-literal `runtime` strings is the bundler's *output* code, not module-side code. - **Named function expressions assigned to prototypes for stack traces**: rewritten in `init/src/node-async-local-storage-patch.js` using a `patches` object of concise methods, which retain `.name` but have no `[[Construct]]` or `prototype`. This pattern is available wherever a maintainer prefers it over the function-keyword form. ## Categories flagged for erights's review Three categories I would appreciate erights's guidance on: 1. **The SES `tame*` / `enable*` / `permits-intrinsics` and related lockdown-path declarations** (~45 sites remaining in SES). These are deferred for a follow-up audit. Each touches the security boundary and warrants per-file reasoning; converting them in one PR would have made the diff difficult to review without ballooning the changeset. I would like guidance on whether these warrant the same treatment, and if so whether a follow-up PR per file group is the right shape. 2. **Module-init forward references** in `captp`, `ocapn` client, and the vendored `assert-fail-as-throw.js`. These can be converted with a file reorder, but the reorder itself is the kind of structural change that warrants a separate decision. 3. **The `cjs-module-analyzer` port of `es-module-lexer`** (~38 sites). The file uses single-pass lexer mutual recursion that leans on hoisting; converting it would force a manual reorder and risk a performance regression in a hot path. Treated as vendored for now. ## Test results Each per-package commit is preceded by `yarn workspace <pkg> test` and `yarn workspace <pkg> lint`, all passing locally: - promise-kit: 7 tests pass; harden: 32 tests pass; eventual-send: 33 tests pass; trampoline: 11 tests pass; import-bundle: 16 tests pass; cli: 10 tests pass; benchmark: lint passes (no tests defined); evasive-transform: 52 tests pass; init: 5 tests pass; bundle-source: 39 tests pass (3 known failures pre-existing); zip: 2 tests pass; ocapn: 260 tests pass; module-source: 57 tests pass; compartment-mapper: 902 tests pass (12 known failures pre-existing); daemon: 119 tests pass; eslint-plugin: 137 tests pass; ses: 513 tests pass (2 known failures, 2 skipped pre-existing). Global `yarn lint` is clean after a prettier pass on the two files prettier preferred a different shape for. ## Closes / refs Refs: continues from feedback in #468 (erights comment 3439684004).
…jects (#542) Follow-up to #474. On #474, @kriscendobot offered to convert the `@endo/eslint-plugin` rule **visitor objects** to concise-method shorthand, and @erights asked for it in a follow-up PR ([comment](#474 (comment))): "Please do so in a follow-up PR." This is that follow-up. ## Why #474 could not do it The `object-shorthand` rule runs with `avoidQuotes: true` (inherited from `eslint-config-airbnb-base` via `@endo/style`). Quoted AST-selector visitor keys (`'Program:exit'(node) { ... }`) are exactly the case `avoidQuotes` exempts, so those handlers were left as non-shorthand properties in #474. ## What this does 1. **Scoped relaxation.** Adds an `overrides` entry to `packages/eslint-plugin/package.json`'s `eslintConfig`, scoped to `lib/rules/*.js`, setting `object-shorthand: ['error', 'always', { avoidQuotes: false }]`. This is a targeted relaxation for the rule-file visitor-object idiom, not a global flip. 2. **Applies the shorthand** to the now-eligible handler. Running `eslint .` in the package after the relaxation flags exactly one handler: the anonymous `'Program:exit': function () { ... }` in `lib/rules/harden-exports.js`. It becomes `'Program:exit'() { ... }`, and the `func-names` disable directive the conversion orphaned is removed. The `assert-fail-as-throw.js` `'Program:exit'` / `Program` handlers are **named** function expressions (`restoreAssertFailAsThrow`, `installAssertFailAsThrow`), which `object-shorthand` never converts (shorthand would drop the name), so they are correctly left untouched. Pure-shorthand conversion: no behavioral change. ## Validation - `yarn workspace @endo/eslint-plugin lint:eslint` — clean. - `yarn workspace @endo/eslint-plugin lint:types` (`tsc`) — clean. - `yarn workspace @endo/eslint-plugin test` — 137 passing. - `prettier --check` on both changed files — clean. - No `yarn.lock` change (config-only). Opened as a draft for the review gamut.
Apply Mark Miller's CHANGES_REQUESTED review (endojs/endo#3312 review 4575447499, mirrored as #474): - Rewrite the hoisting-hazards and arrow/concise-method this-binding rationale per his suggestion blocks (no TDZ on declarations; harden/freeze after a declaration does not prevent earlier mutation; arrow lexical this vs caller-this-sensitive concise methods). - Recommend concise-method generators/async generators by default; correct the false claim that function*/async function* is the only spelling. - Correct the sloppy-mode this-detection note: concise methods are caller-this-sensitive, not lexical. - Replace the function-keyword TypeScript-assertion exception: convert assertModuleConfiguration to an arrow (less-hazardous runtime) carrying the asserts narrowing via JSDoc @overload, so neither the function keyword nor a @ts-expect-error is needed; document preferring suppression over runtime hazards. - Apply his decision-question rewrite. - Clarify the import-hook getImportsFromRecord "annoying" comment (the dual record shape, orthogonal to the arrow/function-keyword spelling).
…eHandledPromise The function is a constructor (invoked with new; checks new.target), so it takes an initial-capital name per the house style. Renames the declaration and its four internal references, and updates the function-keyword.md reference. Addresses erights review on #474.
Replaces the four per-property assignments with defineProperties(prototype, getOwnPropertyDescriptors(patches)), which transfers each method via [[DefineOwnProperty]] rather than [[Set]]. Addresses erights review on #474.
…ion sites Adds a 'See docs/house-style/function-keyword.md' note at each non-constructor site that deliberately keeps the function keyword: the getThis sloppy-mode probe, the captp/ocapn module-init forward references, the eslint-plugin safeRequire forward reference, and the standalone generator sentinels in trampoline and ses. Addresses erights review on #474.
…ethods per erights review erights' CHANGES_REQUESTED review on #474 rejected deferring the object-member generator conversions to follow-up work and asked for them in this PR. Convert every naturally-object-member `function*`/`async function*` site to a concise generator method, dropping the `function` keyword while preserving behavior, name, and JSDoc typing: - captp/src/atomics.js: `trapHost` becomes a concise `async *trapHost()` method on a hardened object; the returned value still satisfies `TrapHost` and stays hardened. - daemon/src/{pet-sitter,pet-store,mail,directory,daemon}.js: the `followNameChanges`, `followIdNameChanges`, `followLocatorNameChanges`, and `followMessages` subscription generators are rewritten as concise generator methods on a single-property object literal and extracted, preserving each `const` binding, name, and `@type` annotation. - daemon/src/{daemon-node-powers,networks/tcp-netstring,web-server-node}.js: the `generateNumbers` connection-number counters likewise. Standalone top-level generator declarations (compartment-mapper's `enumerate`/`chooseModuleDescriptor`/`getParserGenerator`/infer-exports; ses's `loadWithoutErrorAnnotation`) and the intrinsic-prototype sentinels legitimately keep the keyword — forcing them into object literals would be worse style. The house-style doc's final section is rewritten to state the object-member generators are converted and to enumerate the standalone cases that keep the keyword, dropping the "follow-up tracked separately" deferral erights objected to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apply Mark Miller's CHANGES_REQUESTED review (endojs/endo#3312 review 4575447499, mirrored as #474): - Rewrite the hoisting-hazards and arrow/concise-method this-binding rationale per his suggestion blocks (no TDZ on declarations; harden/freeze after a declaration does not prevent earlier mutation; arrow lexical this vs caller-this-sensitive concise methods). - Recommend concise-method generators/async generators by default; correct the false claim that function*/async function* is the only spelling. - Correct the sloppy-mode this-detection note: concise methods are caller-this-sensitive, not lexical. - Replace the function-keyword TypeScript-assertion exception: convert assertModuleConfiguration to an arrow (less-hazardous runtime) carrying the asserts narrowing via JSDoc @overload, so neither the function keyword nor a @ts-expect-error is needed; document preferring suppression over runtime hazards. - Apply his decision-question rewrite. - Clarify the import-hook getImportsFromRecord "annoying" comment (the dual record shape, orthogonal to the arrow/function-keyword spelling).
…eHandledPromise The function is a constructor (invoked with new; checks new.target), so it takes an initial-capital name per the house style. Renames the declaration and its four internal references, and updates the function-keyword.md reference. Addresses erights review on #474.
Replaces the four per-property assignments with defineProperties(prototype, getOwnPropertyDescriptors(patches)), which transfers each method via [[DefineOwnProperty]] rather than [[Set]]. Addresses erights review on #474.
…ion sites Adds a 'See docs/house-style/function-keyword.md' note at each non-constructor site that deliberately keeps the function keyword: the getThis sloppy-mode probe, the captp/ocapn module-init forward references, the eslint-plugin safeRequire forward reference, and the standalone generator sentinels in trampoline and ses. Addresses erights review on #474.
…ethods per erights review erights' CHANGES_REQUESTED review on #474 rejected deferring the object-member generator conversions to follow-up work and asked for them in this PR. Convert every naturally-object-member `function*`/`async function*` site to a concise generator method, dropping the `function` keyword while preserving behavior, name, and JSDoc typing: - captp/src/atomics.js: `trapHost` becomes a concise `async *trapHost()` method on a hardened object; the returned value still satisfies `TrapHost` and stays hardened. - daemon/src/{pet-sitter,pet-store,mail,directory,daemon}.js: the `followNameChanges`, `followIdNameChanges`, `followLocatorNameChanges`, and `followMessages` subscription generators are rewritten as concise generator methods on a single-property object literal and extracted, preserving each `const` binding, name, and `@type` annotation. - daemon/src/{daemon-node-powers,networks/tcp-netstring,web-server-node}.js: the `generateNumbers` connection-number counters likewise. Standalone top-level generator declarations (compartment-mapper's `enumerate`/`chooseModuleDescriptor`/`getParserGenerator`/infer-exports; ses's `loadWithoutErrorAnnotation`) and the intrinsic-prototype sentinels legitimately keep the keyword — forcing them into object literals would be worse style. The house-style doc's final section is rewritten to state the object-member generators are converted and to enumerate the standalone cases that keep the keyword, dropping the "follow-up tracked separately" deferral erights objected to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Per erights's review on
endojs/endo-but-for-bots#468 (comment 3439684004),
this PR retires
function-keyword functions in favor of arrowand concise-method syntax across the package sources, except
for the legitimate-exception categories enumerated in
designs/retire-function-keyword.md.The motivating hazards per erights:
function-keyword functions have both[[Construct]]and[[Call]]behaviors (can be called withnew).prototypeproperty pointing at anirrelevant prototype object.
freezeis not equivalent toharden, leaving hazardous mutability behind.hazards in import cycles (TDZ-skipping).
Arrow functions and concise methods have none of these
properties; they are the desired default. The exceptions
documented in the design file are kept and explained.
Per-package conversions
isPromiseenqueuepostpone; keptbaseHandledPromise(documented constructor)syncTrampoline,asyncTrampoline; keptfunction* () {}sentinelimportBundle,wrapInescapableCompartment; keptCompartment(constructor)promptbenchmark,assert,truthy,testAsyncLocalStoragepatches as concise methodsbundleZipBase64,bundleScript; demo files left intentionalfunctiondeclarations insignature,crc32,format-reader,format-writerdecodeSyrup,encodeSyrup,peekTypeHint,makeSyrupWriter,compareUint8ArraysmakeModulePlugins,createStaticRecord,curryImporter,makeImportExpr; keptModuleSource/AbstractModuleSourceconstructors and generatorsattenuate*,getImportsFromRecord,makeImportNowHookMaker, anonymous error throwers; kept generators and assertion fnformulateMarshalValueisImmutableDataProperty,getLineNumber,initProperty/initProperties/sampleGlobals,mayBe*/validate*helpers inmodule-link.js, innerimports/executeinmodule-instance.jsTotal: ~93 conversion sites across 17 packages.
Legitimate exceptions kept
Documented in detail in
designs/retire-function-keyword.md.Summary:
PseudoTypedArray(immutable-arraybuffer),baseHandledPromise(eventual-send),NewCompartment/Compartment(import-bundle, ses), the SES inert-constructorpattern (
InertConstructorforFunction,Date,RegExp,Error,Symbol),ModuleSourceandAbstractModuleSource(module-source).
arrow-function spelling exists; preserved in
trampoline,captp,compartment-mapper(multiple),daemon(many),stream,syrup-frame,netstring,module-source/src-xs.cjs-module-analyzer(port of
es-module-lexerwith mutual recursion via hoisting),test262-runner/test262(tc39 test suite under separate license).thisdetection:function getThis() { return this; }in
ses/src/assert-sloppy-mode.js. Arrowthisis lexical; thefunction-keyword
thisis whatSES_NO_SLOPPYneeds.function assertX(...): asserts x is Ycannot be expressed as a const arrow under the current TS checker;
one site in
compartment-mapper/src/compartment-map.js.convertValToSlot/convertSlotToValincaptp/src/captp.js,serializeAndSendMessagein
ocapn/src/client/ocapn.js, andsafeRequireineslint-plugin/lib/rules/assert-fail-as-throw.js. Each isreferenced before its declaration during module init, which a
constarrow cannot satisfy without restructuring the file.Restructuring is intentionally out of scope for this PR.
function observeImports/function wrapCjsFunctortext insidecompartment-mapper/src/bundle-mjs.jsandbundle-cjs.js's template-literalruntimestrings is thebundler's output code, not module-side code.
traces: rewritten in
init/src/node-async-local-storage-patch.jsusing a
patchesobject of concise methods, which retain.namebut have no
[[Construct]]orprototype. This pattern isavailable wherever a maintainer prefers it over the
function-keyword form.
Categories flagged for erights's review
Three categories I would appreciate erights's guidance on:
tame*/enable*/permits-intrinsicsand relatedlockdown-path declarations (~45 sites remaining in SES). These
are deferred for a follow-up audit. Each touches the security
boundary and warrants per-file reasoning; converting them in one
PR would have made the diff difficult to review without
ballooning the changeset. I would like guidance on whether
these warrant the same treatment, and if so whether a follow-up
PR per file group is the right shape.
captp,ocapnclient,and the vendored
assert-fail-as-throw.js. These can beconverted with a file reorder, but the reorder itself is the
kind of structural change that warrants a separate decision.
cjs-module-analyzerport ofes-module-lexer(~38sites). The file uses single-pass lexer mutual recursion that
leans on hoisting; converting it would force a manual reorder
and risk a performance regression in a hot path. Treated as
vendored for now.
Test results
Each per-package commit is preceded by
yarn workspace <pkg> testand
yarn workspace <pkg> lint, all passing locally:33 tests pass; trampoline: 11 tests pass; import-bundle: 16
tests pass; cli: 10 tests pass; benchmark: lint passes (no
tests defined); evasive-transform: 52 tests pass; init: 5 tests
pass; bundle-source: 39 tests pass (3 known failures
pre-existing); zip: 2 tests pass; ocapn: 260 tests pass;
module-source: 57 tests pass; compartment-mapper: 902 tests
pass (12 known failures pre-existing); daemon: 119 tests pass;
eslint-plugin: 137 tests pass; ses: 513 tests pass (2 known
failures, 2 skipped pre-existing).
Global
yarn lintis clean after a prettier pass on the twofiles prettier preferred a different shape for.
Closes / refs
Refs: continues from feedback in
#468 (erights comment 3439684004).