Repository navigation
SetupWrapper: make impossible cases unrepresentable - #12434
Open
andreabedini wants to merge 5 commits into
Open
andreabedini wants to merge 5 commits into
andreabedini wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The changelog has missing required PR metadata and overstates the scope of the improved Make diagnostic.
2 open findings
What changed in this PR
Refactors SetupWrapper so invalid in-library setup states are excluded by its types and partial helper functions become total.
Changes:
- Indexes setup methods by wrapper specifications and carries required in-library arguments directly.
- Replaces partial setup-script and version-selection paths with explicit errors and non-empty inputs.
- Documents the behavior changes.
| File | Description |
|---|---|
cabal-install/src/Distribution/Client/SetupWrapper.hs |
Introduces type-safe setup-method selection and removes partial cases. |
changelog.d/setupwrapper-unreachable.md |
Records the user-visible diagnostics and refactor. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
andreabedini
force-pushed
the
setupwrapper-unreachable
branch
from
October 12, 2026 07:15
d84cdf3 to
c5235d8
Compare
sheaf
approved these changes
Oct 12, 2026
sheaf
left a comment
sheaf
left a comment
Collaborator
There was a problem hiding this comment.
Nice improvement, the code is clearer and more straightforward with fewer error cases in the middle of it.
Bodigrim
reviewed
Oct 12, 2026
Index SetupMethod by SetupWrapperSpec instead of a separate SetupKind,
and have LibraryMethod carry the in-library arguments (InLibraryRun),
with post-configure phases holding the LocalBuildInfo directly. The
in-library method can then only be constructed from SetupRunnerArgs
that permit it (inLibraryMethod), which removes the two internal
errors in setupWrapper:
- "NotInLibrary argument but getSetup chose InLibrary"
- "in-library post-conf but no LBI"
A post-configure phase given NotInLibraryNoLBI now uses the external
method, consistent with how the package was configured.
Also:
- buildTypeScript takes a GeneratedSetupScript, so it is total.
build-type Make (reachable on the v1 path) and Hooks with Cabal
< 3.13 are reported with die' in updateSetupScript.
- bestVersion takes a NonEmpty, removing the partial
firstMaximumBy and the fromMaybe/error in installedCabalVersion.
- Drop AllowInLibrary, SetupKind and the ASetup existential.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QTUcSRH5Ew1Q7PP35S8agF
InLibraryArgs (what callers pass) and InLibraryRun (what the chosen in-library method carries) were the same GADT except for the post-configure slot: InLibraryLBI in one, LocalBuildInfo in the other. Parameterise InLibraryArgs by what sits in that slot and drop the duplicate. inLibraryMethod becomes the function that fills the slot. Call sites are unchanged since the constructor names stay the same. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Suggested in review of haskell#12434. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3.13 is the odd development series; Hooks shipped in 3.14. Use the release number in the messages and the version guards. Suggested in review of haskell#12434. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
InLibraryLBI was a Maybe with a story attached. With InLibraryArgs now parameterised by what sits in the LocalBuildInfo slot, the story fits in its Haddock and inLibraryMethod becomes a plain traversal of the slot. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
andreabedini
force-pushed
the
setupwrapper-unreachable
branch
from
October 16, 2026 06:55
c5235d8 to
2515550
Compare
Collaborator
Author
|
The macOS failures (and the cancelled Bootstrap jobs) are the runner image issue in #12465 / actions/runner-images#14940, not this PR. Master fails the same way. |
Bodigrim
reviewed
Oct 17, 2026
| `LocalBuildInfo`, no longer type checks. The corresponding internal `error` | ||
| calls are gone. | ||
|
|
||
| `build-type: Make` packages on the legacy v1 path (still parsed for |
Collaborator
There was a problem hiding this comment.
I think "legacy v1 path" is not a relevant clarification - build-type: Make was available for v2 commands as well (and worked as well as in v1), so let's say just "build-type: Make packages with cabal-version < 3.18".
Collaborator
I think it was fixed on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Recently SetupWrapper gained fancy types to deal with the v1/v2 code paths but that change still left behind impossible cases like
and
This PR reorganise the code so that those impossible cases do not exist at all.
Index SetupMethod by SetupWrapperSpec instead of a separate SetupKind, and have LibraryMethod carry the in-library arguments, with post-configure phases holding the LocalBuildInfo directly. The in-library method can then only be constructed from SetupRunnerArgs that permit it (inLibraryMethod), which removes the two internal errors above.
A post-configure phase without a LocalBuildInfo now uses the external method, consistent with how the package was configured.
Also:
Follow-up commits from review: InLibraryArgs is parameterised by what sits in the LocalBuildInfo slot (so the in-library method carries
InLibraryArgs LocalBuildInfo flagsand there is no separate type for it), the generated scripts are named*SetupScript, version references say 3.14 rather than 3.13, and the last commit replacesInLibraryLBIwithMaybe LocalBuildInfo.The PR has been developed with the help of Claude.
Claude-Session: https://claude.ai/code/session_01QTUcSRH5Ew1Q7PP35S8agF
significance: significantin the changelog file.