From 57d4cd7032aeeaeb9d388d1342610d7381f846dd Mon Sep 17 00:00:00 2001 From: Martin Najemi Date: Fri, 7 Aug 2026 18:27:28 +0200 Subject: [PATCH] fix: Whole-file taint on comment-only helper changes Risk: low --- CHANGELOG.md | 9 +++- VERSION | 2 +- internal/analyzer/analyzer.go | 17 ++++--- internal/analyzer/astdiff.go | 92 +++++++++++++++++++++++++++++++++-- internal/tsparse/tsparse.go | 4 ++ 5 files changed, 111 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e38db8..d5443df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [0.25.2] - 2026-08-07 +## [0.25.3] - 2026-08-07 ### Fixed +- Replaced the fine-grained path's whole-file `*` fallback with import-time-aware detection, so an empty symbol diff no longer floods every importer. Previously, when a changed file produced no symbol-level diff, `FindAffectedFiles` tainted the **entire file** with `*` — so a comment-only edit, a formatting change, a type-only change, or reordering imports in a widely-imported helper flagged all its consumers. That blanket is removed; a change now wildcards a file only when something that actually **runs at import time** changed: + - a top-level side-effect statement (`console.log(...)`, `describe(...)`, `test(...)`, etc.) — already detected via `hasSideEffectStmtChanges`; + - a bare side-effect import (`import "./x"`) added, removed, or re-pointed. +- Named-import **re-pointing** is now handled precisely instead of via the blanket. When a binding keeps its name but resolves to a different module/export (`import { x } from "./a"` → `"./b"`, or `{ a as x }` → `{ b as x }`), its usages don't change textually and the symbol diff missed them; those usages are now tainted directly. Reordering imports and re-pointing a *type-only* import (`import type`, when `includeTypes` is off) correctly taint nothing. Import statement type-only-ness is now tracked on `tsparse.Import`. + +Together these keep genuine import-time changes flagged while eliminating the large false-positive class where a comment or dead-code edit to a shared file re-ran every dependent target. - Removing an unused export no longer floods every importer with taint. A deleted symbol was logged but never recorded as a change, so the per-file AST diff returned *no affected symbols* — and in the fine-grained path (`FindAffectedFiles`) an empty diff falls through to tainting the **whole file** with `*`. Deleting one unused export from a widely-imported helper (e.g. `ERROR_MESSAGE` from `gdc-ldm-modeler-e2e`'s `playwright/helpers/selectors.ts`) therefore tainted every file that imported it, flagging all ~48 dependent specs. Deleted symbols are now recorded as changed and propagate by name, so a removed export taints exactly the files that imported *that* symbol: an unused one taints nobody, while a removed *used* export still flags its importers (no false negative). The deleted names are appended after intra-file propagation (which walks only surviving symbols) and before the whole-file side-effect fallback, so a deletion-only change is carried precisely instead of being widened. ## [0.25.1] - 2026-08-07 @@ -402,6 +408,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Multi-stage Docker build - Automated vendor upgrade workflow +[0.25.3]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.2...v0.25.3 [0.25.2]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.1...v0.25.2 [0.25.1]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.0...v0.25.1 [0.25.0]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.24.13...v0.25.0 diff --git a/VERSION b/VERSION index 5a179f3..1bb9b67 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.25.2 \ No newline at end of file +0.25.3 \ No newline at end of file diff --git a/internal/analyzer/analyzer.go b/internal/analyzer/analyzer.go index 12f7b15..64fe2e1 100644 --- a/internal/analyzer/analyzer.go +++ b/internal/analyzer/analyzer.go @@ -1327,25 +1327,26 @@ func FindAffectedFiles(globPattern string, filterPattern string, upstreamTaint m } changedSymbols := findAffectedSymbolsByASTDiff(oldAnalysis, analysis, oldContent, includeTypes) log.Debugf(" %s: affected symbols (AST diff): %v", stem, changedSymbols) - if tainted[stem] == nil { - tainted[stem] = make(map[string]bool) - } if oldAnalysis == nil { - // New file: taint all symbols + // New file: taint all symbols (their behavior is entirely new). log.Debugf(" %s: new file — tainting all symbols", stem) + tainted[stem] = make(map[string]bool) for _, sym := range analysis.Symbols { tainted[stem][sym.Name] = true } tainted[stem]["*"] = true } else if len(changedSymbols) > 0 { + tainted[stem] = make(map[string]bool) for _, s := range changedSymbols { tainted[stem][s] = true } - } else { - // File changed but no symbol-level diff detected (e.g. changes in - // test()/describe() blocks or other non-declaration code). - tainted[stem]["*"] = true } + // else: the AST diff found nothing that affects consumers (comments, + // formatting, type-only, or reordered imports). Import-time side effects + // (top-level statements, bare `import "x"`) and re-pointed value imports are + // already surfaced as "*"/affected symbols by findAffectedSymbolsByASTDiff, + // so there is deliberately NO whole-file fallback here (the old blanket `*` + // over-tainted every importer on a comment-only or dead-code change). } // Seed from upstream workspace taint diff --git a/internal/analyzer/astdiff.go b/internal/analyzer/astdiff.go index 7d3d767..2ddb0e9 100644 --- a/internal/analyzer/astdiff.go +++ b/internal/analyzer/astdiff.go @@ -131,6 +131,16 @@ func findAffectedSymbolsByASTDiff(oldAnalysis *tsparse.FileAnalysis, newAnalysis } } + // Re-pointed value imports: a local binding keeps its name but now resolves to + // a different module/export (`import { x } from "./a"` → "./b", or + // `{ a as x }` → `{ b as x }`). The usages don't change textually, so the + // symbol diff above misses them — taint the symbols that use the re-pointed + // binding. (Type-only imports are excluded unless includeTypes.) + if repointed := repointedImportBindings(oldAnalysis, newAnalysis, includeTypes); len(repointed) > 0 { + log.Debugf(" re-pointed import bindings: %v", repointed) + affected = append(affected, findTaintedSymbolsByUsage(newAnalysis, repointed)...) + } + // Intra-file propagation: if symbol A changed and symbol B references A, // then B is also affected. E.g. `UiPagedVirtualListNotWrapped` changed, // `UiPagedVirtualList = memo(UiPagedVirtualListNotWrapped)` is also affected. @@ -215,9 +225,13 @@ func findAffectedSymbolsByASTDiff(oldAnalysis *tsparse.FileAnalysis, newAnalysis } if normalizeWhitespace(oldText) != normalizeWhitespace(newText) { // File changed but no symbol was affected — changes are outside symbols. - // Check if the changes include runtime side-effect statements. - if hasSideEffectStmtChanges(oldAnalysis.SourceFile, newAnalysis.SourceFile) { - log.Debugf(" file changed with RUNTIME side-effect statements — tainting all symbols") + // Wildcard only when something that RUNS at import time changed: a + // top-level side-effect statement, or a bare `import "x"` side-effect + // import. Comment / formatting / type-only / import-reordering changes + // fall through untainted. + if hasSideEffectStmtChanges(oldAnalysis.SourceFile, newAnalysis.SourceFile) || + bareImportsChanged(oldAnalysis, newAnalysis) { + log.Debugf(" file changed with import-time side effects — tainting all symbols") // Use "*" wildcard to mark all exports as affected. // This handles barrel/entrypoint files that have no symbol declarations // but whose runtime side effects affect all importers. @@ -509,3 +523,75 @@ func isSideEffectStatement(stmt *ast.Node) bool { return true } } + +// importBindingOrigins maps each local binding introduced by a value import to a +// stable "origin" key (source + source-side name). Bare side-effect imports (no +// bindings) and — unless includeTypes — `import type` statements are excluded. +func importBindingOrigins(a *tsparse.FileAnalysis, includeTypes bool) map[string]string { + origins := make(map[string]string) + if a == nil { + return origins + } + for _, imp := range a.Imports { + if imp.IsTypeOnly && !includeTypes { + continue + } + for i, local := range imp.LocalNames { + src := "" + if i < len(imp.Names) { + src = imp.Names[i] + } + origins[local] = imp.Source + "\x00" + src + } + } + return origins +} + +// repointedImportBindings returns local binding names present both before and +// after the change that now resolve to a different module/export. +func repointedImportBindings(oldA, newA *tsparse.FileAnalysis, includeTypes bool) []string { + if oldA == nil || newA == nil { + return nil + } + oldOrigins := importBindingOrigins(oldA, includeTypes) + newOrigins := importBindingOrigins(newA, includeTypes) + var repointed []string + for local, newOrigin := range newOrigins { + if oldOrigin, ok := oldOrigins[local]; ok && oldOrigin != newOrigin { + repointed = append(repointed, local) + } + } + return repointed +} + +// bareImportSources returns the set of module specifiers imported purely for +// their side effects (`import "./x"` — no bindings). These execute at import time. +func bareImportSources(a *tsparse.FileAnalysis) map[string]bool { + sources := make(map[string]bool) + if a == nil { + return sources + } + for _, imp := range a.Imports { + if len(imp.Names) == 0 && len(imp.LocalNames) == 0 { + sources[imp.Source] = true + } + } + return sources +} + +// bareImportsChanged reports whether the set of side-effect imports differs +// between old and new (added, removed, or re-pointed) — an import-time behavior +// change that warrants whole-file taint. +func bareImportsChanged(oldA, newA *tsparse.FileAnalysis) bool { + oldSet := bareImportSources(oldA) + newSet := bareImportSources(newA) + if len(oldSet) != len(newSet) { + return true + } + for s := range newSet { + if !oldSet[s] { + return true + } + } + return false +} diff --git a/internal/tsparse/tsparse.go b/internal/tsparse/tsparse.go index 9f2c350..f7e47a2 100644 --- a/internal/tsparse/tsparse.go +++ b/internal/tsparse/tsparse.go @@ -19,6 +19,7 @@ type Import struct { // LocalNames[i] is "Y" (what this file references in its body). LocalNames []string Source string // module specifier (e.g., "./Button/Button.js") + IsTypeOnly bool // true for `import type { … }` / `import type X` (whole-statement type-only) } type Export struct { @@ -135,8 +136,10 @@ func extractImports(stmt *ast.Node, analysis *FileAnalysis) { source := strings.Trim(imp.ModuleSpecifier.Text(), "\"'`") var names, localNames []string + typeOnly := false if imp.ImportClause != nil { clause := imp.ImportClause.AsImportClause() + typeOnly = clause.PhaseModifier == ast.KindTypeKeyword if clause.Name() != nil { n := clause.Name().Text() names = append(names, n) @@ -171,6 +174,7 @@ func extractImports(stmt *ast.Node, analysis *FileAnalysis) { Names: names, LocalNames: localNames, Source: source, + IsTypeOnly: typeOnly, }) }