Reimplement cabal check (#8427)

* Fix Semigroup target instance

When two target names are the same, `mappend`ing them should not
error but just pick the first name.

* Add `desugarBuildToolSimple`

* Reimplement cabal check

* Reorder test output

* Fix autogen modules tests .cabal files

* Add a number of tests

* Add test for #7423

i.e. Do not warn on -O2 if under off-by-default package configuration
flag conditional.

* Add a regression for:

    * Add another -WErrr test
        This is to make sure we do *not* report it if it is under
        a user, off-by-default flag.
    * Add test for non manual user flags.
    * Add “absolute path in extra-lib-dirs” test
    * Add if/else test
    * Add “dircheck on abspath” check
    * Add Package version internal test
    * Add PackageVersionsStraddle test

* Add changelog for #8427

* Integrate various reviews

* Integrate Artem’s review

(review) Clarify `combineNames` documentation

By explaining the way it operates (working if the two names are equal
or one is empty) and renaming the function from `combineName` to
`combineNames`.

(review) Use guards instead of if/then/else

(review) Match inside argument list

(review) Replace “white” with “allow”

(review) Fix typo in comment

(review) Fix typo in Check module documentation

(review) Harmonise indentation for `data` decls

First field goes in a new line than the data constructor, so we
have more space.

(review) Rename `Prim` module to `Types`

(review) Add checkPackageFilesGPD

`checkPackageFiles` — which works on PD — was used to perform IO. We
introduce a function that does the same thing but works on GPD (which
is more principled).

`checkPackageFiles` cannot just be removed, since it is part of the
interface of Distribution.PackageDescription.Check. Deprecation can
be planned once “new check” is up and running.

* Integrate Andreas’ review

(review) Add named section to missing upper bound check

“miss upper bound” checks will now list target type and name (“On
executable 'myexe', these packages miss upper bounds”) for easier
fixing by the user.

(review) remove `cabal gen-bounds` suggestion

Reasonable as `cabal gen-bounds` is stricter than `cabal check`, see
#8427 (comment)
Once `gen-bounds` behaves in line with `check` we can readd the
suggestion.

(review) Do not warn on shared bounds

When a target which depends on an internal library shares some
dependencies with the latter, do not warn on upper bounds.

An example is clearer

    library
     build-depends: text < 5
    ⁝
     build-depends: myPackage,        ← no warning, internal
                    text,             ← no warning, shared bound
                    monadacme         ← warning!

* Integrate Artem’s review /II

(review) Split Check.hs

Check.hs has been split in multiple file, each une sub 1000 lines:

Check              857 lines
Check.Common       147 lines
Check.Conditional  204 lines
Check.Monad        352 lines
Check.Paths        387 lines
Check.Target       765 lines
Check.Warning      865 lines

Migration guide:
- Check              GPD/PD checks plus work-tree checks.
- Check.Common       common types and functions that are
                     *not* part of monadic checking setup.
- Check.Conditional  checks on CondTree and related matter
                     (variables, duplicate modules).
- Check.Monad        Backbone of the checks, monadic inter-
                     face and related functions.
- Check.Paths        Checks on files, directories, globs.
- Check.Target       Checks on realised targets (libraries,
                     executables, benchmarks, testsuites).
- Check.Warning      Datatypes and strings for warnings
                     and severities.

(review) remove useless section header

(review) Fix typo

(review) Add warnings documentation (list)

For each warning, we document constructor/brief description
in the manual.  This might not be much useful as not but it
will come handy when introducing `--ignore=WARN` and similar
flags.

* (review Andreas) Clarify CheckExplanation comment

Whoever modifies `CheckExplanation` data constructors needs to be
aware that the documentation in  doc/cabal-commands.rst  has to be
updated too.

* Move internal Check modules to `other-modules`

No need to expose Distribution.PackageDescription.Check.*
to the world. API for checking, for cabal-install and other
tools, should be in Distribution.PackageDescription.Check.

* Make fourmolu happy

Cabal codebase has now a formatter/style standard (see #8950).

“Ravioli ravioli, give me the formuoli”

* Do not check for OptO in scripts

See #8963 for reason and clarification requests.

* Remove useless PackageId parameter

It is now in the Reader part of CheckM monad.

* Do not check PVP on internal targets

Internal: testsuite, benchmark.
See #8361.

* Make hlint happy

* Fix #9122

When checking internal version ranges, we need to make sure we
are not mistaking a libraries with the same name but from different
packages. See #9132.

* Fix grammar

neither…nor, completing what done in #9162

* Integrate Brandon’s review: grammar

* Remove unnecessary `-fvia-C` check

Brandon’s review/II.

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Author: 2 people

Cabal-syntax/src/Distribution/Types/Benchmark.hs

@@ -48,24 +48,12 @@ instance Monoid Benchmark where
 instance Semigroup Benchmark where
   a <> b =
     Benchmark
-      { benchmarkName = combine' benchmarkName
+      { benchmarkName = combineNames a b benchmarkName "benchmark"
       , benchmarkInterface = combine benchmarkInterface
       , benchmarkBuildInfo = combine benchmarkBuildInfo
       }
     where
       combine field = field a `mappend` field b
-      combine' field = case ( unUnqualComponentName $ field a
-                            , unUnqualComponentName $ field b
-                            ) of
-        ("", _) -> field b
-        (_, "") -> field a
-        (x, y) ->
-          error $
-            "Ambiguous values for test field: '"
-              ++ x
-              ++ "' and '"
-              ++ y
-              ++ "'"
 
 emptyBenchmark :: Benchmark
 emptyBenchmark = mempty

Cabal-syntax/src/Distribution/Types/Executable.hs

@@ -40,25 +40,13 @@ instance Monoid Executable where
 instance Semigroup Executable where
   a <> b =
     Executable
-      { exeName = combine' exeName
+      { exeName = combineNames a b exeName "executable"
       , modulePath = combine modulePath
       , exeScope = combine exeScope
       , buildInfo = combine buildInfo
       }
     where
       combine field = field a `mappend` field b
-      combine' field = case ( unUnqualComponentName $ field a
-                            , unUnqualComponentName $ field b
-                            ) of
-        ("", _) -> field b
-        (_, "") -> field a
-        (x, y) ->
-          error $
-            "Ambiguous values for executable field: '"
-              ++ x
-              ++ "' and '"
-              ++ y
-              ++ "'"
 
 emptyExecutable :: Executable
 emptyExecutable = mempty

Cabal-syntax/src/Distribution/Types/ForeignLib.hs

@@ -140,7 +140,7 @@ instance NFData ForeignLib where rnf = genericRnf
 instance Semigroup ForeignLib where
   a <> b =
     ForeignLib
-      { foreignLibName = combine' foreignLibName
+      { foreignLibName = combineNames a b foreignLibName "foreign library"
       , foreignLibType = combine foreignLibType
       , foreignLibOptions = combine foreignLibOptions
       , foreignLibBuildInfo = combine foreignLibBuildInfo
@@ -150,18 +150,6 @@ instance Semigroup ForeignLib where
       }
     where
       combine field = field a `mappend` field b
-      combine' field = case ( unUnqualComponentName $ field a
-                            , unUnqualComponentName $ field b
-                            ) of
-        ("", _) -> field b
-        (_, "") -> field a
-        (x, y) ->
-          error $
-            "Ambiguous values for executable field: '"
-              ++ x
-              ++ "' and '"
-              ++ y
-              ++ "'"
       combine'' field = field b
 
 instance Monoid ForeignLib where

Cabal-syntax/src/Distribution/Types/TestSuite.hs

@@ -51,25 +51,13 @@ instance Monoid TestSuite where
 instance Semigroup TestSuite where
   a <> b =
     TestSuite
-      { testName = combine' testName
+      { testName = combineNames a b testName "test"
       , testInterface = combine testInterface
       , testBuildInfo = combine testBuildInfo
       , testCodeGenerators = combine testCodeGenerators
       }
     where
       combine field = field a `mappend` field b
-      combine' field = case ( unUnqualComponentName $ field a
-                            , unUnqualComponentName $ field b
-                            ) of
-        ("", _) -> field b
-        (_, "") -> field a
-        (x, y) ->
-          error $
-            "Ambiguous values for test field: '"
-              ++ x
-              ++ "' and '"
-              ++ y
-              ++ "'"
 
 emptyTestSuite :: TestSuite
 emptyTestSuite = mempty

Cabal-syntax/src/Distribution/Types/UnqualComponentName.hs

@@ -9,11 +9,12 @@ module Distribution.Types.UnqualComponentName
   , mkUnqualComponentName
   , packageNameToUnqualComponentName
   , unqualComponentNameToPackageName
+  , combineNames
   ) where
 
 import Distribution.Compat.Prelude
 import Distribution.Utils.ShortText
-import Prelude ()
+import Prelude as P (null)
 
 import Distribution.Parsec
 import Distribution.Pretty
@@ -105,3 +106,33 @@ packageNameToUnqualComponentName = UnqualComponentName . unPackageNameST
 -- @since 2.0.0.2
 unqualComponentNameToPackageName :: UnqualComponentName -> PackageName
 unqualComponentNameToPackageName = mkPackageNameST . unUnqualComponentNameST
+
+-- | Combine names in targets if one name is empty or both names are equal
+-- (partial function).
+-- Useful in 'Semigroup' and similar instances.
+combineNames
+  :: a
+  -> a
+  -> (a -> UnqualComponentName)
+  -> String
+  -> UnqualComponentName
+combineNames a b tacc tt
+  -- One empty or the same.
+  | P.null unb
+      || una == unb =
+      na
+  | P.null una = nb
+  -- Both non-empty, different.
+  | otherwise =
+      error $
+        "Ambiguous values for "
+          ++ tt
+          ++ " field: '"
+          ++ una
+          ++ "' and '"
+          ++ unb
+          ++ "'"
+  where
+    (na, nb) = (tacc a, tacc b)
+    una = unUnqualComponentName na
+    unb = unUnqualComponentName nb

Cabal-tests/tests/CheckTests.hs

@@ -71,7 +71,7 @@ checkTest fp = cabalGoldenTest fp correct $ do
             -- Note: parser warnings are reported by `cabal check`, but not by
             -- D.PD.Check functionality.
             unlines (map (showPWarning fp) ws) ++
-            unlines (map show (checkPackage gpd Nothing))
+            unlines (map show (checkPackage gpd))
         Left (_, errs) -> unlines $ map (("ERROR: " ++) . showPError fp) $ NE.toList errs
   where
     input = "tests" </> "ParserTests" </> "regressions" </> fp

Cabal-tests/tests/HackageTests.hs

@@ -196,7 +196,7 @@ parseCheckTest fpath bs = do
                              Parsec.parseGenericPackageDescription bs
     case parsec of
         Right gpd -> do
-            let checks = checkPackage gpd Nothing
+            let checks = checkPackage gpd
             let w [] = 0
                 w _  = 1
 

Cabal-tests/tests/ParserTests/regressions/all-upper-bound.check

@@ -1,6 +1,6 @@
-These packages miss upper bounds:
+On library, these packages miss upper bounds:
+  - somelib
   - alphalib
   - betalib
   - deltalib
-  - somelib
-Please add them, using `cabal gen-bounds` for suggestions. For more information see:  https://pvp.haskell.org/
+Please add them. There is more information at https://pvp.haskell.org/

Cabal-tests/tests/ParserTests/regressions/bad-glob-syntax.check

@@ -1,2 +1,2 @@
-In the 'extra-source-files' field: invalid file glob 'foo/blah-*.hs'. Wildcards '*' may only totally replace the file's base name, not only parts of it.
 In the 'extra-source-files' field: invalid file glob 'foo/*/bar'. A wildcard '**' is only allowed as the final parent directory. Stars must not otherwise appear in the parent directories.
+In the 'extra-source-files' field: invalid file glob 'foo/blah-*.hs'. Wildcards '*' may only totally replace the file's base name, not only parts of it.

Cabal-tests/tests/ParserTests/regressions/decreasing-indentation.cabal

@@ -24,7 +24,7 @@ Flag UseBinary
   Description: Use the binary package for serializing keys.
 
 Library
- build-depends: base >= 3
+ build-depends: base < 3
  if flag(UseBinary)
    build-depends: binary <10
    CPP-Options: -DUSE_BINARY
@@ -34,7 +34,7 @@ Library
  exposed-modules: Codec.Crypto.RSA
 
 Executable test_rsa
-    build-depends: base >= 3
+    build-depends: base < 3
   CPP-Options: -DRSA_TEST
   Main-Is: Test.hs
   Other-Modules: Codec.Crypto.RSA
@@ -52,7 +52,7 @@ Executable warnings
 
 -- Increasing indentation is also possible if we use braces to delimit field contents.
 Executable warnings2
-  build-depends: { base <5 }
+  build-depends: { base < 5 }
     main-is: { warnings2.hs }
       Other-Modules: FooBar
 
@@ -62,9 +62,9 @@ flag splitBase
 
 Executable warnings3
     if flag(splitBase)
-        build-depends: base >= 3
+        build-depends: base < 3
     else
-        build-depends: base <  3
+        build-depends: base < 5
 
  Main-Is: warnings3.hs
  Other-Modules: