Remove duplicated constraints from CondTree

[leana8959]

CondTree is defined as follows:

cabal/Cabal-syntax/src/Distribution/Types/CondTree.hs

Lines 57 to 61 in c5c2072

	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).

cabal/Cabal-syntax/src/Distribution/PackageDescription/Parsec.hs

Lines 485 to 496 in c5c2072

	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:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
  • Is the change significant? If so, remember to add significance: significant in the changelog file.
• The documentation has been updated, if necessary.
• Manual QA notes have been included.
• Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

[leana8959]

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)

[Bodigrim]

@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:

cabal/Distribution/Configuration.hs

Lines 221 to 224 in 0dbb6d4

	-- 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]

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

[leana8959]

Hello,

I have tried to reproduce the problem on master. Indeed, changing mkSimpleVersion = C.mkVersion [n] (previously [n, 0, 0]) would break duplicate dependencies and duplicate flagged dependencies tests with the same error. I suppose this is behaviour is not a result of my refactor.

Maybe disabling the trailing zero warning would be a good solution to make the basic space leak test pass again? This check is a bit pedantic in the context of solver memery usage testing.

[Bodigrim]

@leana8959 this needs a changelog entry under changelog.d/.

[geekosaur]

I haven't finished reading the diffs, I need to poke at a CI fix for an upstream dependency changing on us.

[geekosaur]

CI will fail; see #11661, which I'll push in as soon as its CI finishes.

[sebright]

@leana8959 I read more of the code for the solver DSL, and my best guess is that the test failure caused by mkSimpleVersion n = C.mkVersion [n] was due to the DSL code not calling mkSimpleVersion consistently. Here are two other places where version numbers would need to be updated:

cabal/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs

Lines 754 to 759 in ec8052b

	exAvPkgId :: ExampleAvailable -> C.PackageIdentifier
	exAvPkgId ex =
	C.PackageIdentifier
	{ pkgName = C.mkPackageName (exAvName ex)
	, pkgVersion = C.mkVersion [exAvVersion ex, 0, 0]
	}

cabal/cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs

Lines 769 to 774 in ec8052b

	exInstPkgId :: ExampleInstalled -> C.PackageIdentifier
	exInstPkgId ex =
	C.PackageIdentifier
	{ pkgName = C.mkPackageName (exInstName ex)
	, pkgVersion = C.mkVersion [exInstVersion ex, 0, 0]
	}

I prefer changing mkSimpleVersion (if calling it in more places fixes the test failure) over suppressing the trailing zeros warning for all of the solver tests.

[leana8959]

@geekosaur I was able to derive Foldable for CondBranch and the test suite passes for ghc 9.14.1 and ghc 9.2.6, seems ok to me to derive this instance.

@sebright Thanks for the help with the testsuite! I managed to fix all tests without supressing warnings.

[leana8959]

Just force pushed to resolve merge conflict. Would appreciate to have a second reviewer so we could get this merged :)