ci: check-empty-catch ignores catch {} inside comments and literals (#213)

.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 = /(?<![\w$.])catch\s*(?:\([^)]*\))?\s*\{/g
 
-// The block after `{`, ending at the brace that closes it. Braces inside strings can
-// only end the walk early, and a truncated block still shows its leading statements,
-// so the walk never invents a violation that is not there.
-const blockOf = (source, open) => {
+// 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}`)
     }

.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, '..', '..')), [])
 })

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