Reimplement `cabal check`

[ffaf1]

Reimplementing cabal check, closes #8211.

For a detailed description of the problem at hand, the proposed solution, benefits and future work check the explanatory article.

Here I will only highlight bits to ease reviews. In brief:

• cabal check goes from “make a soup with flattenPackageDescription, check it” to “start from GenericPackageDescription and walk down, perform the appropriate checks near the relevant datatype”.
• Same goes for conditional blocks: the various ifs inside a .cabal are merged in a single ball of mud no more. Rather we visit the CondTree in a principled manner, building the target from slices and then validating it in its appropriate context.
• Checks happen inside CheckM m, a wrapped RSTW transformer where m abstracts over some IO/.tar archive/VCS filesystem.
• the only API changes are one function removal (function which is not used nor in this repo, nor in hackage-server, nor in any haskell repo, nor in stack) and a signature change for checkPackage, eliminating a useless parameter. I tackled the rewrite with the idea to minimise disruption.

To the reviewer: a good way of approaching this is to to avoid the two “large diffs” files at first. Check first how other modules were impacted and the two preliminary patches. Then you can dive in PackageDescription/Check.hs:

-- ☞ N.B.
--
-- Part of the tools/scaffold used to perform check is found in
-- Distribution.PackageDescription.Check.Prim. Summary of that module (for
-- how we use it here):
-- 1. we work inside a 'Check m a' monad (where `m` is an abstraction to
--    run non-pure checks);
-- 2. 'checkP', 'checkPre' functions perform checks (respectively pure and
--    non-pure);
-- 3. 'PackageCheck' and 'CheckExplanation' are types for warning severity
--    and description.


-- ------------------------------------------------------------
-- * Checking interface
-- ------------------------------------------------------------

-- | 'checkPackagePrim' is the most general way to invoke package checks.
-- We pass to it two interfaces (one to check contents of packages, the
-- other to inspect working tree for orphan files) and before that a
-- Boolean to indicate whether we want pure checks or not. Based on these
-- parameters, some checks will be performed, some omitted.
-- Generality over @m@ means we could do non pure checks in monads other
-- than IO (e.g. a virtual filesystem, like a zip file, a VCS filesystem,
-- etc).
checkPackagePrim :: Monad m => Bool -> Maybe (CheckPackageContentOps m) ->
                    Maybe (CheckPreDistributionOps m) ->
                    GenericPackageDescription -> m [PackageCheck]
⁝

and you should find your way.

Testsuite added in #8248 and #8250 passes. As a proof of what can be done after the rewrite, this PR also:

• closes #7423.

If not merged by the time this is approved, #8441 and #8361 might need to be updated.

This patch is big, the logic of checks sometimes tricky, I expect breakage. I took care to minimise API change, this should make bugs pop up faster and allow me to fix them faster. Once everything is stable, features can be accomodated appropriately.

• [x] Patches conform to the coding conventions.
• [x] Any changes that could be relevant to users have been recorded in the changelog.
• [x] The documentation has been updated, if necessary.

[jappeace]

no need to wait for me, I'm good at rebasing :wink:

[Mikolaj]

@andreasabel: hi! Would you like one us to review this before your review, or should we stick to the plan and review after? No pressure. ;)

[ulysses4ever]

Mikolaj wrote above:

@andreasabel: hi! Would you like one us to review this before your review, or should we stick to the plan and review after? No pressure. ;)

Just a gentle ping, @andreasabel.

[ffaf1]

With #8361 merged I need to rebase this, please hold on.

[ffaf1]

Note to self, this is the work needed to get it merged:

• [x] rebase against #8441
• [x] rebase against #8657
• [x] rebase against #8651
• [x] rebase against #8361
• [x] rebase against #8534
• [x] rebase against #8747
• [x] Data structures (each constructor/field) in Prim.hs should all be commented.
• [x] non-typed (Bool, Int) function argument again should be commented.
• [x] checkBuildInfo should be split.
• [x] rename TarAnn to something else.
• [x] Switch from RWS to a RW transformer.
• [x] develop an automated fetch test to compare old and new cabal check behaviour (see if hackage-cli can be of help?).
• [x] decide whether we actually want to check for upper bounds on benchmarks and tests

