Merge pull request #95 from CodeDeficient/claude/project-thread-36cl1x-void-catch

Skip void p.catch(() => {}) and try blocks that declare locals first

Author: CodeDeficient

CHANGELOG.md

@@ -9,6 +9,7 @@ All notable changes will be documented in this file.
 - `swallowed_error` (medium): a `catch` block whose only statements are `console` calls. The error is logged and the code carries on as if the operation succeeded. A comment in the block explaining why that is safe, a rethrow, or any other statement (such as `setError(e)`) means it is not reported, and so does a fallback after the `try`, as for `empty_catch`.
 
 ### Changed
+- `empty_catch` skips `void p.catch(() => {})`, where `void` says the result is dropped on purpose, and a `try` that declares locals before assigning the outer variable (`const stats = await stat(p); isDir = stats.isDirectory();`). hono uses both.
 - The package ships compiled JavaScript (`dist/`) and runs it directly with Node. It used to run the TypeScript sources through `tsx` on every call: `karpeslop --version` took 1.5 s and an install was 43 MB. It now takes 0.6 s and 31 MB, most of it the TypeScript compiler the rules use to parse code. `tsx` is no longer a runtime dependency.
 - `fresh_package_version` skips packages the scanned workspace publishes itself (any `name` in a scanned package.json) and lockfile entries that link a workspace folder. TanStack Query got 184 warnings for `@tanstack/*` versions it had just released.
 - Folders named `build`, `out` or `temp` are skipped only at the scan root or next to a package.json, where they hold build output. Deeper ones are often source: Next.js keeps its build pipeline in `packages/next/src/build/`, which was never scanned.

README.md

@@ -28,7 +28,7 @@ npx karpeslop@latest
 | `placeholder_implementation` | high | A body that returns a constant (or nothing) under a TODO or placeholder comment; a short body under a comment that says the real code is elsewhere ("Simulate API call", "In a real app you would..."); a body that only logs and returns success; or `throw new Error("Not implemented")` |
 | `elided_code_comment` | high | `// ... rest of the code`, `// ...existing code...`, `{/* ... other fields */}`, `// Add more cases as needed` and similar, in line, block and JSX comments |
 | `placeholder_value` | high | `"YOUR_API_KEY"`, `"<your-token>"`, `"Bearer <API_KEY>"`, `"sk-xxxxxx"`, `"sk_test_your_key_here"`, `"postgres://postgres:password@..."`, `"https://your-project.supabase.co"`, `"xxxx"` stored as a key or token, an `example.com` URL stored in config, and a hard-coded fallback for a secret (`process.env.JWT_SECRET \|\| "secret"`) |
-| `empty_catch` | high | `catch {}` or `.catch(() => {})` with no comment explaining why and no `_`-named binding. Not reported when the `try` returns and code after it is the fallback (`try { return parse(x) } catch {}` then a default), or when the `try` only assigns variables declared before it that are read after it (`let found = fallback; try { found = probe() } catch {}`) |
+| `empty_catch` | high | `catch {}` or `.catch(() => {})` with no comment explaining why, no `_`-named binding and no `void` in front. Not reported when the `try` returns and code after it is the fallback (`try { return parse(x) } catch {}` then a default), or when the `try` only assigns variables declared before it that are read after it (`let found = fallback; try { found = probe() } catch {}`) |
 | `swallowed_error` | medium | A `catch` block that only logs (`console.error('Failed', err)`) and lets execution continue as if nothing failed, or logs and then returns `[]`, `{}` or `''`, which the caller cannot tell apart from an empty success |
 | `unsafe_type_assertion` | high | `as any` |
 | `unsafe_double_type_assertion` | high | `as unknown as T` and other chained casts |

rules.ts

@@ -387,6 +387,13 @@ function skipParentheses(expr: ts.Expression): ts.Expression {
   return current;
 }
 
