Skip to content

Reimplement cabal check #8211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
4 tasks done
ffaf1 opened this issue Jun 13, 2022 · 5 comments · Fixed by #8427, #8250, #8248 or #8269
Closed
4 tasks done

Reimplement cabal check #8211

ffaf1 opened this issue Jun 13, 2022 · 5 comments · Fixed by #8427, #8250, #8248 or #8269

Comments

@ffaf1
Copy link
Collaborator

ffaf1 commented Jun 13, 2022

edit: done! The meat of this is in #8427, thanks everyone for helping!

Where are we now

cabal check is a tool to check the correctness of a .cabal file and more generally to provide additional useful output (warnings, especially related to uploading the package to Hackage).

As now cabal check is an imprecise tool: simple example (#7423), where -O2 gets a warning even when it is passed under a (cabal) flag.

A number of these errors are caused by the way the checker operates today. Immediately noticeable: inefficiency (we go through pkg numerous times), lack of scoping awareness.

Where we want to be

The correct way to tackle the problem is to:

  • Document and write tests. check is a fairly complex system in some of its part and this will minimise risks of disruption to the users.
  • Rely on types, not strings or similar as far as possible.
  • Inspect each element of GenericPackageDescription once. So traverse the AST focusing on what we need (and not a bit more) in each step.

Steps to achieve the goal

  • write tests to document how we would like cabal check to behave. We should scoop as much as possible from the issues in the issue tracker. The resulting modifications will have the shape of a number of small pull requests, easy to parse and easily mergeable. Done in cabal check testuite: add sanity checks #8248 and cabal-check testsuite: add Package/File tests #8250.
  • rewrite checkPackage to pattern match on GenericPackageDescription (instead of using accessors). This buys us the peace of mind that if GenericPackageDescription is modified (fields added), then a compilation error will gently point the user to add relevant tests. This should a slightly bigger modification and maybe not uncontroversial (accessors are handier than pattern matching a big type). Done in Reimplement cabal check #8427.
  • tipify cabal check errors (as now they are Strings). Again a propaedeutic task, this should be relatively self contained and uncontroversial. Done in cabal check: add typed errors #8269.
  • write the traversing function. First flesh out the types (get comments), then implement it. The eventual pull request will be probably bigger and slightly more difficult to comment on; this is were having written a lot of tests will come handy. This could be done as a third party tool first (disadvantages: being less close to Cabal codebase means more to rewrite). Done again in Reimplement cabal check #8427, had to rebase/squash commits as cabal introduced autoformatters meanwhile.

This issue will be useful to document development and gather comments.

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Jun 22, 2022

First batch of tests here: #8248 There is quite some work to do.

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Jul 1, 2022

With #8248 and #8250 merged (105 tests in total) the “write a testsuite” box is checked. We are now reasonably sure reimplementation won’t break current behaviour; we are now reasonably sure in the future we will be warned if a check becomes redundant (e.g. because of improved parsing).

Next step: convert String errors to typed ones.

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Jul 21, 2022

With #8269 merged (structured errors), half of the work is hopefully done.

Next steps: study GenericPackageDescription, see which checks are reimplementable by just pattern matching on the AST, see which ones are not. Most likely we will need to introduce/check scoping (how to do it? pass some kind of state around?).

Once ready: write a short implementation proposal.

@andreasabel
Copy link
Member

@ffaf1: Great work so far! Please remember to submit your Contributor Midterm Evaluation to GSoc by tomorrow!

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Jul 28, 2022

Thanks for the reminder, will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment