From 191d312cb8aec1d0736142b91ed0e04b86309dc4 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sun, 18 Oct 2026 12:16:05 +0800 Subject: [PATCH] ci: check-empty-catch ignores catch {} inside comments and literals (#213) --- .github/scripts/check-empty-catch.mjs | 138 ++++++++++++++++++--- .github/scripts/check-empty-catch.test.mjs | 45 ++++++- docs/code-standards.md | 2 +- 3 files changed, 163 insertions(+), 22 deletions(-) diff --git a/.github/scripts/check-empty-catch.mjs b/.github/scripts/check-empty-catch.mjs index 88f3deb51..238286f85 100644 --- a/.github/scripts/check-empty-catch.mjs +++ b/.github/scripts/check-empty-catch.mjs @@ -3,43 +3,141 @@ // reason; a block with statements handles the error and needs none. Only the // statement form is checked: the `void p.catch(() => {})` promise form is a separate // fire-and-forget idiom, and test helpers may retry through a bare catch. +// The text `catch {}` inside a comment, a string, a template or a regex literal is +// not code, so it is never reported (#213). // Usage: node .github/scripts/check-empty-catch.mjs (exit 1 lists every violation) import { readdirSync, readFileSync } from 'node:fs' import { join } from 'node:path' const CATCH = /(? { +// After one of these (or at the start), a `/` opens a regex literal, not a division. +const REGEX_AFTER = new Set([...'(,=:[!&|?{};+-*%~^<>']) +const REGEX_KEYWORDS = /(?:^|[^\w$])(?:return|typeof|instanceof|in|of|new|delete|void|throw|case|do|else|yield|await)$/ + +// Two copies of the source with the same length and line breaks: `bare` blanks every +// comment and the inside of every string, template and regex literal (the quotes stay, +// so a string statement is still code); `commented` blanks the literals only, so the +// comments are left to read. A quote or `/` that does not close on its own line is not +// a literal and is left as code: a misread costs at most that line, never the file. +export function maskSource(source) { + const bare = source.split('') + const commented = source.split('') + const blank = (from, to, { comment = false } = {}) => { + for (let at = from; at < to; at += 1) { + if (source[at] === '\n') continue + bare[at] = ' ' + if (!comment) commented[at] = ' ' + } + } + // Each entry is the `{` depth of an open `${`; the template resumes when it closes. + const templates = [] let depth = 0 - for (let at = open; at < source.length; at += 1) { - if (source[at] === '{') depth += 1 - if (source[at] === '}') { + let last = '' // the last code character that is not whitespace + let at = 0 + // Scans a template body from `at` (after the backtick or a `}`): blanks it up to the + // closing backtick or the next `${`, and returns where code resumes. + const templateBody = (start) => { + let end = start + while (end < source.length) { + if (source[end] === '\\') { end += 2; continue } + if (source[end] === '`') { blank(start, end); return { next: end + 1, open: false } } + if (source[end] === '$' && source[end + 1] === '{') { blank(start, end); return { next: end + 2, open: true } } + end += 1 + } + blank(start, source.length) + return { next: source.length, open: false } + } + // A literal closed by `quote` on the same line, from `start` (after the opening + // quote); returns its closing index or -1. A regex skips over its `[...]` classes. + const closeOnLine = (start, quote) => { + let inClass = false + for (let end = start; end < source.length; end += 1) { + const char = source[end] + if (char === '\n') return -1 + if (char === '\\') { end += 1; continue } + if (quote === '/') { + if (char === '[') inClass = true + else if (char === ']') inClass = false + else if (char === '/' && !inClass) return end + continue + } + if (char === quote) return end + } + return -1 + } + while (at < source.length) { + const char = source[at] + const next = source[at + 1] + if (char === '/' && next === '/') { + const end = source.indexOf('\n', at) + const stop = end === -1 ? source.length : end + blank(at, stop, { comment: true }) + at = stop + continue + } + if (char === '/' && next === '*') { + const end = source.indexOf('*/', at + 2) + const stop = end === -1 ? source.length : end + 2 + blank(at, stop, { comment: true }) + at = stop + continue + } + if (char === '"' || char === "'") { + const end = closeOnLine(at + 1, char) + if (end !== -1) { blank(at + 1, end); last = char; at = end + 1; continue } + } + if (char === '/' && (last === '' || REGEX_AFTER.has(last) || REGEX_KEYWORDS.test(source.slice(Math.max(0, at - 12), at).trimEnd()))) { + const end = closeOnLine(at + 1, '/') + if (end !== -1) { blank(at + 1, end); last = '/'; at = end + 1; continue } + } + if (char === '`') { + const { next: resume, open } = templateBody(at + 1) + if (open) templates.push(depth) + last = open ? '{' : '`' + at = resume + continue + } + if (char === '{') depth += 1 + if (char === '}') { + if (templates.length > 0 && templates.at(-1) === depth) { + templates.pop() + const { next: resume, open } = templateBody(at + 1) + if (open) templates.push(depth) + last = open ? '{' : '`' + at = resume + continue + } depth -= 1 - if (depth === 0) return source.slice(open + 1, at) } + if (!/\s/.test(char)) last = char + at += 1 } - return source.slice(open + 1) + return { bare: bare.join(''), commented: commented.join('') } } -// What remains of the block once comments are gone; `comment` says whether any was. -const stripComments = (block) => { - let comment = false - const code = block.replace(/\/\/[^\n]*|\/\*[\s\S]*?\*\//g, (found) => { - comment = true - return '' - }) - return { code, comment } +// The index of the brace that closes the block opened at `open`, in masked source +// (no braces of strings or comments left to miscount), or the end of the source. +const closeOf = (code, open) => { + let depth = 0 + for (let at = open; at < code.length; at += 1) { + if (code[at] === '{') depth += 1 + if (code[at] === '}') { + depth -= 1 + if (depth === 0) return at + } + } + return code.length } export function scanSource(name, source) { const found = [] - for (const match of source.matchAll(CATCH)) { + const { bare, commented } = maskSource(source) + for (const match of bare.matchAll(CATCH)) { const open = match.index + match[0].length - 1 - const { code, comment } = stripComments(blockOf(source, open)) - if (code.trim() === '' && !comment) { + const close = closeOf(bare, open) + // No code left once comments and literals are blanked, and no comment either. + if (bare.slice(open + 1, close).trim() === '' && commented.slice(open + 1, close).trim() === '') { const line = source.slice(0, match.index).split('\n').length found.push(`${name}:${line}`) } diff --git a/.github/scripts/check-empty-catch.test.mjs b/.github/scripts/check-empty-catch.test.mjs index b8f660e6a..55ef94eaf 100644 --- a/.github/scripts/check-empty-catch.test.mjs +++ b/.github/scripts/check-empty-catch.test.mjs @@ -1,7 +1,7 @@ import { test } from 'node:test' import assert from 'node:assert/strict' import { join } from 'node:path' -import { scanSource, scanTree } from './check-empty-catch.mjs' +import { maskSource, scanSource, scanTree } from './check-empty-catch.mjs' test('scanSource: a catch block with no statement and no comment is a violation', () => { assert.deepEqual(scanSource('a.js', 'try { x() } catch {}\n'), ['a.js:1']) @@ -22,6 +22,49 @@ test('scanSource: the promise form, plain blocks and other identifiers are out o assert.deepEqual(scanSource('a.js', 'for (;;) {}\nconst o = { catch: 1 }\n'), []) }) +test('scanSource: `catch {}` inside a comment, string, template or regex is not code (#213)', () => { + assert.deepEqual(scanSource('a.js', '// never write try { x() } catch {} here\n'), []) + assert.deepEqual(scanSource('a.js', '/* e.g. try { x() } catch (e) {} */\nconst a = 1\n'), []) + assert.deepEqual(scanSource('a.js', "const hint = 'try { x() } catch {}'\n"), []) + assert.deepEqual(scanSource('a.js', 'const hint = "catch (e) {}"\n'), []) + assert.deepEqual(scanSource('a.js', 'const doc = `\n try { x() } catch {}\n`\n'), []) + assert.deepEqual(scanSource('a.js', 'const doc = `${a ? `catch {}` : b} and catch {}`\n'), []) + assert.deepEqual(scanSource('a.js', 'const bare = /catch\\s*\\{\\}/g\n'), []) + assert.deepEqual(scanSource('a.js', 'if (/catch {}/.test(line)) run()\n'), []) +}) + +test('scanSource: literals never hide a real bare catch, and braces in them do not end a block', () => { + // A real one after every kind of literal, on the following lines. + const tricky = [ + "const a = 'it\\'s } here', b = \"{\", c = `x ${'}'} y`, re = /[/}]/", + 'const half = total / 2 / count // a division, not a regex', + 'try { x() } catch {}', + ].join('\n') + assert.deepEqual(scanSource('a.js', tricky + '\n'), ['a.js:3']) + // A `}` inside a string in the block is code, so the block is not empty, and it does + // not close the block early either. + assert.deepEqual(scanSource('a.js', "try { x() } catch { log('}') }\n"), []) + // A comment still counts as the reason, a string-looking comment too. + assert.deepEqual(scanSource('a.js', "try { x() } catch { /* 'gone' is fine */ }\n"), []) + // A bare catch inside a template expression is code. + assert.deepEqual(scanSource('a.js', 'const s = `${(() => { try { x() } catch {} })()}`\n'), ['a.js:1']) + // An unclosed quote on a line is left as code: the next line is still scanned. + assert.deepEqual(scanSource('a.js', "const odd = 1 ' 2\ntry { x() } catch {}\n"), ['a.js:2']) +}) + +test('maskSource keeps length and line breaks, blanks literals, and keeps comments in `commented`', () => { + const source = "const s = 'a{b' // c}\nconst t = `x${y}z`\n" + const { bare, commented } = maskSource(source) + assert.equal(bare.length, source.length) + assert.equal(bare.split('\n').length, source.split('\n').length) + assert.doesNotMatch(bare, /a\{b|c\}/) + assert.match(bare, /\$\{y\}/) + assert.match(commented, /\/\/ c\}/) + assert.doesNotMatch(commented, /a\{b/) + // Astral characters count as two UTF-16 units, like String indexes. + assert.equal(maskSource("const e = '😀'\n").bare.length, "const e = '😀'\n".length) +}) + test('the shipped sources carry no bare catch', () => { assert.deepEqual(scanTree(join(import.meta.dirname, '..', '..')), []) }) diff --git a/docs/code-standards.md b/docs/code-standards.md index cb1a43279..6a8139331 100644 --- a/docs/code-standards.md +++ b/docs/code-standards.md @@ -26,7 +26,7 @@ When you report status on a change: ## Silent catches -- A `catch` that swallows the error says why in a comment inside the block (or handles it); `check-empty-catch` in `.github/scripts/` fails the build on a bare one (#205). +- A `catch` that swallows the error says why in a comment inside the block (or handles it); `check-empty-catch` in `.github/scripts/` fails the build on a bare one (#205). Text inside comments, strings, templates and regex literals is not code, so a `catch {}` written there is never reported (#213). - The `void p.catch(() => {})` promise form is a separate fire-and-forget idiom and test-helper retries stay out of the gate's scope. ## Security: fences and remote strings