diff --git a/AGENTS.md b/AGENTS.md index 2ac2a2693..89412ae8a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,6 +39,7 @@ actually touches its subject. 3. [FunctionalScript and TypeScript (`fjs/`)](#3-functionalscript-and-typescript-fjs) 4. [Rust (`nanvm-lib/`)](#4-rust-nanvm-lib) 5. [Pull requests and releases](#5-pull-requests-and-releases) +6. [External tools](#6-external-tools) --- @@ -123,3 +124,26 @@ Commit-message format and the PR checklist: [CONTRIBUTING.md](./CONTRIBUTING.md#opening-a-pull-request). Changelog entry rules, breaking changes, and versioning: [changelog/README.md](./changelog/README.md). + +## 6. External tools + +**Do not call an external tool from our code — a CI step, a script, a +generator — without approval first.** `grep`, `sed`, `awk` and their kin +included. + +Text matching is not analysis. A pattern over source text cannot tell a JSDoc +tag from the same characters inside a string or a comment, so a check built on +one returns confident answers it has no basis for. A `grep` guard for `@module` +placement flagged the very file whose assertions named the guard, and its +companion could not have seen a missing tag in any file that mentioned the tag +anywhere — a check that cannot fail is indistinguishable from one that passes. +Where a rule needs real analysis, the answer is an established tool that parses +what it checks — ESLint for JavaScript, Clippy for Rust — proposed and approved +before it is added, never a pattern that approximates one. + +**Leaving the check undone is the better trade against that complexity.** A +rule no available tool can express stays written down and unenforced. That is +honest, and cheaper than machinery whose failures are silent. + +Keep simple tasks simple; a script earns its place only where the task genuinely +is not. Instances predating this rule are not precedent for new ones. diff --git a/fjs/AGENTS.md b/fjs/AGENTS.md index e43afc9fe..1faa78133 100644 --- a/fjs/AGENTS.md +++ b/fjs/AGENTS.md @@ -48,18 +48,19 @@ the `proof.mjs` filename convention. A `proof.f.mjs` is authored `.f.mjs` like any other. Its relative **runtime** imports must target `.f.mjs` modules. Type-only APIs may live in an authored `types.ts` companion and are referenced directly through that real source path. -Its leading module JSDoc block may include, for example: +Its leading JSDoc block may include, for example: ```js /** * ... * - * @module - * * @import { Phantom } from '../phantom/types.ts' */ ``` +No `@module`: a proof's documentation is not published, so the tag has nothing +to attach it to (§2). + JSDoc `@import` introduces no runtime dependency; a `types.ts` file naming the same path from TypeScript uses `import type` instead. A type that several modules need independently of one implementation belongs in `types.ts`, not in a JSDoc @@ -132,12 +133,41 @@ normally the part of the contract that matters. ## 2. Documentation Use JSDoc for module documentation in both JavaScript and TypeScript source. -The `@module` tag belongs only to a package's entry-point file — `module.f.mjs` / -`module.mjs` — not to `proof.f.mjs`, `types.ts`, or any other file. A `module.*` -file starts with one module JSDoc block carrying `@module`, followed by one blank -line before the first source-level import or declaration. A `proof.*` or other -non-`module.*` file has no `@module` tag and no required leading documentation -block; one is still needed if the file has `@import` tags to hold, per below. + +**`@module` is what makes a leading block *be* module documentation.** It is not +a marker of entry-point-ness. `deno doc` reads the tag and nothing else: a file +whose leading block carries it gets that prose as its `module_doc`, and a file +without it gets no `module_doc` at all — the block is dropped, not demoted. +Verified against the pinned Deno (`fjs/ci/config/module.f.mjs`), for `.mjs` and +`.ts` alike; the tag need not be in the first block, only in some block. + +**So the tag goes wherever a file has module-level documentation a reader is +meant to get from `deno doc`** — `module.f.mjs`, `types.ts`, `private.ts` — and a +`module.*` file always has some. A file whose leading block only holds `@import` +tags has nothing to attach and wants no `@module`. Where a file's documentation +reaches no reader, the tag buys nothing; `proof.*` is the clear case. + +Which reader differs by file kind, and the tag does not decide it. +`module.f.mjs` and `types.ts` are public API surface. `private.ts` is not: it +holds implementation-private types outside the public declaration closure, and +[`todo/separate-private-types.md`](./todo/separate-private-types.md) plans to +drop its generated declarations from the package altogether. Its prose is for +contributors reading the sources, so the tag belongs there — but a public +documentation build must not be pointed at it. + +Put it in the leading block, followed by one blank line before the first +source-level import or declaration. + +The tree does not obey this yet. #1756 stripped the tag from 102 files and +#1750 from 16 `private.ts`, on the older reading; the prose survives in source +and `deno doc` cannot see it. Restoring it is +[`fjs/todo/module-tag-restore.md`](./todo/module-tag-restore.md); until that +lands, an untagged `types.ts` or `private.ts` is debt rather than an example to +copy. + +The tag is necessary, not sufficient. It decides whether `deno doc` *can* see a +file's module documentation; whether anything is generated from that file is a +separate question of what the documentation build is pointed at. Group all module-level `@import` tags into one leading JSDoc comment block — the same block as `@module` in a `module.*` file, or a standalone block at the top of diff --git a/fjs/ci/todo/node26-typedef-gate-reaches-consumers.md b/fjs/ci/todo/node26-typedef-gate-reaches-consumers.md new file mode 100644 index 000000000..31f91896c --- /dev/null +++ b/fjs/ci/todo/node26-typedef-gate-reaches-consumers.md @@ -0,0 +1,78 @@ +## node26-typedef-gate-reaches-consumers. `fjs ci` ships our `@typedef` rule + +**Priority:** P5 +**Status:** open + +### Problem + +`fjs ci` is offered to other projects as "FunctionalScript's default workflow" +([`fjs/README.md`](../../README.md)), and `ci(setup)` lets a caller vary only +`nodeExtra`, which reaches the per-OS platform jobs. The canonical Node jobs +come from `nodeVersionJobs` unconditionally, so every consumer's generated +`ci.yml` also gets the `node26` job. + +Most of that job is a documented contract and works as intended. +[`../README.md`](../README.md) states which commands a consuming +`package.json` must provide — `cov` and `ci-update` — shows the typical +definitions, and explains that a project chains its own generators into +`ci-update` so the drift check covers them for free. This repository's own +`ci-update` spells itself `node ./fjs/module.mjs ci && …` only to avoid +depending on the package bin before the package is installed, which that README +says outright. So `npm run ci-update` and its drift check are an extension +point, not a private gate. + +**One step is not covered by that contract: the file-scope JSDoc `@typedef` +prohibition.** It comes from root `AGENTS.md`, nothing asks a consumer to adopt +it, and no `Setup` field turns it off. A project that follows the documented +setup exactly — defines both scripts, writes ordinary JSDoc — gets a red +`node26` for breaking a rule that is not theirs, and the failure names a +convention they have never read. + +P5: a reviewer notices this kind of thing, and no project outside this +repository is known to run `fjs ci` at all — see the question under the +options. Raise it the day one turns up. + +That narrowness is the finding. Because the rest of the job does work for a +consumer who follows the documentation, a convention gate added to it is not +lost in an already-broken job: it is the one thing standing between them and a +green build. A pair of `@module` gates was very nearly added here for that +reason and reverted first — [root `AGENTS.md` +§6](../../../AGENTS.md#6-external-tools) now rules that approach out — but §6 +governs *how* such a check is built, not whether `node26` is where it belongs. + +### Proposal + +No design agreed; the choice is what `fjs ci` is *for*. + +- **Split the job.** `nodeVersionJobs` yields the portable per-version jobs; + this repository's convention gates move to a `nodeExtra`-style hook it passes + itself. A consumer keeps the documented `cov`/`ci-update` contract and gets + none of our conventions. +- **Or narrow the claim.** Keep the job as it is and say in `fjs/README.md` and + [`../README.md`](../README.md) that `fjs ci` generates *this* repository's + workflow, and that other projects should use `fjs run ` — + which `fjs/README.md` already offers as the escape hatch. + +The first is the better API and the second is honest about today's. Either +settles it; leaving both claims standing is what should not continue. + +Worth checking before choosing: whether any project outside this repository +actually runs `fjs ci`. If none does, the second option costs a paragraph and +the first is speculative generality. + +### Tasks + +- [ ] Decide which of the two the command is. +- [ ] Apply it, and make `fjs/README.md` and [`../README.md`](../README.md) + agree — today the first offers the command to other projects and the + second says the directory defines "the GitHub Actions workflow for this + repository". + +### Related + +- [`../node/module.f.mjs`](../node/module.f.mjs) — `node26Steps`, the job in + question. +- [`../module.f.mjs`](../module.f.mjs) — `ci(setup)` and `canonicalJobs`, where + the jobs are assembled and `nodeExtra` stops short. +- [`../README.md`](../README.md) — describes the generator as this + repository's; `fjs/README.md` offers it to others. diff --git a/fjs/todo/module-tag-restore.md b/fjs/todo/module-tag-restore.md new file mode 100644 index 000000000..b9daa1010 --- /dev/null +++ b/fjs/todo/module-tag-restore.md @@ -0,0 +1,105 @@ +## module-tag-restore. Put `@module` back where documentation is published + +**Priority:** P3 +**Status:** open + +### Problem + +[#1756](https://github.com/functionalscript/functionalscript/pull/1756) stripped +`@module` from 102 files, on the reading that the tag marks a package entry +point. That reading was wrong. The tag is what makes a leading JSDoc block *be* +module documentation: `deno doc` emits `module_doc` for a file carrying it and +nothing at all for a file without it — the block is dropped, not demoted. +Verified against the pinned Deno, for `.mjs` and `.ts` alike. + +98 of those 102 files had real prose in the block, and +[#1750](https://github.com/functionalscript/functionalscript/pull/1750) did the +same to 16 `private.ts` on the same reading — all 16 have prose and none carry +the tag. Their module documentation is still in the source and `deno doc` cannot +see it. Nothing is broken at runtime. + +The tag is necessary, not sufficient. +[`../website/todo/publish-deno-doc-to-website.md`](../website/todo/publish-deno-doc-to-website.md) +currently plans `deno doc --html **/module.f.mjs`, a glob that excludes every +`types.ts` and `private.ts`, so restoring the tag alone would not put these +descriptions on the website. Restoring it is what makes them *available* to be +read at all; where they are then shown is a separate decision, and it is not the +same decision for the two file kinds. + +**`types.ts` yes, `private.ts` no.** `types.ts` is the public type-level API, so +widening that glob to reach it belongs to the website issue. `private.ts` holds +implementation-private types outside the public declaration closure, and +[`separate-private-types.md`](./separate-private-types.md) plans to drop its +generated declarations from the package in Stage 2 — putting them on the public +API site would publish exactly what that design removes. Its prose is worth the +tag for contributors reading the sources or running `deno doc` themselves; it is +not website input. + +[`../AGENTS.md`](../AGENTS.md) §2 now states the rule correctly — the tag goes +wherever a file has module-level documentation a reader is meant to get from +`deno doc`, whoever that reader is. This issue is the tree catching up. + +### Proposal + +Four groups, and the first two are mechanical. + +**1. Restore — 89 `types.ts`, all with prose.** Put `@module` back in the +leading block. `types.ts` is the entry point of the type-level API and its +emitted declarations are what a package consumer reads, so this is squarely +documentation a reader is meant to get. The exact text is recoverable per file: + +```sh +git show 0233904^: +``` + +**2. Restore — 16 `private.ts`, all with prose.** Stripped by #1750 rather than +#1756, so they are not in the 102, but the same reading and the same fix. +`../AGENTS.md` §2 names `private.ts` alongside `types.ts`, and now says why the +audience is not the same one: the tag makes the prose reachable by `deno doc` for +a contributor, and the public site must stay pointed away from these files. Do +not carry this group into the website glob. + +**3. Leave — 11 proof files** (8 with prose, 3 with a bare tag). Proof +documentation is not published, so by the rule the tag has nothing to attach to, +and `../AGENTS.md` §1.2's proof example now shows a block without it. Worth +confirming rather than assuming: if `deno doc` is ever pointed at proofs, the +answer flips. The three bare-tag ones lost nothing either way. + +**4. Judge individually — two files that are neither.** + +- `fjs/bnf/testlib.f.mjs` — its block held only `@import` tags, no prose. Under + the rule there is nothing to attach, so it wants no tag. Nothing to restore. +- `fjs/emergent_testing/browser.mjs` — real prose ("Browser-native proof + execution and report rendering", and why it has no Node dependencies). It is + a published module in the package, so it reads like group 1. + +### Tasks + +- [ ] Restore the tag in the 89 `types.ts` files. +- [ ] Restore it in the 16 `private.ts` files, without adding them to any public + documentation build. +- [ ] Restore `fjs/emergent_testing/browser.mjs`; leave `fjs/bnf/testlib.f.mjs`. +- [ ] Confirm the proof decision, and record it in + [`../AGENTS.md`](../AGENTS.md) §2 rather than only here. +- [ ] Correct the copy of the old rule in + [`../../todo/migrate-typescript-to-mjs.md`](../../todo/migrate-typescript-to-mjs.md) + ("Module header and import ordering"), which still states the tag belongs + only to an entry point — it was restated there rather than linked, so it + did not move when §2 did. +- [ ] Drop §2's paragraph saying the tree does not obey the rule yet, once it + does. +- [ ] Spot-check with `deno doc --json` on a restored file that `module_doc` + comes back, rather than trusting the edit. + +### Related + +- [`../AGENTS.md`](../AGENTS.md) §2 — the rule, and why the tag exists. +- [`../website/todo/publish-deno-doc-to-website.md`](../website/todo/publish-deno-doc-to-website.md) + — the other half for group 1: its `**/module.f.mjs` glob would have to widen to + `types.ts` before those descriptions reach a website reader. Not to + `private.ts`. +- [`separate-private-types.md`](./separate-private-types.md) — why `private.ts` + is contributor-facing only, and why Stage 2 drops its declarations from the + package. +- [`../../todo/jsdoc-verification.md`](../../todo/jsdoc-verification.md) — how a + rule like this might be checked at all, which is why it drifted twice unnoticed. diff --git a/todo/jsdoc-verification.md b/todo/jsdoc-verification.md new file mode 100644 index 000000000..2547b1044 --- /dev/null +++ b/todo/jsdoc-verification.md @@ -0,0 +1,73 @@ +## jsdoc-verification. Investigate how JSDoc correctness could be checked + +**Priority:** P4 +**Status:** open + +### Problem + +Several JSDoc rules are documented and none are checkable. `fjs/AGENTS.md` §2 +states where `@module` goes, how `@import` tags are grouped, and what a leading +block must contain; the root `AGENTS.md` prohibits file-scope `@typedef` in +authored `.mjs`. `tsc` sees none of it — a tag in the wrong place, missing, or +absent from a file that needs one all type-check clean. + +The consequences are not hypothetical. `@module` drifted onto 102 files against +the documented rule, was stripped from all of them on a misreading of *why* the +rule existed, and the misreading survived a merge because nothing could tell the +difference. That is three passes over the same tag with no signal at any point. + +The obvious repair is not available. Root [`AGENTS.md` +§6](../AGENTS.md#6-external-tools) rules out approximating this with a text +pattern, and for a good reason discovered the hard way: a `grep` for `@module` +cannot distinguish a JSDoc tag from the same characters in a string or a +comment, and the guard built that way flagged the file whose assertions named +it. A checker has to parse. + +### Proposal + +An investigation, not a design. What to establish: + +- **What already parses this.** `deno doc --json` yields `module_doc` and + per-symbol `jsDoc` — enough to answer "does this file publish module + documentation", which is most of the `@module` rule, with no new dependency + and a tool the repository already pins. Whether it can see tag *placement* + and `@import` grouping is the open question. +- **ESLint**, named in §6 as the kind of tool this wants. `eslint-plugin-jsdoc` + covers tag presence and shape; whether it can express repository-specific + rules (this tag in this file kind) without custom rules of our own is what to + find out. Adding it needs approval per §6, and + [`../todo/eslint.md`](./eslint.md) already holds that discussion — check it + before opening a second one. +- **The TypeScript compiler API**, which already parses every file `tsc` reads + and exposes JSDoc nodes. No new tool to approve, but it means writing a + checker, which is the cost §6 warns about. + +Then decide, per rule, whether it is worth checking at all. §6's position is +that an unenforced written rule beats machinery whose failures are silent, so +"none of these are good enough, leave the rules to review" is a legitimate +outcome of this investigation and should be recorded as one rather than left +open. + +**P4 because reviewers do catch these.** Both `@module` reversals were found by +review — one by a human, one by a bot — before either reached a release. This +is worth doing when a tool makes it cheap, not worth building a tool for. + +### Tasks + +- [ ] Establish what `deno doc --json` can and cannot answer about tag + placement. +- [ ] Read [`eslint.md`](./eslint.md) and fold this in rather than duplicating + it, if the answer is ESLint. +- [ ] Pick one rule as the trial — `@module` presence is the narrowest — and + say what checking it would cost. +- [ ] Decide, and record "not worth it" as an answer if that is the answer. + +### Related + +- [`../AGENTS.md`](../AGENTS.md#6-external-tools) §6 — why not a text pattern, + and that a real tool needs approval first. +- [`../fjs/AGENTS.md`](../fjs/AGENTS.md) §2 — the `@module` and `@import` rules + this would check. +- [`../fjs/todo/module-tag-restore.md`](../fjs/todo/module-tag-restore.md) — the + drift this issue exists because of. +- [`eslint.md`](./eslint.md) — the standing ESLint discussion. diff --git a/todo/types-ts-module-tag.md b/todo/types-ts-module-tag.md deleted file mode 100644 index 612a827c1..000000000 --- a/todo/types-ts-module-tag.md +++ /dev/null @@ -1,41 +0,0 @@ -## `@module` in `types.ts` - -**Priority:** P4 -**Status:** open - -### Problem - -[`fjs/AGENTS.md`](../fjs/AGENTS.md) §2 says the `@module` tag "belongs only to a -package's entry-point file — `module.f.mjs` / `module.mjs` — not to -`proof.f.mjs`, `types.ts`, or any other file". - -90 of the 94 `types.ts` files in the tree carry it anyway. The rule and the -practice have been contradicting each other long enough that a new file copying -its neighbours lands on the wrong side of the documented convention, which is how -this was noticed: a review bot flagged `fjs/web/types.ts` for a tag every sibling -also has ([#1693](https://github.com/functionalscript/functionalscript/pull/1693)). - -### Proposal - -Decide which one is right, then make the tree say it once: - -- if the rule is right, strip `@module` from the 90 files — a mechanical change, - and worth checking against declaration emit first, since the leading block is - what carries a `types.d.ts` file's documentation; -- if the practice is right, amend `fjs/AGENTS.md` to say that a `types.ts` - companion is a module in its own right and carries the tag. - -The second reading has something going for it: `types.ts` *is* the entry point of -the type-level API, and its emitted `types.d.ts` is what a package consumer -reads. Whichever way it goes, the point is that it stops being a coin flip per -file. - -### Tasks - -- [ ] Decide the convention. -- [ ] Apply it to every `types.ts`, or amend `fjs/AGENTS.md`. -- [ ] Make `fjs/web/types.ts` match the outcome — it is currently the minority. - -### Related - -- `fjs/AGENTS.md` §2 — the rule as written.