ci: Enable -Werror=compat -Werror=default

[langston-barrett]

To prevent warnings sneaking in without us noticing! Also, switch to a non-deprecated Haskell setup action to get a non-ancient version of Cabal.

I don't feel strongly about this, just thought it would be nice and figured we could do it if we all agreed.

[RyanGlScott]

I don't have a problem with -Werror myself, but I reserve the right to complain if I find a particular class of warning to be annoying. In fact, this PR already goes as far as disabling a warning in Data.Parameterized.ClassesC, so I think you're on the same page as I am :)

[langston-barrett]

One point to consider would be whether -Wall -Wcompat is the right set to error on. As Ryan noted above, -Wall actually has some questionable inhabitants. Perhaps we should just -Werror=compat -Werror=default? The warnings in default are somewhat more conservative, while still containing most of all.

[RyanGlScott]

Perhaps we should just -Werror=compat -Werror=default?

I'd be fine with that.

[sauclovian-g]

I don't have a strong opinion, other than the warning about unused values, which IME is critical for avoiding accidental loops. (I could probably have an opinion if I sat down with the manual and the list of warnings; LMK if it would be useful for me to do that. As it is, the last time I did that was in I think GHC 6.x.)

[kquick]

As Ryan hinted at, this comes down to sort of a philosophy/perspective on the management of the code.

If the codebase is not just CI but CD to a live environment (e.g. your business web-page), then you definitely want your controls to be tight. It could be argued that this repository is somewhat the opposite: research code where more importance is placed on the functionality than the correctness (... I did say "could be argued"!); in that latter scenario, resolving all compilation warnings and similar activities tends to add friction that could outweigh the utility. To put it another way, if I didn't care about these warnings enough to fix them during my development, are they a required must-fix before sharing this code?

My thinking is similar to Ryan's statement: I'm OK with this at present, but I think this should be an encouragement and not a requirement: we should feel free to disable individual warnings or Werror entirely in the future without this being a controversial change or a change in the overall stance for this package. I'm also fine with -Werror=compat -Werror=default.

I think we should have a position on the development requirements for this codebase, and those should be documented (probably in a Development.md to avoid bloating the README.md). I'm definitely open to discussion regarding my proposal of what our position should be above, but would request that this PR be supplemented with the documentation of this position for the benefit of ourselves, future maintainers, and users. I think this also becomes relevant for those future maintainers when a future compiler version introduces a new Werror failure.

Also, lets make a separate PR for the action version bump, since it's unrelated to the rest of this PR and associated discussions.

[sauclovian-g]

The reason I like -Werror is that if there are expected warnings, it's very easy for other unexpected warnings to slip in unnoticed. (There are other ways to attend to this concern, but they're usually more expensive...)

[langston-barrett]

Also, lets make a separate PR for the action version bump, since it's unrelated to the rest of this PR and associated discussions.

Good call, #177

[langston-barrett]

To put it another way, if I didn't care about these warnings enough to fix them during my development, are they a required must-fix before sharing this code?

Completely agreed that if code with warnings is intentionally included, we should trust one anothers' judgement over the compiler's. Also agreed with @sauclovian-g that it is hard to reliably keep track of which warnings were intentionally ignored vs. unintentionally added without some automated assistance (especially reliably, especially with multiple developers, especially over long timespans).

I think we should have a position on the development requirements for this codebase, and those should be documented (probably in a Development.md to avoid bloating the README.md). I'm definitely open to discussion regarding my proposal of what our position should be above, but would request that this PR be supplemented with the documentation of this position for the benefit of ourselves, future maintainers, and users. I think this also becomes relevant for those future maintainers when a future compiler version introduces a new Werror failure.

Great point, I've added some documentation. It mentions some of the concerns mentioned above (i.e., correctness vs. rapid/frictionless prototyping), and one not yet mentioned above: there are downstream codebases that have their own concerns.

Let me know what you think! Thanks for the thoughtful feedback, all!

[langston-barrett]

One point to consider would be whether -Wall -Wcompat is the right set to error on. As Ryan noted above, -Wall actually has some questionable inhabitants. Perhaps we should just -Werror=compat -Werror=default? The warnings in default are somewhat more conservative, while still containing most of all.

other than the warning about unused values, which IME is critical for avoiding accidental loops.

I just noticed that -Wunused-binds is actually not in -Wdefault, but only in -Wextra... As noted above, I tend to land on the more aggressive side of warnings-enforcement. I usually enable -Werror for all of -Wall (plus a few other things, minus -Wtrustworthy-safe) so I would a fortiori also be fine with -Werror=extra. @kquick noted that he generally doesn't go as far as -Werror=default, so presumably would be less enthused about -Wextra. @sauclovian-g indicated in the quote above that he doesn't really care about the particular set except that it should specifically include -Wunused-binds... I'm worried that we might be on the verge of bike-shedding here 😅 Rather than turn on -Werror=extra or even -Werror=unused-binds, I propose we appeal to authority (a classic fallacy, but perhaps helpful to avoid bikeshedding here), keep -Werror=default, and trust the GHC developers to maintain -Wdefault as a reasonable list.