Duplicate project import as a warning only by philderbeast · Pull Request #10933 · haskell/cabal

philderbeast · 2025-04-21T15:38:53Z

Depends-on #11773.

I'll squash commits before applying the merge label if this pull request is approved.

Pretty much the same as #9933 but gives a warning instead of an error when duplicate imports that are not cyclical are detected. I did this work in Oct 2024 but didn't raise a pull request for it then. I was reporting the warning during parsing but needed an IORef for that. After Matthew Pickering suggesting it, I'm now doing the reporting after parsing.

Add a new module cabal-install/src/Distribution/Client/ProjectConfig/Import.hs and move some stuff there from cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs that was only ever used in cabal-install.
Shorten the haddock sections in Distribution.Client.ProjectConfig.Legacy so that they don't wrap when rendered.

For messages, I added some doctests and ended up with two ways of comparing ProjectConfigPath.

-- | A comparison that puts projects first, URLs last and sorts the other paths
-- lexically.
compareLexically :: ProjectConfigPath -> ProjectConfigPath -> Ordering

-- | A comparison that puts projects first, URLs last and sorts the other paths
-- by putting longer paths after shorter ones as measured by the number of path
-- segments. If still equal, then sorting is lexical.
compareSegmentally:: ProjectConfigPath -> ProjectConfigPath -> Ordering

I changed the project skeleton, retaining the info we have already (because we have to read the file), that the item is a local file or a URI.

- type ProjectConfigSkeleton = CondTree ConfVar ([ProjectConfigPath], ProjectConfig)
+ type ProjectConfigSkeleton = CondTree ConfVar ([(Maybe URI, ProjectConfigPath)], ProjectConfig)

With that information, I'm able to avoid monitoring remote files:

- monitorFiles $ map monitorFileHashed (projectConfigPathRoot <$> projectSkeletonImports pcs)
+ monitorFiles
+  [ monitorFileHashed (projectConfigPathRoot path)
+  | (Nothing, path) <- projectSkeletonImports pcs
+  ]

I added another type to help classify between, root, file import and URI import. I used this to help detect and report duplicates:

-- | Isomorphic with 'ProjectConfigPath' but with separate constructors for the
-- root, imported file and imported URI.
data ProjectNode a where
  ProjectRoot :: FilePath -> ProjectNode ProjectFilePath
  ProjectFileImport :: FilePath -> ProjectConfigPath -> ProjectNode FilePath
  ProjectUriImport :: URI -> ProjectConfigPath -> ProjectNode URI

I developed this with ghc-9.14.1 and required some extra type annotations for prior GHC versions to compile, those in seenImport and instance Ord (ProjectNode a).

I moved the following types and functions to the Import module too. These were either in the Legacy parser or in both parsers.

type ProjectConfigSkeleton = CondTree ConfVar ([(Maybe URI, ProjectConfigPath)], ProjectConfig)

projectSkeletonImports :: ProjectConfigSkeleton -> [(Maybe URI, ProjectConfigPath)]
projectSkeletonImports = fst . view traverseCondTreeA

fetchImportConfig
  :: FilePath
  -> HttpTransport
  -> Verbosity
  -> FilePath
  -> ProjectConfigPath
  -> IO (Maybe URI, BS.ByteString)

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

ulysses4ever

Thanks!

philderbeast · 2025-08-12T00:47:01Z

I've updated the tests and moved them to cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.test.hs, fixing them for Windows and for the changes in the project parser.

philderbeast · 2025-08-12T01:55:35Z

This pull request is complete for the --project-file-parser=legacy. I'd prefer to add this feature to the parsec parser on a separate pull request.

mpickering · 2025-08-12T15:42:44Z

Is there an issue which describe the problem or feature this PR is adding?

mpickering · 2025-08-12T15:43:52Z

This pull request is complete for the --project-file-parser=legacy. I'd prefer to add this feature to the parsec parser on a separate pull request.

In the next + 1 release the legacy parser will be removed, so I would advise implementing the feature firstly for the parsec parser.

philderbeast · 2025-08-12T15:58:44Z

Is there an issue which describe the problem or feature this PR is adding?

From the description in this pull request:

Pretty much the same as #9933

From the description of #9933:

A follow on from #9578.

From the description of #9578:

Fixes #9562.

Before I started working on this stuff, there were some tests for cyclical imports but none for duplicates without cycles. This pull request adds these tests and reporting those duplicates as a warning when we'd punted on including this reporting as an error in an earlier pull request in the chain.

philderbeast · 2026-04-23T14:51:22Z

