fix: Side-effect taint stopping at first importer - #91
Conversation
📝 WalkthroughWalkthroughThe analyzer now marks import-time side-effect changes with a dedicated sentinel and propagates that marker transitively through imports and re-exports. The release version changes to 0.25.5, with corresponding changelog updates. ChangesSide-effect taint propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ASTDiff
participant AnalyzeLibraryPackage
participant FindAffectedFiles
participant Importer
ASTDiff->>AnalyzeLibraryPackage: Provide "*" and sideEffectTaint
AnalyzeLibraryPackage->>Importer: Taint all importer symbols
Importer->>AnalyzeLibraryPackage: Propagate markers through imports and re-exports
FindAffectedFiles->>Importer: Taint all importer symbols
Importer->>FindAffectedFiles: Continue BFS propagation
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/analyzer/analyzer.go`:
- Around line 786-801: Restrict import-time side-effect propagation to runtime
import or re-export edges by checking the edge’s non-type-only status before the
currentTainted[sideEffectTaint] handling in internal/analyzer/analyzer.go lines
786-801. Apply the same runtime-edge condition in FindAffectedFiles at
internal/analyzer/analyzer.go lines 1644-1654; type-only re-exports must not
taint barrel symbols or propagate sideEffectTaint.
In `@internal/analyzer/astdiff.go`:
- Around line 243-247: Update the affected-symbol logic around
hasSideEffectStmtChanges and bareImportsChanged so these checks run
independently of len(affected) and symbol-level changes. Whenever either
side-effect condition is true, append both "*" and sideEffectTaint, while
preserving existing handling for ordinary symbol changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 292df3ee-cad5-42f2-9c16-39d0601ebe43
📒 Files selected for processing (4)
CHANGELOG.mdVERSIONinternal/analyzer/analyzer.gointernal/analyzer/astdiff.go
| // Import-time side-effect transitivity: if the imported module has an | ||
| // import-time side effect (sideEffectTaint), importing it re-runs that | ||
| // side effect here, so this file becomes side-effectful too — taint all | ||
| // its symbols and carry "*" + sideEffectTaint so it keeps flowing to this | ||
| // file's own importers/re-exporters (a barrel re-exporting a side-effectful | ||
| // module becomes side-effectful itself). | ||
| // TODO: make this precise using the "sideEffects" field in each package's | ||
| // package.json — a module marked side-effect-free is tree-shaken and not | ||
| // re-executed on import, so it should not propagate. Until then we assume | ||
| // the worst and propagate through every import/re-export edge. Follow-up. | ||
| if currentTainted[sideEffectTaint] { | ||
| for _, sym := range importerAnalysis.Symbols { | ||
| newlyTainted = append(newlyTainted, sym.Name) | ||
| } | ||
| newlyTainted = append(newlyTainted, "*", sideEffectTaint) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not propagate import-time side effects through type-only re-export edges.
Both graph builders include exp.IsTypeOnly re-exports as import edges. If the source has "*" and sideEffectTaint, a statement such as export type { T } from "./source" reaches these blocks and taints all runtime symbols in the barrel. Type-only re-exports do not load ./source at 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/analyzer/analyzer.go` around lines 786 - 801, Restrict import-time
side-effect propagation to runtime import or re-export edges by checking the
edge’s non-type-only status before the currentTainted[sideEffectTaint] handling
in internal/analyzer/analyzer.go lines 786-801. Apply the same runtime-edge
condition in FindAffectedFiles at internal/analyzer/analyzer.go lines 1644-1654;
type-only re-exports must not taint barrel symbols or propagate sideEffectTaint.
| // 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) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detect import-time side effects even when symbols also changed.
The enclosing fallback runs only when len(affected) == 0. If one diff changes an exported symbol and a top-level side-effect statement, this block does not run. The result omits sideEffectTaint, so importer and re-export propagation stops for that side-effect change.
Evaluate hasSideEffectStmtChanges and bareImportsChanged independently of symbol-level changes. Append "*" and sideEffectTaint whenever either check is true.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/analyzer/astdiff.go` around lines 243 - 247, Update the
affected-symbol logic around hasSideEffectStmtChanges and bareImportsChanged so
these checks run independently of len(affected) and symbol-level changes.
Whenever either side-effect condition is true, append both "*" and
sideEffectTaint, while preserving existing handling for ordinary symbol changes.
2 participants
Risk: low
Summary by CodeRabbit
Bug Fixes
Documentation
Chores