refactor: retire function-keyword in favor of arrow/method syntax - #3312
Open
kriskowal wants to merge 19 commits into
Open
refactor: retire function-keyword in favor of arrow/method syntax#3312kriskowal wants to merge 19 commits into
kriskowal wants to merge 19 commits into
Conversation
|
kriskowal
force-pushed
the
chore/retire-function-keyword
branch
from
June 25, 2026 17:01
8dfde6e to
aaad5c7
Compare
erights
requested changes
Jun 25, 2026
Comment on lines
+18
to
+22
| 4. Function-keyword declarations additionally have hoisting hazards. | ||
| A function declaration is hoisted and fully initialized before the module | ||
| body runs — it has no temporal dead zone — so in an import cycle one side | ||
| can observe the function as a value before the rest of the module has run, | ||
| masking initialization-order bugs. |
Contributor
There was a problem hiding this comment.
Suggested change
| 4. Function-keyword declarations additionally have hoisting hazards. | |
| A function declaration is hoisted and fully initialized before the module | |
| body runs — it has no temporal dead zone — so in an import cycle one side | |
| can observe the function as a value before the rest of the module has run, | |
| masking initialization-order bugs. | |
| 4. Function-keyword declarations additionally have hoisting hazards. | |
| A function declaration `f` is hoisted and fully initialized before the | |
| module body runs — it has no temporal dead zone — masking | |
| initialization-order bugs. For example, a `harden(f)` or `freeze(f)` | |
| immediately after the function-keyword declaration of `f` does not | |
| prevent other code from mutating `f` before it is frozen. | |
| - This hazard exists even among code within one module, though | |
| these are less urgent because the eslint `no-use-before-define` | |
| rule reliably flags these hazards. | |
| - For an `export function f` exported function-keyword function | |
| declaration `f` in an import cycle, the importer can observe the | |
| function as a value before the rest of the exporting module has run. | |
| This case cannot be reliably avoided by other means. |
Comment on lines
+24
to
+30
| The arrow function `() => {}` form has none of these hazards: no | ||
| `[[Construct]]`, no `prototype`, no early initialization (a `const` binding | ||
| stays in its temporal dead zone until evaluated), and `freeze` is equivalent to | ||
| `harden`. | ||
| Concise-method syntax (`{ name() {} }`, `{ get name() {} }`, | ||
| `{ set name(v) {} }`) likewise has no `[[Construct]]` and no `prototype`, | ||
| while still binding `this`. |
Contributor
There was a problem hiding this comment.
Suggested change
| The arrow function `() => {}` form has none of these hazards: no | |
| `[[Construct]]`, no `prototype`, no early initialization (a `const` binding | |
| stays in its temporal dead zone until evaluated), and `freeze` is equivalent to | |
| `harden`. | |
| Concise-method syntax (`{ name() {} }`, `{ get name() {} }`, | |
| `{ set name(v) {} }`) likewise has no `[[Construct]]` and no `prototype`, | |
| while still binding `this`. | |
| The arrow function `() => {}` form has none of these hazards: no | |
| `[[Construct]]`, no `prototype`, no early initialization (a `const` binding | |
| stays in its temporal dead zone until evaluated), and `freeze` is equivalent to | |
| `harden`. An arrow function lexically binds `this`, meaning that it is insensitive | |
| to the `this`-binding provided by its callers. | |
| Concise-method syntax (`{ name() {} }`, `{ get name() {} }`, | |
| `{ set name(v) {} }`) likewise has no `[[Construct]]` and no `prototype`, | |
| while being sensitive to the `this`-binding provided by its callers. |
|
|
||
| ECMAScript provides no arrow-function spelling for generators or | ||
| async-generators. | ||
| A `function*` or `async function*` expression is the only way to write one. |
Contributor
There was a problem hiding this comment.
Not true. These could be written with concise method syntax. Unless there is a separate reason for using function-keyword functions, we should prefer concise method syntax for generators and async generators.
Comment on lines
+127
to
+129
| Arrow functions and concise methods bind `this` lexically, which would make | ||
| `getThis()` return the module-scope `this` (always `undefined` under modules), | ||
| defeating the check. |
Contributor
There was a problem hiding this comment.
Arrow functions bind this lexically, meaning they are insensitive to the this binding provided by their callers. Both function-keyword functions and concise methods do not, meaning they are sensitive to the this binding provided by their callers in the same way. I believe this is just as true for sloppy mode though I have not checked. If so, we should still prefer concise methods for this case.
Comment on lines
+133
to
+140
| `function assertX(...): asserts x is Y` requires a function declaration under | ||
| the current TypeScript checker; converting to an arrow drops the `asserts` | ||
| narrowing and the compiler emits TS2775 ("Assertions require every name in the | ||
| call target to be declared with an explicit type annotation"). | ||
| Where the function is an assertion, the declaration stays. | ||
| Concrete site: | ||
| `packages/compartment-mapper/src/compartment-map.js`: | ||
| `function assertModuleConfiguration`. |
Contributor
There was a problem hiding this comment.
Yuck. I don't even understand this. But if in all of endo these are the only exceptions, maybe I don't need to.
I will note that I hate to introduce actual runtime hazards in order to workaround a weakness of our static checking tools. In general, I prefer an @ts-expect-error or @ts-ignore to suppress the weakness of static checking tools in order to preserve less hazardous runtime behavior, which is what counts.
Member
There was a problem hiding this comment.
I'm not sure I understand the claim either, but I think I have a counterexample.
Comment on lines
+20
to
+21
| // Named so stack traces and `.name` keep reporting `postpone` for | ||
| // postponed operations, without reintroducing the `function` keyword. |
Member
There was a problem hiding this comment.
SES also has a defineName helper for this purpose that should probably be made available in @endo/common.
| * @typedef {object} AsyncLocalStorageInternal | ||
| * @property {boolean} enabled | ||
| * @property {typeof _propagate} _propagate | ||
| * @property {(resource: object, triggerResource: object, type?: string) => void} _propagate |
Contributor
There was a problem hiding this comment.
If this is just a drive-by, fine, I don't need to understand it.
OTOH, if it is somehow entangled with the purpose of this PR, I'd like to first understand how before approving.
Comment on lines
+104
to
+107
| AsyncLocalStorage.prototype._propagate = patches._propagate; | ||
| AsyncLocalStorage.prototype.enterWith = patches.enterWith; | ||
| AsyncLocalStorage.prototype.run = patches.run; | ||
| AsyncLocalStorage.prototype.getStore = patches.getStore; |
Contributor
There was a problem hiding this comment.
Having done the (very nice!) refactoring of bundling all these concise methods as the methods of one patches object, I wonder if these four lines should instead be
Suggested change
| AsyncLocalStorage.prototype._propagate = patches._propagate; | |
| AsyncLocalStorage.prototype.enterWith = patches.enterWith; | |
| AsyncLocalStorage.prototype.run = patches.run; | |
| AsyncLocalStorage.prototype.getStore = patches.getStore; | |
| defineProperty(AsyncLocalStorage.prototype, getOwnPropertyDescriptors(patches)); |
?
If, for some bizarre reason, we actually prefer assignment semantics here over descriptor preservation, should they be
Suggested change
| AsyncLocalStorage.prototype._propagate = patches._propagate; | |
| AsyncLocalStorage.prototype.enterWith = patches.enterWith; | |
| AsyncLocalStorage.prototype.run = patches.run; | |
| AsyncLocalStorage.prototype.getStore = patches.getStore; | |
| Object.assign(AsyncLocalStorage.prototype, patches); |
?
| The author must remember to harden the wrapping closure, not just freeze it. | ||
| We accept this trade-off because the alternative is no generator at all. | ||
|
|
||
| Examples kept under this exception: |
Contributor
There was a problem hiding this comment.
The constructor cases are obvious enough that documenting these in this file alone is sufficient, especially if the identifiers are initial-caps. For the others, please put in a comment at least just saying "See" this function-keyword.md file.
| emulates a built-in TypedArray constructor (uses `new.target`, | ||
| `construct(...)`, and exposes `prototype`). | ||
| - `packages/eventual-send/src/handled-promise.js`: | ||
| `function baseHandledPromise`, which the author already documented as |
Contributor
There was a problem hiding this comment.
Should this be
Suggested change
| `function baseHandledPromise`, which the author already documented as | |
| `function BaseHandledPromise`, which the author already documented as |
?
As of this PR, it is especially important to maintain the constructor-names-are-initial-caps rule because the difference is even more important.
erights
reviewed
Jun 25, 2026
erights
left a comment
erights
left a comment
Contributor
There was a problem hiding this comment.
I also notice there is no changeset. Is this because it is a pure refactor with no behavioral change for correct clients? But there's still the potential breakage for "incorrect" clients.
kriscendobot
pushed a commit
to endojs/endo-but-for-bots
that referenced
this pull request
Jun 26, 2026
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).
kriskowal
force-pushed
the
chore/retire-function-keyword
branch
from
July 1, 2026 17:55
5ca6a7c to
0393e69
Compare
Contributor
|
The (24.x macos015) CI failure looks like a flake, so I'm rerunning failed tests. |
kriskowal
force-pushed
the
chore/retire-function-keyword
branch
from
July 1, 2026 23:50
0393e69 to
a97452b
Compare
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.
kriskowal
force-pushed
the
chore/retire-function-keyword
branch
from
July 2, 2026 15:12
a97452b to
81e6225
Compare
This was referenced Jul 16, 2026
kriscendobot
pushed a commit
to endojs/endo-but-for-bots
that referenced
this pull request
Jul 17, 2026
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).
kriscendobot
pushed a commit
to endojs/endo-but-for-bots
that referenced
this pull request
Jul 17, 2026
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).
kriscendobot
pushed a commit
to endojs/endo-but-for-bots
that referenced
this pull request
Jul 17, 2026
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Retires
function-keyword functions in favor of arrow and concise-method syntax across package sources, except for a documented set of legitimate-exception categories. Addsdocs/house-style/function-keyword.mdcapturing the rule and rationale, and links it fromAGENTS.md.The motivating hardened-JS hazards of
function-keyword functions:[[Construct]]and[[Call]]behaviors (callable withnew).prototypeproperty pointing at an irrelevant object.freezeis not equivalent toharden, leaving hazardous mutability behind.Arrow functions and concise methods have none of these; they are the desired default. Exceptions are documented and explained.
Ferried from the bot fork: endojs/endo-but-for-bots#474, rebased onto current
master. Originated from erights's review on endojs/endo-but-for-bots#468.Per-package conversions
~93 conversion sites across 17 packages: promise-kit, harden, eventual-send, trampoline, import-bundle, cli, benchmark, evasive-transform, init, bundle-source, zip (21), ocapn/syrup (19), module-source, compartment-mapper, daemon, eslint-plugin (8), and low-risk helpers in ses (11). See
docs/house-style/function-keyword.mdfor the full rule.Legitimate exceptions kept (see docs/house-style/function-keyword.md)
baseHandledPromise(eventual-send),NewCompartment/Compartment(import-bundle, ses), the SES inert-constructor pattern,ModuleSource/AbstractModuleSource.cjs-module-analyzer(port ofes-module-lexer, mutual recursion via hoisting),test262-runner/test262.thisdetection:function getThis() { return this; }inses/src/assert-sloppy-mode.js.function assertX(...): asserts x is Yincompartment-mapper/src/compartment-map.js.convertValToSlot/convertSlotToVal(captp),serializeAndSendMessage(ocapn client),safeRequire(eslint-plugin) — referenced before declaration during module init; restructuring is out of scope here.observeImports/wrapCjsFunctortext inside compartment-mapper'sbundle-mjs.js/bundle-cjs.jsruntime strings (that's emitted output, not module-side code).Ferry note: immutable-arraybuffer deferred
The bot-fork PR also converted a couple of sites in
packages/immutable-arraybuffer/src/lib.js, but those sit inside the freezable-TypedArray emulation block that is still in flight upstream as #3311. That hunk is intentionally omitted from this PR and should ride with #3311 (or a trivial follow-up) once it lands. No other content was dropped.Categories flagged for review
tame*/enable*/ permits-intrinsics / lockdown-path declarations (~45 sites remaining in SES) — deferred for a per-file follow-up audit since each touches the security boundary.assert-fail-as-throw.js— convertible with a file reorder, which warrants a separate decision.cjs-module-analyzerport ofes-module-lexer(~38 sites) — single-pass lexer mutual recursion leaning on hoisting; treated as vendored to avoid a hot-path regression.Test results
yarn lintand per-package test suites pass on top of currentmaster, including: compartment-mapper 902, ocapn 260, ses 510, eslint-plugin 137, module-source 57, evasive-transform 52, bundle-source 39, eventual-send 33, harden 32, import-bundle 16, trampoline 11, promise-kit 7, init 5, zip 2.Refs
Continues from feedback in endojs/endo-but-for-bots#468 and #474.