diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 61cc12ed..161fc1b0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,12 @@ jobs: - name: Lint parts in packages/react (oxlint) run: bun run lint:oxlint + # Lists every allowlisted file and inline exception (also in the job + # summary). Fails only on a malformed exception comment. + - name: Report lint exceptions + if: ${{ !cancelled() }} + run: bun run lint:exceptions + type-check: runs-on: ubuntu-latest timeout-minutes: 20 diff --git a/package.json b/package.json index 5d5afd49..ba58785a 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,7 @@ "check": "biome check . && bun run lint:oxlint", "check:fix": "biome check --fix . && bun run lint:oxlint --fix", "lint:oxlint": "oxlint --disable-nested-config packages/react", + "lint:exceptions": "bun tooling/lint/report-exceptions.ts", "format": "biome format .", "format:fix": "biome format --write .", "sgrep:watch": "sgrep watch .", diff --git a/packages/react/AGENTS.md b/packages/react/AGENTS.md index 9693c416..cc332701 100644 --- a/packages/react/AGENTS.md +++ b/packages/react/AGENTS.md @@ -199,3 +199,5 @@ When a rule is wrong for one line, disable it for that line and say why: `bazza/disable-needs-reason` rejects an exception with no rule or no reason, a `bazza/*` exception wider than one line, and `eslint-disable` comments (this repo has no ESLint, and oxlint ignores them). The `Allowlists` entries in `.oxlintrc.json` list files that broke a rule when it was added. Such a file is exempt from that rule until someone fixes it and deletes its line. Don't add files to an allowlist. New code follows the rules or uses a reasoned `oxlint-disable-next-line`. + +`bun run lint:exceptions` lists every allowlisted file by rule and every inline exception with its reason. CI runs it and adds the list to the job summary. It fails only on a malformed exception, which catches a bare `/* oxlint-disable */` that has silenced `bazza/disable-needs-reason` itself. diff --git a/tooling/lint/harness.ts b/tooling/lint/harness.ts index e27d669b..0d8ecb85 100644 --- a/tooling/lint/harness.ts +++ b/tooling/lint/harness.ts @@ -4,10 +4,10 @@ import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { fileURLToPath } from 'node:url' import { promisify } from 'node:util' +import { readOxlintConfig } from './source-text.ts' const run = promisify(execFile) const here = dirname(fileURLToPath(import.meta.url)) -const repoRoot = join(here, '../..') const oxlint = join(here, 'node_modules/.bin/oxlint') const bazzaPlugin = join(here, 'bazza-plugin.mjs') @@ -95,10 +95,9 @@ export function fixWith( }) } -/** The repo's `.oxlintrc.json`, with comments stripped and the plugin path made absolute. */ -async function repoConfig(): Promise { - const raw = await readFile(join(repoRoot, '.oxlintrc.json'), 'utf8') - const config = JSON.parse(raw.replace(/^\s*\/\/.*$/gm, '')) +/** The repo's `.oxlintrc.json`, with the plugin path made absolute. */ +function repoConfig(): object { + const config = readOxlintConfig() delete config.$schema config.jsPlugins = [bazzaPlugin] return config @@ -108,6 +107,6 @@ async function repoConfig(): Promise { * Lints `files` laid out at repo paths with the repo's own `.oxlintrc.json`, * so the tests see the same scoping CI does. */ -export async function lintWithRepoConfig(files: Files): Promise { - return lintIn(await repoConfig(), files) +export function lintWithRepoConfig(files: Files): Promise { + return lintIn(repoConfig(), files) } diff --git a/tooling/lint/report-exceptions.test.ts b/tooling/lint/report-exceptions.test.ts new file mode 100644 index 00000000..84f7c3d5 --- /dev/null +++ b/tooling/lint/report-exceptions.test.ts @@ -0,0 +1,242 @@ +import { existsSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { + brokenExceptions, + findConfigExceptions, + findInlineExceptions, + formatReport, + type InlineException, +} from './report-exceptions.ts' +import { + comments, + parseJsonc, + readOxlintConfig, + repoRoot, +} from './source-text.ts' + +describe('comments', () => { + it("doesn't let an apostrophe in JSX text swallow later comments", () => { + const source = `export const A = () =>

