From aa7371b066ac5535c9a56573bc56aae8670e83ce Mon Sep 17 00:00:00 2001 From: Martin Najemi Date: Tue, 11 Aug 2026 02:34:55 +0200 Subject: [PATCH] fix: Symbol changes masking import-time side effects Risk: low --- CHANGELOG.md | 6 ++++ VERSION | 2 +- internal/analyzer/astdiff.go | 57 ++++++++++++++++++------------------ 3 files changed, 36 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7a4651..136c1eb 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.6] - 2026-08-11 + +### Fixed +- Import-time side effects are now detected even when the same diff also changes symbols. The `hasSideEffectStmtChanges` / `bareImportsChanged` checks ran only as a fallback when the symbol-level AST diff came up empty (`len(affected) == 0`), so a single commit that both edited an exported symbol **and** added/removed a top-level side-effect statement (`console.log(...)`) or a bare `import "x"` produced only the symbol taint — the `"*"` wildcard and `__side-effect__` sentinel were dropped, cutting off consumers of the file's *other* exports and the transitive barrel propagation added in 0.25.5. The side-effect checks now run independently of symbol-level results and append `"*" + __side-effect__` (plus all non-type-only symbols) whenever either fires. No over-taint is reintroduced: the checks compare only top-level side-effect statement text and the bare-import set, so a pure declaration edit still doesn't trigger them, and comment / formatting / type-only / import-reordering changes still taint nothing. + ## [0.25.5] - 2026-08-10 ### Fixed @@ -418,6 +423,7 @@ Together these keep genuine import-time changes flagged while eliminating the la - Multi-stage Docker build - Automated vendor upgrade workflow +[0.25.6]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.5...v0.25.6 [0.25.5]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.4...v0.25.5 [0.25.4]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.3...v0.25.4 [0.25.3]: https://github.com/gooddata/gooddata-goodchanges/compare/v0.25.2...v0.25.3 diff --git a/VERSION b/VERSION index 2e25e5e..5abd2e1 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.25.5 \ No newline at end of file +0.25.6 \ No newline at end of file diff --git a/internal/analyzer/astdiff.go b/internal/analyzer/astdiff.go index 454c9ad..47f3dd8 100644 --- a/internal/analyzer/astdiff.go +++ b/internal/analyzer/astdiff.go @@ -223,35 +223,36 @@ func findAffectedSymbolsByASTDiff(oldAnalysis *tsparse.FileAnalysis, newAnalysis // 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. - if len(affected) == 0 && oldAnalysis != nil { - oldText := "" - if oldAnalysis.SourceFile != nil { - oldText = oldAnalysis.SourceFile.Text() - } - if normalizeWhitespace(oldText) != normalizeWhitespace(newText) { - // File changed but no symbol was affected — changes are outside 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, plus the - // sideEffectTaint sentinel so the *import-time* nature propagates - // through import/re-export edges (a barrel importing this becomes - // side-effectful too). - affected = append(affected, "*", sideEffectTaint) - for _, sym := range newAnalysis.Symbols { - if sym.IsTypeOnly && !includeTypes { - continue - } - affected = append(affected, sym.Name) + // Import-time side effects are checked INDEPENDENTLY of symbol-level changes: + // one diff can both edit an exported symbol and add/remove a top-level + // side-effect statement (console.log, describe(...)) or a bare `import "x"`. + // Gating this behind len(affected) == 0 dropped the "*"/sideEffectTaint in the + // mixed case, cutting off importers of other symbols and the transitive + // side-effect propagation. The checks compare only top-level side-effect + // statement text / bare-import sets, so a pure declaration edit does not + // trigger them — comment / formatting / type-only / import-reordering changes + // still fall through untainted. + if oldAnalysis != nil { + 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, plus the + // sideEffectTaint sentinel so the *import-time* nature propagates + // through import/re-export edges (a barrel importing this becomes + // side-effectful too). + affected = append(affected, "*", sideEffectTaint) + for _, sym := range newAnalysis.Symbols { + if sym.IsTypeOnly && !includeTypes { + continue } - } else { + affected = append(affected, sym.Name) + } + } else if len(affected) == 0 { + oldText := "" + if oldAnalysis.SourceFile != nil { + oldText = oldAnalysis.SourceFile.Text() + } + if normalizeWhitespace(oldText) != normalizeWhitespace(newText) { log.Debugf(" file changed but no symbols affected (comments/imports only)") } }