From 5b7df1471f4873377497d3026e80039ab5e2cd48 Mon Sep 17 00:00:00 2001 From: Martin Najemi Date: Fri, 7 Aug 2026 17:49:48 +0200 Subject: [PATCH] fix: Removed exports over-tainting all importers Risk: low --- CHANGELOG.md | 6 ++++++ VERSION | 2 +- internal/analyzer/astdiff.go | 23 ++++++++++++++++++++--- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15d852d..6e38db8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ 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 + +### Fixed +- 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 ### Fixed @@ -397,6 +402,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.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 [0.24.13]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.24.12...v0.24.13 diff --git a/VERSION b/VERSION index 5d60147..5a179f3 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.25.1 \ No newline at end of file +0.25.2 \ No newline at end of file diff --git a/internal/analyzer/astdiff.go b/internal/analyzer/astdiff.go index 6b10bd0..7d3d767 100644 --- a/internal/analyzer/astdiff.go +++ b/internal/analyzer/astdiff.go @@ -107,16 +107,27 @@ func findAffectedSymbolsByASTDiff(oldAnalysis *tsparse.FileAnalysis, newAnalysis affected = append(affected, sym.Name) } - // Log deleted symbols (in old but not in new) + // Deleted symbols (in old but not in new) are themselves a change: removing an + // export can break whoever imported it, so record the deleted names and let + // them propagate to importers. This keeps detection precise — a removed + // *unused* export taints nobody, instead of the empty-diff falling through to a + // whole-file taint downstream. Type-only deletions are ignored unless includeTypes. + var deleted []string if oldAnalysis != nil { newSymbolNames := make(map[string]bool) for _, sym := range newAnalysis.Symbols { newSymbolNames[sym.Name] = true } for _, sym := range oldAnalysis.Symbols { - if !newSymbolNames[sym.Name] { - log.Debugf(" %s: DELETED symbol", sym.Name) + if newSymbolNames[sym.Name] { + continue } + if sym.IsTypeOnly && !includeTypes { + log.Debugf(" %s: DELETED type-only symbol (skipped, includeTypes=false)", sym.Name) + continue + } + log.Debugf(" %s: DELETED symbol", sym.Name) + deleted = append(deleted, sym.Name) } } @@ -188,6 +199,12 @@ func findAffectedSymbolsByASTDiff(oldAnalysis *tsparse.FileAnalysis, newAnalysis } } + // Removed symbols propagate to whoever imported them. Appended after the + // intra-file rebuild (which only walks NEW symbols and would drop them) and + // before the fallback below, so a change that is purely a deletion is carried + // by these names rather than misrouted into a whole-file side-effect taint. + affected = append(affected, deleted...) + // Fallback: if no symbols were detected but the file clearly changed, // check if changes are outside any symbol (e.g. top-level side effects, // copyright comments). If there are runtime side-effect changes, taint all symbols.