-
Notifications
You must be signed in to change notification settings - Fork 0
fix: Side-effect taint stopping at first importer #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 0.25.4 | ||
| 0.25.5 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,14 @@ import ( | |
| "goodchanges/tsgo-vendor/pkg/scanner" | ||
| ) | ||
|
|
||
| // sideEffectTaint is a sentinel taint token marking that a file has an *import-time | ||
| // side effect* change (a changed top-level side-effect statement, or a bare | ||
| // `import "x"` side-effect import) — as opposed to an ordinary whole-file "*" taint | ||
| // (e.g. a new file). It propagates through import/re-export edges so a barrel that | ||
| // re-exports a side-effectful module becomes side-effectful itself. It is not a | ||
| // valid JS identifier, so it can never collide with or be matched as a real symbol. | ||
| const sideEffectTaint = "__side-effect__" | ||
|
|
||
| // findAffectedSymbolsByASTDiff compares OLD and NEW file ASTs to find which symbols changed. | ||
| // Returns symbol names that have runtime changes (or type-only changes if includeTypes is true). | ||
| // | ||
|
|
@@ -232,10 +240,11 @@ func findAffectedSymbolsByASTDiff(oldAnalysis *tsparse.FileAnalysis, newAnalysis | |
| 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. | ||
| affected = append(affected, "*") | ||
| // 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) | ||
|
Comment on lines
+243
to
+247
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Detect import-time side effects even when symbols also changed. The enclosing fallback runs only when Evaluate 🤖 Prompt for AI Agents |
||
| for _, sym := range newAnalysis.Symbols { | ||
| if sym.IsTypeOnly && !includeTypes { | ||
| continue | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not propagate import-time side effects through type-only re-export edges.
Both graph builders include
exp.IsTypeOnlyre-exports as import edges. If the source has"*"andsideEffectTaint, a statement such asexport type { T } from "./source"reaches these blocks and taints all runtime symbols in the barrel. Type-only re-exports do not load./sourceat runtime.internal/analyzer/analyzer.go#L786-L801: apply side-effect propagation only when the edge represents a runtime import or re-export.internal/analyzer/analyzer.go#L1644-L1654: apply the same runtime-edge condition inFindAffectedFiles.📍 Affects 1 file
internal/analyzer/analyzer.go#L786-L801(this comment)internal/analyzer/analyzer.go#L1644-L1654🤖 Prompt for AI Agents