[ffaf1]

After rebase, patch is ready for review. If any part of the code is unclear, I will add more comments.

---

While this is getting reviewed, I plan to write a simple program that:

1. downloads a random package (and a random version of that package) from Hackage;
2. runs it through cabal check, both old and new version;
3. checks if Warning: Hackage would reject this package. is present in one but not in the other.

Do that a thousand times and we can be positive the reimplementation will not break Hackage, checking for both type I and type II errors.

I have never used Hackage API, if anyone has suggestions on programs I could cannibalise, I would be glad.

[ffaf1]

@mergify rebase

[mergify[bot]]

rebase

✅ Nothing to do for rebase action

[ffaf1]

1000 tests to run.
Testing pandoc-plantuml-diagrams-0.1.0.3… ok
Testing asn1-data-0.2.2… ok
Testing gps2htmlReport-0.1… ok
Testing fsmActions-0.3.0… ok
Testing lp-diagrams-svg-1.1… ok
Testing cake3-0.3.0.0… ok
Testing prizm-2.0.1… ok
Testing hprotoc-1.7.0… ok
Testing Graph500-0.4.0… ok
Testing cascading-0.1.0… ok
Testing arbor-monad-logger-0.1.1.3… ok
⁝

Today I wrote an autotester which fetches a package from Hackage, runs it through both versions cabal check, reports if Warning: Hackage would reject this package. is present in one but not the other.

This managed to catch a few bugs, which I fixed and wrote tests for.

There are still inconsistencies on one of the checks, I plan to fix them tomorrow.

[ffaf1]

Done. I am confident the accept/refuse machanism works as intended.

[ulysses4ever]

Is there a human-readable directory of warnings that the new check can produce?