Don't

/* on line 1 */ +import a from 'a' +const url = 'https://x' // on line 3 +const s = 'escaped \\ +newline' // on line 5 +` + expect(comments(source).map((c) => [c.line, c.value.trim()])).toEqual([ + [1, 'on line 1'], + [3, 'on line 3'], + [5, 'on line 5'], + ]) + }) + + it('ends an unclosed quote at the end of its line', () => { + const source = `const r = /"/ +// on line 2 +export const A = () =>

Bob's

+// on line 4 +` + expect(comments(source).map((c) => [c.line, c.value.trim()])).toEqual([ + [2, 'on line 2'], + [4, 'on line 4'], + ]) + }) + + it('skips comment markers inside strings and template literals', () => { + const source = `const url = 'https://example.com' // real one +const glob = "**/*.test.tsx" +const t = \`/* not a comment \${a /* inside code */ + '//'} still text // no\` +/* block + spans lines */ const b = 1 // after +` + expect(comments(source).map((c) => [c.line, c.value.trim()])).toEqual([ + [1, 'real one'], + [3, 'inside code'], + [4, 'block\n spans lines'], + [5, 'after'], + ]) + }) +}) + +describe('parseJsonc', () => { + it('drops comments and trailing commas but keeps strings intact', () => { + expect( + parseJsonc(`{ + // line comment + "url": "https://example.com", /* block */ + "glob": "src/*.{ts,}", + "list": [1, 2,], +}`), + ).toEqual({ + url: 'https://example.com', + glob: 'src/*.{ts,}', + list: [1, 2], + }) + }) +}) + +describe('findInlineExceptions', () => { + it('finds every disable directive, but not directive text in strings', () => { + const source = `// oxlint-disable-next-line bazza/use-client -- server-only helper +export const a = 1 +const b = 2 // eslint-disable-line react-hooks/exhaustive-deps +/* oxlint-disable */ +{/* oxlint-disable-next-line no-console -- JSX comment */} +const fixture = '// oxlint-disable-line' +const glob = 'src/**/*.ts' // oxlint-disable-line no-console -- after a glob +` + const found = findInlineExceptions(['a.tsx'], () => source) + expect(found.map((e) => [e.line, e.tool, e.rules, e.reason])).toEqual([ + [1, 'oxlint', ['bazza/use-client'], 'server-only helper'], + [3, 'eslint', ['react-hooks/exhaustive-deps'], ''], + [4, 'oxlint', [], ''], + [5, 'oxlint', ['no-console'], 'JSX comment'], + [7, 'oxlint', ['no-console'], 'after a glob'], + ]) + expect(found.map((e) => e.problem !== null)).toEqual([ + false, + true, + true, + false, + false, + ]) + }) +}) + +describe('findConfigExceptions', () => { + it('separates allowlists (literal paths) from rules turned off by pattern', () => { + const { allowlists, patterns } = findConfigExceptions({ + overrides: [ + { + files: ['packages/react/**/*.ts'], + rules: { 'bazza/use-client': 'error' }, + }, + { + files: ['packages/react/src/**/*.test.tsx'], + rules: { 'bazza/use-client': 'off' }, + }, + { + files: ['packages/react/src/b.ts', 'packages/react/src/a.ts'], + rules: { 'bazza/use-client': 'off' }, + }, + { + files: ['packages/react/src/c.ts'], + rules: { 'bazza/part-namespace': 'allow', 'bazza/use-client': 0 }, + }, + { + files: ['packages/react/src/d.ts'], + rules: { 'bazza/use-client': 'error' }, + }, + { + files: ['packages/react/src/e.ts'], + rules: { 'bazza/use-client': ['off', {}] }, + }, + ], + }) + expect(allowlists).toEqual([ + { rule: 'bazza/part-namespace', files: ['packages/react/src/c.ts'] }, + { + rule: 'bazza/use-client', + files: [ + 'packages/react/src/a.ts', + 'packages/react/src/b.ts', + 'packages/react/src/c.ts', + 'packages/react/src/e.ts', + ], + }, + ]) + expect(patterns).toEqual([ + { + files: ['packages/react/src/**/*.test.tsx'], + rules: ['bazza/use-client'], + }, + ]) + }) + + it("reads the repo's own config: allowlisted files exist, tests are scoped off", () => { + const { allowlists, patterns } = findConfigExceptions(readOxlintConfig()) + expect(allowlists.length).toBeGreaterThan(0) + for (const { files } of allowlists) { + for (const file of files) { + expect(existsSync(join(repoRoot, file)), file).toBe(true) + } + } + expect(patterns).toContainEqual({ + files: ['packages/react/src/**/*.test.{ts,tsx}'], + rules: expect.arrayContaining([ + 'bazza/use-client', + 'bazza/part-namespace', + ]), + }) + }) +}) + +describe('brokenExceptions and formatReport', () => { + const exception = (overrides: Partial): InlineException => ({ + file: 'a.ts', + line: 1, + tool: 'oxlint', + rules: ['no-console'], + reason: 'CLI output', + problem: null, + ...overrides, + }) + + it('fails on malformed exceptions, except eslint comments in a file already allowlisted for them', () => { + const allowlists = [{ rule: 'bazza/disable-needs-reason', files: ['a.ts'] }] + const staleEslint = exception({ + tool: 'eslint', + reason: '', + problem: 'no ESLint', + }) + const bareInAllowlisted = exception({ + line: 2, + rules: [], + reason: '', + problem: 'must name the rule', + }) + const bareElsewhere = exception({ + file: 'b.ts', + rules: [], + reason: '', + problem: 'must name the rule', + }) + const broken = brokenExceptions(allowlists, [ + staleEslint, + bareInAllowlisted, + bareElsewhere, + ]) + expect([...broken]).toEqual([bareInAllowlisted, bareElsewhere]) + }) + + it('counts entries and distinct files, and marks missing files and problems', () => { + const bad = exception({ + line: 4, + rules: [], + reason: '', + problem: 'must name the rule', + }) + const lines = formatReport( + { + allowlists: [ + { rule: 'bazza/part-namespace', files: ['here.ts'] }, + { rule: 'bazza/use-client', files: ['gone.ts', 'here.ts'] }, + ], + patterns: [{ files: ['**/*.test.ts'], rules: ['bazza/use-client'] }], + }, + [exception({}), bad], + new Set([bad]), + (file) => file === 'here.ts', + ) + expect(lines[0]).toBe( + 'Lint exceptions in packages/react: 3 allowlist entries across 2 file(s), 2 inline.', + ) + expect(lines).toContain(' gone.ts (file no longer exists)') + expect(lines).toContain(' **/*.test.ts: bazza/use-client') + expect(lines).toContain(' a.ts:1 no-console -- CLI output') + expect(lines).toContain( + ' a.ts:4 (all rules) -- (no reason) ✗ must name the rule', + ) + expect(lines.at(-1)).toContain("1 lint exception(s) aren't acceptable") + }) +}) diff --git a/tooling/lint/report-exceptions.ts b/tooling/lint/report-exceptions.ts new file mode 100644 index 00000000..1acc5c29 --- /dev/null +++ b/tooling/lint/report-exceptions.ts @@ -0,0 +1,216 @@ +/** + * Prints every lint exception in `packages/react`: each rule's allowlist in + * `.oxlintrc.json`, rules turned off by pattern, and every inline + * `oxlint-disable` comment with its reason. The counts are for people to + * watch and burn down, not a gate. + * + * It does fail (exit 1) on a malformed exception: no rule named, no reason, a + * file-wide `bazza/*` exception, or `eslint-disable`. That check has to live + * here too, because a bare `/* oxlint-disable *\/` can also silence + * `bazza/disable-needs-reason`, the lint rule meant to catch it, and a text + * scan can't be silenced. + */ +import { execFileSync } from 'node:child_process' +import { appendFileSync, existsSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import { directiveProblem, parseDirective } from './bazza-plugin.mjs' +import { comments, readOxlintConfig, repoRoot } from './source-text.ts' + +/** The extensions oxlint lints in `packages/react` (see `.oxlintrc.json`). */ +const extensions = ['ts', 'tsx', 'mts', 'cts', 'js', 'jsx', 'mjs', 'cjs'] + +export interface InlineException { + readonly file: string + readonly line: number + readonly tool: 'oxlint' | 'eslint' + readonly rules: readonly string[] + readonly reason: string + readonly problem: string | null +} + +export interface Allowlist { + readonly rule: string + readonly files: readonly string[] +} + +export interface PatternOff { + readonly files: readonly string[] + readonly rules: readonly string[] +} + +/** Every `oxlint-disable` / `eslint-disable` comment in `files`. */ +export function findInlineExceptions( + files: readonly string[], + read: (file: string) => string, +): InlineException[] { + const found: InlineException[] = [] + for (const file of files) { + for (const comment of comments(read(file))) { + const parsed = parseDirective(comment.value) + if (parsed?.action !== 'disable') continue + found.push({ + file, + line: comment.line, + tool: parsed.tool === 'eslint' ? 'eslint' : 'oxlint', + rules: parsed.rules, + reason: parsed.reason, + problem: directiveProblem(parsed), + }) + } + } + return found +} + +interface Override { + files?: string[] + rules?: Record +} + +/** oxlint's spellings of "off". */ +function isOff(level: unknown): boolean { + if (Array.isArray(level)) return isOff(level[0]) + return level === 'off' || level === 'allow' || level === 0 +} + +const isPattern = (path: string) => /[*?{[]/.test(path) + +/** + * The exceptions a config grants: allowlists (rules turned off for literal + * file paths) and rules turned off by pattern (like the override that exempts + * tests from the part rules). + */ +export function findConfigExceptions(config: { overrides?: Override[] }): { + allowlists: Allowlist[] + patterns: PatternOff[] +} { + const allowlisted = new Map>() + const patterns: PatternOff[] = [] + for (const override of config.overrides ?? []) { + const files = override.files ?? [] + const off = Object.entries(override.rules ?? {}) + .filter(([, level]) => isOff(level)) + .map(([rule]) => rule) + if (files.length === 0 || off.length === 0) continue + const literal = files.filter((file) => !isPattern(file)) + const patterned = files.filter(isPattern) + for (const rule of off) { + const set = allowlisted.get(rule) ?? new Set() + for (const file of literal) set.add(file) + if (literal.length > 0) allowlisted.set(rule, set) + } + if (patterned.length > 0) patterns.push({ files: patterned, rules: off }) + } + const allowlists = [...allowlisted] + .map(([rule, set]) => ({ rule, files: [...set].sort() })) + .sort((a, b) => a.rule.localeCompare(b.rule)) + return { allowlists, patterns } +} + +/** + * The malformed exceptions that fail the report. An `eslint-disable` comment + * in a file allowlisted for `bazza/disable-needs-reason` is on the burn-down + * list already, and oxlint ignores it, so it doesn't count. A malformed + * `oxlint-disable` always counts: it can silence rules. + */ +export function brokenExceptions( + allowlists: readonly Allowlist[], + inline: readonly InlineException[], +): Set { + const exempt = new Set( + allowlists.find((a) => a.rule === 'bazza/disable-needs-reason')?.files, + ) + return new Set( + inline.filter( + (exception) => + exception.problem && + !(exception.tool === 'eslint' && exempt.has(exception.file)), + ), + ) +} + +/** The report as lines of text. */ +export function formatReport( + { allowlists, patterns }: { allowlists: Allowlist[]; patterns: PatternOff[] }, + inline: readonly InlineException[], + broken: ReadonlySet, + exists: (file: string) => boolean, +): string[] { + const entries = allowlists.reduce((sum, a) => sum + a.files.length, 0) + const files = new Set(allowlists.flatMap((a) => a.files)).size + const lines = [ + `Lint exceptions in packages/react: ${entries} allowlist entries across ${files} file(s), ${inline.length} inline.`, + '', + 'Allowlisted files, by rule (fix a file, then delete its line in .oxlintrc.json):', + ] + for (const { rule, files: listed } of allowlists) { + lines.push(` ${rule} (${listed.length})`) + for (const file of listed) { + lines.push( + ` ${file}${exists(file) ? '' : ' (file no longer exists)'}`, + ) + } + } + lines.push('', 'Rules turned off by pattern:') + if (patterns.length === 0) lines.push(' none') + for (const pattern of patterns) { + lines.push(` ${pattern.files.join(', ')}: ${pattern.rules.join(', ')}`) + } + lines.push('', 'Inline exceptions:') + if (inline.length === 0) lines.push(' none') + for (const exception of inline) { + const rules = exception.rules.join(', ') || '(all rules)' + const reason = exception.reason || '(no reason)' + const mark = !exception.problem + ? '' + : broken.has(exception) + ? ` ✗ ${exception.problem}` + : ` (allowlisted) ${exception.problem}` + lines.push( + ` ${exception.file}:${exception.line} ${rules} -- ${reason}${mark}`, + ) + } + if (broken.size > 0) { + lines.push( + '', + `${broken.size} lint exception(s) aren't acceptable; see ✗ above and "Lint" in packages/react/AGENTS.md.`, + ) + } + return lines +} + +/** Tracked source files in `packages/react` that exist in the working tree. */ +function sourceFiles(): string[] { + const out = execFileSync( + 'git', + [ + 'ls-files', + '-z', + '--', + ...extensions.map((ext) => `packages/react/*.${ext}`), + ], + { cwd: repoRoot, encoding: 'utf8' }, + ) + return out + .split('\0') + .filter((file) => file && existsSync(join(repoRoot, file))) +} + +if ((import.meta as { main?: boolean }).main) { + const config = findConfigExceptions(readOxlintConfig()) + const inline = findInlineExceptions(sourceFiles(), (file) => + readFileSync(join(repoRoot, file), 'utf8'), + ) + const broken = brokenExceptions(config.allowlists, inline) + const lines = formatReport(config, inline, broken, (file) => + existsSync(join(repoRoot, file)), + ) + console.log(lines.join('\n')) + const summary = process.env.GITHUB_STEP_SUMMARY + if (summary) { + appendFileSync( + summary, + `## Lint exceptions\n\n\`\`\`text\n${lines.join('\n')}\n\`\`\`\n`, + ) + } + if (broken.size > 0) process.exitCode = 1 +} diff --git a/tooling/lint/source-text.ts b/tooling/lint/source-text.ts new file mode 100644 index 00000000..4662bced --- /dev/null +++ b/tooling/lint/source-text.ts @@ -0,0 +1,161 @@ +/** + * Reading comments and JSONC from source text without a parser. Shared by the + * exceptions report and the test harness. + */ +import { readFileSync } from 'node:fs' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' + +export const repoRoot = join(dirname(fileURLToPath(import.meta.url)), '../..') + +export interface Comment { + /** The text between `//` and the line end, or between `/*` and `*\/`. */ + readonly value: string + readonly start: number + readonly end: number + readonly line: number +} + +/** + * Every comment in `source`, skipping string and template literals, so a + * `//` in a URL or a `/*` in a glob isn't mistaken for one. An apostrophe in + * JSX text (`

Don't

`) doesn't open a string, and a quoted string ends at + * the end of its line, since JS strings can't span lines. + * + * Known limits: regular expression literals and JSX text aren't recognised. + * A quote after a non-word character (`Bob's`) or a `//` in either can + * hide the rest of that line; a `/*` in either can hide more. + */ +export function comments(source: string): Comment[] { + const found: Comment[] = [] + // Template literals nest through `${ … }`; each entry counts open braces. + const templateBraces: number[] = [] + let line = 1 + let i = 0 + const skipString = (quote: string) => { + i++ + while (i < source.length && source[i] !== quote && source[i] !== '\n') { + if (source[i] === '\\') { + i++ + if (source[i] === '\n') line++ + } + i++ + } + // Stop before an unescaped newline so the main loop counts it. + if (source[i] === quote) i++ + } + const skipTemplate = () => { + i++ + while (i < source.length) { + const char = source[i] + if (char === '\\') { + if (source[i + 1] === '\n') line++ + i += 2 + continue + } + if (char === '\n') line++ + if (char === '`') { + i++ + return + } + if (char === '$' && source[i + 1] === '{') { + templateBraces.push(0) + i += 2 + return + } + i++ + } + } + while (i < source.length) { + const char = source[i] + const next = source[i + 1] + if (char === '\n') { + line++ + i++ + } else if (char === '/' && next === '/') { + const end = source.indexOf('\n', i) + const stop = end === -1 ? source.length : end + found.push({ + value: source.slice(i + 2, stop), + start: i, + end: stop, + line, + }) + i = stop + } else if (char === '/' && next === '*') { + const close = source.indexOf('*/', i + 2) + const stop = close === -1 ? source.length : close + 2 + const value = source.slice(i + 2, close === -1 ? stop : close) + found.push({ value, start: i, end: stop, line }) + line += value.split('\n').length - 1 + i = stop + } else if ( + (char === '"' || char === "'") && + // A quote right after a letter or digit is text (`Don't`), not a + // string: JS never starts a string literal there. + !/[\w$]/.test(source[i - 1] ?? '') + ) { + skipString(char) + } else if (char === '`') { + skipTemplate() + } else if (templateBraces.length > 0 && (char === '{' || char === '}')) { + const depth = templateBraces.length - 1 + if (char === '{') { + templateBraces[depth] = (templateBraces[depth] ?? 0) + 1 + i++ + } else if (templateBraces[depth] === 0) { + // The `}` that closes `${`: back inside the template literal. + templateBraces.pop() + i-- + skipTemplate() + } else { + templateBraces[depth] = (templateBraces[depth] ?? 1) - 1 + i++ + } + } else { + i++ + } + } + return found +} + +/** Removes commas that directly precede `}` or `]`, outside JSON strings. */ +function dropTrailingCommas(json: string): string { + let out = '' + let inString = false + for (let i = 0; i < json.length; i++) { + const char = json[i] + if (inString) { + out += char + if (char === '\\') out += json[++i] ?? '' + else if (char === '"') inString = false + } else if (char === '"') { + inString = true + out += char + } else if (char === ',' && /^\s*[}\]]/.test(json.slice(i + 1))) { + // A trailing comma: drop it. + } else { + out += char + } + } + return out +} + +/** Parses JSONC: JSON with comments and trailing commas. */ +export function parseJsonc(source: string): unknown { + let text = '' + let from = 0 + for (const comment of comments(source)) { + text += source.slice(from, comment.start) + from = comment.end + } + text += source.slice(from) + return JSON.parse(dropTrailingCommas(text)) +} + +/** The repo's `.oxlintrc.json`, parsed. */ +export function readOxlintConfig(): Record { + return parseJsonc( + readFileSync(join(repoRoot, '.oxlintrc.json'), 'utf8'), + ) as Record +}