Repository navigation
ci: check-empty-catch ignores catch {} inside comments and literals (#213) - #266
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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()))) { |
There was a problem hiding this comment.
With valid const hit = total / /catch {}/.test(s), the first division leaves last as /, so the second slash is not considered a regex opener and scanSource falsely reports the regex text; valid if (ok) /catch {}/.test(s) has the same result. The heuristic also fails in the other direction: obj.return / value; try {} catch {} /tail/ treats the division after a property named return as a regex and blanks the real empty catch, so the CI gate can both reject valid source and let the prohibited construct ship. Track lexical operand context and add both cases to the regression suite instead of relying on a last character and 12-character suffix.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
Part of #213: this is item 2, the mechanical fix. Item 1, the promise-form rule, still needs a decision, so #213 stays open.
What changed.
check-empty-catch.mjsnow masks the source before it matches.maskSource()returns two copies of the source, each with the same length and line breaks:bareblanks every comment and the inside of every string, template and regex literal. The quotes stay, so a string statement still counts as code.commentedblanks only the literals, so the comments can still be read.A
catch {}written inside a comment or a literal is no longer reported.Also fixed:
}inside a string can no longer end a block early.${…}is still scanned.Fail-safe kept. A quote or
/that does not close on its own line is treated as code, so a misread costs at most one line and never hides the rest of the file.Tokenizer check. All 551 tracked
.js/.mjsfiles come out with balanced{}and()after masking. The shipped sources still have no bare catch.Tests:
catch {}in each kind of comment and literal, nested templates, regex literals vs. division, a real bare catch after every kind of literal,}inside a string in the block, a bare catch inside a template expression, an unclosed quote, and UTF-16 length with astral characters.docs/code-standards.mdnotes the rule. No changelog fragment, because users cannot see this change.Checks (tip of this branch):
TZ=UTC ARCH_DOCS_BASE=upstream/main npm run verifypassed (core 803/803, social 185/185, tree 56/56,.github/scripts81/81, timeline/interop/live-e2e green, arch-docs OK).Item 1 needs a decision. There are about 51 bare non-
void.catch(() => {} | undefined | null)calls in the shipped sources. Many of them, like.catch(() => undefined)used as a value fallback, are deliberate. One possible rule: a.catch(() => {})whose result is dropped (a statement or anawaitstatement) needs avoidprefix or a why-comment, while.catch(() => undefined)used as a value stays allowed. I have not started that.