Skip to content

Remove duplicated constraints from CondTree#11626

Open
leana8959 wants to merge 1 commit intohaskell:masterfrom
leana8959:remove-duplicated-constraints
Open

Remove duplicated constraints from CondTree#11626
leana8959 wants to merge 1 commit intohaskell:masterfrom
leana8959:remove-duplicated-constraints

Conversation

@leana8959
Copy link
Collaborator

CondTree is defined as follows:

data CondTree v c a = CondNode
{ condTreeData :: a
, condTreeConstraints :: c
, condTreeComponents :: [CondBranch v c a]
}

CondTree is often instantiated with a being a component that has BuildInfo and c as [Dependency]. The [Dependency] is derived from the BuildInfo during construction (see the last line).

parseCondTree v hasElif grammar commonStanzas fromBuildInfo cond = go
where
go fields0 = do
(fields, endo) <-
if v >= CabalSpecV3_0
then processImports v fromBuildInfo commonStanzas fields0
else traverse (warnImport v) fields0 >>= \fields1 -> return (catMaybes fields1, id)
let (fs, ss) = partitionFields fields
x <- parseFieldGrammar v fs grammar
branches <- concat <$> traverse parseIfs ss
return $ endo $ CondNode x (cond x) branches

The accessors are exposed, this duplication of [Dependency] can cause the data to be inconsistent.

Cabal exact print aims to allow modifications to the GPD. Not having a single source of truth can also confuse programmers using GPD as an API to exact print.

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

@leana8959
Copy link
Collaborator Author

leana8959 commented Mar 16, 2026

cabal-install:mem-use-tests module's "basic space leak test" fails due to the dummy packages having bounds with trailing zeros.

Removing these trailing zeros causes the test "duplicate dependencies" and "duplicate flagged dependencies" to fail.

Maybe I uncovered a bug in the test suite with this PR?

When mkSimpleVersion n = C.mkVersion [n]:

space leak test works, duplicated flag tests don't.

$ cabal test cabal-install:mem-use-tests
Test suite mem-use-tests: RUNNING...
Memory Usage
  UnitTests.Distribution.Solver.Modular.MemoryUsage
    basic space leak test:          OK (6.66s)
    package with many flags:        OK (2.03s)
    issue #2899:                    OK (1.39s)
    duplicate dependencies:         FAIL
      Unexpected error:
      Could not resolve dependencies:
      [__0] trying: A-1.0.0 (user goal)
      [__1] next goal: B (dependency of A +/-flag-1 +/-flag-10 +/-flag-11 +/-flag-12 +/-flag-13 +/-flag-14 +/-flag-15 +/-flag-16 +/-flag-17 +/-flag-18 +/-flag-19 +/-flag-2 +/-
      ... elided
    duplicate flagged dependencies: FAIL (0.05s)

When mkSimpleVersion n = C.mkVersion [n, 0, 0]

space leak test trips gpd check due to trailing zeros. duplicated flag tests work.

$ cabal test cabal-install:mem-use-tests
Test suite mem-use-tests: RUNNING...
Memory Usage
  UnitTests.Distribution.Solver.Modular.MemoryUsage
    basic space leak test:          FAIL
      Exception: invalid GenericPackageDescription for package pkg-1-2.0.0: [[tz-upper-bounds] On library, these packages have upper bounds with trailing zeros:
        - pkg-2
      Please avoid trailing zeros for upper bounds.]
      Use -p '/basic space leak test/' to rerun this test only.
    package with many flags:        OK (2.01s)
    issue #2899:                    OK (1.39s)
    duplicate dependencies:         OK (0.01s)
    duplicate flagged dependencies: OK (0.08s)

@leana8959 leana8959 force-pushed the remove-duplicated-constraints branch from ff16a51 to 815d6c2 Compare March 16, 2026 17:07
@Bodigrim
Copy link
Collaborator

@leana8959 I suggest you squash commits and mark as ready for review, if you want someone's feedback. People often perceive "drafts" as private experiments, not asking for immediate feedback.


Essentially the PR removes condTreeConstraints from CondTree, which was apparently pretty much unused. I tried to trace back why it was introduced in the first place. I arrived back to 2007:

-- Each path is annotated with a number of constraints, which must be
-- fulfilled and a modifier, describing which information to add to the final
-- value. We record constraints and data separately, to allow possible
-- optimizations later on.

Almost twenty years after the "possible optimizations later on" never materialized :) So high time to clean it up.

@leana8959 leana8959 force-pushed the remove-duplicated-constraints branch from 815d6c2 to 48f0848 Compare March 16, 2026 22:52
@leana8959 leana8959 force-pushed the remove-duplicated-constraints branch from 48f0848 to 947f3bc Compare March 16, 2026 22:53
@leana8959
Copy link
Collaborator Author

@Bodigrim You're right, I rebased and fixed the formatting. Glad to know that this refactor is heading in the right direction!

@leana8959 leana8959 marked this pull request as ready for review March 16, 2026 22:54
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.

2 participants