+/** The nearest ancestor that is not a parenthesized expression. */
+function skipParenthesesUp(node: ts.Node): ts.Node {
+  let current = node.parent;
+  while (ts.isParenthesizedExpression(current)) current = current.parent;
+  return current;
+}
+
 /** The cast directly inside this one, looking through parentheses: `(x as A) as B`. */
 function innerCast(node: AssertionNode): AssertionNode | undefined {
   const inner = skipParentheses(node.expression);
@@ -1046,7 +1053,11 @@ function isFallbackCatch(clause: ts.CatchClause): boolean {
  * a return after the try.
  */
 function assignsOnlyOuterVariables(tryStatement: ts.TryStatement): boolean {
-  const statements = tryStatement.tryBlock.statements;
+  // Local declarations first feed the assignment: `const stats = await stat(p); isDir = stats.isDirectory();`.
+  // One after it is work whose failure the catch would hide.
+  const all = tryStatement.tryBlock.statements;
+  const firstAssignment = all.findIndex(statement => !ts.isVariableStatement(statement));
+  const statements = firstAssignment === -1 ? [] : all.slice(firstAssignment);
   if (statements.length === 0) return false;
   const scope = enclosingFunctionOrFile(tryStatement);
   return statements.every(statement => {
@@ -1245,8 +1256,9 @@ export function analyzeSource(sourceText: string, fileName: string): Finding[] {
     if (ts.isCallExpression(node) && ts.isPropertyAccessExpression(node.expression) &&
       node.expression.name.text === 'catch' && node.arguments.length === 1) {
       const handler = node.arguments[0];
+      // `void p.catch(() => {})` says the result is dropped on purpose.
       if ((ts.isArrowFunction(handler) || ts.isFunctionExpression(handler)) && ts.isBlock(handler.body) &&
-        handler.body.statements.length === 0 &&
+        handler.body.statements.length === 0 && !ts.isVoidExpression(skipParenthesesUp(node)) &&
         commentsWithin(handler.body).length === 0 && !isIgnoredBinding(handler.parameters[0]?.name)) {
         report('empty_catch', node.expression.name);
       }

tests/rules.test.ts

@@ -195,6 +195,8 @@ test('empty catch with an underscore binding is deliberate', () => {
   assert.equal(countOf('try { a(); } catch (_err: any) {}', 'empty_catch'), 0);
   assert.equal(countOf('p.catch((_err) => {});', 'empty_catch'), 0);
   assert.equal(countOf('p.catch((err) => {});', 'empty_catch'), 1);
+  assert.equal(countOf('void reader.cancel().catch(() => {});', 'empty_catch'), 0);
+  assert.equal(countOf('void (reader.cancel().catch(() => {}));', 'empty_catch'), 0);
   assert.equal(countOf('try { a(); } catch (err) {}', 'empty_catch'), 1);
 });
 
@@ -217,6 +219,7 @@ test('empty catch: a variable assigned in the try keeps its earlier value as the
     'function f(s: S) {\n  let recursive = true;\n  try {\n    recursive = isRecursive(s);\n  } catch {}\n  return recursive;\n}',
     'async function isDir(p: string) {\n  let dir;\n  try {\n    dir = (await stat(p)).isDirectory();\n  } catch {}\n  return dir;\n}',
     'let als: unknown;\nfor (const probe of probes) {\n  try {\n    als = probe();\n  } catch {}\n  if (als) break;\n}',
+    'async function isDir(p: string) {\n  let dir;\n  try {\n    const stats = await stat(p);\n    dir = stats.isDirectory();\n  } catch {}\n  return dir;\n}',
     'function f(src: S) {\n  let x = 1;\n  try {\n    x = load();\n  } catch {}\n  ({ a = x } = src);\n  return a;\n}'
   ]) {
     assert.equal(countOf(code, 'empty_catch'), 0, code);
@@ -230,7 +233,10 @@ test('empty catch: a variable assigned in the try keeps its earlier value as the
     // Written again, or read only by a callback's own parameter of the same name.
     'function f(src: S) {\n  let x = 1;\n  try {\n    x = load();\n  } catch {}\n  ({ x } = src);\n  [x] = [2];\n}',
     'function f(xs: number[]) {\n  let x = 1;\n  try {\n    x = load();\n  } catch {}\n  return xs.map((x) => x * 2);\n}',
-    'function f(xs: number[]) {\n  let x = 1;\n  try {\n    x = load();\n  } catch {}\n  for (const x of xs) use(x);\n  if (xs.length) { const x = 2; use(x); }\n  xs.forEach(() => { var x = 3; use(x); });\n}'
+    'function f(xs: number[]) {\n  let x = 1;\n  try {\n    x = load();\n  } catch {}\n  for (const x of xs) use(x);\n  if (xs.length) { const x = 2; use(x); }\n  xs.forEach(() => { var x = 3; use(x); });\n}',
+    'function f() {\n  try {\n    const x = load();\n  } catch {}\n}',
+    // A declaration after the assignment is more work the catch would hide.
+    'function f() {\n  let x = 1;\n  try {\n    x = load();\n    const y = save(x);\n  } catch {}\n  return x;\n}'
   ]) {
     assert.equal(countOf(code, 'empty_catch'), 1, code);
   }