From 7c98c3ce7b76310485ac7775c7f84fbc073c728d Mon Sep 17 00:00:00 2001 From: Wolfvin Date: Fri, 3 Jul 2026 14:59:27 +0000 Subject: [PATCH] fix(affected): parse multi-line, side-effect, and re-export imports (closes #189) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #184 fixed the workspace-as-first-arg heuristic in commands/affected.py so the existing ts_workspace fixture (single-line `import { x } from './y'`) returned affected_count > 0. But get_affected_files still returned affected_count: 0 for three common real-world TS/JS import shapes, because _parse_js_imports silently skipped them — leaving reverse_graph incomplete so BFS had no edges to traverse. Root cause (verified by tracing _build_import_graph output): 1. Multi-line imports — `import {\n foo,\n} from './bar'` The ES import regex used `.*?` which does not cross newlines, so any import statement split across lines (Prettier default, ESLint `multi-line` rule) was silently skipped. 2. Side-effect imports — `import './polyfills'` (no `from` clause) The ES import regex required `from`, so side-effect imports were silently skipped even though they create a real dependency edge. 3. Re-exports — `export { x } from './bar'`, `export * from './bar'`, `export type { Foo } from './bar'` The parser only looked for `import`, never `export ... from`, so barrel files (index.ts re-exporting from core.ts) were invisible to the reverse graph. When the parser misses an import, reverse_graph loses an edge. If the changed file's only inbound edge was the missed import, BFS finds no dependents and affected_count stays at 0 — the "BFS never starts" symptom from issue #189. Fix: - Replace `.*?` with `[\s\S]*?` in the ES import regex so it matches across newlines (multi-line imports). - Add a dedicated `import\s+["']...["']` regex for side-effect imports. The regex is disjoint from the `from` form (requires a quote immediately after import+whitespace), so order does not matter. - Add a new `export\s+[\s\S]*?from\s+["']...["']` regex for re-exports. Tests: 5 new tests in TestIssue189ImportParserPatterns covering multi-line imports, side-effect imports, re-exports, the existing ts_workspace fixture (DoD sanity check), and --include-source with multi-line imports. All 4 bug-exposing tests fail on the pre-fix code and pass after the fix; the 5th (DoD sanity check) passes both before and after. Regression: 63 passed in tests/test_affected_command.py (58 existing + 5 new). 107 passed across affected_command + command_registry + command_count + doctor. 151 passed across parser + formatter + orient suites (5 skipped, pre-existing). No new failures. Definition of Done (issue #189): - [x] codelens affected auth/google-auth-cache.ts returns affected_count > 0 (verified via test_existing_ts_workspace_fixture_still_finds_affected) - [x] unresolved[] is empty when input file exists and is imported by other files (asserted in every new regression test) - [x] Use existing ts_workspace fixture and assert affected_count >= 1 (test_existing_ts_workspace_fixture_still_finds_affected) --- scripts/dependents_engine.py | 29 ++++- tests/test_affected_command.py | 205 +++++++++++++++++++++++++++++++++ 2 files changed, 232 insertions(+), 2 deletions(-) diff --git a/scripts/dependents_engine.py b/scripts/dependents_engine.py index 1e72689..e7d146d 100755 --- a/scripts/dependents_engine.py +++ b/scripts/dependents_engine.py @@ -571,8 +571,33 @@ def _parse_js_imports(file_path: str, rel_path: str, workspace: str) -> List[str imports = [] file_dir = os.path.dirname(rel_path) - # ES imports - for m in re.finditer(r'import\s+.*?from\s+["\']([^"\']+)["\']', content): + # Issue #189: previous regex used `.*?` which doesn't match newlines, + # so multi-line imports like `import {\n foo,\n} from './bar'` were + # silently skipped. This left reverse_graph incomplete, which made + # `codelens affected` return affected_count: 0 even when real + # dependents existed. Use `[\s\S]*?` to match across newlines. + # + # ES imports (with from) — handles single-line AND multi-line forms. + for m in re.finditer(r'import\s+[\s\S]*?from\s+["\']([^"\']+)["\']', content): + resolved = _resolve_relative_import(m.group(1), file_dir, workspace) + if resolved: + imports.append(resolved) + + # Issue #189: side-effect imports — `import './polyfills'` (no `from`). + # Must run AFTER the `from` form so we don't double-resolve, but the + # regex is disjoint: it requires a quote immediately after `import\s+`, + # which the `{ ... } from` form cannot match. + for m in re.finditer(r'import\s+["\']([^"\']+)["\']', content): + resolved = _resolve_relative_import(m.group(1), file_dir, workspace) + if resolved: + imports.append(resolved) + + # Issue #189: re-exports — `export { x } from './bar'`, + # `export * from './bar'`, `export type { Foo } from './bar'`. + # These create a real dependency edge (the re-exporting module + # depends on the source module), so the reverse graph must include + # them or `codelens affected` will miss downstream consumers. + for m in re.finditer(r'export\s+[\s\S]*?from\s+["\']([^"\']+)["\']', content): resolved = _resolve_relative_import(m.group(1), file_dir, workspace) if resolved: imports.append(resolved) diff --git a/tests/test_affected_command.py b/tests/test_affected_command.py index f35522b..e011b51 100644 --- a/tests/test_affected_command.py +++ b/tests/test_affected_command.py @@ -621,3 +621,208 @@ def test_ts_no_workspace_arg_uses_cwd(self, ts_workspace): assert result["workspace"] == os.path.abspath(ts_workspace) assert "auth/google-auth-cache.ts" in result["changed_files"] assert result["unresolved"] == [] + + +# ─── Issue #189: BFS never starts when import parser misses TS patterns ── +# +# PR #184 fixed the workspace-as-first-arg heuristic so the existing +# ts_workspace fixture (single-line `import { x } from './y'`) works. But +# `_parse_js_imports` still missed three common TS/JS import shapes: +# +# 1. Multi-line imports — `import {\n foo,\n} from './bar'` +# Root cause: regex used `.*?` which doesn't cross newlines. +# 2. Side-effect imports — `import './polyfills'` (no `from` clause). +# Root cause: regex required `from`. +# 3. Re-exports — `export { x } from './bar'`, `export * from './bar'`. +# Root cause: regex only matched `import`, not `export ... from`. +# +# When the parser misses an import, the reverse_graph loses an edge. If the +# changed file's only inbound edge was the missed import, BFS finds no +# dependents and `affected_count` stays at 0 — even though real dependents +# exist. This is the "BFS never starts" symptom described in issue #189. + + +class TestIssue189ImportParserPatterns: + """Regression tests for issue #189: `_parse_js_imports` must handle + multi-line imports, side-effect imports, and re-exports — otherwise + `_build_import_graph` produces an incomplete reverse_graph and + `codelens affected` returns ``affected_count: 0`` even when real + dependents exist. + """ + + @pytest.fixture + def ts_workspace_multiline(self, tmp_path): + """TS workspace where login.ts uses a multi-line import statement. + + Mirrors the existing `ts_workspace` fixture but exercises the + multi-line import shape that real-world TS code uses pervasively + (Prettier default, ESLint `multi-line` rule, etc.). + """ + ws = tmp_path / "ws_multiline" + ws.mkdir() + (ws / "auth").mkdir() + (ws / "tests").mkdir() + (ws / "auth" / "google-auth-cache.ts").write_text( + "export const cache = new Map();\n" + "export function getCachedToken(key: string): string | null {\n" + " return cache.get(key) || null;\n" + "}\n", + encoding="utf-8", + ) + (ws / "auth" / "login.ts").write_text( + "import {\n" + " getCachedToken,\n" + "} from './google-auth-cache';\n" + "export function login(user: string): boolean {\n" + " const token = getCachedToken(user);\n" + " return token !== null;\n" + "}\n", + encoding="utf-8", + ) + (ws / "tests" / "login.test.ts").write_text( + "import { login } from '../auth/login';\n" + "test('login returns false for unknown user', () => {\n" + " expect(login('unknown')).toBe(false);\n" + "});\n", + encoding="utf-8", + ) + return str(ws) + + @pytest.fixture + def ts_workspace_side_effect(self, tmp_path): + """TS workspace where app.ts has a side-effect import (no `from`).""" + ws = tmp_path / "ws_side_effect" + ws.mkdir() + (ws / "auth").mkdir() + (ws / "tests").mkdir() + (ws / "auth" / "polyfills.ts").write_text( + "export const POLYFILL_VERSION = '1.0.0';\n", + encoding="utf-8", + ) + (ws / "auth" / "app.ts").write_text( + "import './polyfills';\n" + "export function app(): string { return 'ok'; }\n", + encoding="utf-8", + ) + (ws / "tests" / "app.test.ts").write_text( + "import { app } from '../auth/app';\n" + "test('app works', () => { expect(app()).toBe('ok'); });\n", + encoding="utf-8", + ) + return str(ws) + + @pytest.fixture + def ts_workspace_reexport(self, tmp_path): + """TS workspace using `export { x } from './core'` (barrel file).""" + ws = tmp_path / "ws_reexport" + ws.mkdir() + (ws / "auth").mkdir() + (ws / "tests").mkdir() + (ws / "auth" / "core.ts").write_text( + "export const TOKEN_KEY = 'cl_token';\n", + encoding="utf-8", + ) + (ws / "auth" / "index.ts").write_text( + "export { TOKEN_KEY } from './core';\n", + encoding="utf-8", + ) + (ws / "tests" / "index.test.ts").write_text( + "import { TOKEN_KEY } from '../auth/index';\n" + "test('key is string', () => { expect(typeof TOKEN_KEY).toBe('string'); });\n", + encoding="utf-8", + ) + return str(ws) + + def test_multiline_import_resolves_and_bfs_finds_dependents( + self, ts_workspace_multiline, + ): + """Multi-line `import {\n foo \n} from './bar'` must be parsed. + + Without the fix, `_parse_js_imports` skipped this import (regex + `.*?` did not cross newlines), `reverse_graph` had no edge from + `google-auth-cache.ts` to `login.ts`, and `affected_count` was 0. + """ + result = get_affected_files( + changed_files=["auth/google-auth-cache.ts"], + workspace=ts_workspace_multiline, + depth=5, + ) + assert result["status"] == "ok" + assert result["unresolved"] == [], ( + "changed file must resolve; unresolved means graph keys are " + "incomplete (the BFS-never-starts symptom from issue #189)" + ) + assert result["stats"]["affected_count"] >= 1, ( + "BFS must find the transitive test dependent via the multi-line " + "import in login.ts" + ) + assert "tests/login.test.ts" in result["affected"] + + def test_side_effect_import_resolves_and_bfs_finds_dependents( + self, ts_workspace_side_effect, + ): + """`import './polyfills'` (no `from`) must be parsed as an edge.""" + result = get_affected_files( + changed_files=["auth/polyfills.ts"], + workspace=ts_workspace_side_effect, + depth=5, + ) + assert result["status"] == "ok" + assert result["unresolved"] == [] + assert result["stats"]["affected_count"] >= 1, ( + "side-effect imports create a real dependency edge — BFS must " + "find the transitive test dependent" + ) + assert "tests/app.test.ts" in result["affected"] + + def test_reexport_creates_dependency_edge( + self, ts_workspace_reexport, + ): + """`export { x } from './core'` must add core.ts → index.ts edge.""" + result = get_affected_files( + changed_files=["auth/core.ts"], + workspace=ts_workspace_reexport, + depth=5, + ) + assert result["status"] == "ok" + assert result["unresolved"] == [] + assert result["stats"]["affected_count"] >= 1, ( + "re-exports create a real dependency edge — BFS must find the " + "transitive test dependent via the barrel file" + ) + assert "tests/index.test.ts" in result["affected"] + + def test_existing_ts_workspace_fixture_still_finds_affected(self, ts_workspace): + """DoD sanity check: the existing fixture from PR #184 must still + return ``affected_count >= 1`` after the parser changes. + + This is the explicit Definition-of-Done assertion from issue #189: + "Use existing ts_workspace fixture in tests/test_affected_command.py + (from PR #184) and assert affected_count >= 1". + """ + result = get_affected_files( + changed_files=["auth/google-auth-cache.ts"], + workspace=ts_workspace, + depth=5, + ) + assert result["status"] == "ok" + assert result["unresolved"] == [] + assert result["stats"]["affected_count"] >= 1 + assert "tests/login.test.ts" in result["affected"] + + def test_multiline_include_source_returns_all_dependents( + self, ts_workspace_multiline, + ): + """`--include-source` must also surface non-test dependents reached + via multi-line imports (cross-check against the existing + `test_ts_include_source_returns_all_dependents`). + """ + result = get_affected_files( + changed_files=["auth/google-auth-cache.ts"], + workspace=ts_workspace_multiline, + depth=5, + include_source=True, + ) + affected = set(result["affected"]) + assert "auth/login.ts" in affected + assert "tests/login.test.ts" in affected