Similarly, but perhaps less important: you say some warnings had to be tried for various reasons (e.g. now they're caught by the parser): is there a directory of those?

[ffaf1]

The parse caught-warnings are marked with TODOs in Check.hs. They are: suspicious flag name, package without name:, package without version:, signatures: with cabal < 2.0, unknown/unsupported testsuite. I did not remove them (yet) because I want to be sure that is true for every spec version (parser is pretty hairy in some places).

Regarding the first question (“Is there a human-readable directory of warnings that the new check can produce”). I am not sure I understood properly but: or you check the pretty printer messages, of you check cabal-testsuite/PackageTests/Check for golden-tests output files.

[ulysses4ever]

I am not sure I understood properly but: or you check the pretty printer messages, of you check cabal-testsuite/PackageTests/Check for golden-tests output files.

Good, this is something. As a user of cabal check I'd expect to see the full list of possible problems that the command can flag in the manual. But if it's too much of an ask, I'd add the pointer to the code in the manual.

In general, I'm a bit surprised that after such a humongous job you done here, the manual wasn't updated at all. Certainly, you possess a unique knowledge, which would be nice to share to some extent. And the manual is the best way to do that, I think.

[andreasabel]

I installed this PR to test the new cabal check. I found that it reports false positives for missing upper bound. In the situation:

name: mypack

library
  build-depends:  foo < 1.0

executable / test-suite
  build-depends: 
    , mypack
    , foo

it should not report missing upper bound for foo in the executable as it is already constrained by the library.

Also, the text Warning: These packages miss upper bounds: should include which component is looked at (library, executable foo, ...).

[ulysses4ever]

@andreasabel to be clear for everyone reading it: the current ("old") cabal check doesn't report it? Because if it does, I don't think we should go out of our way trying to fix it. After all, it looks like a deep semantic reasoning is necessary to figure it out (you have to play the solver, I think). And there's a workaround for cases like this: either put bound in both cases or, better yet, use a common stanza with this dependency.

[andreasabel]

@ulysses4ever wrote:

@andreasabel to be clear for everyone reading it: the current ("old") cabal check doesn't report it? Because if it does, I don't think we should go out of our way trying to fix it.

I have to rebuild cabal to check this. But I would think that if the "old" check reports it, it is a recent regression which be better not release now.

After all, it looks like a deep semantic reasoning is necessary to figure it out (you have to play the solver, I think).

Then maybe it is nothing that should go into cabal check at all, but into a checker that involves the solver. But I think one could take the inherited bounds into account without running the solver, by checking whether the native library is part of the build-constraints.

And there's a workaround for cases like this: either put bound in both cases or,

This would be a regression, obfuscating the .cabal file and making it harder to maintain (e.g. in revisions).

better yet, use a common stanza with this dependency.

This works, but only from certain cabal-versions up. But even then, I think one should not be forced to introduce common stanzas, esp. while they are not properly supported by cabal check (they are expanded out before the checker runs).

[andreasabel]

@ulysses4ever asked:

to be clear for everyone reading it: the current ("old") cabal check doesn't report it?

I checked, current master does not report false positives. So the problem must have been introduced when porting to this PR.

Also, current master groups all missing bounds into one report, avoiding duplicates. Unless we can report them (correctly) per section (library/test-suite...), we should also do such here.

Current master:

Warning: These packages miss upper bounds:
- HUnit
- async
- bytestring
- containers
- enclosed-exceptions
- exceptions
- filepath
- hspec
- hspec-contrib
- lifted-async
- lifted-base
- mtl
- process
- text
- transformers
- transformers-base
Please add them, using `cabal gen-bounds` for suggestions. For more
information see: https://pvp.haskell.org/

This PR:

Warning: These packages miss upper bounds:
- async
- bytestring
- containers
- enclosed-exceptions
- exceptions
- filepath
- lifted-async
- lifted-base
- mtl
- process
- text
- transformers
- transformers-base
Please add them, using `cabal gen-bounds` for suggestions. For more
information see: https://pvp.haskell.org/
Warning: These packages miss upper bounds:
- base
- bytestring
- directory
- filepath
- lifted-async
- mtl
- text
- transformers
- unix-compat
- hspec
- hspec-contrib
- HUnit
Please add them, using `cabal gen-bounds` for suggestions. For more
information see: https://pvp.haskell.org/

Problems:

• complains about base, directory,unix-compat which have upper bound in library that should be inherited
• duplicate reporting of same package
• duplicate addvice on cabal gen-bounds

[ffaf1]

(sorry for repost, I misdeleted it)

A note on “it reports false positives for missing upper bound”. With this .cabal file:

cabal-version: 3.0
name: mypack
version: 2
maintainer: fffaaa
category: asdasd
synopsis: asdcasdcs
description: cdscsd acs dcs dss
license: MIT

library
  exposed-modules: Foo
  build-depends:  text < 5.0
  license: GPL-3.0-or-later

executable prova
  main-is: Prova.hs
  build-depends:
      mypack
    , text
  default-language: Haskell2010

cabal gen-bounds says:

Resolving dependencies...

The following packages need bounds and here is a suggested starting point.
You can copy and paste this into the build-depends section in your .cabal
file and it should work (with the appropriate removal of commas).

Note that version bounds are a statement that you've successfully built and
tested your package and expect it to work with any of the specified package
versions (PROVIDED that those packages continue to conform with the PVP).
Therefore, the version bounds generated here are the most conservative
based on the versions that you are currently building with. If you know
your package will work with versions outside the ranges generated here,
feel free to widen them.

text >= 1.2.5 && < 1.3,

I will fix the behaviour of cabal check, but I would really like gen-bounds and check to go hand in hand on this matter.

Imagine the UX: Dario runs cabal check, gets warned about one missing bound. The suggestion is to run cabal gen-bounds, which prints five more suggestions. Which hint to follow?

---

Re: duplicate reporting, duplicate suggestion. The hint is displayed per target. Adding the target name and “inheritance” will reduce spam and make it clearer hopefully, I will paste test output and see if everyone if fine with it.

[andreasabel]

Yeah, I am not happy by the recommendation of cabal gen-bounds by cabal check. (Which was added lately.) I don't think that cabal gen-bounds is more than a proof of concept atm. It works with only the ghc in your PATH (#4956), and it does cannot generate bounds for a range of GHC versions. Also, I ran it in the context above and it suggested a containers-0.6.... for GHC-8.2.2 when that GHC ships with 0.5.10. I wouldn't be unhappy if you deleted the recommendation of cabal gen-bounds from cabal check, or at least add clear caveats.

[ulysses4ever]

@ffaf1 would it be possible to split the 4K LOC file into smaller chunks? You already have nice sections there, so it should be easy. I really-really don't like files over 1K. We just got split one giant https://github.com/haskell/cabal/pull/8130 -- let's crush another one while we're on it.

[ffaf1]

Comment to track what I still have to do:

• [x] add check descriptions to manual
• [x] try to split Check.hs and Type.hs

[ffaf1]

Upper bounds behaviour. For this .cabal file:

cabal-version: 3.0
name: mypack
version: 2
maintainer: the Cabal Investors Group
category: Test
synopsis: upper
description: no upper boundaries
license: GPL-3.0-or-later

library
  exposed-modules: Foo
  build-depends:  text < 5.0
  default-language: Haskell2010

executable exe
  main-is: Prova.hs
  build-depends:
      mypack
    , text
    , aeson
  default-language: Haskell2010

benchmark bench
  type: exitcode-stdio-1.0
  main-is: Bench.hs
  build-depends:
    , text
    , aeson
  default-language: Haskell2010

Master outputs:

Warning: These warnings may cause trouble when distributing the package:
Warning: These packages miss upper bounds:
- aeson
Please add them, using `cabal gen-bounds` for suggestions. For more
information see: https://pvp.haskell.org/

Mine outputs:

Warning: These warnings may cause trouble when distributing the package:
Warning: On executable 'exe', these packages miss upper bounds:
- aeson
Please add them. More informations at https://pvp.haskell.org/
Warning: On benchmark 'bench', these packages miss upper bounds:
- text
- aeson
Please add them. More informations at https://pvp.haskell.org/

I like mine better (precise target, correct text error for bench, unobtrusive suggestion). Let me know if this cuts the mustard, @andreasabel.

[andreasabel]

@ffaf1 wrote:

I like mine better (precise target, correct text error for bench, unobtrusive suggestion). Let me know if this cuts the mustard, @andreasabel.

Yes, perfect! I reinstalled your branch and reran my case, and I am quite satisfied with the output now! Fixes my concerns.

[ffaf1]

As suggested, I have added a table to document each check. Rendering is disgusting (no wrapping, have to horizontal-scroll on smaller screens).

There is a workaround but it involves overriding css, hence potentially breaking readthedocs responsive stylesheet.

edit: list rendering.

[ulysses4ever]

@ffaf1 very cool! I doubt people will be so much interested in constructor names that they should appear so prominently. Maybe just a list would suffice? E.g.

• (ParseWarning) Warnings inherited from parser.

That would solve the issue on mobile...

[ffaf1]

I doubt people will be so much interested in constructor names that they should appear so prominently.

They will, soon! cabal check --ignore MissingUpperBounds is only one PR away.

Maybe just a list would suffice?

That is what I will do if nobody comes up with another solution, Relevant ticket on readthedocs repo has been open for nine long years.

[andreasabel]

I think a well-chosen constructor name can serve as a name for a warning. (We do like this in Agda.) To not change warning names accidentially via a refactoring though, there should be a big alert in the code that these constructors should not be renamed.

[ffaf1]

• [x] Rebase against #8897
• [x] rebase against #8898
• [x] rebase against #8908
• [x] and most importantly against #8950 (fourmolu formatting)

Sadly fourmolu style means that more or less every line in the PR changed. Each comment up until now has been addressed.

[ffaf1]

• [x] rebase #9004

[ffaf1]

Rebase on #9051, additional boundaries where needed in tests for if/then blocks.