diff --git a/.github/workflows/lint-suppressions.yml b/.github/workflows/lint-suppressions.yml new file mode 100644 index 00000000000..9c75ffe25b0 --- /dev/null +++ b/.github/workflows/lint-suppressions.yml @@ -0,0 +1,35 @@ +name: Lint Suppressions + +on: + pull_request: + types: [opened, synchronize, labeled, unlabeled] + merge_group: + +permissions: + contents: read + +jobs: + lint-suppressions: + name: Lint suppressions + if: ${{ github.event_name != 'merge_group' && !contains(github.event.pull_request.labels.*.name, 'allow-new-suppressions') }} + runs-on: ubuntu-latest + strategy: + matrix: + node-version: [24.x] + steps: + - name: Checkout and setup environment + uses: MetaMask/action-checkout-and-setup@v3 + with: + is-high-risk-environment: false + persist-credentials: false + node-version: ${{ matrix.node-version }} + fetch-depth: 2 + - name: Run yarn lint:suppressions + run: yarn lint:suppressions + - name: Require clean working directory + shell: bash + run: | + if ! git diff --exit-code; then + echo "Working tree dirty at end of job" + exit 1 + fi diff --git a/oxlint-suppressions.json b/oxlint-suppressions.json index 50a72b60d16..00d40012f1a 100644 --- a/oxlint-suppressions.json +++ b/oxlint-suppressions.json @@ -7601,27 +7601,11 @@ } }, "scripts/create-package/cli.test.ts": { - "no-shadow": { - "count": 1 - }, "typescript/no-unsafe-argument": { "count": 2 } }, - "scripts/create-package/commands.test.ts": { - "no-shadow": { - "count": 1 - } - }, - "scripts/create-package/index.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/create-package/utils.test.ts": { - "no-shadow": { - "count": 1 - }, "typescript/no-unsafe-assignment": { "count": 2 } @@ -7631,11 +7615,6 @@ "count": 1 } }, - "scripts/lib/changelog-conflicts.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/lib/changelog-conflicts.ts": { "n/no-unsupported-features/node-builtins": { "count": 1 @@ -7644,16 +7623,6 @@ "count": 2 } }, - "scripts/lib/lint-tsc.test.ts": { - "no-shadow": { - "count": 1 - } - }, - "scripts/lib/tsc-suppressions.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/lib/workspaces.ts": { "jsdoc/require-param": { "count": 1 @@ -7679,11 +7648,6 @@ "count": 2 } }, - "scripts/lint-tsc.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/lint-tsconfigs/utils.ts": { "n/no-unsupported-features/node-builtins": { "count": 1 @@ -7697,11 +7661,6 @@ "count": 1 } }, - "scripts/merge-changelog-conflicts.test.ts": { - "no-shadow": { - "count": 1 - } - }, "scripts/update-readme-content.ts": { "n/no-unsupported-features/node-builtins": { "count": 1 diff --git a/oxlint.config.ts b/oxlint.config.ts index 2163303abe1..755a3070e33 100644 --- a/oxlint.config.ts +++ b/oxlint.config.ts @@ -103,6 +103,14 @@ export default createConfig({ }, }, + { + // Jest does not inject its globals in ESM, so the scripts' tests import + // `jest` from `@jest/globals`. Declaring it as a global as well would + // make every one of those imports shadow it. + files: ['scripts/**/*.test.ts'], + rules: { 'no-shadow': ['error', { allow: ['jest'] }] }, + }, + { files: ['scripts/**/*.ts'], rules: { diff --git a/package.json b/package.json index 45da92639bf..de107894c48 100644 --- a/package.json +++ b/package.json @@ -38,13 +38,14 @@ "lint:misc": "oxfmt --ignore-path .gitignore", "lint:misc:check": "yarn lint:misc --check", "lint:oxlint": "oxlint", + "lint:suppressions": "node scripts/lint-suppressions.ts", "lint:teams": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-teams-json.ts", "lint:tsc": "tsc --build tsconfig.lint.json", - "lint:tsc:check": "node --experimental-strip-types scripts/lint-tsc.ts", + "lint:tsc:check": "node scripts/lint-tsc.ts", "lint:tsc:clean": "yarn lint:tsc:only-clean && yarn lint:tsc", "lint:tsc:only-clean": "rimraf -g 'packages/*/.tsc-lint-cache' '.tsc-lint-cache'", - "lint:tsc:prune": "node --experimental-strip-types scripts/lint-tsc.ts --prune-suppressions", - "lint:tsc:suppress": "node --experimental-strip-types scripts/lint-tsc.ts --suppress-all", + "lint:tsc:prune": "node scripts/lint-tsc.ts --prune-suppressions", + "lint:tsc:suppress": "node scripts/lint-tsc.ts --suppress-all", "lint:tsconfigs": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-tsconfigs/lint-tsconfigs.ts", "lint:tsconfigs:all": "yarn workspaces foreach --all --parallel --interlaced --verbose run lint:tsconfigs", "lint:tsconfigs:fix": "node --import ./scripts/resolver/register.ts --experimental-transform-types scripts/lint-tsconfigs/lint-tsconfigs.ts --fix", diff --git a/scripts/lib/lint-suppressions.test.ts b/scripts/lib/lint-suppressions.test.ts new file mode 100644 index 00000000000..e951e31e254 --- /dev/null +++ b/scripts/lib/lint-suppressions.test.ts @@ -0,0 +1,227 @@ +import { jest } from '@jest/globals'; + +jest.unstable_mockModule('execa', () => ({ + execa: jest.fn(), +})); + +jest.unstable_mockModule('./tsc-suppressions.ts', () => ({ + SUPPRESSIONS_FILE_NAME: 'tsc-suppressions.json', + readSuppressions: jest.fn(), +})); + +const { execa } = await import('execa'); +const tscSuppressions = await import('./tsc-suppressions.ts'); +const { findAddedSuppressions, printAddedSuppressions, lintSuppressions } = + await import('./lint-suppressions.ts'); + +describe('findAddedSuppressions', () => { + it('flags a file that the baseline does not suppress at all', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { 'no-shadow': { count: 1 } } }, + base: {}, + }), + ).toStrictEqual([ + { filePath: 'a.ts', rule: 'no-shadow', count: 1, baseCount: 0 }, + ]); + }); + + it('flags a rule that the baseline does not suppress within a file it does', () => { + expect( + findAddedSuppressions({ + current: { + 'a.ts': { 'no-shadow': { count: 1 }, 'id-length': { count: 1 } }, + }, + base: { 'a.ts': { 'no-shadow': { count: 1 } } }, + }), + ).toStrictEqual([ + { filePath: 'a.ts', rule: 'id-length', count: 1, baseCount: 0 }, + ]); + }); + + it('flags a count that has grown', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 3 } } }, + base: { 'a.ts': { TS2322: { count: 2 } } }, + }), + ).toStrictEqual([ + { filePath: 'a.ts', rule: 'TS2322', count: 3, baseCount: 2 }, + ]); + }); + + it('allows a count that is unchanged', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 2 } } }, + base: { 'a.ts': { TS2322: { count: 2 } } }, + }), + ).toStrictEqual([]); + }); + + it('allows a count that has shrunk', () => { + expect( + findAddedSuppressions({ + current: { 'a.ts': { TS2322: { count: 1 } } }, + base: { 'a.ts': { TS2322: { count: 5 } } }, + }), + ).toStrictEqual([]); + }); + + it('allows a rule or a file to disappear entirely', () => { + expect( + findAddedSuppressions({ + current: {}, + base: { 'a.ts': { TS2322: { count: 5 }, TS7005: { count: 1 } } }, + }), + ).toStrictEqual([]); + }); + + it('reports every addition, not just the first', () => { + expect( + findAddedSuppressions({ + current: { + 'a.ts': { TS2322: { count: 1 } }, + 'b.ts': { TS7005: { count: 2 } }, + }, + base: {}, + }), + ).toHaveLength(2); + }); +}); + +describe('printAddedSuppressions', () => { + beforeEach(() => { + jest.spyOn(console, 'log').mockReturnValue(undefined); + }); + + it('announces success when nothing was added, naming the file', () => { + printAddedSuppressions('oxlint-suppressions.json', []); + + expect(console.log).toHaveBeenCalledWith( + '✅ Nothing has been added to oxlint-suppressions.json. Good job!', + ); + }); + + it('prints each addition and how to resolve it', () => { + printAddedSuppressions('oxlint-suppressions.json', [ + { filePath: 'a.ts', rule: 'no-shadow', count: 3, baseCount: 2 }, + ]); + + const output = jest.mocked(console.log).mock.calls.flat().join('\n'); + expect(output).toContain('oxlint-suppressions.json'); + expect(output).toContain('a.ts'); + expect(output).toContain('no-shadow'); + expect(output).toContain('3'); + expect(output).toContain('2'); + }); +}); + +/** + * Stubs the Git commands the check runs. + * + * @param suppressions - The baseline the commands should produce. + */ +function mockGit(suppressions = '{}'): void { + jest.mocked(execa).mockImplementation((async ( + _file: string, + args: string[], + ) => { + if (args[0] === 'merge-base') { + return { stdout: 'abc123\n' }; + } + return { stdout: suppressions }; + }) as never); +} + +describe('lintSuppressions', () => { + let originalProcess: typeof globalThis.process; + + beforeEach(() => { + originalProcess = globalThis.process; + // The exit code is reset because another test file may have set it. + // `GITHUB_ACTIONS` is cleared because this suite runs in CI, where it is + // set, which would otherwise send every test down the CI path. + globalThis.process = { + ...globalThis.process, + exitCode: undefined, + env: { ...globalThis.process.env, GITHUB_ACTIONS: undefined }, + }; + jest.spyOn(console, 'log').mockReturnValue(undefined); + mockGit(); + jest.mocked(tscSuppressions.readSuppressions).mockResolvedValue({}); + }); + + afterEach(() => { + globalThis.process = originalProcess; + }); + + it('reads the baseline from the first parent of the merge commit CI checks out', async () => { + process.env.GITHUB_ACTIONS = 'true'; + + await lintSuppressions([]); + + expect(execa).toHaveBeenCalledWith( + 'git', + ['show', 'HEAD^1:oxlint-suppressions.json'], + expect.anything(), + ); + expect(execa).toHaveBeenCalledWith( + 'git', + ['show', 'HEAD^1:tsc-suppressions.json'], + expect.anything(), + ); + }); + + it('takes the merge base when running outside of CI, where a merge commit means something else', async () => { + await lintSuppressions([]); + + expect(jest.mocked(execa)).toHaveBeenCalledWith( + 'git', + ['merge-base', 'HEAD', 'origin/main'], + expect.anything(), + ); + expect(execa).toHaveBeenCalledWith( + 'git', + ['show', 'abc123:oxlint-suppressions.json'], + expect.anything(), + ); + expect(execa).toHaveBeenCalledWith( + 'git', + ['show', 'abc123:tsc-suppressions.json'], + expect.anything(), + ); + }); + + it('takes the merge base against the branch it is given', async () => { + await lintSuppressions(['origin/release']); + + expect(jest.mocked(execa)).toHaveBeenCalledWith( + 'git', + ['merge-base', 'HEAD', 'origin/release'], + expect.anything(), + ); + }); + + it('leaves the exit code alone when nothing has been added', async () => { + await lintSuppressions([]); + + expect(process.exitCode).toBeUndefined(); + }); + + it('exits with a non-zero code when a file has grown', async () => { + jest + .mocked(tscSuppressions.readSuppressions) + .mockResolvedValue({ 'a.ts': { 'no-shadow': { count: 1 } } }); + + await lintSuppressions([]); + + expect(process.exitCode).toBe(1); + }); + + it('throws when a baseline cannot be read, rather than passing', async () => { + jest.mocked(execa).mockRejectedValue(new Error('unknown revision')); + + await expect(lintSuppressions([])).rejects.toThrow('unknown revision'); + }); +}); diff --git a/scripts/lib/lint-suppressions.ts b/scripts/lib/lint-suppressions.ts new file mode 100644 index 00000000000..2b40d0b0411 --- /dev/null +++ b/scripts/lib/lint-suppressions.ts @@ -0,0 +1,170 @@ +import { execa } from 'execa'; +import path from 'path'; +import { fileURLToPath } from 'url'; + +import { + SUPPRESSIONS_FILE_NAME as TSC_SUPPRESSIONS_FILE_NAME, + readSuppressions, +} from './tsc-suppressions.ts'; + +const REPO_ROOT = path.join( + path.dirname(fileURLToPath(import.meta.url)), + '..', + '..', +); + +/** + * The file in which the lint problems that are knowingly ignored are recorded. + */ +const OXLINT_SUPPRESSIONS_FILE_NAME = 'oxlint-suppressions.json'; + +/** + * The suppressions files this guards, which Oxlint and the type error checker + * write in the same shape. + */ +const SUPPRESSIONS_FILE_NAMES = [ + OXLINT_SUPPRESSIONS_FILE_NAME, + TSC_SUPPRESSIONS_FILE_NAME, +]; + +/** + * The branch this work is destined for when there is no merge commit to read it + * from, as there is not when running this locally. + */ +const FALLBACK_BASE_REF = 'origin/main'; + +/** + * Problems that are knowingly ignored, counted by file and then by the rule or + * error code that reports them. Oxlint and the type error checker both write + * their suppressions this way. + */ +export type Suppressions = Record>; + +/** + * A suppression that covers more problems than the baseline it is compared + * against, meaning that problems have been added rather than fixed. + */ +export type AddedSuppression = { + filePath: string; + rule: string; + count: number; + baseCount: number; +}; + +/** + * Finds the suppressions that cover more problems than a baseline does. + * + * Suppressions are meant to be worked off, never added to: a problem that is + * new should be fixed rather than recorded. Removing suppressions, or shrinking + * their counts, is always allowed. + * + * @param args - The arguments to this function. + * @param args.current - The suppressions as they now stand. + * @param args.base - The suppressions to measure them against. + * @returns Every suppression that grew or appeared, in file order. + */ +export function findAddedSuppressions({ + current, + base, +}: { + current: Suppressions; + base: Suppressions; +}): AddedSuppression[] { + const added: AddedSuppression[] = []; + + for (const [filePath, currentByRule] of Object.entries(current)) { + for (const [rule, { count }] of Object.entries(currentByRule)) { + const baseCount = base[filePath]?.[rule]?.count ?? 0; + if (count > baseCount) { + added.push({ filePath, rule, count, baseCount }); + } + } + } + + return added; +} + +/** + * Prints the suppressions that have been added to a file, if any. + * + * @param fileName - The suppressions file the additions were found in. + * @param added - The added suppressions to print. + */ +export function printAddedSuppressions( + fileName: string, + added: AddedSuppression[], +): void { + if (added.length === 0) { + console.log(`✅ Nothing has been added to ${fileName}. Good job!`); + return; + } + + console.log(`❌ Detected problems added to ${fileName}:\n`); + for (const suppression of added) { + console.log( + ` ${suppression.filePath}: ${suppression.rule} (${suppression.count} suppressed, was ${suppression.baseCount})`, + ); + } + console.log( + '\nSuppressions may only be removed, never added. Fix these problems rather than suppressing them.', + ); +} + +/** + * Finds the commit holding the suppressions files to measure against. + * + * CI checks a pull request out as the merge of the branch into its base, so the + * base is its first parent, and the files in the working tree already have the + * base's own changes folded in. A merge commit made by hand is the other way + * round, its first parent being the branch, so the shape of the commit is not + * enough to go on and only CI's checkout is taken at face value. Anywhere else + * the merge base with the target branch stands in for it. + * + * @param targetRef - The branch this work is destined for. + * @returns The ref to read the baseline from. + */ +async function resolveBaseRef(targetRef: string): Promise { + if (process.env.GITHUB_ACTIONS === 'true') { + return 'HEAD^1'; + } + + const { stdout } = await execa('git', ['merge-base', 'HEAD', targetRef], { + cwd: REPO_ROOT, + }); + return stdout.trim(); +} + +/** + * Checks that no problems have been added to any of the suppressions files. + * + * The lint and type error checks keep new problems from landing, but their + * escape hatch — regenerating a suppressions file — can be used to paper over + * one rather than fix it. This closes that hatch: measured against the base + * branch, these files may only shrink. + * + * Pass a branch to measure against one other than `origin/main`. + * + * @param argv - The arguments passed to this script. + */ +export async function lintSuppressions(argv: readonly string[]): Promise { + const baseRef = await resolveBaseRef(argv[0] ?? FALLBACK_BASE_REF); + let didPass = true; + + for (const fileName of SUPPRESSIONS_FILE_NAMES) { + // Left to throw if the ref or the file is missing, as a check that cannot + // find its baseline must not wave the change through. + const { stdout } = await execa('git', ['show', `${baseRef}:${fileName}`], { + cwd: REPO_ROOT, + }); + const base = JSON.parse(stdout) as Suppressions; + const current = await readSuppressions(path.join(REPO_ROOT, fileName)); + + const added = findAddedSuppressions({ current, base }); + printAddedSuppressions(fileName, added); + didPass = didPass && added.length === 0; + } + + if (!didPass) { + process.exitCode = 1; + } +} diff --git a/scripts/lib/lint-tsc.test.ts b/scripts/lib/lint-tsc.test.ts index d968c7736dd..09913561bdc 100644 --- a/scripts/lib/lint-tsc.test.ts +++ b/scripts/lib/lint-tsc.test.ts @@ -10,6 +10,7 @@ jest.unstable_mockModule('execa', () => ({ })); jest.unstable_mockModule('./tsc-suppressions.ts', () => ({ + SUPPRESSIONS_FILE_NAME: 'tsc-suppressions.json', parseTscOutput: jest.fn(), isTscError: jest.fn(), addSuppressions: jest.fn(), diff --git a/scripts/lib/lint-tsc.ts b/scripts/lib/lint-tsc.ts index b1dd88aa563..b4ee3db8972 100644 --- a/scripts/lib/lint-tsc.ts +++ b/scripts/lib/lint-tsc.ts @@ -3,6 +3,7 @@ import path from 'path'; import { fileURLToPath } from 'url'; import { + SUPPRESSIONS_FILE_NAME, addSuppressions, compareErrorsToSuppressions, isTscError, @@ -19,8 +20,6 @@ const REPO_ROOT = path.join( '..', ); -const SUPPRESSIONS_FILE_NAME = 'tsc-suppressions.json'; - /** * Typechecks every package in the repo and compares the type errors it finds * against `tsc-suppressions.json`, failing if any error is not suppressed there diff --git a/scripts/lib/tsc-suppressions.ts b/scripts/lib/tsc-suppressions.ts index fd6d5956c18..f35478a0e1b 100644 --- a/scripts/lib/tsc-suppressions.ts +++ b/scripts/lib/tsc-suppressions.ts @@ -1,5 +1,10 @@ import fs from 'fs/promises'; +/** + * The file in which the type errors that are knowingly ignored are recorded. + */ +export const SUPPRESSIONS_FILE_NAME = 'tsc-suppressions.json'; + /** * A diagnostic reported by `tsc`. Most belong to a file; those that report a * broken build rather than a type error do not. diff --git a/scripts/lint-suppressions.test.ts b/scripts/lint-suppressions.test.ts new file mode 100644 index 00000000000..f925459eae4 --- /dev/null +++ b/scripts/lint-suppressions.test.ts @@ -0,0 +1,19 @@ +import { jest } from '@jest/globals'; + +jest.unstable_mockModule('./lib/lint-suppressions.ts', () => ({ + lintSuppressions: jest.fn(), +})); + +const { lintSuppressions } = await import('./lib/lint-suppressions.ts'); + +describe('lint-suppressions', () => { + it('runs the check with the arguments it was given', async () => { + jest.mocked(lintSuppressions).mockResolvedValue(undefined); + + // Importing the entry point runs it, which is the behaviour under test. + await import('./lint-suppressions.ts'); + + expect(lintSuppressions).toHaveBeenCalledTimes(1); + expect(lintSuppressions).toHaveBeenCalledWith(process.argv.slice(2)); + }); +}); diff --git a/scripts/lint-suppressions.ts b/scripts/lint-suppressions.ts new file mode 100644 index 00000000000..0ecda08fda5 --- /dev/null +++ b/scripts/lint-suppressions.ts @@ -0,0 +1,7 @@ +/** + * Entry point file for the `lint:suppressions` script. + */ + +import { lintSuppressions } from './lib/lint-suppressions.ts'; + +await lintSuppressions(process.argv.slice(2));