@ulysses4ever are you able to take another look at this?

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR changes Cabal’s handling of duplicate (non-cyclical) project imports by reporting them as warnings (with more detailed duplicate-path information) rather than errors, and refactors import-related logic into a dedicated Import module.

Changes:

Add Distribution.Client.ProjectConfig.Import (migrating import logic/messaging previously living in cabal-install-solver and parsers).
Update legacy + parsec project parsers and project file monitoring to track whether an import is a local file vs a URI, avoiding monitoring remote imports.
Add/adjust tests and golden outputs to validate duplicate-import warnings across parser modes and script/project parsing.

Reviewed changes

Copilot reviewed 40 out of 51 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
changelog.d/pr-9933	Adds a changelog entry documenting duplicate import detection/reporting changes.
cabal.validate.project	Drops `+legacy-comparison` from validation config (reduces comparison-mode coverage in this project file).
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/yops-0.script.hs	New script-based fixture for duplicate import warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops/woops-9.config	New config fixture (imports stackage config).
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops/woops-7.config	New config fixture for duplicate import paths.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops/woops-5.config	New config fixture for duplicate import paths.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops/woops-3.config	New config fixture for duplicate import paths.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops/woops-1.config	New config fixture for duplicate import paths.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops-8.config	New config fixture for duplicate import paths.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops-6.config	New config fixture for duplicate import paths.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops-4.config	New config fixture for duplicate import paths.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops-2.config	New config fixture for duplicate import paths.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops-0.script.hs	New script-based fixture for duplicate import warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/woops-0.project	New project-based fixture for duplicate import warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/foo.cabal	New minimal package for the UniquePathDuplicates test project.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.test.hs	Adds coverage asserting duplicate-import warnings for multiple parsers and scripts/projects.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.script-parser-parsec.out	Golden output for script + parsec parser warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.script-parser-legacy.out	Golden output for script + legacy parser warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.script-parser-fallback.out	Golden output for script + fallback parser warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.script-parser-default.out	Golden output for script + default parser warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.parser-parsec.out	Golden output for project + parsec parser warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.parser-legacy.out	Golden output for project + legacy parser warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.parser-fallback.out	Golden output for project + fallback parser warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/cabal.parser-default.out	Golden output for project + default parser warnings.
cabal-testsuite/PackageTests/ProjectImport/UniquePathDuplicates/Foo.hs	New module for the UniquePathDuplicates package.
cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromSimple/cabal.test.hs	Marks test flaky on an additional CI issue id.
cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromSimple/cabal.out	Updates golden to include duplicate-import warnings.
cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromComplex/cabal.out	Updates golden to include duplicate-import warnings (incl. URI imports).
cabal-testsuite/PackageTests/ConditionalAndImport/hops.expect.txt	Updates expected “configuration is affected by” listing ordering/format.
cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs	Removes prior assertion that duplicate imports via different paths are not warned about.
cabal-testsuite/PackageTests/ConditionalAndImport/cabal.out	Removes corresponding golden output lines for the removed assertion.
cabal-install/src/Distribution/Client/ScriptUtils.hs	Reports duplicate-import warnings for script parsing results.
cabal-install/src/Distribution/Client/ProjectPlanning.hs	Imports new Import module (supports new behavior path).
cabal-install/src/Distribution/Client/ProjectConfig/Parsec.hs	Moves import-fetching + skeleton tracking to Import module; adjusts types to include URI/export changes.
cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs	Same as Parsec: uses Import module, adjusts skeleton type to retain URI classification, and updates parse signatures.
cabal-install/src/Distribution/Client/ProjectConfig/Import.hs	New module implementing import fetching + duplicate detection/reporting + messages.
cabal-install/src/Distribution/Client/ProjectConfig.hs	Avoids monitoring remote files; reports duplicate warnings after parsing for legacy/parsec paths; updates parse signatures.
cabal-install/cabal-install.cabal	Exposes the new `Distribution.Client.ProjectConfig.Import` module.
cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs	Moves message helpers out; adds `ProjectFilePath` and two explicit ordering functions (`compareLexically`, `compareSegmentally`).
.typos-srcs.toml	Excludes a new test fixture path from typos scanning.

Comments suppressed due to low confidence (1)

changelog.d/pr-9933:1

The PR behavior described in the title/description is specifically “warning only” for non-cyclical duplicates, but the changelog synopsis/description doesn’t mention that this is a warning (vs an error). Consider updating the synopsis/description to explicitly state that Cabal now emits warnings for these duplicates.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ulysses4ever · 2026-04-23T15:42:14Z

