diff --git a/scripts/deadcode_engine.py b/scripts/deadcode_engine.py index 4dcf2c2..31ffb5f 100755 --- a/scripts/deadcode_engine.py +++ b/scripts/deadcode_engine.py @@ -643,6 +643,23 @@ def _detect_unused_variables(content: str, ext: str, rel_path: str) -> List[Dict if re.match(r'^\d[\d_]*$', var_name): continue + # This detector only counts occurrences WITHIN THE SAME FILE + # (clean_content is this file's content). An `export const X = ...` + # is by definition meant to be used from OTHER files — this + # same-file heuristic has no way to see that usage and will always + # find exactly 1 occurrence (the declaration itself), false-flagging + # every exported value passed by reference elsewhere (e.g. Express + # middleware: `export const fooLimiter = rateLimit(...)` used as + # `app.post(path, fooLimiter)` in a different file — fooLimiter is + # never "called" or re-mentioned in its own file, so it looked + # unused here even though 3+ other files import and use it). + # Cross-file usage is `unused_exports`' job (it walks the import + # graph); this same-file scan must defer to it, not duplicate a + # weaker version of the same check. + _export_prefix = clean_content[max(0, start_pos - 20):start_pos] + if re.search(r'\bexport\s*$', _export_prefix): + continue + # Skip common patterns that are used indirectly skip_names = {'_', 'e', 'err', 'error', 'res', 'req', 'ctx', 'props', 'state', 'ref', 'config', 'module'} if var_name in skip_names or var_name.startswith('_'): diff --git a/scripts/smell_engine.py b/scripts/smell_engine.py index b441d41..13dfb9d 100755 --- a/scripts/smell_engine.py +++ b/scripts/smell_engine.py @@ -1379,9 +1379,30 @@ def _detect_magic_values(content: str, ext: str, rel_path: str) -> List[Dict]: in_docstring = False + in_block_comment = False for i, line in enumerate(lines): stripped = line.strip() + # Track /* ... */ and /** ... */ block comments (JS/TS/Java/C/C++/ + # Rust/Go/CSS). Only single-line `//` was excluded before this fix — + # a JSDoc block's continuation lines never start with `//`, they + # start with `/*` (opening) or `*` (continuation), so every number + # written in doc-comment prose (issue refs like "#1091", coordinate + # ranges like "[-90, 90]", ISO-format examples like "8601") was + # scanned as if it were live code. Real false positive: KAW81 API's + # routes/public/orders/create.ts flagged 17 "magic numbers" that were + # 100% issue-number references inside JSDoc (`#775`, `#1194`, ...). + if in_block_comment: + if '*/' in stripped: + in_block_comment = False + continue + if stripped.startswith('/*'): + if '*/' not in stripped: + in_block_comment = True + continue + if stripped.startswith('*'): # JSDoc continuation line: " * text" + continue + # Track docstring boundaries if '"""' in stripped or "'''" in stripped: count = stripped.count('"""') + stripped.count("'''") diff --git a/tests/test_deadcode_engine.py b/tests/test_deadcode_engine.py index f33a997..c321ba0 100644 --- a/tests/test_deadcode_engine.py +++ b/tests/test_deadcode_engine.py @@ -64,6 +64,29 @@ def test_unused_variable_detection(self): finally: shutil.rmtree(ws, ignore_errors=True) + def test_exported_var_used_only_in_other_file_not_flagged(self): + """An `export const X = ...` that is never re-mentioned in its OWN + file must NOT be flagged unused_vars, even though this detector only + scans same-file occurrences. Real-world false positive: Express + middleware exported and passed by reference in a different file + (`app.post(path, fooLimiter)`) — fooLimiter is never called or + re-mentioned in the file that declares it, so the same-file count + was always 1 (the declaration itself). Cross-file usage is + `unused_exports`' job, not this detector's — an exported symbol + must always be exempted here regardless of same-file usage count.""" + code = """ +import rateLimit from 'express-rate-limit'; +export const fooLimiter = rateLimit({ windowMs: 60000, max: 10 }); +""" + ws = self._create_workspace(code, "ratelimit.ts") + try: + result = detect_dead_code(ws) + assert result["status"] == "ok" + unused_names = [v["variable"] for v in result["results"].get("unused_vars", [])] + assert "fooLimiter" not in unused_names + finally: + shutil.rmtree(ws, ignore_errors=True) + def test_return_structure(self): """Verify the complete return structure of detect_dead_code.""" code = "function test() { return true; }" diff --git a/tests/test_smell_engine.py b/tests/test_smell_engine.py index c65bf8e..3f17a66 100644 --- a/tests/test_smell_engine.py +++ b/tests/test_smell_engine.py @@ -49,6 +49,37 @@ def test_many_parameters(self): finally: shutil.rmtree(ws, ignore_errors=True) + def test_magic_values_ignores_numbers_in_block_comments(self): + """Numbers inside /* */ and /** */ block comments must NOT be + flagged as magic numbers. Before this fix, only single-line `//` + comments were excluded — JSDoc continuation lines (starting with + `*`, not `//`) were scanned as live code. Real false positive: + KAW81 API's routes/public/orders/create.ts flagged 17 "magic + numbers" that were 100% GitHub issue references (`#775`, `#1194`) + and coordinate-range docs (`[-90, 90]`) inside JSDoc blocks.""" + code = """interface Payload { + /** + * #1091: Voucher applied to this order. Optional — null/undefined = + * no voucher (backward compat). + */ + voucherId?: string | null; + /** + * #775: Delivery latitude — must be a finite number in range [-90, 90]. + */ + deliveryLat?: number; +} +""" + ws = self._create_workspace(code, "payload.ts") + try: + result = detect_smells(ws, categories=["magic_values"]) + magic = result["by_category"].get("magic_values", []) + flagged_values = [m["value"] for m in magic] + assert 1091 not in flagged_values, f"False positive: {magic}" + assert 775 not in flagged_values, f"False positive: {magic}" + assert 90 not in flagged_values, f"False positive: {magic}" + finally: + shutil.rmtree(ws, ignore_errors=True) + def test_clean_code_high_score(self): code = """ function add(a, b) { return a + b; }