From 787696772b37a5112b2fcac0a9abca6edad62998 Mon Sep 17 00:00:00 2001 From: ColumbusLabs <287001685+ColumbusLabs@users.noreply.github.com> Date: Sun, 21 Jun 2026 00:36:41 -0400 Subject: [PATCH 1/2] feat: add code smell detectors for silent failures and dead code (#257, #258, #259) Add empty-catch, swallowed-error, floating-promise, and commented-out-code to the core pack, plus python-error-handling and kotlin-empty-catch for language packs, with docs, schema, and tests. --- CHANGELOG.md | 6 + docs/example-report.md | 41 ++- docs/rule-packs.md | 6 + docs/rules.md | 130 ++++++++++ schema/debtlens.config.schema.json | 99 ++++++++ src/config/defaults.ts | 5 + src/config/packs.ts | 9 + src/detectors/commentedOutCode.ts | 208 ++++++++++++++++ src/detectors/errorHandling.ts | 257 +++++++++++++++++++ src/detectors/floatingPromise.ts | 289 +++++++++++++++++++++ src/detectors/index.ts | 12 +- src/detectors/kotlin/errorHandling.ts | 194 ++++++++++++++ src/detectors/kotlin/index.ts | 1 + src/detectors/python/errorHandling.ts | 305 +++++++++++++++++++++++ src/detectors/python/index.ts | 1 + tests/cli/plugins.test.ts | 4 +- tests/config/packs.test.ts | 14 +- tests/detectors/commentedOutCode.test.ts | 82 ++++++ tests/detectors/errorHandling.test.ts | 114 +++++++++ tests/detectors/floatingPromise.test.ts | 143 +++++++++++ tests/detectors/kotlin.test.ts | 61 +++++ tests/detectors/python.test.ts | 79 ++++++ 22 files changed, 2047 insertions(+), 13 deletions(-) create mode 100644 src/detectors/commentedOutCode.ts create mode 100644 src/detectors/errorHandling.ts create mode 100644 src/detectors/floatingPromise.ts create mode 100644 src/detectors/kotlin/errorHandling.ts create mode 100644 src/detectors/python/errorHandling.ts create mode 100644 tests/detectors/commentedOutCode.test.ts create mode 100644 tests/detectors/errorHandling.test.ts create mode 100644 tests/detectors/floatingPromise.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index d8b8c56..77b73f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ All notable changes to DebtLens are documented here. This project adheres to ### Added +- **Code smell detectors** for silent failures and dead code: `empty-catch`, `swallowed-error`, + `floating-promise`, and `commented-out-code` in the core TS/JS pack; `python-error-handling` + in the Python pack; and `kotlin-empty-catch` in the Kotlin pack + ([#257](https://github.com/ColumbusLabs/DebtLens/issues/257), + [#258](https://github.com/ColumbusLabs/DebtLens/issues/258), + [#259](https://github.com/ColumbusLabs/DebtLens/issues/259)). - **Swift core pack** (`--pack swift`) with `swift-todo-comment`, `swift-large-function`, `swift-dead-abstraction`, and `swift-duplicate-logic` for `.swift` files ([#194](https://github.com/ColumbusLabs/DebtLens/issues/194)). - **SwiftUI pack** (`--pack swiftui`) with `swiftui-large-view` and `swiftui-state-sprawl` diff --git a/docs/example-report.md b/docs/example-report.md index 6e4e92f..87ad59a 100644 --- a/docs/example-report.md +++ b/docs/example-report.md @@ -1,12 +1,12 @@ # DebtLens Report -Scanned **3** files with **28** rules in **174ms**. +Scanned **3** files with **32** rules in **162ms**. ## Summary -- Total issues: **8** +- Total issues: **9** - High: **2** -- Medium: **2** +- Medium: **3** - Low: **4** - Info: **0** @@ -14,7 +14,7 @@ Scanned **3** files with **28** rules in **174ms**. | File | Why | Top rules | | --- | --- | --- | -| `src/Dashboard.tsx` | 1 high-severity finding; 2 medium-severity findings; 5 distinct rules | dead-abstraction (3), effect-complexity (1), prop-drilling (1) | +| `src/Dashboard.tsx` | 1 high-severity finding; 3 medium-severity findings; 6 distinct rules | dead-abstraction (3), effect-complexity (1), floating-promise (1) | | `src/duplicateOne.ts` | 1 high-severity finding; 1 duplicate cluster | duplicate-logic (1) | ## High severity @@ -32,6 +32,8 @@ Evidence: Suggestion: Consider colocating the data owner closer to consumers, using a composition slot, or extracting a focused context for stable cross-cutting values. +Review prompt: Can data ownership move closer to consumers via composition, context, or a colocated hook? + ### Duplicate logic — `src/duplicateOne.ts:1` normalizeMovieRelease is 100% structurally similar to normalizeGameRelease. @@ -44,6 +46,8 @@ Evidence: Suggestion: Compare the two implementations. Extract shared behavior only if the variation is intentional and stable; otherwise delete the weaker duplicate. +Review prompt: Are both copies intentional? If not, which copy should become canonical and what breaks if you merge them? + ## Medium severity @@ -58,6 +62,8 @@ Evidence: Suggestion: Group related state in a reducer, extract state machines into a hook, or move server/cache state out of component state. +Review prompt: Which state variables change together? Could a reducer or domain hook replace independent useState calls? + ### Effect complexity — `src/Dashboard.tsx:23` This useEffect spans 26 lines, has 9 dependencies, 3 branches, and 14 nested calls. @@ -72,6 +78,23 @@ Evidence: Suggestion: Split unrelated effects, move imperative workflows into named functions, or replace derived state effects with memoized values. +Review prompt: Does this effect do one job? If not, what is the smallest split that preserves behavior? + +### Floating promise — `src/Dashboard.tsx:47` + +Promise work inside useEffect is not awaited or error-handled. + +Confidence: **88%** + +Evidence: +- Expression: load() +- Not awaited, returned, assigned, void-marked, or passed as an argument +- Inside React effect without cleanup or error handling +- Calls async function +- Type checker reports Promise + +Suggestion: Await the promise inside the effect, chain .catch(), or move async work into a named helper with explicit error handling. + ## Low severity @@ -86,6 +109,8 @@ Evidence: Suggestion: Convert the marker into a tracked issue, add a removal condition, or fix it before more code depends on it. +Review prompt: Is there a tracked issue and removal condition? If not, create one or fix the debt now. + ### Dead abstraction — `src/Dashboard.tsx:67` ReleaseHero looks like a thin wrapper: it only forwards to a single JSX element. @@ -97,6 +122,8 @@ Evidence: Suggestion: Keep the wrapper only if it creates a stable domain boundary. Otherwise inline it or add the missing behavior where this abstraction belongs. +Review prompt: Does this wrapper enforce a boundary today? If not, inline it or add the missing behavior. + ### Dead abstraction — `src/Dashboard.tsx:71` ReleaseGrid looks like a thin wrapper: it only forwards to a single JSX element. @@ -108,6 +135,8 @@ Evidence: Suggestion: Keep the wrapper only if it creates a stable domain boundary. Otherwise inline it or add the missing behavior where this abstraction belongs. +Review prompt: Does this wrapper enforce a boundary today? If not, inline it or add the missing behavior. + ### Dead abstraction — `src/Dashboard.tsx:75` ReleaseFooter looks like a thin wrapper: it only forwards to a single JSX element. @@ -119,12 +148,14 @@ Evidence: Suggestion: Keep the wrapper only if it creates a stable domain boundary. Otherwise inline it or add the missing behavior where this abstraction belongs. +Review prompt: Does this wrapper enforce a boundary today? If not, inline it or add the missing behavior. + ## Rule correlations | File | Rules | Issues | | --- | --- | ---: | -| `src/Dashboard.tsx` | dead-abstraction (3), effect-complexity (1), prop-drilling (1), state-sprawl (1), todo-comment (1) | 7 | +| `src/Dashboard.tsx` | dead-abstraction (3), effect-complexity (1), floating-promise (1), prop-drilling (1), state-sprawl (1), todo-comment (1) | 8 | ## Duplicate logic clusters diff --git a/docs/rule-packs.md b/docs/rule-packs.md index 8f2930e..56aafc3 100644 --- a/docs/rule-packs.md +++ b/docs/rule-packs.md @@ -38,6 +38,10 @@ For a user-facing selection table, see [`pack-chooser.md`](./pack-chooser.md). | `barrel-file` | **core** | Re-export-only barrels that obscure import graphs | Low | | `weak-test-boundary` | **core** | Production imports from test-only modules | Medium | | `api-surface-sprawl` | **core** | Files exporting too many public symbols | Medium | +| `empty-catch` | **core** | Empty or comment-only catch blocks that silently ignore errors | Medium | +| `swallowed-error` | **core** | Catch blocks that only log without rethrowing or returning | Medium | +| `floating-promise` | **core** | Unawaited promise-returning calls and effect fire-and-forget | Medium | +| `commented-out-code` | **core** | Contiguous comment lines that look like dead code | Low | | `large-component` | **react** | React-style components with too many lines, hooks, or branch points | Medium | | `state-sprawl` | **react** | Components/hooks with many local stateful hooks | Medium | | `effect-complexity` | **react** | Long or overloaded React effect hooks | Medium | @@ -56,6 +60,7 @@ For a user-facing selection table, see [`pack-chooser.md`](./pack-chooser.md). | `python-complex-control-flow` | **python** | Branch-heavy or deeply nested Python functions | Medium | | `python-dead-abstraction` | **python** | Thin Python functions that only pass arguments through | Low | | `python-todo-comment` | **python** | TODO/FIXME/HACK/temporary implementation comments in Python files | Low | +| `python-error-handling` | **python** | Empty/bare except blocks and log-only Python error handlers | Medium | | `python-route-sprawl` | **python-web** | Flask/Blueprint or Django URL modules registering too many routes | Medium | | `vue-todo-comment` | **vue** | TODO/FIXME/HACK/temporary comments inside Vue SFC script blocks | Low | | `vue-large-script` | **vue** | Oversized Vue SFC scripts or script functions | Medium | @@ -67,6 +72,7 @@ For a user-facing selection table, see [`pack-chooser.md`](./pack-chooser.md). | `kotlin-large-function` | **kotlin** | Kotlin functions over line or branch-count budgets | Medium | | `kotlin-dead-abstraction` | **kotlin** | Thin Kotlin functions that only pass arguments through | Low | | `kotlin-todo-comment` | **kotlin** | TODO/FIXME/HACK/temporary implementation comments in Kotlin files | Low | +| `kotlin-empty-catch` | **kotlin** | Empty or comment-only Kotlin catch blocks | Medium | | `swift-duplicate-logic` | **swift** | Near-duplicate Swift functions using normalized function-body similarity | Medium | | `swift-large-function` | **swift** | Swift functions over line or branch-count budgets | Medium | | `swift-dead-abstraction` | **swift** | Thin Swift functions that only pass arguments through | Low | diff --git a/docs/rules.md b/docs/rules.md index 30df550..7f046f5 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -978,6 +978,136 @@ When this is a false positive: Confidence: **0.86**. Test-only path conventions are strong signals. +## `empty-catch` + +Flags `try/catch` blocks whose handler body is empty or contains only comments, silently ignoring errors. + +Default thresholds: + +- `empty-catch.allowCommentOnly`: `0` (set to `1` to allow comment-only catch bodies) + +Why it matters: empty catch blocks hide failures, make debugging harder, and are a common artifact of quick fixes or assistant-generated code. + +Good fixes: + +- handle the error explicitly and return a typed result +- rethrow or wrap the error with context +- document why ignoring the error is safe, or use an inline suppression with a reason + +When this is a false positive: + +- best-effort cleanup where failure is intentionally ignored and documented +- catch blocks that delegate to a shared error handler in a macro or generated wrapper + +Confidence: **0.88** for empty or comment-only bodies. + +## `swallowed-error` + +Flags catch blocks that only log an error (`console.error`, `logger.*`, etc.) without rethrowing, returning a handled result, or otherwise acting on the failure. + +Why it matters: logging alone often looks like handling but still leaves callers unaware that work failed. + +Good fixes: + +- rethrow or return a typed error result +- escalate to monitoring with structured context +- handle the failure path the caller depends on + +When this is a false positive: + +- catch blocks that log and then return a fallback value or error object +- catch blocks that reference the error binding for metrics beyond the log call + +Confidence: **0.72**. Log-only handlers are advisory because some teams intentionally log-and-continue at boundaries. + +## `floating-promise` + +Flags promise-returning calls used as standalone expression statements without `await`, `return`, `void`, or `.catch(...)` handling. + +Default thresholds: + +- `floating-promise.allowVoid`: `1` (treat `void expr` as intentional fire-and-forget) +- `floating-promise.maxPerFile`: `12` + +Why it matters: unawaited promises can fail silently, race, or trigger unhandled rejections — especially inside React effects. + +Good fixes: + +- `await` the call inside an async function +- return the promise to the caller +- use `void fn()` only when fire-and-forget is intentional +- add `.catch(...)` or effect cleanup when starting async work in hooks + +When this is a false positive: + +- deliberate fire-and-forget marked with `void` +- callbacks passed directly to APIs that manage promise lifecycle + +Confidence: **0.65–0.82**. Higher when the callee is clearly async or returns `Promise<...>`. + +## `commented-out-code` + +Flags contiguous comment lines that look like executable code rather than prose or debt markers. + +Default thresholds: + +- `commented-out-code.minLines`: `2` +- `commented-out-code.maxPerFile`: `12` + +Why it matters: commented-out code rots, confuses readers, and duplicates history that version control already preserves. + +Good fixes: + +- delete dead code and rely on git history +- restore the code if it is still needed +- replace with a short comment explaining why something was removed, if context matters + +When this is a false positive: + +- commented examples in documentation blocks +- license or SPDX header comments +- short comment fragments that happen to look code-like + +Confidence: **0.60–0.80**, scaling with run length. This rule is intentionally conservative. + +## `python-error-handling` + +Flags Python `try/except` handlers that are empty (`pass` only), bare `except:`, or broad `except Exception:` blocks that only log without meaningful handling. + +Why it matters: silently swallowed Python exceptions are hard to diagnose in production and common in fast edits. + +Good fixes: + +- catch specific exception types +- rethrow or wrap with context +- return an explicit error result instead of logging alone + +When this is a false positive: + +- intentional best-effort cleanup with documented rationale +- handlers that log and then return a fallback value + +Confidence: **0.82–0.90** for empty/bare handlers; lower for log-only broad handlers. + +## `kotlin-empty-catch` + +Flags Kotlin `try/catch` blocks whose catch body is empty or contains only comments. + +Why it matters: empty catch blocks hide runtime failures and make Android or server Kotlin code harder to debug. + +Good fixes: + +- handle the exception explicitly +- rethrow or wrap with context +- document intentional suppression + +When this is a false positive: + +- generated or framework boilerplate with documented suppression +- catch blocks that delegate to shared error handling + +Confidence: **0.88** for empty or comment-only catch bodies. + ## `api-surface-sprawl` Flags files exporting too many public symbols. diff --git a/schema/debtlens.config.schema.json b/schema/debtlens.config.schema.json index 6be9384..ba78aa8 100644 --- a/schema/debtlens.config.schema.json +++ b/schema/debtlens.config.schema.json @@ -95,6 +95,10 @@ "naming-drift", "barrel-file", "weak-test-boundary", + "empty-catch", + "swallowed-error", + "floating-promise", + "commented-out-code", "api-surface-sprawl", "story-only-component", "python-todo-comment", @@ -102,6 +106,7 @@ "python-complex-control-flow", "python-duplicate-logic", "python-dead-abstraction", + "python-error-handling", "python-route-sprawl", "vue-todo-comment", "vue-large-script", @@ -113,6 +118,7 @@ "kotlin-large-function", "kotlin-duplicate-logic", "kotlin-dead-abstraction", + "kotlin-empty-catch", "swift-todo-comment", "swift-large-function", "swift-duplicate-logic", @@ -320,6 +326,21 @@ "compose-state-hoisting.maxLocalState": { "type": "number" }, + "empty-catch.allowCommentOnly": { + "type": "number" + }, + "floating-promise.allowVoid": { + "type": "number" + }, + "floating-promise.maxPerFile": { + "type": "number" + }, + "commented-out-code.minLines": { + "type": "number" + }, + "commented-out-code.maxPerFile": { + "type": "number" + }, "swiftui-large-view.maxLines": { "type": "number" }, @@ -549,6 +570,38 @@ "high" ] }, + "empty-catch": { + "enum": [ + "info", + "low", + "medium", + "high" + ] + }, + "swallowed-error": { + "enum": [ + "info", + "low", + "medium", + "high" + ] + }, + "floating-promise": { + "enum": [ + "info", + "low", + "medium", + "high" + ] + }, + "commented-out-code": { + "enum": [ + "info", + "low", + "medium", + "high" + ] + }, "api-surface-sprawl": { "enum": [ "info", @@ -605,6 +658,14 @@ "high" ] }, + "python-error-handling": { + "enum": [ + "info", + "low", + "medium", + "high" + ] + }, "python-route-sprawl": { "enum": [ "info", @@ -693,6 +754,14 @@ "high" ] }, + "kotlin-empty-catch": { + "enum": [ + "info", + "low", + "medium", + "high" + ] + }, "swift-todo-comment": { "enum": [ "info", @@ -955,6 +1024,26 @@ "minimum": 0, "maximum": 1 }, + "empty-catch": { + "type": "number", + "minimum": 0, + "maximum": 1 + }, + "swallowed-error": { + "type": "number", + "minimum": 0, + "maximum": 1 + }, + "floating-promise": { + "type": "number", + "minimum": 0, + "maximum": 1 + }, + "commented-out-code": { + "type": "number", + "minimum": 0, + "maximum": 1 + }, "api-surface-sprawl": { "type": "number", "minimum": 0, @@ -990,6 +1079,11 @@ "minimum": 0, "maximum": 1 }, + "python-error-handling": { + "type": "number", + "minimum": 0, + "maximum": 1 + }, "python-route-sprawl": { "type": "number", "minimum": 0, @@ -1045,6 +1139,11 @@ "minimum": 0, "maximum": 1 }, + "kotlin-empty-catch": { + "type": "number", + "minimum": 0, + "maximum": 1 + }, "swift-todo-comment": { "type": "number", "minimum": 0, diff --git a/src/config/defaults.ts b/src/config/defaults.ts index 1f05393..299421d 100644 --- a/src/config/defaults.ts +++ b/src/config/defaults.ts @@ -78,6 +78,11 @@ export const defaultConfig: Required line.rawLine.trim().slice(0, 220)) + .slice(0, 4), + suggestion: "Delete the commented-out code and rely on version control, or restore it if it is still needed.", + })); + + countForFile += 1; + if (countForFile >= maxPerFile) break; + } + } + + return issues; + }, +}; + +function extractCommentLines(lines: string[]): CommentLine[] { + const commentLines: CommentLine[] = []; + let inBlockComment = false; + + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? ""; + const lineNumber = index + 1; + + if (/debtlens-disable-(?:next-line|file)/i.test(line)) continue; + + if (inBlockComment) { + const endIndex = line.indexOf("*/"); + if (endIndex >= 0) { + inBlockComment = false; + const beforeClose = line.slice(0, endIndex); + const text = stripBlockCommentPrefix(beforeClose); + if (text.length > 0 || beforeClose.trim().length > 0) { + commentLines.push({ lineNumber, text, rawLine: line }); + } else { + commentLines.push({ lineNumber, text: "", rawLine: line }); + } + continue; + } + + commentLines.push({ + lineNumber, + text: stripBlockCommentPrefix(line), + rawLine: line, + }); + continue; + } + + const blockStart = line.indexOf("/*"); + if (blockStart >= 0 && isOnlyWhitespace(line.slice(0, blockStart))) { + const afterStart = line.slice(blockStart + 2); + const endIndex = afterStart.indexOf("*/"); + if (endIndex >= 0) { + const text = stripBlockCommentPrefix(afterStart.slice(0, endIndex)); + commentLines.push({ lineNumber, text, rawLine: line }); + continue; + } + + inBlockComment = true; + const text = stripBlockCommentPrefix(afterStart); + commentLines.push({ lineNumber, text, rawLine: line }); + continue; + } + + const lineCommentMatch = line.match(/^\s*\/\/(.*)$/); + if (lineCommentMatch) { + commentLines.push({ + lineNumber, + text: (lineCommentMatch[1] ?? "").trim(), + rawLine: line, + }); + } + } + + return commentLines; +} + +function stripBlockCommentPrefix(line: string): string { + return line.replace(/^\s*\*?\s?/, "").trim(); +} + +function isOnlyWhitespace(value: string): boolean { + return value.trim().length === 0; +} + +function groupCommentRuns(commentLines: CommentLine[]): CommentRun[] { + if (commentLines.length === 0) return []; + + const runs: CommentRun[] = []; + let current: CommentLine[] = [commentLines[0]!]; + + for (let index = 1; index < commentLines.length; index += 1) { + const line = commentLines[index]!; + const previous = commentLines[index - 1]!; + if (line.lineNumber === previous.lineNumber + 1) { + current.push(line); + continue; + } + + runs.push({ + startLine: current[0]!.lineNumber, + endLine: current[current.length - 1]!.lineNumber, + lines: current, + }); + current = [line]; + } + + runs.push({ + startLine: current[0]!.lineNumber, + endLine: current[current.length - 1]!.lineNumber, + lines: current, + }); + + return runs; +} + +function isExcludedRun( + run: CommentRun, + todoPatterns: ReturnType, +): boolean { + for (const line of run.lines) { + if (isLicenseOrCopyrightLine(line.rawLine, line.text)) return true; + if (todoPatterns.some((pattern) => pattern.regex.test(line.rawLine))) return true; + } + return false; +} + +function isLicenseOrCopyrightLine(rawLine: string, text: string): boolean { + const combined = `${rawLine}\n${text}`; + return /SPDX-License-Identifier/i.test(combined) + || /\bCopyright\b/i.test(combined) + || /\bLicensed under\b/i.test(combined) + || /\bAll rights reserved\b/i.test(combined); +} + +function runHasCodeLikeLine(run: CommentRun): boolean { + return run.lines.some((line) => isCodeLikeComment(line.text)); +} + +function isCodeLikeComment(text: string): boolean { + const trimmed = text.trim(); + if (!trimmed) return false; + if (isProseOnly(trimmed)) return false; + return looksLikeCode(trimmed); +} + +function looksLikeCode(text: string): boolean { + if (/[;{})\]]$/.test(text)) return true; + return /\b(?:const|let|var|function|import|return|class|export|async|await)\b/.test(text) + || /\bif\s*\(/.test(text) + || /\bfor\s*\(/.test(text) + || /\bwhile\s*\(/.test(text); +} + +function isProseOnly(text: string): boolean { + if (looksLikeCode(text)) return false; + if (/[;{}()[\]=<>]/.test(text)) return false; + return /^[\s"A-Za-z0-9_,.'!?-]+$/.test(text) && /\s/.test(text); +} + +function confidenceForRunLength(runLength: number, minLines: number): number { + const extraLines = Math.max(0, runLength - minLines); + return Math.min(0.8, 0.6 + extraLines * 0.04); +} diff --git a/src/detectors/errorHandling.ts b/src/detectors/errorHandling.ts new file mode 100644 index 0000000..2e0d518 --- /dev/null +++ b/src/detectors/errorHandling.ts @@ -0,0 +1,257 @@ +import { Node, SyntaxKind } from "ts-morph"; +import type { CatchClause, Node as MorphNode, Statement } from "ts-morph"; +import type { DebtIssue, Detector, DetectorContext } from "../core/types.js"; +import { createIssue } from "../utils/createIssue.js"; +import { nodeLineSpan } from "../utils/lines.js"; + +const MAX_FINDINGS_PER_FILE = 12; +const disableNextLinePattern = /debtlens-disable-next-line\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; +const disableFilePattern = /debtlens-disable-file\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; + +export const emptyCatchDetector: Detector = { + id: "empty-catch", + name: "Empty catch block", + description: "Flags catch blocks that silently ignore errors without handling or rethrowing.", + defaultSeverity: "medium", + tags: ["error-handling", "maintainability", "review"], + detect(context: DetectorContext): DebtIssue[] { + const allowCommentOnly = context.getThreshold("empty-catch.allowCommentOnly", 0) >= 1; + const issues: DebtIssue[] = []; + + for (const file of context.files) { + let countForFile = 0; + const suppressedRules = parseFileSuppressions(file.content); + + for (const tryStatement of file.sourceFile.getDescendantsOfKind(SyntaxKind.TryStatement)) { + const catchClause = tryStatement.getCatchClause(); + if (!catchClause) continue; + + const span = nodeLineSpan(catchClause); + if (isSuppressed(suppressedRules, "empty-catch", span.startLine)) continue; + + const classification = classifyCatchBody(catchClause); + if (classification === "handled") continue; + if (classification === "comment-only" && allowCommentOnly) continue; + + issues.push(createEmptyCatchIssue(file.relativePath, catchClause, classification)); + countForFile += 1; + if (countForFile >= MAX_FINDINGS_PER_FILE) break; + } + } + + return issues; + }, +}; + +export const swallowedErrorDetector: Detector = { + id: "swallowed-error", + name: "Swallowed error", + description: "Flags catch blocks that only log an error without rethrowing, returning, or otherwise handling it.", + defaultSeverity: "medium", + tags: ["error-handling", "maintainability", "review"], + detect(context: DetectorContext): DebtIssue[] { + const issues: DebtIssue[] = []; + + for (const file of context.files) { + let countForFile = 0; + const suppressedRules = parseFileSuppressions(file.content); + + for (const tryStatement of file.sourceFile.getDescendantsOfKind(SyntaxKind.TryStatement)) { + const catchClause = tryStatement.getCatchClause(); + if (!catchClause) continue; + + const span = nodeLineSpan(catchClause); + if (isSuppressed(suppressedRules, "swallowed-error", span.startLine)) continue; + if (!isSwallowedErrorCatch(catchClause)) continue; + + issues.push(createSwallowedErrorIssue(file.relativePath, catchClause)); + countForFile += 1; + if (countForFile >= MAX_FINDINGS_PER_FILE) break; + } + } + + return issues; + }, +}; + +type CatchBodyClassification = "empty" | "comment-only" | "handled"; + +function classifyCatchBody(catchClause: CatchClause): CatchBodyClassification { + const block = catchClause.getBlock(); + const statements = block.getStatements(); + const meaningfulStatements = statements.filter((statement) => !Node.isEmptyStatement(statement)); + if (meaningfulStatements.length > 0) return "handled"; + + if (statements.length === 0 && !blockHasComments(block)) return "empty"; + if (statements.every((statement) => Node.isEmptyStatement(statement)) && !blockHasComments(block)) { + return "empty"; + } + + return "comment-only"; +} + +function blockHasComments(block: MorphNode): boolean { + const text = block.getText().replace(/^\{|\}$/g, "").trim(); + if (!text) return false; + const withoutComments = text + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/\/\/[^\n\r]*/g, "") + .trim(); + return withoutComments.length === 0; +} + +function isSwallowedErrorCatch(catchClause: CatchClause): boolean { + const block = catchClause.getBlock(); + const meaningfulStatements = block.getStatements().filter((statement) => !Node.isEmptyStatement(statement)); + if (meaningfulStatements.length === 0) return false; + if (meaningfulStatements.some((statement) => Node.isThrowStatement(statement))) return false; + if (meaningfulStatements.some((statement) => Node.isReturnStatement(statement))) return false; + if (!meaningfulStatements.every((statement) => isLoggingStatement(statement))) return false; + if (usesCatchBindingBeyondLogArgs(catchClause, meaningfulStatements)) return false; + return true; +} + +function isLoggingStatement(statement: Statement): boolean { + if (!Node.isExpressionStatement(statement)) return false; + const expression = statement.getExpression(); + if (!Node.isCallExpression(expression)) return false; + return isLoggingCallExpression(expression); +} + +function isLoggingCallExpression(call: MorphNode): boolean { + if (!Node.isCallExpression(call)) return false; + const callee = call.getExpression(); + if (!Node.isPropertyAccessExpression(callee)) return false; + + const target = callee.getExpression().getText(); + const method = callee.getName(); + if (target === "console") return method === "error" || method === "warn" || method === "log"; + if (target === "logger") return true; + return false; +} + +function usesCatchBindingBeyondLogArgs(catchClause: CatchClause, statements: Statement[]): boolean { + const bindingName = catchClause.getVariableDeclaration()?.getName(); + if (!bindingName) return false; + + const logCalls = statements.flatMap((statement) => + statement.getDescendantsOfKind(SyntaxKind.CallExpression).filter(isLoggingCallExpression), + ); + + for (const statement of statements) { + for (const identifier of statement.getDescendantsOfKind(SyntaxKind.Identifier)) { + if (identifier.getText() !== bindingName) continue; + if (isLoggingCalleeIdentifier(identifier)) continue; + if (!isWithinLoggingCallArgument(identifier, logCalls)) return true; + } + } + + return false; +} + +function isLoggingCalleeIdentifier(identifier: MorphNode): boolean { + const parent = identifier.getParent(); + return Node.isPropertyAccessExpression(parent) && parent.getNameNode() === identifier; +} + +function isWithinLoggingCallArgument(identifier: MorphNode, logCalls: MorphNode[]): boolean { + for (const logCall of logCalls) { + if (!Node.isCallExpression(logCall)) continue; + for (const argument of logCall.getArguments()) { + if (argument === identifier) return true; + if (identifier.getAncestors().some((ancestor) => ancestor === argument)) return true; + } + } + return false; +} + +interface FileSuppressionRules { + fileRules: Set; + nextLineRules: Map>; + inlineLineRules: Map>; +} + +function parseFileSuppressions(content: string): FileSuppressionRules { + const fileRules = new Set(); + const nextLineRules = new Map>(); + const inlineLineRules = new Map>(); + const lines = content.split(/\r?\n/); + + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? ""; + const lineNumber = index + 1; + + const fileMatch = line.match(disableFilePattern); + if (fileMatch?.[1] && fileMatch[2]?.trim()) { + fileRules.add(fileMatch[1].toLowerCase()); + inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), fileMatch[1])); + continue; + } + + const nextLineMatch = line.match(disableNextLinePattern); + if (!nextLineMatch?.[1] || !nextLineMatch[2]?.trim()) continue; + + const ruleId = nextLineMatch[1].toLowerCase(); + inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), ruleId)); + nextLineRules.set(lineNumber + 1, addRule(nextLineRules.get(lineNumber + 1), ruleId)); + } + + return { fileRules, nextLineRules, inlineLineRules }; +} + +function addRule(existing: Set | undefined, ruleId: string): Set { + const rules = existing ?? new Set(); + rules.add(ruleId.toLowerCase()); + return rules; +} + +function isSuppressed(rules: FileSuppressionRules, ruleId: string, line: number): boolean { + const normalizedRuleId = ruleId.toLowerCase(); + if (rules.fileRules.has(normalizedRuleId)) return true; + if (rules.nextLineRules.get(line)?.has(normalizedRuleId)) return true; + if (rules.inlineLineRules.get(line)?.has(normalizedRuleId)) return true; + return false; +} + +function createEmptyCatchIssue( + file: string, + catchClause: CatchClause, + classification: CatchBodyClassification, +): DebtIssue { + const span = nodeLineSpan(catchClause); + const detail = classification === "comment-only" + ? "contains only comments" + : classification === "empty" + ? "is empty" + : "ignores errors"; + + return createIssue({ + detector: emptyCatchDetector, + severity: "medium", + confidence: 0.88, + file, + location: { startLine: span.startLine, endLine: span.endLine }, + message: `Catch block ${detail} and silently ignores errors.`, + evidence: [ + `Catch body: ${classification}`, + catchClause.getBlock().getText().trim().slice(0, 220), + ], + suggestion: "Handle the error explicitly, rethrow it, or document why ignoring it is safe.", + }); +} + +function createSwallowedErrorIssue(file: string, catchClause: CatchClause): DebtIssue { + const span = nodeLineSpan(catchClause); + return createIssue({ + detector: swallowedErrorDetector, + severity: "medium", + confidence: 0.72, + file, + location: { startLine: span.startLine, endLine: span.endLine }, + message: "Catch block only logs the error without rethrowing or returning a handled result.", + evidence: [ + catchClause.getBlock().getText().trim().slice(0, 220), + ], + suggestion: "Rethrow, return a typed error result, or escalate to monitoring instead of logging alone.", + }); +} diff --git a/src/detectors/floatingPromise.ts b/src/detectors/floatingPromise.ts new file mode 100644 index 0000000..36dd810 --- /dev/null +++ b/src/detectors/floatingPromise.ts @@ -0,0 +1,289 @@ +import { Node, SyntaxKind } from "ts-morph"; +import type { CallExpression, Expression, ExpressionStatement, Node as MorphNode } from "ts-morph"; +import type { DebtIssue, Detector, DetectorContext } from "../core/types.js"; +import { createIssue } from "../utils/createIssue.js"; +import { lineAt, nodeLineSpan } from "../utils/lines.js"; + +const EFFECT_HOOKS = new Set(["useEffect", "useLayoutEffect", "useInsertionEffect"]); +const disableNextLinePattern = /debtlens-disable-next-line\s+floating-promise(?:\s+--\s+.+)?.*/i; +const disableFilePattern = /debtlens-disable-file\s+floating-promise(?:\s+--\s+.+)?.*/i; + +export const floatingPromiseDetector: Detector = { + id: "floating-promise", + name: "Floating promise", + description: "Flags promise-returning calls that are not awaited, returned, void-marked, or error-handled.", + defaultSeverity: "medium", + tags: ["async", "promises", "react", "maintainability"], + detect(context: DetectorContext): DebtIssue[] { + const allowVoid = context.getThreshold("floating-promise.allowVoid", 1) >= 1; + const maxPerFile = context.getThreshold("floating-promise.maxPerFile", 12); + const issues: DebtIssue[] = []; + + for (const file of context.files) { + if (isFileSuppressed(file.content)) continue; + + let countForFile = 0; + for (const statement of file.sourceFile.getDescendantsOfKind(SyntaxKind.ExpressionStatement)) { + if (countForFile >= maxPerFile) break; + if (isLineSuppressed(file.content, statement)) continue; + + const call = extractFloatingCall(statement, allowVoid); + if (!call) continue; + if (isSkippedContext(call)) continue; + if (hasCatchOnChain(call)) continue; + + const promiseLike = assessPromiseLike(call, context); + if (!promiseLike) continue; + + const effectHook = getEnclosingEffectHook(call); + const span = nodeLineSpan(call); + const evidence = [ + `Expression: ${truncate(call.getText())}`, + "Not awaited, returned, assigned, void-marked, or passed as an argument", + ...(effectHook ? ["Inside React effect without cleanup or error handling"] : []), + ...promiseLike.evidence, + ]; + + issues.push(createIssue({ + detector: floatingPromiseDetector, + severity: effectHook ? "medium" : floatingPromiseDetector.defaultSeverity, + confidence: promiseLike.confidence, + file: file.relativePath, + location: { startLine: span.startLine, endLine: span.endLine }, + message: effectHook + ? `Promise work inside ${effectHook} is not awaited or error-handled.` + : "Promise-returning call is not awaited, returned, or explicitly handled.", + evidence, + suggestion: effectHook + ? "Await the promise inside the effect, chain .catch(), or move async work into a named helper with explicit error handling." + : "Await the call, return it to the caller, attach .catch(), or prefix with void when fire-and-forget is intentional.", + })); + + countForFile += 1; + } + } + + return issues; + }, +}; + +function extractFloatingCall(statement: ExpressionStatement, allowVoid: boolean): CallExpression | undefined { + let expression = unwrapParentheses(statement.getExpression()); + if (Node.isVoidExpression(expression)) { + if (allowVoid) return undefined; + expression = unwrapParentheses(expression.getExpression()); + } + return Node.isCallExpression(expression) ? expression : undefined; +} + +function isSkippedContext(node: MorphNode): boolean { + return isInsideAwait(node) + || isInsideReturn(node) + || isInsideVariableDeclaration(node) + || isPassedAsArgument(node); +} + +function isInsideAwait(node: MorphNode): boolean { + return node.getFirstAncestorByKind(SyntaxKind.AwaitExpression) !== undefined; +} + +function isInsideReturn(node: MorphNode): boolean { + return node.getFirstAncestorByKind(SyntaxKind.ReturnStatement) !== undefined; +} + +function isInsideVariableDeclaration(node: MorphNode): boolean { + return node.getFirstAncestorByKind(SyntaxKind.VariableDeclaration) !== undefined; +} + +function isPassedAsArgument(node: MorphNode): boolean { + const parent = node.getParent(); + if (!parent) return false; + if (!Node.isCallExpression(parent) && !Node.isNewExpression(parent)) return false; + return parent.getArguments().some((argument) => argument === node); +} + +function hasCatchOnChain(call: CallExpression): boolean { + let current: MorphNode = call; + while (Node.isCallExpression(current)) { + const callee = current.getExpression(); + if (Node.isPropertyAccessExpression(callee) && callee.getName() === "catch") { + return true; + } + + if (Node.isPropertyAccessExpression(callee) && Node.isCallExpression(callee.getExpression())) { + current = callee.getExpression(); + continue; + } + + break; + } + + return false; +} + +interface PromiseAssessment { + confidence: number; + evidence: string[]; +} + +function assessPromiseLike(call: CallExpression, context: DetectorContext): PromiseAssessment | undefined { + const evidence: string[] = []; + let confidence = 0.68; + + if (chainUsesMethod(call, "then")) { + evidence.push("Promise chain uses .then()"); + confidence = 0.8; + } + + const rootCall = getInnermostCall(call); + const rootName = getCallTargetName(rootCall); + if (rootName === "fetch") { + evidence.push("Calls fetch()"); + confidence = Math.max(confidence, 0.82); + } + + if (rootName === "Promise") { + evidence.push("Calls Promise.*"); + confidence = Math.max(confidence, 0.84); + } + + if (isAsyncCallee(rootCall)) { + evidence.push("Calls async function"); + confidence = Math.max(confidence, 0.8); + } + + const typeText = getPromiseReturnTypeText(call); + if (typeText) { + evidence.push(`Type checker reports ${typeText}`); + confidence = Math.max(confidence, 0.88); + } + + if (evidence.length === 0) return undefined; + return { confidence, evidence }; +} + +function chainUsesMethod(call: CallExpression, methodName: string): boolean { + let current: MorphNode = call; + while (Node.isCallExpression(current)) { + const callee = current.getExpression(); + if (Node.isPropertyAccessExpression(callee) && callee.getName() === methodName) { + return true; + } + + if (Node.isPropertyAccessExpression(callee) && Node.isCallExpression(callee.getExpression())) { + current = callee.getExpression(); + continue; + } + + break; + } + + return false; +} + +function getInnermostCall(call: CallExpression): CallExpression { + let current = call; + while (true) { + const callee = current.getExpression(); + if (Node.isPropertyAccessExpression(callee)) { + const inner = callee.getExpression(); + if (Node.isCallExpression(inner)) { + current = inner; + continue; + } + } + break; + } + + return current; +} + +function getCallTargetName(call: CallExpression): string | undefined { + const expression = call.getExpression(); + if (Node.isIdentifier(expression)) return expression.getText(); + if (Node.isPropertyAccessExpression(expression)) { + if (Node.isIdentifier(expression.getExpression())) { + return expression.getExpression().getText(); + } + return expression.getName(); + } + return undefined; +} + +function isAsyncCallee(call: CallExpression): boolean { + const expression = call.getExpression(); + if (!Node.isIdentifier(expression)) return false; + + const declarations = expression.getSymbol()?.getDeclarations() ?? []; + for (const declaration of declarations) { + if ( + (Node.isFunctionDeclaration(declaration) + || Node.isFunctionExpression(declaration) + || Node.isArrowFunction(declaration)) + && declaration.isAsync() + ) { + return true; + } + } + + return false; +} + +function getPromiseReturnTypeText(call: CallExpression): string | undefined { + try { + const returnType = call.getReturnType(); + const text = returnType.getText(call); + if (/Promise\s*<|Promise$/i.test(text)) { + return truncate(text, 80); + } + } catch { + return undefined; + } + + return undefined; +} + +function getEnclosingEffectHook(node: MorphNode): string | undefined { + for (const ancestor of node.getAncestors()) { + if (!Node.isArrowFunction(ancestor) && !Node.isFunctionExpression(ancestor)) continue; + + const parent = ancestor.getParent(); + if (!Node.isCallExpression(parent)) continue; + + const hookName = getExpressionName(parent.getExpression()); + if (!hookName || !EFFECT_HOOKS.has(hookName)) continue; + if (parent.getArguments()[0] !== ancestor) continue; + return hookName; + } + + return undefined; +} + +function getExpressionName(expression: MorphNode): string | undefined { + if (Node.isIdentifier(expression)) return expression.getText(); + if (Node.isPropertyAccessExpression(expression)) return expression.getName(); + return undefined; +} + +function unwrapParentheses(expression: Expression): Expression { + let current: Expression = expression; + while (Node.isParenthesizedExpression(current)) { + current = current.getExpression(); + } + return current; +} + +function isFileSuppressed(content: string): boolean { + return content.split(/\r?\n/).some((line) => disableFilePattern.test(line)); +} + +function isLineSuppressed(content: string, node: MorphNode): boolean { + const lineNumber = nodeLineSpan(node).startLine; + if (lineNumber <= 1) return false; + return disableNextLinePattern.test(lineAt(content, lineNumber - 1)); +} + +function truncate(text: string, maxLength = 120): string { + const normalized = text.replace(/\s+/g, " ").trim(); + return normalized.length <= maxLength ? normalized : `${normalized.slice(0, maxLength - 3)}...`; +} diff --git a/src/detectors/index.ts b/src/detectors/index.ts index 6dd8c48..65e28c1 100644 --- a/src/detectors/index.ts +++ b/src/detectors/index.ts @@ -7,14 +7,17 @@ import { contextProviderSprawlDetector } from "./contextProviderSprawl.js"; import { complexControlFlowDetector } from "./complexControlFlow.js"; import { configDriftDetector } from "./configDrift.js"; import { dataLoaderSprawlDetector } from "./dataLoaderSprawl.js"; +import { commentedOutCodeDetector } from "./commentedOutCode.js"; import { deadAbstractionDetector } from "./deadAbstraction.js"; +import { emptyCatchDetector, swallowedErrorDetector } from "./errorHandling.js"; +import { floatingPromiseDetector } from "./floatingPromise.js"; import { duplicateLogicDetector } from "./duplicateLogic.js"; import { duplicatedLiteralDetector } from "./duplicatedLiteral.js"; import { effectComplexityDetector } from "./effectComplexity.js"; import { handlerDepthDetector } from "./handlerDepth.js"; import { hookDependencySmellDetector } from "./hookDependencySmell.js"; import { importCycleDetector } from "./importCycle.js"; -import { kotlinDeadAbstractionDetector, kotlinDuplicateLogicDetector, kotlinLargeFunctionDetector, kotlinTodoCommentDetector } from "./kotlin/index.js"; +import { kotlinDeadAbstractionDetector, kotlinDuplicateLogicDetector, kotlinEmptyCatchDetector, kotlinLargeFunctionDetector, kotlinTodoCommentDetector } from "./kotlin/index.js"; import { swiftDeadAbstractionDetector, swiftDuplicateLogicDetector, swiftLargeFunctionDetector, swiftTodoCommentDetector } from "./swift/index.js"; import { swiftuiLargeViewDetector, swiftuiStateSprawlDetector } from "./swiftui/index.js"; import { largeComponentDetector } from "./largeComponent.js"; @@ -25,6 +28,7 @@ import { pythonComplexControlFlowDetector, pythonDeadAbstractionDetector, pythonDuplicateLogicDetector, + pythonErrorHandlingDetector, pythonLargeFunctionDetector, pythonRouteSprawlDetector, pythonTodoCommentDetector, @@ -73,6 +77,10 @@ export const allDetectors: Detector[] = [ namingDriftDetector, barrelFileDetector, weakTestBoundaryDetector, + emptyCatchDetector, + swallowedErrorDetector, + floatingPromiseDetector, + commentedOutCodeDetector, apiSurfaceSprawlDetector, storyOnlyComponentDetector, pythonTodoCommentDetector, @@ -80,6 +88,7 @@ export const allDetectors: Detector[] = [ pythonComplexControlFlowDetector, pythonDuplicateLogicDetector, pythonDeadAbstractionDetector, + pythonErrorHandlingDetector, pythonRouteSprawlDetector, vueTodoCommentDetector, vueLargeScriptDetector, @@ -91,6 +100,7 @@ export const allDetectors: Detector[] = [ kotlinLargeFunctionDetector, kotlinDuplicateLogicDetector, kotlinDeadAbstractionDetector, + kotlinEmptyCatchDetector, swiftTodoCommentDetector, swiftLargeFunctionDetector, swiftDuplicateLogicDetector, diff --git a/src/detectors/kotlin/errorHandling.ts b/src/detectors/kotlin/errorHandling.ts new file mode 100644 index 0000000..8c9c1e2 --- /dev/null +++ b/src/detectors/kotlin/errorHandling.ts @@ -0,0 +1,194 @@ +import type { DebtIssue, Detector, DetectorContext } from "../../core/types.js"; +import { createIssue } from "../../utils/createIssue.js"; + +const MAX_FINDINGS_PER_FILE = 12; +const disableNextLinePattern = /debtlens-disable-next-line\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; +const disableFilePattern = /debtlens-disable-file\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; + +export const kotlinEmptyCatchDetector: Detector = { + id: "kotlin-empty-catch", + name: "Kotlin empty catch block", + description: "Flags catch blocks that silently ignore errors without handling or rethrowing.", + defaultSeverity: "medium", + tags: ["kotlin", "error-handling", "maintainability", "review"], + languages: ["kotlin"], + detect(context: DetectorContext): DebtIssue[] { + const issues: DebtIssue[] = []; + + for (const file of context.files) { + let countForFile = 0; + const suppressedRules = parseFileSuppressions(file.content); + const lines = file.content.split(/\r?\n/); + + for (const catchBlock of findAllKotlinCatchBlocks(lines)) { + if (isSuppressed(suppressedRules, kotlinEmptyCatchDetector.id, catchBlock.catchLine)) continue; + + const classification = classifyKotlinCatchBody(catchBlock.bodyText); + if (classification === "handled") continue; + + issues.push(createKotlinEmptyCatchIssue(file.relativePath, catchBlock, classification)); + countForFile += 1; + if (countForFile >= MAX_FINDINGS_PER_FILE) break; + } + } + + return issues; + }, +}; + +type CatchBodyClassification = "empty" | "comment-only" | "handled"; + +interface KotlinCatchBlock { + catchLine: number; + endLine: number; + bodyText: string; +} + +interface FileSuppressionRules { + fileRules: Set; + nextLineRules: Map>; + inlineLineRules: Map>; +} + +function findAllKotlinCatchBlocks(lines: string[]): KotlinCatchBlock[] { + const text = lines.join("\n"); + const masked = maskKotlinComments(text); + const blocks: KotlinCatchBlock[] = []; + const catchPattern = /\bcatch\s*\(/g; + let match = catchPattern.exec(masked); + + while (match) { + const lineIndex = countLineBreaks(text.slice(0, match.index)); + const block = parseKotlinCatchBlock(lines, lineIndex); + if (block) blocks.push(block); + match = catchPattern.exec(masked); + } + + return blocks; +} + +function parseKotlinCatchBlock(lines: string[], catchStartIndex: number): KotlinCatchBlock | undefined { + const text = lines.slice(catchStartIndex).join("\n"); + const code = maskKotlinComments(text); + const catchIndex = code.search(/\bcatch\s*\(/); + if (catchIndex < 0) return undefined; + + const braceIndex = code.indexOf("{", catchIndex); + if (braceIndex < 0) return undefined; + + const endOffset = findMatchingBraceEnd(code, braceIndex); + if (endOffset < 0) return undefined; + + const blockText = text.slice(catchIndex, endOffset + 1); + const bodyText = blockText.slice(blockText.indexOf("{") + 1, blockText.lastIndexOf("}")); + const catchLine = catchStartIndex + countLineBreaks(text.slice(0, catchIndex)) + 1; + const endLine = catchStartIndex + countLineBreaks(text.slice(0, endOffset + 1)) + 1; + + return { + catchLine, + endLine, + bodyText, + }; +} + +function classifyKotlinCatchBody(bodyText: string): CatchBodyClassification { + const withoutComments = bodyText + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/\/\/[^\n\r]*/g, "") + .trim(); + + if (!withoutComments) { + return bodyText.trim() ? "comment-only" : "empty"; + } + + return "handled"; +} + +function createKotlinEmptyCatchIssue( + file: string, + catchBlock: KotlinCatchBlock, + classification: CatchBodyClassification, +): DebtIssue { + const detail = classification === "comment-only" + ? "contains only comments" + : "is empty"; + + return createIssue({ + detector: kotlinEmptyCatchDetector, + severity: "medium", + confidence: 0.88, + file, + location: { startLine: catchBlock.catchLine, endLine: catchBlock.endLine }, + message: `Catch block ${detail} and silently ignores errors.`, + evidence: [ + `Catch body: ${classification}`, + `{${catchBlock.bodyText.trim().slice(0, 220)}}`, + ], + suggestion: "Handle the error explicitly, rethrow it, or document why ignoring it is safe.", + }); +} + +function findMatchingBraceEnd(code: string, openIndex: number): number { + let depth = 0; + for (let index = openIndex; index < code.length; index += 1) { + const char = code[index] ?? ""; + if (char === "{") depth += 1; + else if (char === "}") { + depth -= 1; + if (depth === 0) return index; + } + } + return -1; +} + +function maskKotlinComments(text: string): string { + return text + .replace(/\/\*[\s\S]*?\*\//g, (match) => " ".repeat(match.length)) + .replace(/\/\/[^\n\r]*/g, (match) => " ".repeat(match.length)); +} + +function countLineBreaks(text: string): number { + return text.match(/\n/g)?.length ?? 0; +} + +function parseFileSuppressions(content: string): FileSuppressionRules { + const fileRules = new Set(); + const nextLineRules = new Map>(); + const inlineLineRules = new Map>(); + const lines = content.split(/\r?\n/); + + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? ""; + const lineNumber = index + 1; + + const fileMatch = line.match(disableFilePattern); + if (fileMatch?.[1] && fileMatch[2]?.trim()) { + fileRules.add(fileMatch[1].toLowerCase()); + inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), fileMatch[1])); + continue; + } + + const nextLineMatch = line.match(disableNextLinePattern); + if (!nextLineMatch?.[1] || !nextLineMatch[2]?.trim()) continue; + + const ruleId = nextLineMatch[1].toLowerCase(); + inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), ruleId)); + nextLineRules.set(lineNumber + 1, addRule(nextLineRules.get(lineNumber + 1), ruleId)); + } + + return { fileRules, nextLineRules, inlineLineRules }; +} + +function addRule(existing: Set | undefined, ruleId: string): Set { + const rules = existing ?? new Set(); + rules.add(ruleId.toLowerCase()); + return rules; +} + +function isSuppressed(rules: FileSuppressionRules, ruleId: string, line: number): boolean { + const normalizedRuleId = ruleId.toLowerCase(); + if (rules.fileRules.has(normalizedRuleId)) return true; + if (rules.nextLineRules.get(line)?.has(normalizedRuleId)) return true; + if (rules.inlineLineRules.get(line)?.has(normalizedRuleId)) return true; + return false; +} diff --git a/src/detectors/kotlin/index.ts b/src/detectors/kotlin/index.ts index 46c0cda..564f5e7 100644 --- a/src/detectors/kotlin/index.ts +++ b/src/detectors/kotlin/index.ts @@ -1,4 +1,5 @@ export { kotlinDeadAbstractionDetector } from "./deadAbstraction.js"; +export { kotlinEmptyCatchDetector } from "./errorHandling.js"; export { kotlinDuplicateLogicDetector } from "./duplicateLogic.js"; export { kotlinLargeFunctionDetector } from "./largeFunction.js"; export { kotlinTodoCommentDetector } from "./todoComment.js"; diff --git a/src/detectors/python/errorHandling.ts b/src/detectors/python/errorHandling.ts new file mode 100644 index 0000000..8608999 --- /dev/null +++ b/src/detectors/python/errorHandling.ts @@ -0,0 +1,305 @@ +import type { DebtIssue, Detector, DetectorContext } from "../../core/types.js"; +import { createIssue } from "../../utils/createIssue.js"; + +const MAX_FINDINGS_PER_FILE = 12; +const disableNextLinePattern = /debtlens-disable-next-line\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; +const disableFilePattern = /debtlens-disable-file\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; + +export const pythonErrorHandlingDetector: Detector = { + id: "python-error-handling", + name: "Python error handling smell", + description: "Flags except blocks that silently ignore errors or only log broad Exception handlers.", + defaultSeverity: "medium", + tags: ["python", "error-handling", "maintainability", "review"], + languages: ["python"], + detect(context: DetectorContext): DebtIssue[] { + const issues: DebtIssue[] = []; + + for (const file of context.files) { + let countForFile = 0; + const suppressedRules = parseFileSuppressions(file.content); + const lines = file.content.split(/\r?\n/); + + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? ""; + if (!/^\s*try\s*:/.test(stripPythonComment(line))) continue; + + const exceptBlocks = findPythonExceptBlocks(lines, index); + for (const exceptBlock of exceptBlocks) { + if (isSuppressed(suppressedRules, pythonErrorHandlingDetector.id, exceptBlock.exceptLine)) continue; + + const reason = classifyPythonExcept(exceptBlock.typePart, exceptBlock.bodyLines); + if (!reason) continue; + + issues.push(createPythonErrorHandlingIssue(file.relativePath, exceptBlock, reason)); + countForFile += 1; + if (countForFile >= MAX_FINDINGS_PER_FILE) break; + } + + if (countForFile >= MAX_FINDINGS_PER_FILE) break; + } + } + + return issues; + }, +}; + +type PythonExceptReason = "pass-or-comment" | "broad-log-only"; + +interface PythonExceptBlock { + exceptLine: number; + endLine: number; + typePart: string | undefined; + bodyLines: string[]; + headerText: string; +} + +interface FileSuppressionRules { + fileRules: Set; + nextLineRules: Map>; + inlineLineRules: Map>; +} + +function findPythonExceptBlocks(lines: string[], tryLineIndex: number): PythonExceptBlock[] { + const tryIndent = indentation(lines[tryLineIndex] ?? ""); + const blocks: PythonExceptBlock[] = []; + let index = tryLineIndex + 1; + + while (index < lines.length) { + const line = lines[index] ?? ""; + if (!line.trim()) { + index += 1; + continue; + } + if (indentation(line) <= tryIndent) break; + index += 1; + } + + while (index < lines.length) { + const line = lines[index] ?? ""; + const trimmed = stripPythonComment(line).trim(); + const lineIndent = indentation(line); + + if (lineIndent < tryIndent) break; + if (lineIndent > tryIndent) { + index += 1; + continue; + } + + if (/^finally\s*:/.test(trimmed)) break; + if (/^else\s*:/.test(trimmed)) { + index = skipPythonBlock(lines, index, lineIndent); + continue; + } + if (!/^except\b/.test(trimmed)) break; + + const header = parsePythonExceptHeader(lines, index, tryIndent); + if (!header) break; + + const bodyLines = header.inlineBody !== undefined + ? [header.inlineBody] + : collectPythonBlockBody(lines, header.endIndex, indentation(lines[header.endIndex] ?? "")); + + blocks.push({ + exceptLine: index + 1, + endLine: bodyLines.length > 0 && header.inlineBody === undefined + ? header.endIndex + bodyLines.length + : header.endIndex + 1, + typePart: header.typePart, + bodyLines, + headerText: header.headerText, + }); + + index = header.endIndex + 1; + if (header.inlineBody === undefined) { + index += bodyLines.length; + } + } + + return blocks; +} + +function parsePythonExceptHeader( + lines: string[], + startIndex: number, + blockIndent: number, +): { endIndex: number; typePart: string | undefined; inlineBody: string | undefined; headerText: string } | undefined { + const headerParts: string[] = []; + + for (let index = startIndex; index < lines.length; index += 1) { + const raw = lines[index] ?? ""; + const lineIndent = indentation(raw); + if (index > startIndex && lineIndent < blockIndent) return undefined; + if (index > startIndex && lineIndent > blockIndent) return undefined; + + const code = stripPythonComment(raw); + headerParts.push(code.trim()); + const combined = headerParts.join(" "); + const colonIndex = code.indexOf(":"); + if (colonIndex < 0) continue; + + const headerMatch = combined.match(/^except(?:\s+([\s\S]+?))?\s*:/); + if (!headerMatch) return undefined; + + const inlineBody = code.slice(colonIndex + 1).trim(); + return { + endIndex: index, + typePart: headerMatch[1]?.trim() || undefined, + inlineBody: inlineBody || undefined, + headerText: combined, + }; + } + + return undefined; +} + +function collectPythonBlockBody(lines: string[], headerIndex: number, headerIndent: number): string[] { + const bodyLines: string[] = []; + for (let index = headerIndex + 1; index < lines.length; index += 1) { + const line = lines[index] ?? ""; + if (!line.trim()) { + bodyLines.push(line); + continue; + } + if (indentation(line) <= headerIndent) break; + bodyLines.push(line); + } + return bodyLines; +} + +function skipPythonBlock(lines: string[], startIndex: number, blockIndent: number): number { + let index = startIndex + 1; + while (index < lines.length) { + const line = lines[index] ?? ""; + if (!line.trim()) { + index += 1; + continue; + } + if (indentation(line) <= blockIndent) break; + index += 1; + } + return index; +} + +function classifyPythonExcept(typePart: string | undefined, bodyLines: string[]): PythonExceptReason | undefined { + const significant = getSignificantPythonLines(bodyLines); + if (significant.some((line) => /^raise\b/.test(line))) return undefined; + + if (isPassOrCommentOnlyExcept(significant, bodyLines)) return "pass-or-comment"; + if (isBareOrBroadException(typePart) && isLogOnlyPythonBody(significant)) return "broad-log-only"; + return undefined; +} + +function isPassOrCommentOnlyExcept(significant: string[], bodyLines: string[]): boolean { + if (significant.length === 0) return true; + return significant.length === 1 && significant[0] === "pass"; +} + +function isBareOrBroadException(typePart: string | undefined): boolean { + if (!typePart) return true; + + const withoutAlias = typePart.split(/\s+as\s+/i)[0]?.trim() ?? ""; + if (withoutAlias.startsWith("(")) { + const inner = withoutAlias.slice(1, -1); + const types = inner.split(",").map((entry) => entry.trim()).filter(Boolean); + return types.length === 0 || types.every((entry) => entry === "Exception"); + } + + return withoutAlias === "Exception"; +} + +function isLogOnlyPythonBody(significant: string[]): boolean { + if (significant.length === 0) return false; + return significant.every(isLogOnlyPythonStatement); +} + +function isLogOnlyPythonStatement(line: string): boolean { + if (line === "pass") return true; + if (/^print\s*\(/.test(line)) return true; + if (/^logging\.(?:debug|info|warning|error|exception|critical)\s*\(/.test(line)) return true; + if (/^logger\.(?:debug|info|warning|error|exception|critical)\s*\(/.test(line)) return true; + return /^[A-Za-z_][\w.]*\.(?:debug|info|warning|error|exception|critical)\s*\(/.test(line); +} + +function getSignificantPythonLines(bodyLines: string[]): string[] { + return bodyLines + .map((line) => stripPythonComment(line).trim()) + .filter((line) => line.length > 0); +} + +function createPythonErrorHandlingIssue( + file: string, + exceptBlock: PythonExceptBlock, + reason: PythonExceptReason, +): DebtIssue { + const message = reason === "pass-or-comment" + ? "Except block only uses pass or comments and silently ignores errors." + : "Bare except or broad Exception handler only logs the error without rethrowing or handling it."; + + const bodyPreview = exceptBlock.bodyLines.join("\n").trim().slice(0, 220) || exceptBlock.headerText.slice(0, 220); + + return createIssue({ + detector: pythonErrorHandlingDetector, + severity: "medium", + confidence: reason === "pass-or-comment" ? 0.88 : 0.74, + file, + location: { startLine: exceptBlock.exceptLine, endLine: exceptBlock.endLine }, + message, + evidence: [ + `Except header: ${exceptBlock.headerText.slice(0, 180)}`, + `Except body: ${bodyPreview}`, + ], + suggestion: "Handle the error explicitly, rethrow it, or document why ignoring it is safe.", + }); +} + +function parseFileSuppressions(content: string): FileSuppressionRules { + const fileRules = new Set(); + const nextLineRules = new Map>(); + const inlineLineRules = new Map>(); + const lines = content.split(/\r?\n/); + + for (let index = 0; index < lines.length; index += 1) { + const line = lines[index] ?? ""; + const lineNumber = index + 1; + + const fileMatch = line.match(disableFilePattern); + if (fileMatch?.[1] && fileMatch[2]?.trim()) { + fileRules.add(fileMatch[1].toLowerCase()); + inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), fileMatch[1])); + continue; + } + + const nextLineMatch = line.match(disableNextLinePattern); + if (!nextLineMatch?.[1] || !nextLineMatch[2]?.trim()) continue; + + const ruleId = nextLineMatch[1].toLowerCase(); + inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), ruleId)); + nextLineRules.set(lineNumber + 1, addRule(nextLineRules.get(lineNumber + 1), ruleId)); + } + + return { fileRules, nextLineRules, inlineLineRules }; +} + +function addRule(existing: Set | undefined, ruleId: string): Set { + const rules = existing ?? new Set(); + rules.add(ruleId.toLowerCase()); + return rules; +} + +function isSuppressed(rules: FileSuppressionRules, ruleId: string, line: number): boolean { + const normalizedRuleId = ruleId.toLowerCase(); + if (rules.fileRules.has(normalizedRuleId)) return true; + if (rules.nextLineRules.get(line)?.has(normalizedRuleId)) return true; + if (rules.inlineLineRules.get(line)?.has(normalizedRuleId)) return true; + return false; +} + +function stripPythonComment(line: string): string { + const index = line.indexOf("#"); + return index >= 0 ? line.slice(0, index) : line; +} + +function indentation(line: string): number { + return line.match(/^\s*/)?.[0].length ?? 0; +} diff --git a/src/detectors/python/index.ts b/src/detectors/python/index.ts index 389d5ad..f0b822f 100644 --- a/src/detectors/python/index.ts +++ b/src/detectors/python/index.ts @@ -1,5 +1,6 @@ export { pythonComplexControlFlowDetector } from "./complexControlFlow.js"; export { pythonDeadAbstractionDetector } from "./deadAbstraction.js"; +export { pythonErrorHandlingDetector } from "./errorHandling.js"; export { pythonDuplicateLogicDetector } from "./duplicateLogic.js"; export { pythonLargeFunctionDetector } from "./largeFunction.js"; export { pythonRouteSprawlDetector } from "./routeSprawl.js"; diff --git a/tests/cli/plugins.test.ts b/tests/cli/plugins.test.ts index 4e75a7f..4d51c41 100644 --- a/tests/cli/plugins.test.ts +++ b/tests/cli/plugins.test.ts @@ -127,7 +127,7 @@ describe("debtlens scan with plugins", () => { const parsed = JSON.parse(result.stdout); assert.equal(result.status, 0); - assert.equal(parsed.summary.rulesRun, 14); + assert.equal(parsed.summary.rulesRun, 18); assert.ok(parsed.issues.some((issue: { ruleId: string }) => issue.ruleId === "no-console")); }); }); @@ -146,7 +146,7 @@ describe("debtlens scan with plugins", () => { const parsed = JSON.parse(result.stdout); assert.equal(result.status, 0); - assert.equal(parsed.summary.rulesRun, 14); + assert.equal(parsed.summary.rulesRun, 18); assert.ok(parsed.issues.some((issue: { ruleId: string; file: string }) => issue.ruleId === "python-marker" && issue.file === "src/service.py")); }); diff --git a/tests/config/packs.test.ts b/tests/config/packs.test.ts index 1a5623a..d54982c 100644 --- a/tests/config/packs.test.ts +++ b/tests/config/packs.test.ts @@ -7,17 +7,17 @@ describe("rule packs", () => { it("lists built-in packs with expected rule counts", () => { const packs = listRulePacks(); assert.equal(packs.length, 19); - assert.equal(getRulePack("core").rules.length, 13); + assert.equal(getRulePack("core").rules.length, 17); assert.deepEqual(getRulePack("core").languages, ["tsjs"]); - assert.equal(getRulePack("react").rules.length, 20); - assert.equal(getRulePack("react-native").rules.length, 21); + assert.equal(getRulePack("react").rules.length, 24); + assert.equal(getRulePack("react-native").rules.length, 25); assert.ok(getRulePack("react-native").rules.includes("rn-host-forwarding")); - assert.equal(getRulePack("next").rules.length, 23); + assert.equal(getRulePack("next").rules.length, 27); assert.ok(getRulePack("next").rules.includes("server-client-boundary")); assert.ok(getRulePack("next").rules.includes("route-handler-size")); assert.ok(getRulePack("next").rules.includes("data-loader-sprawl")); assert.deepEqual(getRulePack("next").duplicatedLiteral?.ignoreStrings, ["use client", "use server"]); - assert.equal(getRulePack("expo").rules.length, 21); + assert.equal(getRulePack("expo").rules.length, 25); assert.ok(getRulePack("node").rules.includes("handler-depth")); assert.ok(getRulePack("node").rules.includes("route-sprawl")); assert.deepEqual(getRulePack("python").rules, [ @@ -26,6 +26,7 @@ describe("rule packs", () => { "python-complex-control-flow", "python-dead-abstraction", "python-todo-comment", + "python-error-handling", ]); assert.deepEqual(getRulePack("python").languages, ["python"]); assert.deepEqual(getRulePack("python-web").rules, [ @@ -34,6 +35,7 @@ describe("rule packs", () => { "python-complex-control-flow", "python-dead-abstraction", "python-todo-comment", + "python-error-handling", "python-route-sprawl", ]); assert.deepEqual(getRulePack("python-web").languages, ["python"]); @@ -57,6 +59,7 @@ describe("rule packs", () => { "kotlin-large-function", "kotlin-dead-abstraction", "kotlin-todo-comment", + "kotlin-empty-catch", ]); assert.deepEqual(getRulePack("kotlin").languages, ["kotlin"]); assert.deepEqual(getRulePack("compose").rules, [ @@ -208,6 +211,7 @@ describe("rule packs", () => { "python-complex-control-flow", "python-dead-abstraction", "python-todo-comment", + "python-error-handling", "python-route-sprawl", "vue-todo-comment", "vue-large-script", diff --git a/tests/detectors/commentedOutCode.test.ts b/tests/detectors/commentedOutCode.test.ts new file mode 100644 index 0000000..79f30c2 --- /dev/null +++ b/tests/detectors/commentedOutCode.test.ts @@ -0,0 +1,82 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { commentedOutCodeDetector } from "../../src/detectors/commentedOutCode.js"; +import { runDetector } from "../helpers/runDetector.js"; + +describe("commented-out-code detector", () => { + it("flags a multi-line commented-out block", async () => { + const src = ` +/* +const old = fetchData(); +return old; +*/ +export const x = 1; +`; + const issues = await runDetector(commentedOutCodeDetector, { "x.ts": src }); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "commented-out-code"); + assert.equal(issues[0]?.location?.startLine, 2); + assert.ok((issues[0]?.confidence ?? 0) >= 0.6); + }); + + it("flags contiguous line comments that look like code", async () => { + const src = ` +// const a = 1; +// const b = a + 2; +export const x = 1; +`; + const issues = await runDetector(commentedOutCodeDetector, { "x.ts": src }); + assert.equal(issues.length, 1); + assert.match(issues[0]?.message ?? "", /commented-out code/); + }); + + it("does not flag a license header", async () => { + const src = ` +// SPDX-License-Identifier: MIT +// Copyright (c) 2024 Example Corp +export const x = 1; +`; + const issues = await runDetector(commentedOutCodeDetector, { "x.ts": src }); + assert.equal(issues.length, 0); + }); + + it("does not flag TODO marker comments", async () => { + const src = ` +// TODO: remove after launch +// const legacy = old(); +export const x = 1; +`; + const issues = await runDetector(commentedOutCodeDetector, { "x.ts": src }); + assert.equal(issues.length, 0); + }); + + it("does not flag a single commented-out line below minLines", async () => { + const src = ` +// const x = 1; +export const y = 2; +`; + const issues = await runDetector(commentedOutCodeDetector, { "x.ts": src }); + assert.equal(issues.length, 0); + }); + + it("raises confidence for longer commented-out runs", async () => { + const short = await runDetector(commentedOutCodeDetector, { + "short.ts": "// const a = 1;\n// return a;\nexport const x = 1;\n", + }); + const long = await runDetector(commentedOutCodeDetector, { + "long.ts": [ + "// const a = 1;", + "// const b = 2;", + "// const c = 3;", + "// const d = 4;", + "// return a + b + c + d;", + "export const x = 1;", + ].join("\n"), + }); + + assert.equal(short.length, 1); + assert.equal(long.length, 1); + assert.ok((long[0]?.confidence ?? 0) > (short[0]?.confidence ?? 1)); + assert.ok((long[0]?.confidence ?? 0) <= 0.8); + }); +}); diff --git a/tests/detectors/errorHandling.test.ts b/tests/detectors/errorHandling.test.ts new file mode 100644 index 0000000..0ee5d61 --- /dev/null +++ b/tests/detectors/errorHandling.test.ts @@ -0,0 +1,114 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { emptyCatchDetector, swallowedErrorDetector } from "../../src/detectors/errorHandling.js"; +import { runDetector } from "../helpers/runDetector.js"; + +describe("empty-catch detector", () => { + it("flags empty catch blocks", async () => { + const issues = await runDetector(emptyCatchDetector, { + "src/worker.ts": ` + export function run() { + try { + return risky(); + } catch (error) { + } + } + `, + }); + + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "empty-catch"); + assert.match(issues[0]?.message ?? "", /empty/i); + }); + + it("flags comment-only catch blocks unless allowCommentOnly is enabled", async () => { + const source = ` + export function run() { + try { + return risky(); + } catch (error) { + // ignored on purpose + } + } + `; + + const flagged = await runDetector(emptyCatchDetector, { "src/worker.ts": source }); + assert.equal(flagged.length, 1); + assert.match(flagged[0]?.message ?? "", /comment/i); + + const allowed = await runDetector(emptyCatchDetector, { "src/worker.ts": source }, { + thresholds: { "empty-catch.allowCommentOnly": 1 }, + }); + assert.equal(allowed.length, 0); + }); + + it("honors disable directives on the catch line", async () => { + const issues = await runDetector(emptyCatchDetector, { + "src/worker.ts": ` + export function run() { + try { + return risky(); + // debtlens-disable-next-line empty-catch -- vendor SDK throws benign noise + } catch (error) { + } + } + `, + }); + + assert.equal(issues.length, 0); + }); +}); + +describe("swallowed-error detector", () => { + it("flags log-only catch blocks", async () => { + const issues = await runDetector(swallowedErrorDetector, { + "src/worker.ts": ` + export function run() { + try { + return risky(); + } catch (error) { + console.error(error); + } + } + `, + }); + + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "swallowed-error"); + assert.match(issues[0]?.message ?? "", /logs the error/i); + }); + + it("does not flag catch blocks that rethrow", async () => { + const issues = await runDetector(swallowedErrorDetector, { + "src/worker.ts": ` + export function run() { + try { + return risky(); + } catch (error) { + console.error(error); + throw error; + } + } + `, + }); + + assert.equal(issues.length, 0); + }); + + it("honors disable directives on the catch line", async () => { + const issues = await runDetector(swallowedErrorDetector, { + "src/worker.ts": ` + export function run() { + try { + return risky(); + // debtlens-disable-next-line swallowed-error -- logged for support triage only + } catch (error) { + console.error(error); + } + } + `, + }); + + assert.equal(issues.length, 0); + }); +}); diff --git a/tests/detectors/floatingPromise.test.ts b/tests/detectors/floatingPromise.test.ts new file mode 100644 index 0000000..b8725bc --- /dev/null +++ b/tests/detectors/floatingPromise.test.ts @@ -0,0 +1,143 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { floatingPromiseDetector } from "../../src/detectors/floatingPromise.js"; +import { runDetector } from "../helpers/runDetector.js"; + +describe("floating-promise detector", () => { + it("does NOT flag an awaited promise", async () => { + const src = ` +export async function load() { + await fetch("/api"); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }); + assert.equal(issues.length, 0); + }); + + it("does NOT flag a returned promise", async () => { + const src = ` +export function load() { + return fetch("/api"); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }); + assert.equal(issues.length, 0); + }); + + it("does NOT flag a void-marked fire-and-forget call by default", async () => { + const src = ` +export function load() { + void fetch("/api"); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }); + assert.equal(issues.length, 0); + }); + + it("flags void-marked calls when allowVoid is disabled", async () => { + const src = ` +export function load() { + void fetch("/api"); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }, { + thresholds: { "floating-promise.allowVoid": 0 }, + }); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "floating-promise"); + }); + + it("does NOT flag a promise chain with .catch()", async () => { + const src = ` +export function load() { + fetch("/api").catch(handleError); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }); + assert.equal(issues.length, 0); + }); + + it("flags a bare fetch call", async () => { + const src = ` +export function load() { + fetch("/api"); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "floating-promise"); + assert.match(issues[0]?.message ?? "", /not awaited/i); + assert.ok(issues[0]?.evidence?.some((entry) => entry.includes("fetch"))); + }); + + it("flags an async function call without await", async () => { + const src = ` +async function loadData() { + return 1; +} + +export function run() { + loadData(); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "run.ts": src }); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "floating-promise"); + assert.ok(issues[0]?.evidence?.some((entry) => entry.includes("async function"))); + }); + + it("flags fire-and-forget work inside useEffect", async () => { + const src = ` +export function Widget() { + useEffect(() => { + fetch("/api"); + }, []); + return null; +} +`; + const issues = await runDetector(floatingPromiseDetector, { "Widget.tsx": src }); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "floating-promise"); + assert.match(issues[0]?.message ?? "", /useEffect/); + assert.ok(issues[0]?.evidence?.some((entry) => entry.includes("Inside React effect"))); + }); + + it("does NOT flag a promise passed as an argument", async () => { + const src = ` +export function run(task: Promise) { + queue(task); +} + +export function load() { + run(fetch("/api")); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }); + assert.equal(issues.length, 0); + }); + + it("honors debtlens-disable-next-line", async () => { + const src = ` +export function load() { + // debtlens-disable-next-line floating-promise -- intentional fire-and-forget + fetch("/api"); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }); + assert.equal(issues.length, 0); + }); + + it("caps findings per file", async () => { + const src = ` +export function load() { + fetch("/1"); + fetch("/2"); + fetch("/3"); +} +`; + const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }, { + thresholds: { "floating-promise.maxPerFile": 2 }, + }); + assert.equal(issues.length, 2); + }); +}); diff --git a/tests/detectors/kotlin.test.ts b/tests/detectors/kotlin.test.ts index 8271964..793d249 100644 --- a/tests/detectors/kotlin.test.ts +++ b/tests/detectors/kotlin.test.ts @@ -7,6 +7,7 @@ import { scan } from "../../src/core/scan.js"; import { kotlinDeadAbstractionDetector, kotlinDuplicateLogicDetector, + kotlinEmptyCatchDetector, kotlinLargeFunctionDetector, kotlinTodoCommentDetector, } from "../../src/detectors/kotlin/index.js"; @@ -219,3 +220,63 @@ fun RenderInvoice(invoice: Invoice) = InvoiceCard(invoice) assert.ok(issues.some((issue) => issue.message.includes("renderReceipt"))); }); }); + +describe("kotlin-empty-catch detector", () => { + it("flags empty and comment-only catch blocks", async () => { + const issues = await runDetector(kotlinEmptyCatchDetector, { + "src/Service.kt": ` +fun load(path: String): String { + try { + return read(path) + } catch (error: Exception) { + } +} + +fun parse(value: String): Int { + try { + return value.toInt() + } catch (error: NumberFormatException) { + // ignored on purpose + } +} +`, + }); + + assert.equal(issues.length, 2); + assert.ok(issues.every((issue) => issue.ruleId === "kotlin-empty-catch")); + assert.ok(issues.some((issue) => issue.message.includes("empty"))); + assert.ok(issues.some((issue) => issue.message.includes("comment"))); + }); + + it("does not flag catch blocks that handle errors", async () => { + const issues = await runDetector(kotlinEmptyCatchDetector, { + "src/Service.kt": ` +fun parse(value: String): Int { + try { + return value.toInt() + } catch (error: NumberFormatException) { + throw error + } +} +`, + }); + + assert.equal(issues.length, 0); + }); + + it("honors disable directives on the catch line", async () => { + const issues = await runDetector(kotlinEmptyCatchDetector, { + "src/Service.kt": ` +fun load(path: String): String { + try { + return read(path) + // debtlens-disable-next-line kotlin-empty-catch -- vendor SDK throws benign noise + } catch (error: Exception) { + } +} +`, + }); + + assert.equal(issues.length, 0); + }); +}); diff --git a/tests/detectors/python.test.ts b/tests/detectors/python.test.ts index 88011f6..9b24bee 100644 --- a/tests/detectors/python.test.ts +++ b/tests/detectors/python.test.ts @@ -15,6 +15,7 @@ import { pythonRouteSprawlDetector, pythonTodoCommentDetector, } from "../../src/detectors/python/index.js"; +import { pythonErrorHandlingDetector } from "../../src/detectors/python/errorHandling.js"; import { extractPythonFunctions, extractPythonModule } from "../../src/detectors/python/parse.js"; import { renderReport } from "../../src/reporters/index.js"; import { runDetector } from "../helpers/runDetector.js"; @@ -611,3 +612,81 @@ def render_invoice(invoice): assert.equal(issues.length, 0); }); }); + +describe("python-error-handling detector", () => { + it("flags pass-only except blocks including bare except", async () => { + const issues = await runDetector(pythonErrorHandlingDetector, { + "src/service.py": ` +def load(path): + try: + return open(path).read() + except: + pass + +def parse(value): + try: + return int(value) + except ValueError: + pass +`, + }); + + assert.equal(issues.length, 2); + assert.ok(issues.every((issue) => issue.ruleId === "python-error-handling")); + assert.ok(issues.some((issue) => issue.message.includes("pass or comments"))); + }); + + it("flags bare except and broad Exception handlers that only log", async () => { + const issues = await runDetector(pythonErrorHandlingDetector, { + "src/service.py": ` +import logging + +logger = logging.getLogger(__name__) + +def load(path): + try: + return open(path).read() + except: + logger.exception("failed to read") + +def parse(value): + try: + return int(value) + except Exception: + print("parse failed") +`, + }); + + assert.equal(issues.length, 2); + assert.ok(issues.every((issue) => issue.message.includes("logs the error"))); + }); + + it("does not flag specific except blocks that rethrow", async () => { + const issues = await runDetector(pythonErrorHandlingDetector, { + "src/service.py": ` +def parse(value): + try: + return int(value) + except ValueError as error: + raise error +`, + }); + + assert.equal(issues.length, 0); + }); + + it("honors disable directives on the except line", async () => { + const issues = await runDetector(pythonErrorHandlingDetector, { + "src/service.py": ` +def load(path): + try: + return open(path).read() + # debtlens-disable-next-line python-error-handling -- vendor SDK throws benign noise + except: + pass +`, + }); + + assert.equal(issues.length, 0); + }); +}); From 7b894ff1443fae8fafd0f83647b5504cf5da3b62 Mon Sep 17 00:00:00 2001 From: ColumbusLabs <287001685+ColumbusLabs@users.noreply.github.com> Date: Sun, 21 Jun 2026 00:59:12 -0400 Subject: [PATCH 2/2] Fix code smell detector suppression handling --- README.md | 22 +++- docs/examples.md | 4 +- docs/language-pack-rfc.md | 2 + docs/rule-packs.md | 16 ++- src/detectors/errorHandling.ts | 57 ---------- src/detectors/floatingPromise.ts | 17 +-- src/detectors/kotlin/errorHandling.ts | 83 ++------------ src/detectors/python/errorHandling.ts | 142 +++++++++++++----------- tests/config/packs.test.ts | 9 ++ tests/core/scan.test.ts | 118 ++++++++++++++++++++ tests/detectors/errorHandling.test.ts | 10 +- tests/detectors/floatingPromise.test.ts | 5 +- tests/detectors/kotlin.test.ts | 47 +++++++- tests/detectors/python.test.ts | 49 +++++++- 14 files changed, 346 insertions(+), 235 deletions(-) diff --git a/README.md b/README.md index 13547aa..56ea251 100644 --- a/README.md +++ b/README.md @@ -86,16 +86,28 @@ language packs. Full taxonomy: [`docs/rule-packs.md`](./docs/rule-packs.md). | --- | --- | --- | --- | | `duplicate-logic` | core | Near-duplicate functions/components using normalized AST/text similarity | Medium | | `test-duplication` | core | Structurally identical test cases across test files | Medium | +| `large-function` | core | Non-component functions over line or branch budgets | Medium | | `import-cycle` | core | Circular relative import graphs | Medium | | `complex-control-flow` | core | Branch-heavy or deeply nested functions | Medium | | `config-drift` | core | Conflicting repeated values across JSON config files | Medium | | `dead-abstraction` | core | Thin wrappers that add little behavior | Low | +| `duplicated-literal` | core | Repeated string/number literals across files | Low | | `todo-comment` | core | TODO/FIXME/HACK/temporary implementation comments | Low | | `naming-drift` | core | Files with multiple competing names for the same domain concept | Info | +| `barrel-file` | core | Re-export-only barrels that obscure import graphs | Low | +| `weak-test-boundary` | core | Production imports from test-only modules | Medium | +| `api-surface-sprawl` | core | Files exporting too many public symbols | Medium | +| `empty-catch` | core | Empty or comment-only catch blocks that silently ignore errors | Medium | +| `swallowed-error` | core | Catch blocks that only log without rethrowing or returning | Medium | +| `floating-promise` | core | Unawaited promise-returning calls and effect fire-and-forget | Medium | +| `commented-out-code` | core | Contiguous comment lines that look like dead code | Low | | `large-component` | react | React-style components with too many lines, hooks, or branch points | Medium | | `state-sprawl` | react | Components/hooks with many local stateful hooks | Medium | | `effect-complexity` | react | Long or overloaded React effect hooks | Medium | +| `hook-dependency-smell` | react | Inline object/array/function literals in hook dependency arrays | Low | +| `context-provider-sprawl` | react | Components wrapping many unrelated Context providers | Medium | | `prop-drilling` | react | Components that forward many props to children | Medium | +| `story-only-component` | react | Exported components whose known consumers are only Storybook stories | Low | | `rn-host-forwarding` | react-native | RN wrappers forwarding many props into host primitives | Medium | | `server-client-boundary` | next | App Router server/client boundary mistakes | High | | `route-handler-size` | next | Oversized Next route/page modules | Medium | @@ -107,6 +119,7 @@ language packs. Full taxonomy: [`docs/rule-packs.md`](./docs/rule-packs.md). | `python-complex-control-flow` | python | Branch-heavy or deeply nested Python functions | Medium | | `python-dead-abstraction` | python | Thin Python pass-through functions | Low | | `python-todo-comment` | python | TODO/FIXME/HACK comments in Python files | Low | +| `python-error-handling` | python | Empty/bare except blocks and log-only Python error handlers | Medium | | `python-route-sprawl` | python-web | Flask/Blueprint or Django URL modules registering too many routes | Medium | | `vue-todo-comment` | vue | TODO/FIXME/HACK comments inside Vue script blocks | Low | | `vue-large-script` | vue | Oversized Vue SFC scripts or script functions | Medium | @@ -118,6 +131,7 @@ language packs. Full taxonomy: [`docs/rule-packs.md`](./docs/rule-packs.md). | `kotlin-large-function` | kotlin | Oversized or branch-heavy Kotlin functions | Medium | | `kotlin-dead-abstraction` | kotlin | Thin Kotlin pass-through functions | Low | | `kotlin-todo-comment` | kotlin | TODO/FIXME/HACK comments in Kotlin files | Low | +| `kotlin-empty-catch` | kotlin | Empty or comment-only Kotlin catch blocks | Medium | | `compose-large-composable` | compose | Oversized or branch-heavy Jetpack Compose functions | Medium | | `compose-state-hoisting` | compose | Composables owning too many local state holders | Medium | @@ -459,23 +473,23 @@ also declare their discovery metadata, so selecting `python`, `vue`, `svelte`, ` | Pack | Rules | | --- | --- | -| `core` | framework-neutral maintainability rules: duplication, large functions, barrels, test boundaries, API surface, TODOs, naming drift | +| `core` | framework-neutral maintainability rules: duplication, large functions, barrels, test boundaries, API surface, TODOs, naming drift, silent failures, floating promises, and dead commented code | | `react` | core + component, hook, provider, prop, and Storybook signals | | `react-native` | react + RN host primitive forwarding | | `next` | react + App Router boundary, route size, and data-loader checks | | `node` | core + Express/Fastify handler depth and route sprawl | -| `python` | Python duplicate functions, large and branch-heavy functions, thin wrappers, and TODO comments | +| `python` | Python duplicate functions, large and branch-heavy functions, thin wrappers, TODO comments, and error-handling smells | | `python-web` | python + Flask/Blueprint and Django URL route sprawl | | `vue` | Vue SFC script TODO, large-script, and duplicate-logic signals | | `svelte` | Svelte SFC script TODO, large-script, and duplicate-logic signals | -| `kotlin` | Kotlin duplicate functions, large functions, thin wrappers, and TODO comments | +| `kotlin` | Kotlin duplicate functions, large functions, thin wrappers, TODO comments, and empty catch blocks | | `swift` | Swift duplicate functions, large functions, thin wrappers, and TODO comments | | `ruby` | Ruby duplicate methods, large methods, thin wrappers, and TODO comments | | `rails` | Ruby core rules plus Rails route and controller sprawl checks | | `compose` | Jetpack Compose oversized composables and local state-hoisting smells | | `swiftui` | SwiftUI oversized views and local state-sprawl checks | | `expo` | React Native tuning for Expo Router projects | -| `ai-assisted-maintainer` | high-signal maintainability checks for assistant-heavy codebases; no authorship claims | +| `ai-assisted-maintainer` | high-signal maintainability checks for assistant-heavy codebases, including silent failures and commented-out code but excluding floating-promise; no authorship claims | | `oss-maintainer` | library API surface, barrels, duplication, tests, and TODO debt | | `ai-workflow-drift` | duplicated or contradictory AI assistant instruction files; no AI-authorship claims | diff --git a/docs/examples.md b/docs/examples.md index a4a10ba..f5fa965 100644 --- a/docs/examples.md +++ b/docs/examples.md @@ -9,11 +9,11 @@ these commands when evaluating output, writing docs, or checking a reporter chan | Next.js App Router | `debtlens scan examples/next --pack next --min-severity info` | Server/client boundary and route/data-loader rules. | | React Native screen | `debtlens scan examples/react-native --pack react-native --min-severity info` | RN host-forwarding and React-family rules. | | Node API | `debtlens scan examples/node-api --pack node --min-severity info` | Route and handler-depth signals for server code. | -| Python service | `debtlens scan examples/python --pack python --min-severity info` | Python duplicate, large-function, control-flow, thin-wrapper, and TODO rules. | +| Python service | `debtlens scan examples/python --pack python --min-severity info` | Python duplicate, large-function, control-flow, thin-wrapper, TODO, and error-handling rules. | | Python web routes | `debtlens scan examples/python-web --pack python-web --min-severity info` | Flask/Blueprint route-sprawl signals plus core Python rules. | | Vue SFC scripts | `debtlens scan examples/vue --pack vue --min-severity info` | Vue script TODO and duplicate-logic signals with `.vue` line mapping. | | Svelte SFC scripts | `debtlens scan examples/svelte --pack svelte --min-severity info` | Svelte script TODO and duplicate-logic signals without React rules. | -| Kotlin service | `debtlens scan examples/kotlin --pack kotlin --min-severity info` | Kotlin duplicate, large-function, thin-wrapper, and TODO rules. | +| Kotlin service | `debtlens scan examples/kotlin --pack kotlin --min-severity info` | Kotlin duplicate, large-function, thin-wrapper, TODO, and empty-catch rules. | | Swift service | `debtlens scan examples/swift --pack swift --min-severity info` | Swift duplicate, large-function, thin-wrapper, and TODO rules. | | SwiftUI screen | `debtlens scan examples/swiftui --pack swiftui --min-severity info` | SwiftUI large-view and state-sprawl rules. | | Ruby service | `debtlens scan examples/ruby --pack ruby --min-severity info` | Ruby duplicate, large-method, thin-wrapper, and TODO rules. | diff --git a/docs/language-pack-rfc.md b/docs/language-pack-rfc.md index f6684b8..77c5560 100644 --- a/docs/language-pack-rfc.md +++ b/docs/language-pack-rfc.md @@ -116,6 +116,8 @@ Current implementation: nesting depth for review-heavy functions. - `python-dead-abstraction` flags single-statement pass-through functions such as `def f(x): return g(x)`. +- `python-error-handling` flags bare/empty except blocks and broad log-only handlers + while ignoring examples inside comments and strings. - `--pack python` widens discovery to `.py` files. Use `--pack core,python` for one merged TS/JS + Python scan. Possible future sidecar command: diff --git a/docs/rule-packs.md b/docs/rule-packs.md index 56aafc3..e49efed 100644 --- a/docs/rule-packs.md +++ b/docs/rule-packs.md @@ -107,6 +107,10 @@ These apply to any TypeScript or JavaScript codebase: - **`barrel-file`** — wide re-export-only files that hide local import graph shape. - **`weak-test-boundary`** — production code importing from test-only fixtures or mocks. - **`api-surface-sprawl`** — files with too many public exports. +- **`empty-catch`** — empty or comment-only catch blocks that silently ignore errors. +- **`swallowed-error`** — catch blocks that only log without rethrowing or returning a handled result. +- **`floating-promise`** — promise-returning calls that are not awaited, returned, void-marked, or error-handled. +- **`commented-out-code`** — contiguous comment blocks that look like dead code. Future core rules may expand these signals into language-specific packs or richer project graph analysis. @@ -149,6 +153,7 @@ to `.py` files and emits the same `ScanResult` shape as TS/JS rules: - **`python-complex-control-flow`** - **`python-dead-abstraction`** - **`python-todo-comment`** +- **`python-error-handling`** Use `--pack core,python` when one scan should cover both TS/JS and Python paths. Python function-based rules use a stdlib-`ast` sidecar when `python3` or `python` is @@ -204,6 +209,7 @@ and `.kts` files and emits the same `ScanResult` shape as TS/JS rules: - **`kotlin-large-function`** - **`kotlin-dead-abstraction`** - **`kotlin-todo-comment`** +- **`kotlin-empty-catch`** Use `--pack core,python,kotlin` when one scan should cover mixed TS/JS, Python, and Kotlin paths. Jetpack Compose-specific UI debt lives in the separate `compose` pack. @@ -222,7 +228,7 @@ and Compose UI debt. ### Maintainer packs -- **`ai-assisted-maintainer`** combines high-signal duplication, literal, function-size, wrapper, TODO, naming, and test-boundary signals. It is about maintainability review only; it does not claim to detect AI-generated authorship. +- **`ai-assisted-maintainer`** combines high-signal duplication, literal, function-size, wrapper, TODO, naming, test-boundary, empty/swallowed error, and commented-out-code signals. It intentionally leaves out `floating-promise` because promise-intent is more project-context-sensitive. It is about maintainability review only; it does not claim to detect AI-generated authorship. - **`oss-maintainer`** focuses on library health: public API size, barrels, duplicate exports/logic, test-boundary leaks, and deferred TODO debt. ## Pack status matrix @@ -233,11 +239,11 @@ and Compose UI debt. | `react-native` | RN host components, platform UI patterns | **Shipped** (React pack plus RN host forwarding) | | `next` | App Router boundaries, server/client splits, data loading | **Shipped** (React pack plus Next-specific rules) | | `node` | Express/Fastify handlers, middleware depth, route sprawl | **Shipped** | -| `python` | Python duplicate functions, large and branch-heavy functions, thin wrappers, and TODO debt | **Shipped** | +| `python` | Python duplicate functions, large and branch-heavy functions, thin wrappers, TODO debt, and error-handling smells | **Shipped** | | `python-web` | Flask/Blueprint and Django URL route ownership | **Shipped** | | `vue` | Vue SFC script TODO, large-script, and duplicate-logic signals | **Shipped** | | `svelte` | Svelte component script TODO, large-script, and duplicate-logic signals | **Shipped** | -| `kotlin` | Kotlin duplicate functions, large functions, thin wrappers, and TODO debt | **Shipped** | +| `kotlin` | Kotlin duplicate functions, large functions, thin wrappers, TODO debt, and empty catch blocks | **Shipped** | | `swift` | Swift duplicate functions, large functions, thin wrappers, and TODO debt | **Shipped** | | `swiftui` | SwiftUI oversized views and local state sprawl | **Shipped** | | `ruby` | Ruby duplicate methods, large methods, thin wrappers, and TODO debt | **Shipped** | @@ -259,10 +265,10 @@ follow the same shared result contract. | Language | Core rules (examples) | Optional UI / framework packs | Status | | --- | --- | --- | --- | -| **Python** | duplicate logic, large functions, complex control flow, dead abstractions, TODO debt | Python web (`python-route-sprawl`) | **Shipped** for core Python and Python web route rules | +| **Python** | duplicate logic, large functions, complex control flow, dead abstractions, TODO debt, error-handling smells | Python web (`python-route-sprawl`) | **Shipped** for core Python and Python web route rules | | **Vue SFC** | script TODOs, large scripts/functions, duplicate script functions | Vue template-specific rules | **Shipped** for script-block MVP | | **Svelte SFC** | script TODOs, large scripts/functions, duplicate script functions | SvelteKit routing and markup-specific rules | **Shipped** for script-block MVP | -| **Kotlin** | duplicate logic, large functions, dead abstractions, TODO debt | Jetpack Compose (`compose-large-composable`, `compose-state-hoisting`) | **Shipped** for core Kotlin and Compose UI rules | +| **Kotlin** | duplicate logic, large functions, dead abstractions, TODO debt, empty catch blocks | Jetpack Compose (`compose-large-composable`, `compose-state-hoisting`) | **Shipped** for core Kotlin and Compose UI rules | | **Swift** | duplicate logic, large types/functions, dead abstractions, TODO debt | SwiftUI (oversized views, state sprawl), UIKit (large view controllers) | **Shipped** for core Swift and SwiftUI rules | | **Ruby** | duplicate logic, large methods, dead abstractions, TODO debt | Rails (`rails-route-sprawl`, `rails-controller-sprawl`) | **Shipped** for core Ruby and Rails framework rules | diff --git a/src/detectors/errorHandling.ts b/src/detectors/errorHandling.ts index 2e0d518..fec9091 100644 --- a/src/detectors/errorHandling.ts +++ b/src/detectors/errorHandling.ts @@ -5,8 +5,6 @@ import { createIssue } from "../utils/createIssue.js"; import { nodeLineSpan } from "../utils/lines.js"; const MAX_FINDINGS_PER_FILE = 12; -const disableNextLinePattern = /debtlens-disable-next-line\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; -const disableFilePattern = /debtlens-disable-file\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; export const emptyCatchDetector: Detector = { id: "empty-catch", @@ -20,15 +18,11 @@ export const emptyCatchDetector: Detector = { for (const file of context.files) { let countForFile = 0; - const suppressedRules = parseFileSuppressions(file.content); for (const tryStatement of file.sourceFile.getDescendantsOfKind(SyntaxKind.TryStatement)) { const catchClause = tryStatement.getCatchClause(); if (!catchClause) continue; - const span = nodeLineSpan(catchClause); - if (isSuppressed(suppressedRules, "empty-catch", span.startLine)) continue; - const classification = classifyCatchBody(catchClause); if (classification === "handled") continue; if (classification === "comment-only" && allowCommentOnly) continue; @@ -54,14 +48,11 @@ export const swallowedErrorDetector: Detector = { for (const file of context.files) { let countForFile = 0; - const suppressedRules = parseFileSuppressions(file.content); for (const tryStatement of file.sourceFile.getDescendantsOfKind(SyntaxKind.TryStatement)) { const catchClause = tryStatement.getCatchClause(); if (!catchClause) continue; - const span = nodeLineSpan(catchClause); - if (isSuppressed(suppressedRules, "swallowed-error", span.startLine)) continue; if (!isSwallowedErrorCatch(catchClause)) continue; issues.push(createSwallowedErrorIssue(file.relativePath, catchClause)); @@ -165,54 +156,6 @@ function isWithinLoggingCallArgument(identifier: MorphNode, logCalls: MorphNode[ return false; } -interface FileSuppressionRules { - fileRules: Set; - nextLineRules: Map>; - inlineLineRules: Map>; -} - -function parseFileSuppressions(content: string): FileSuppressionRules { - const fileRules = new Set(); - const nextLineRules = new Map>(); - const inlineLineRules = new Map>(); - const lines = content.split(/\r?\n/); - - for (let index = 0; index < lines.length; index += 1) { - const line = lines[index] ?? ""; - const lineNumber = index + 1; - - const fileMatch = line.match(disableFilePattern); - if (fileMatch?.[1] && fileMatch[2]?.trim()) { - fileRules.add(fileMatch[1].toLowerCase()); - inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), fileMatch[1])); - continue; - } - - const nextLineMatch = line.match(disableNextLinePattern); - if (!nextLineMatch?.[1] || !nextLineMatch[2]?.trim()) continue; - - const ruleId = nextLineMatch[1].toLowerCase(); - inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), ruleId)); - nextLineRules.set(lineNumber + 1, addRule(nextLineRules.get(lineNumber + 1), ruleId)); - } - - return { fileRules, nextLineRules, inlineLineRules }; -} - -function addRule(existing: Set | undefined, ruleId: string): Set { - const rules = existing ?? new Set(); - rules.add(ruleId.toLowerCase()); - return rules; -} - -function isSuppressed(rules: FileSuppressionRules, ruleId: string, line: number): boolean { - const normalizedRuleId = ruleId.toLowerCase(); - if (rules.fileRules.has(normalizedRuleId)) return true; - if (rules.nextLineRules.get(line)?.has(normalizedRuleId)) return true; - if (rules.inlineLineRules.get(line)?.has(normalizedRuleId)) return true; - return false; -} - function createEmptyCatchIssue( file: string, catchClause: CatchClause, diff --git a/src/detectors/floatingPromise.ts b/src/detectors/floatingPromise.ts index 36dd810..17f4018 100644 --- a/src/detectors/floatingPromise.ts +++ b/src/detectors/floatingPromise.ts @@ -2,11 +2,9 @@ import { Node, SyntaxKind } from "ts-morph"; import type { CallExpression, Expression, ExpressionStatement, Node as MorphNode } from "ts-morph"; import type { DebtIssue, Detector, DetectorContext } from "../core/types.js"; import { createIssue } from "../utils/createIssue.js"; -import { lineAt, nodeLineSpan } from "../utils/lines.js"; +import { nodeLineSpan } from "../utils/lines.js"; const EFFECT_HOOKS = new Set(["useEffect", "useLayoutEffect", "useInsertionEffect"]); -const disableNextLinePattern = /debtlens-disable-next-line\s+floating-promise(?:\s+--\s+.+)?.*/i; -const disableFilePattern = /debtlens-disable-file\s+floating-promise(?:\s+--\s+.+)?.*/i; export const floatingPromiseDetector: Detector = { id: "floating-promise", @@ -20,12 +18,9 @@ export const floatingPromiseDetector: Detector = { const issues: DebtIssue[] = []; for (const file of context.files) { - if (isFileSuppressed(file.content)) continue; - let countForFile = 0; for (const statement of file.sourceFile.getDescendantsOfKind(SyntaxKind.ExpressionStatement)) { if (countForFile >= maxPerFile) break; - if (isLineSuppressed(file.content, statement)) continue; const call = extractFloatingCall(statement, allowVoid); if (!call) continue; @@ -273,16 +268,6 @@ function unwrapParentheses(expression: Expression): Expression { return current; } -function isFileSuppressed(content: string): boolean { - return content.split(/\r?\n/).some((line) => disableFilePattern.test(line)); -} - -function isLineSuppressed(content: string, node: MorphNode): boolean { - const lineNumber = nodeLineSpan(node).startLine; - if (lineNumber <= 1) return false; - return disableNextLinePattern.test(lineAt(content, lineNumber - 1)); -} - function truncate(text: string, maxLength = 120): string { const normalized = text.replace(/\s+/g, " ").trim(); return normalized.length <= maxLength ? normalized : `${normalized.slice(0, maxLength - 3)}...`; diff --git a/src/detectors/kotlin/errorHandling.ts b/src/detectors/kotlin/errorHandling.ts index 8c9c1e2..5076746 100644 --- a/src/detectors/kotlin/errorHandling.ts +++ b/src/detectors/kotlin/errorHandling.ts @@ -1,9 +1,8 @@ import type { DebtIssue, Detector, DetectorContext } from "../../core/types.js"; import { createIssue } from "../../utils/createIssue.js"; +import { maskKotlinTrivia } from "./parse.js"; const MAX_FINDINGS_PER_FILE = 12; -const disableNextLinePattern = /debtlens-disable-next-line\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; -const disableFilePattern = /debtlens-disable-file\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; export const kotlinEmptyCatchDetector: Detector = { id: "kotlin-empty-catch", @@ -17,12 +16,9 @@ export const kotlinEmptyCatchDetector: Detector = { for (const file of context.files) { let countForFile = 0; - const suppressedRules = parseFileSuppressions(file.content); const lines = file.content.split(/\r?\n/); for (const catchBlock of findAllKotlinCatchBlocks(lines)) { - if (isSuppressed(suppressedRules, kotlinEmptyCatchDetector.id, catchBlock.catchLine)) continue; - const classification = classifyKotlinCatchBody(catchBlock.bodyText); if (classification === "handled") continue; @@ -44,22 +40,15 @@ interface KotlinCatchBlock { bodyText: string; } -interface FileSuppressionRules { - fileRules: Set; - nextLineRules: Map>; - inlineLineRules: Map>; -} - function findAllKotlinCatchBlocks(lines: string[]): KotlinCatchBlock[] { const text = lines.join("\n"); - const masked = maskKotlinComments(text); + const masked = maskKotlinTrivia(text); const blocks: KotlinCatchBlock[] = []; const catchPattern = /\bcatch\s*\(/g; let match = catchPattern.exec(masked); while (match) { - const lineIndex = countLineBreaks(text.slice(0, match.index)); - const block = parseKotlinCatchBlock(lines, lineIndex); + const block = parseKotlinCatchBlock(text, masked, match.index); if (block) blocks.push(block); match = catchPattern.exec(masked); } @@ -67,12 +56,7 @@ function findAllKotlinCatchBlocks(lines: string[]): KotlinCatchBlock[] { return blocks; } -function parseKotlinCatchBlock(lines: string[], catchStartIndex: number): KotlinCatchBlock | undefined { - const text = lines.slice(catchStartIndex).join("\n"); - const code = maskKotlinComments(text); - const catchIndex = code.search(/\bcatch\s*\(/); - if (catchIndex < 0) return undefined; - +function parseKotlinCatchBlock(text: string, code: string, catchIndex: number): KotlinCatchBlock | undefined { const braceIndex = code.indexOf("{", catchIndex); if (braceIndex < 0) return undefined; @@ -81,8 +65,8 @@ function parseKotlinCatchBlock(lines: string[], catchStartIndex: number): Kotlin const blockText = text.slice(catchIndex, endOffset + 1); const bodyText = blockText.slice(blockText.indexOf("{") + 1, blockText.lastIndexOf("}")); - const catchLine = catchStartIndex + countLineBreaks(text.slice(0, catchIndex)) + 1; - const endLine = catchStartIndex + countLineBreaks(text.slice(0, endOffset + 1)) + 1; + const catchLine = countLineBreaks(text.slice(0, catchIndex)) + 1; + const endLine = countLineBreaks(text.slice(0, endOffset + 1)) + 1; return { catchLine, @@ -92,12 +76,9 @@ function parseKotlinCatchBlock(lines: string[], catchStartIndex: number): Kotlin } function classifyKotlinCatchBody(bodyText: string): CatchBodyClassification { - const withoutComments = bodyText - .replace(/\/\*[\s\S]*?\*\//g, "") - .replace(/\/\/[^\n\r]*/g, "") - .trim(); + const withoutTrivia = maskKotlinTrivia(bodyText).trim(); - if (!withoutComments) { + if (!withoutTrivia) { return bodyText.trim() ? "comment-only" : "empty"; } @@ -141,54 +122,6 @@ function findMatchingBraceEnd(code: string, openIndex: number): number { return -1; } -function maskKotlinComments(text: string): string { - return text - .replace(/\/\*[\s\S]*?\*\//g, (match) => " ".repeat(match.length)) - .replace(/\/\/[^\n\r]*/g, (match) => " ".repeat(match.length)); -} - function countLineBreaks(text: string): number { return text.match(/\n/g)?.length ?? 0; } - -function parseFileSuppressions(content: string): FileSuppressionRules { - const fileRules = new Set(); - const nextLineRules = new Map>(); - const inlineLineRules = new Map>(); - const lines = content.split(/\r?\n/); - - for (let index = 0; index < lines.length; index += 1) { - const line = lines[index] ?? ""; - const lineNumber = index + 1; - - const fileMatch = line.match(disableFilePattern); - if (fileMatch?.[1] && fileMatch[2]?.trim()) { - fileRules.add(fileMatch[1].toLowerCase()); - inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), fileMatch[1])); - continue; - } - - const nextLineMatch = line.match(disableNextLinePattern); - if (!nextLineMatch?.[1] || !nextLineMatch[2]?.trim()) continue; - - const ruleId = nextLineMatch[1].toLowerCase(); - inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), ruleId)); - nextLineRules.set(lineNumber + 1, addRule(nextLineRules.get(lineNumber + 1), ruleId)); - } - - return { fileRules, nextLineRules, inlineLineRules }; -} - -function addRule(existing: Set | undefined, ruleId: string): Set { - const rules = existing ?? new Set(); - rules.add(ruleId.toLowerCase()); - return rules; -} - -function isSuppressed(rules: FileSuppressionRules, ruleId: string, line: number): boolean { - const normalizedRuleId = ruleId.toLowerCase(); - if (rules.fileRules.has(normalizedRuleId)) return true; - if (rules.nextLineRules.get(line)?.has(normalizedRuleId)) return true; - if (rules.inlineLineRules.get(line)?.has(normalizedRuleId)) return true; - return false; -} diff --git a/src/detectors/python/errorHandling.ts b/src/detectors/python/errorHandling.ts index 8608999..9db96b5 100644 --- a/src/detectors/python/errorHandling.ts +++ b/src/detectors/python/errorHandling.ts @@ -2,8 +2,6 @@ import type { DebtIssue, Detector, DetectorContext } from "../../core/types.js"; import { createIssue } from "../../utils/createIssue.js"; const MAX_FINDINGS_PER_FILE = 12; -const disableNextLinePattern = /debtlens-disable-next-line\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; -const disableFilePattern = /debtlens-disable-file\s+([a-z0-9-]+)(?:\s+--\s+(.+))?/i; export const pythonErrorHandlingDetector: Detector = { id: "python-error-handling", @@ -17,18 +15,16 @@ export const pythonErrorHandlingDetector: Detector = { for (const file of context.files) { let countForFile = 0; - const suppressedRules = parseFileSuppressions(file.content); const lines = file.content.split(/\r?\n/); + const codeLines = maskPythonStringsAndComments(file.content).split(/\r?\n/); - for (let index = 0; index < lines.length; index += 1) { - const line = lines[index] ?? ""; - if (!/^\s*try\s*:/.test(stripPythonComment(line))) continue; + for (let index = 0; index < codeLines.length; index += 1) { + const line = codeLines[index] ?? ""; + if (!/^\s*try\s*:/.test(line)) continue; - const exceptBlocks = findPythonExceptBlocks(lines, index); + const exceptBlocks = findPythonExceptBlocks(lines, codeLines, index); for (const exceptBlock of exceptBlocks) { - if (isSuppressed(suppressedRules, pythonErrorHandlingDetector.id, exceptBlock.exceptLine)) continue; - - const reason = classifyPythonExcept(exceptBlock.typePart, exceptBlock.bodyLines); + const reason = classifyPythonExcept(exceptBlock.typePart, exceptBlock.bodyCodeLines); if (!reason) continue; issues.push(createPythonErrorHandlingIssue(file.relativePath, exceptBlock, reason)); @@ -51,22 +47,17 @@ interface PythonExceptBlock { endLine: number; typePart: string | undefined; bodyLines: string[]; + bodyCodeLines: string[]; headerText: string; } -interface FileSuppressionRules { - fileRules: Set; - nextLineRules: Map>; - inlineLineRules: Map>; -} - -function findPythonExceptBlocks(lines: string[], tryLineIndex: number): PythonExceptBlock[] { +function findPythonExceptBlocks(lines: string[], codeLines: string[], tryLineIndex: number): PythonExceptBlock[] { const tryIndent = indentation(lines[tryLineIndex] ?? ""); const blocks: PythonExceptBlock[] = []; let index = tryLineIndex + 1; while (index < lines.length) { - const line = lines[index] ?? ""; + const line = codeLines[index] ?? ""; if (!line.trim()) { index += 1; continue; @@ -76,8 +67,8 @@ function findPythonExceptBlocks(lines: string[], tryLineIndex: number): PythonEx } while (index < lines.length) { - const line = lines[index] ?? ""; - const trimmed = stripPythonComment(line).trim(); + const line = codeLines[index] ?? ""; + const trimmed = line.trim(); const lineIndent = indentation(line); if (lineIndent < tryIndent) break; @@ -88,17 +79,20 @@ function findPythonExceptBlocks(lines: string[], tryLineIndex: number): PythonEx if (/^finally\s*:/.test(trimmed)) break; if (/^else\s*:/.test(trimmed)) { - index = skipPythonBlock(lines, index, lineIndent); + index = skipPythonBlock(codeLines, index, lineIndent); continue; } if (!/^except\b/.test(trimmed)) break; - const header = parsePythonExceptHeader(lines, index, tryIndent); + const header = parsePythonExceptHeader(lines, codeLines, index, tryIndent); if (!header) break; const bodyLines = header.inlineBody !== undefined ? [header.inlineBody] : collectPythonBlockBody(lines, header.endIndex, indentation(lines[header.endIndex] ?? "")); + const bodyCodeLines = header.inlineBodyCode !== undefined + ? [header.inlineBodyCode] + : collectPythonBlockBody(codeLines, header.endIndex, indentation(codeLines[header.endIndex] ?? "")); blocks.push({ exceptLine: index + 1, @@ -107,6 +101,7 @@ function findPythonExceptBlocks(lines: string[], tryLineIndex: number): PythonEx : header.endIndex + 1, typePart: header.typePart, bodyLines, + bodyCodeLines, headerText: header.headerText, }); @@ -121,32 +116,43 @@ function findPythonExceptBlocks(lines: string[], tryLineIndex: number): PythonEx function parsePythonExceptHeader( lines: string[], + codeLines: string[], startIndex: number, blockIndent: number, -): { endIndex: number; typePart: string | undefined; inlineBody: string | undefined; headerText: string } | undefined { +): { + endIndex: number; + typePart: string | undefined; + inlineBody: string | undefined; + inlineBodyCode: string | undefined; + headerText: string; +} | undefined { const headerParts: string[] = []; + const headerCodeParts: string[] = []; for (let index = startIndex; index < lines.length; index += 1) { const raw = lines[index] ?? ""; + const code = codeLines[index] ?? ""; const lineIndent = indentation(raw); if (index > startIndex && lineIndent < blockIndent) return undefined; if (index > startIndex && lineIndent > blockIndent) return undefined; - const code = stripPythonComment(raw); - headerParts.push(code.trim()); - const combined = headerParts.join(" "); + headerParts.push(raw.trim()); + headerCodeParts.push(code.trim()); + const combined = headerCodeParts.join(" "); const colonIndex = code.indexOf(":"); if (colonIndex < 0) continue; const headerMatch = combined.match(/^except(?:\s+([\s\S]+?))?\s*:/); if (!headerMatch) return undefined; - const inlineBody = code.slice(colonIndex + 1).trim(); + const inlineBody = raw.slice(colonIndex + 1).trim(); + const inlineBodyCode = code.slice(colonIndex + 1).trim(); return { endIndex: index, typePart: headerMatch[1]?.trim() || undefined, inlineBody: inlineBody || undefined, - headerText: combined, + inlineBodyCode: inlineBodyCode || undefined, + headerText: headerParts.join(" "), }; } @@ -185,12 +191,12 @@ function classifyPythonExcept(typePart: string | undefined, bodyLines: string[]) const significant = getSignificantPythonLines(bodyLines); if (significant.some((line) => /^raise\b/.test(line))) return undefined; - if (isPassOrCommentOnlyExcept(significant, bodyLines)) return "pass-or-comment"; + if (isPassOrCommentOnlyExcept(significant)) return "pass-or-comment"; if (isBareOrBroadException(typePart) && isLogOnlyPythonBody(significant)) return "broad-log-only"; return undefined; } -function isPassOrCommentOnlyExcept(significant: string[], bodyLines: string[]): boolean { +function isPassOrCommentOnlyExcept(significant: string[]): boolean { if (significant.length === 0) return true; return significant.length === 1 && significant[0] === "pass"; } @@ -223,7 +229,7 @@ function isLogOnlyPythonStatement(line: string): boolean { function getSignificantPythonLines(bodyLines: string[]): string[] { return bodyLines - .map((line) => stripPythonComment(line).trim()) + .map((line) => line.trim()) .filter((line) => line.length > 0); } @@ -253,51 +259,55 @@ function createPythonErrorHandlingIssue( }); } -function parseFileSuppressions(content: string): FileSuppressionRules { - const fileRules = new Set(); - const nextLineRules = new Map>(); - const inlineLineRules = new Map>(); - const lines = content.split(/\r?\n/); - - for (let index = 0; index < lines.length; index += 1) { - const line = lines[index] ?? ""; - const lineNumber = index + 1; +function maskPythonStringsAndComments(content: string): string { + const chars = [...content]; + let index = 0; - const fileMatch = line.match(disableFilePattern); - if (fileMatch?.[1] && fileMatch[2]?.trim()) { - fileRules.add(fileMatch[1].toLowerCase()); - inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), fileMatch[1])); + while (index < content.length) { + const char = content[index] ?? ""; + if (char === "#") { + const start = index; + while (index < content.length && content[index] !== "\n") index += 1; + maskRange(chars, start, index); continue; } - const nextLineMatch = line.match(disableNextLinePattern); - if (!nextLineMatch?.[1] || !nextLineMatch[2]?.trim()) continue; + if (char === "\"" || char === "'") { + const start = index; + const quote = char; + const triple = content.slice(index, index + 3) === quote.repeat(3); + index += triple ? 3 : 1; - const ruleId = nextLineMatch[1].toLowerCase(); - inlineLineRules.set(lineNumber, addRule(inlineLineRules.get(lineNumber), ruleId)); - nextLineRules.set(lineNumber + 1, addRule(nextLineRules.get(lineNumber + 1), ruleId)); - } + while (index < content.length) { + if (triple && content.slice(index, index + 3) === quote.repeat(3)) { + index += 3; + break; + } + if (!triple && content[index] === "\\" && index + 1 < content.length) { + index += 2; + continue; + } + if (!triple && content[index] === quote) { + index += 1; + break; + } + index += 1; + } - return { fileRules, nextLineRules, inlineLineRules }; -} + maskRange(chars, start, index); + continue; + } -function addRule(existing: Set | undefined, ruleId: string): Set { - const rules = existing ?? new Set(); - rules.add(ruleId.toLowerCase()); - return rules; -} + index += 1; + } -function isSuppressed(rules: FileSuppressionRules, ruleId: string, line: number): boolean { - const normalizedRuleId = ruleId.toLowerCase(); - if (rules.fileRules.has(normalizedRuleId)) return true; - if (rules.nextLineRules.get(line)?.has(normalizedRuleId)) return true; - if (rules.inlineLineRules.get(line)?.has(normalizedRuleId)) return true; - return false; + return chars.join(""); } -function stripPythonComment(line: string): string { - const index = line.indexOf("#"); - return index >= 0 ? line.slice(0, index) : line; +function maskRange(chars: string[], start: number, end: number): void { + for (let index = start; index < end; index += 1) { + if (chars[index] !== "\n") chars[index] = " "; + } } function indentation(line: string): number { diff --git a/tests/config/packs.test.ts b/tests/config/packs.test.ts index d54982c..a51ea18 100644 --- a/tests/config/packs.test.ts +++ b/tests/config/packs.test.ts @@ -111,6 +111,15 @@ describe("rule packs", () => { assert.throws(() => getRulePack("ember"), /Unknown rule pack "ember"/); }); + it("keeps ai-assisted-maintainer code-smell coverage explicit", () => { + const rules = getRulePack("ai-assisted-maintainer").rules; + + assert.ok(rules.includes("empty-catch")); + assert.ok(rules.includes("swallowed-error")); + assert.ok(rules.includes("commented-out-code")); + assert.equal(rules.includes("floating-promise"), false); + }); + it("applies pack rules when no explicit rules are configured", () => { const options = mergeConfig(".", { pack: "core" }, { cwd: process.cwd() }); assert.equal(options.pack, "core"); diff --git a/tests/core/scan.test.ts b/tests/core/scan.test.ts index a2eb8ca..5b51431 100644 --- a/tests/core/scan.test.ts +++ b/tests/core/scan.test.ts @@ -98,6 +98,124 @@ describe("scan integration", () => { } }); + it("uses central suppression accounting for valid suppressions on new detectors", async () => { + const dir = mkdtempSync(join(tmpdir(), "debtlens-scan-new-rule-suppress-")); + try { + mkdirSync(join(dir, "src")); + writeFileSync( + join(dir, "src", "worker.ts"), + ` +export function load() { + // debtlens-disable-next-line floating-promise -- intentional fire-and-forget startup ping + fetch("/api/startup"); +} +`, + ); + + const result = await scan({ + cwd: dir, + target: dir, + include: ["**/*.{ts,tsx,js,jsx}"], + exclude: [], + minSeverity: "info", + rules: ["floating-promise"], + thresholds: defaultConfig.thresholds, + maxFiles: defaultConfig.maxFiles, + auditSuppressions: true, + }); + + assert.equal(result.summary.totalIssues, 0); + assert.equal(result.summary.filterStats?.suppressedByInline, 1); + assert.equal(result.suppressions?.[0]?.ruleId, "floating-promise"); + assert.equal(result.suppressionDirectives?.[0]?.status, "used"); + assert.equal(result.suppressionDirectives?.[0]?.suppressedIssueCount, 1); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("keeps new detector findings when suppression reasons are missing", async () => { + const dir = mkdtempSync(join(tmpdir(), "debtlens-scan-new-rule-missing-reason-")); + try { + mkdirSync(join(dir, "src")); + writeFileSync( + join(dir, "src", "worker.ts"), + ` +export function load() { + // debtlens-disable-next-line floating-promise + fetch("/api/startup"); +} +`, + ); + + const result = await scan({ + cwd: dir, + target: dir, + include: ["**/*.{ts,tsx,js,jsx}"], + exclude: [], + minSeverity: "info", + rules: ["floating-promise"], + thresholds: defaultConfig.thresholds, + maxFiles: defaultConfig.maxFiles, + auditSuppressions: true, + }); + + assert.equal(result.summary.totalIssues, 1); + assert.equal(result.issues[0]?.ruleId, "floating-promise"); + assert.equal(result.summary.filterStats?.suppressedByInline, undefined); + assert.ok(result.summary.warnings?.some((warning) => /reason is missing/.test(warning))); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("applies file-level suppressions to every matching new error-handling finding", async () => { + const dir = mkdtempSync(join(tmpdir(), "debtlens-scan-new-rule-file-suppress-")); + try { + mkdirSync(join(dir, "src")); + writeFileSync( + join(dir, "src", "worker.ts"), + ` +// debtlens-disable-file empty-catch -- legacy SDK throws harmless telemetry errors +export function one() { + try { + risky(); + } catch (error) { + } +} + +export function two() { + try { + risky(); + } catch (error) { + } +} +`, + ); + + const result = await scan({ + cwd: dir, + target: dir, + include: ["**/*.{ts,tsx,js,jsx}"], + exclude: [], + minSeverity: "info", + rules: ["empty-catch"], + thresholds: defaultConfig.thresholds, + maxFiles: defaultConfig.maxFiles, + auditSuppressions: true, + }); + + assert.equal(result.summary.totalIssues, 0); + assert.equal(result.summary.filterStats?.suppressedByInline, 2); + assert.equal(result.suppressions?.length, 2); + assert.equal(result.suppressionDirectives?.[0]?.kind, "file"); + assert.equal(result.suppressionDirectives?.[0]?.status, "used"); + assert.equal(result.suppressionDirectives?.[0]?.suppressedIssueCount, 2); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it("applies Next pack duplicated-literal ignores during a scan", async () => { const dir = mkdtempSync(join(tmpdir(), "debtlens-scan-next-literals-")); try { diff --git a/tests/detectors/errorHandling.test.ts b/tests/detectors/errorHandling.test.ts index 0ee5d61..f5a62f4 100644 --- a/tests/detectors/errorHandling.test.ts +++ b/tests/detectors/errorHandling.test.ts @@ -42,7 +42,7 @@ describe("empty-catch detector", () => { assert.equal(allowed.length, 0); }); - it("honors disable directives on the catch line", async () => { + it("emits raw findings when a central suppression directive is present", async () => { const issues = await runDetector(emptyCatchDetector, { "src/worker.ts": ` export function run() { @@ -55,7 +55,8 @@ describe("empty-catch detector", () => { `, }); - assert.equal(issues.length, 0); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "empty-catch"); }); }); @@ -95,7 +96,7 @@ describe("swallowed-error detector", () => { assert.equal(issues.length, 0); }); - it("honors disable directives on the catch line", async () => { + it("emits raw findings when a central suppression directive is present", async () => { const issues = await runDetector(swallowedErrorDetector, { "src/worker.ts": ` export function run() { @@ -109,6 +110,7 @@ describe("swallowed-error detector", () => { `, }); - assert.equal(issues.length, 0); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "swallowed-error"); }); }); diff --git a/tests/detectors/floatingPromise.test.ts b/tests/detectors/floatingPromise.test.ts index b8725bc..99744de 100644 --- a/tests/detectors/floatingPromise.test.ts +++ b/tests/detectors/floatingPromise.test.ts @@ -116,7 +116,7 @@ export function load() { assert.equal(issues.length, 0); }); - it("honors debtlens-disable-next-line", async () => { + it("emits raw findings when a central suppression directive is present", async () => { const src = ` export function load() { // debtlens-disable-next-line floating-promise -- intentional fire-and-forget @@ -124,7 +124,8 @@ export function load() { } `; const issues = await runDetector(floatingPromiseDetector, { "load.ts": src }); - assert.equal(issues.length, 0); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "floating-promise"); }); it("caps findings per file", async () => { diff --git a/tests/detectors/kotlin.test.ts b/tests/detectors/kotlin.test.ts index 793d249..1399fc1 100644 --- a/tests/detectors/kotlin.test.ts +++ b/tests/detectors/kotlin.test.ts @@ -264,7 +264,49 @@ fun parse(value: String): Int { assert.equal(issues.length, 0); }); - it("honors disable directives on the catch line", async () => { + it("does not flag catch examples inside normal or triple-quoted strings", async () => { + const issues = await runDetector(kotlinEmptyCatchDetector, { + "src/Service.kt": ` +fun docs(): String { + val inline = "try { read(path) } catch (error: Exception) { }" + val block = """ + try { + read(path) + } catch (error: Exception) { + } + """ + return inline + block +} +`, + }); + + assert.equal(issues.length, 0); + }); + + it("still flags real catch blocks after ignored examples", async () => { + const issues = await runDetector(kotlinEmptyCatchDetector, { + "src/Service.kt": ` +val docs = """ +try { + read(path) +} catch (error: Exception) { +} +""" + +fun load(path: String): String { + try { + return read(path) + } catch (error: Exception) { + } +} +`, + }); + + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "kotlin-empty-catch"); + }); + + it("emits raw findings when a central suppression directive is present", async () => { const issues = await runDetector(kotlinEmptyCatchDetector, { "src/Service.kt": ` fun load(path: String): String { @@ -277,6 +319,7 @@ fun load(path: String): String { `, }); - assert.equal(issues.length, 0); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "kotlin-empty-catch"); }); }); diff --git a/tests/detectors/python.test.ts b/tests/detectors/python.test.ts index 9b24bee..9104f3b 100644 --- a/tests/detectors/python.test.ts +++ b/tests/detectors/python.test.ts @@ -675,7 +675,51 @@ def parse(value): assert.equal(issues.length, 0); }); - it("honors disable directives on the except line", async () => { + it("does not flag try/except examples inside strings, docstrings, or comments", async () => { + const issues = await runDetector(pythonErrorHandlingDetector, { + "src/service.py": ` +def docs(): + example = "try:\\n risky()\\nexcept:\\n pass" + """ + try: + risky() + except Exception: + logger.exception("ignored") + """ + # try: + # risky() + # except: + # pass + return example +`, + }); + + assert.equal(issues.length, 0); + }); + + it("still flags real try/except blocks after ignored examples", async () => { + const issues = await runDetector(pythonErrorHandlingDetector, { + "src/service.py": ` +DOCS = """ +try: + risky() +except: + pass +""" + +def load(path): + try: + return open(path).read() + except: + pass +`, + }); + + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "python-error-handling"); + }); + + it("emits raw findings when a central suppression directive is present", async () => { const issues = await runDetector(pythonErrorHandlingDetector, { "src/service.py": ` def load(path): @@ -687,6 +731,7 @@ def load(path): `, }); - assert.equal(issues.length, 0); + assert.equal(issues.length, 1); + assert.equal(issues[0]?.ruleId, "python-error-handling"); }); });