@philderbeast thank you for the ping, I will see what I can do. First thing that jumps at me is the size of the PR. Do you think there's any chance to split it up a bit?..

philderbeast · 2026-04-23T15:53:05Z

First thing that jumps at me is the size of the PR.

It looks bigger than it is. I moved some stuff from cabal-install-solver to cabal-install with commit 6fe4b87.

philderbeast · 2026-04-23T15:57:33Z

Do you think there's any chance to split it up a bit?..

This is the last of the work started with #9578 in Dec 2023, some of which was split. I'm hoping this is the last of that batch.

ulysses4ever · 2026-04-23T20:06:23Z

do we really ned to store a 3KLOC file in our repo for tests?

I think we do, the parsing time is real and measurable, especially when an import is duplicated, as this pull request is warning against.

the parsing time is real and measurable

this is exactly the reason i think we shouldn't have such big files in regression tests: what's the point of doing otherwise exactly? our test suite only checks correctness for the most part (there are specific parts that test performance, but PackageTests, i.e., integration tests, isn't that part).

ulysses4ever · 2026-04-23T20:09:07Z

+    normSep p =
+        if buildOS == Windows
+            then
+                Windows.joinPath $ Windows.splitDirectories


could save on indentation a lot if used guards...

I've reformatted for less indent but not used guards. This code snippet was moved, lambda lifted, but I didn't change it.

I'm surprised that this package is still in the styling TODO list:

cabal/Makefile

Line 47 in f444d1b

cabal-install-solver \

I enabled fourmolu for cabal-install-solver to see how it would reformat that code.

splitPath :: FilePath -> [FilePath] splitPath = FP.splitPath . normSep where normSep p = if buildOS == Windows then Windows.joinPath $ Windows.splitDirectories [if Posix.isPathSeparator c then Windows.pathSeparator else c | c <- p] else Posix.joinPath $ Posix.splitDirectories [if Windows.isPathSeparator c then Posix.pathSeparator else c | c <- p]

There's one less indent with guards but the extra indent with the if is due to fourmolu putting then and else indented on new lines.

splitPath :: FilePath -> [FilePath] splitPath = FP.splitPath . normSep where normSep p | Windows <- buildOS = Windows.joinPath $ Windows.splitDirectories [if Posix.isPathSeparator c then Windows.pathSeparator else c | c <- p] | otherwise = Posix.joinPath $ Posix.splitDirectories [if Windows.isPathSeparator c then Posix.pathSeparator else c | c <- p]

Indentation itself isn't so much of a problem imo. My point is that guards look more idiomatic haskell.

- Add Y-forking import test - A test for detecting when the same config is imported via many different paths - Error on duplicate imports - Do the filtering in duplicateImportMsg - Use duplicateImportMsg for cycles too - Add haddocks to IORef parameter - Add changelog entry - Use ordNub instead of nub - Use NubList - Share implement of duplicate and cyclical messages - Update expectation for non-cyclical duplicate import - Only show a warning - Add woops project with a time cost - Use noticeDoc instead of warn - Render duplicate imports - Add Ord instance for Dupes, sort on dupesNormLocPath - Fixups after rebase - Satisfy hlint - Remove -XMultiWayIf - Remove mention of yops from the changelog - Satisfy fix-whitespace - Test with a time cost of duplicate imports - Fewer imports from PrettyPrint qualified as Disp - Add data ProjectImport replacing tuples - Combine fields as ProjectImport - Rename field to dupesImports - Add haddocks to Dupes fields - Mark test as flaky - Any test accessing stackage seems susceptible - Move unique duplicates to own test - Use legacy parser for path duplicates test - Add foo.cabal package so that packages exist - Satisfy fix-whitespace - Use local version of lts-21.25 - Remove repo - Use </> for expected paths - Note that this change gives a warning. - Exclude lts-21.25.config from typos - Detect duplicate imports after parsing - Merge upstream - Update duplicate import counts - Satisfy fourmolu - Move duplicate detection to calling module - Add parsing module haddock section header - Export only reportDuplicateImports - Test all project parser options - Add script tests - Add haddocks to reportDuplicateImportsh - Use Data.Function ((&)) pipelining - Inline do blockh - Satisfy fix-whitespace - Redo Ord Dupes - Try to enforce sorting - Add docs to ProjectImport - Add instances & doctests for sorting ProjectImport - Fix sorting of ProjectConfigPath - Update test expectation with correct sorting - Compare versions of compare - Update cabal.out for dedup tests - Rename to compareForDisplay - Add more detail to docProjectConfigFiles docs - Add haddocks and rename comparisons - Don't repeat the root comment - Pattern match on NonEmpty not List - Sort expectation by import chain length

- Satisfy hlint, remove unused LANGUAGE pragmas - Bubble up Maybe URI - Add ProjectRoot constructorh - Rename ProjectImport to ProjectNode - Add Maybe URI to project skeleton - Use ProjectFilePath - Satisfy hlint - Add projectNodes - Redo detectDupes - Move duplicate imports to Import module - Move fetchImportConfig to Import module - Don't export parseProjectSkeleton - Don't re-export ProjectConfigSkeleton - Remove unnecessary primes - Add splitImports - Remove ProjectNodes and projectNodes - Move stuff to Client.ProjectConfig.Import - Fixup after rebase - Satisfy fourmolu

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from 16a11bd to 95ec86a Compare April 21, 2025 15:41

geekosaur approved these changes Apr 21, 2025

View reviewed changes

ulysses4ever approved these changes Apr 21, 2025

View reviewed changes

Comment thread cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs Outdated

Comment thread cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs Outdated

Comment thread cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs Outdated

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 3 times, most recently from c230d20 to b61e031 Compare April 27, 2025 00:52

sebright added the re: project-file Concerning cabal.project files label May 8, 2025

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from a926d99 to ec9ea13 Compare July 6, 2025 19:58

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from ec9ea13 to 7ab2b20 Compare July 15, 2025 14:19

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 2 times, most recently from d5552f3 to 0c99ae5 Compare July 25, 2025 12:25

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 3 times, most recently from 7fe8c13 to 61d8a74 Compare August 11, 2025 15:10

philderbeast mentioned this pull request Aug 11, 2025

Detect duplicate non-cyclical project imports #9933

Closed

5 tasks

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 2 times, most recently from fd40a31 to 814b84c Compare August 11, 2025 19:29

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from 65bba25 to 3d032f3 Compare August 12, 2025 00:51

philderbeast mentioned this pull request Aug 12, 2025

Difference in --project-file-parser=compare #11152

Closed

mpickering reviewed Aug 13, 2025

View reviewed changes

Comment thread cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs Outdated

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 2 times, most recently from 3021e83 to bd7c594 Compare August 14, 2025 13:28

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch 2 times, most recently from 557c9b3 to f62f929 Compare August 27, 2025 13:09

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from f62f929 to 90cafdb Compare March 12, 2026 12:59

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from 2f8e4be to 329734e Compare April 23, 2026 14:50

ulysses4ever requested a review from Copilot April 23, 2026 14:54

Copilot started reviewing on behalf of ulysses4ever April 23, 2026 15:03 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

Comment thread cabal-install/src/Distribution/Client/ProjectConfig/Import.hs

Comment thread cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs

haskell deleted a comment from Copilot AI Apr 23, 2026

ulysses4ever reviewed Apr 23, 2026

View reviewed changes

Comment thread changelog.d/pr-9933

ulysses4ever reviewed Apr 23, 2026

View reviewed changes

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from 0bba984 to f7b2de5 Compare April 24, 2026 00:54

philderbeast added 12 commits April 29, 2026 19:38

Don't monitor (Nothing :: Maybe URI)

f9a6f0a

Add type annotation for older ghc versions

164726b

Setup for doctests

bad2090

Need -XConstraintKinds with older ghcs

7977677

Update .out files for UniquePathDuplicates

8fc7288

Don't repeat recordMode

998983c

Remove +legacy-comparison

d1fc303

Note significance in changelog and movements

efe0ed1

Less indent

761f330

Remove redundant comparison

f237576

philderbeast force-pushed the fix/y-fork-project-import-warning-only branch from d43ac3b to f237576 Compare April 29, 2026 23:38

philderbeast mentioned this pull request Apr 30, 2026

Refactor/before import warning #11773

Open

6 tasks

Conversation

philderbeast commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulysses4ever left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philderbeast commented Aug 12, 2025

Uh oh!

philderbeast commented Aug 12, 2025

Uh oh!

mpickering commented Aug 12, 2025

Uh oh!

mpickering commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philderbeast commented Aug 12, 2025

Uh oh!

Uh oh!

philderbeast commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

ulysses4ever commented Apr 23, 2026

Uh oh!

philderbeast commented Apr 23, 2026

Uh oh!

philderbeast commented Apr 23, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

philderbeast commented Apr 21, 2025 •

edited

Loading

mpickering commented Aug 12, 2025 •

edited

Loading