Cabal Error Refactor

[SuganyaAK]

Fixes #8618 #8543

---

This PR introduces three major changes

1. A datatype CabalException for the Cabal package that has just enough constructors to cover a few modules I have committed as part of this PR, which will incrementally grow and a future idea of creating CabalInstallException for Cabal-Install Package.
2. A function dieWithException which will replace die' function and throws Exceptions.
3. A datatype VerboseException which will be polymorphic over CabalException and CabalInstallException

---

I would appreciate your feedback, review comments and suggestions.

1. To identify/differentiate cabal errors, I used "C=" as suggested in issue Integration with the error message index? #8543.
2. For the start, I used random numbers for error codes. I plan to Index these error codes and document it, I request suggestions or directions on this.

---

Please 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.
• The documentation has been updated, if necessary.
• Include manual QA notes if your PR relates to cabal-install.

Bonus points for added automated tests!

[ulysses4ever]

One immediate objection that comes to mind: let's not put more stuff in Utils.hs and instead have a separate Errors module?

[ulysses4ever]

More generally, CI runs with -Werror, which means that in order to get a useful feedback from it you should make sure that your code compiles without warnings.

Also, we format the code base with fourmolu-0.12. Install it and run make style to make sure you adhere to its rules...

[SuganyaAK]

One immediate objection that comes to mind: let's not put more stuff in Utils.hs and instead have a separate Errors module?

Yes I agree. It will be easy to refer to the exception types as well.
errors.hs or exceptions.hs which will be a better module name u think? and creating in Distribution.Simple will make sense right?

[ulysses4ever]

I don't think the name or location matter all that much: we can always bike-sched it. I'd look what others do (e.g. Stack) or just go with Distribution.Simple.Errors.

[gbaz]

This is a good start. You'll eventually want a version of withMetadata that gets the callStack from VerboseException passed in -- or to inline that function into the display instance.

Also, I think the idea is that each type of exception gets its own constructor with its own code. So each individual BenchmarkException etc. should have a constructor eventually, rather than one catchall constructor which takes a string. Then there should be a function exceptionToMessage or the like that maps each exception constructor to the rendered string.

[SuganyaAK]

This is a good start. You'll eventually want a version of withMetadata that gets the callStack from VerboseException passed in -- or to inline that function into the display instance.

Also, I think the idea is that each type of exception gets its own constructor with its own code. So each individual BenchmarkException etc. should have a constructor eventually, rather than one catchall constructor which takes a string. Then there should be a function exceptionToMessage or the like that maps each exception constructor to the rendered string.

Thank you, I have created a function exceptionWithMetada which takes in the callStack from VerboseException and made a few changes to the code as it already sets AlwaysMark and VerboseTrace.

The new module errors.hs has the constructors for Cabal Exceptions , functions exceptionCode(renders error codes) and exceptionMessage(renders error messages).

On the idea of each type of exception having its own constructor, I have incorporated the same in the modules bench.hs and install.hs.
However in the install.hs module, I have grouped a few dieWithException calls to one constructor CompilerNotInstalled as the errors are similar. Please let me know is this makes sense.

[ulysses4ever]

One more thing: there's a "Manual QA process": https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#qa-notes It'd be best if in the original post in the PR under the heading "QA Notes" you put instructions on how to create a simple project that fails and prints a message with an error code.

[gbaz]

The "rip" changes look good, but note the new failures.

Also I think you are printing callstacks at too low a verbosity -- note that there were no callstacks printed in these circumstances before.

[SuganyaAK]

The "rip" changes look good, but note the new failures.

Also I think you are printing callstacks at too low a verbosity -- note that there were no callstacks printed in these circumstances before.

I see for all the test cases that are failed in this run, the marked verbose input is "-vverbose +markoutput +nowrap". I think the recent failures that popped up, happened after I replaced the die' call sites with new constructors in the Glob.hs . I could see this with the error code that shows. ` Unfortunately all these tests passed on my local, so I couldn't see this coming.

[gbaz]

I believe this is basically ready for review, and it would be good to rapidly review and merge so that other work can proceed along the same lines on top of this basic exception/reporting structure...

[geekosaur]

OOC, why is it spelled "Soure" instead of "Source"?

[ulysses4ever]

It's a typo?

[geekosaur]

Suggested change

	dieWithException verbosity $ CantFindSoureModule module_
	dieWithException verbosity $ CantFindSourceModule module_

[SuganyaAK]

Yes it was a typo, Thanks for pointing out. Changed to "CantFindSourceModule"

[Kleidukos]

Nicely packed first PR for an ambitious goal, thank you very much @SuganyaAK

[SuganyaAK]

Nicely packed first PR for an ambitious goal, thank you very much @SuganyaAK

Thank you

Nicely packed first PR for an ambitious goal, thank you very much @SuganyaAK

Thank you @Kleidukos

[Kleidukos]

LGTM, thanks!

[ulysses4ever]

@mergify refresh

[mergify]

refresh

✅ Pull request refreshed