Skip to content

fix(modules): preserve selected matrix on loaded formulas - #169

Open
MeteorsLiu wants to merge 3 commits into
xgo-dev:mainfrom
MeteorsLiu:fix/matrix-target-propagation
Open

fix(modules): preserve selected matrix on loaded formulas#169
MeteorsLiu wants to merge 3 commits into
xgo-dev:mainfrom
MeteorsLiu:fix/matrix-target-propagation

Conversation

@MeteorsLiu

@MeteorsLiu MeteorsLiu commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Preserve the selected formula matrix on every Formula returned from a formulaContext.

The implementation includes:

  • Initialize the context matrix in newFormulaContext and pass it into each lazily created formulaModule constructor.
  • Inject the matrix in formulaModule.at() after cloning, covering all module loading paths without mutating cached formulas.
  • Remove duplicate matrix plumbing from convertToModules and resolveDeps.
  • Add regressions for direct at() hooks, the Load hook path, and custom options overriding defaults.
  • Verify with go test ./internal/modules/... -count=1 and targeted race tests.

This gives matrix-dependent formulas one consistent source of target settings during dependency resolution, builds, and tests.

@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: preserve matrix on loaded formulas

The fix is correct, minimal, and well-targeted. Root cause confirmed: formulaModule.at() returns a fresh formula.Clone(f) on every call (internal/modules/source.go:85,93), and Clone re-runs Main() which resets target to the parsed defaults. So the injectMatrix call in resolveDeps (load.go:298) mutates a different clone than the one convertToModules stores in the returned Module. Without the new injection at load.go:149, the formula handed to OnBuild/OnTest would carry only default options and miss the user-selected matrix. The change closes exactly that gap.

Test coverage is layered and appropriate: TestLoad_InjectsTargetBeforeFilterAndOnRequire asserts observable OnBuild metadata, TestLoad_FilterRejectsSelectedMatrix covers the negative path, and the new TestConvertToModules_InjectsSelectedMatrix is a focused unit test. Map/slice cloning (maps.Clone/slices.Clone) correctly avoids aliasing the caller's matrix into the formula. injectMatrix is idempotent because DefaultOptions is never overwritten and remains the source of truth for re-deriving options.

No blocking issues found. A few minor suggestions are inline; the rest are optional follow-ups below.

Optional follow-ups (non-blocking):

  • TestConvertToModules_InjectsSelectedMatrix (load_test.go:492-500) asserts by re-doing the same unexported-field reflection the production code uses, so a bug in the reflection helper could be masked in both. Asserting via observable behavior (running OnBuild) would validate behavior rather than implementation — as TestLoad_InjectsTargetBeforeFilterAndOnRequire already does.
  • The unsafe target-injection reflection is now reached from a second call site. A future refactor to expose an exported injector on formula.Formula would centralize the fragile access in one package.

Findings without inline locations

  • internal/modules/load.go:298: This comment says injection is done "before filter and onRequire so both hooks see the same target," but after this PR onBuild also consumes the injected matrix (via convertToModules; the new testdata branches on target.require/target.options in onBuild). Consider generalizing the wording so it doesn't read as an exhaustive list of the hooks that depend on injection.

Comment thread internal/modules/load.go Outdated
if err != nil {
return nil, err
}
injectMatrix(f, c.matrix)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a short comment here explaining why re-injection is needed. Unlike the sibling call in resolveDeps (which serves Filter/OnRequire), at() returns a fresh formula.Clone(f) (source.go:85,93) whose target is reset to defaults, so the matrix must be re-injected on this clone for the build-phase OnBuild hook to see the selected target.require/target.options. The rationale differs from the resolveDeps call site and isn't obvious from the code alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant