coverage: Use OverloadedRecordDot instead of RecordWildCards

[wolfgangwalther]

As mentioned in #2627 (comment) and elsewhere before, we currently have quite a few false-negatives for coverage: Fields in record types are marked as uncovered, because we use RecordWildCard, which isn't supported by HPC.

While RecordWildCard is not supported, I just found out that OverloadedRecordDot actually is.

This is just a small example of what it could look like.

The key change is the following. Instead of

handleRequest :: AuthResult -> [...]
handleRequest AuthResult{..} [...]
[...]
Query.setPgLocals [...] authClaims authRole [...]

we do

handleRequest :: AuthResult -> [...]
handleRequest authResult [...]
[...]
Query.setPgLocals [...] authResult.claims authResult.role

I'm pretty sure the dot syntax will feel more natural to a lot of people.

Imho, it only makes sense when we remove all the prefixes for record fields, as done in the example above. authResult.authClaims etc. would just read very clumsy.

At the same time, this also allows us to use the type itself in a namespaced way, too. We previously had to:

import PostgREST.Auth (AuthResult (..))

just to have the record field selectors available for RecordWildCards. But now we don't need to do that anymore - and we can just use Auth.AuthResult from the qualified import. Of course, this then allows us to drop the prefix from the type, too, so we can export just Result from Auth.hs and use it as Auth.Result.

If we were to use this style consistently, we'd get code coverage of our type definitions fixed. Before this MR, the authClaims field in the following was uncovered:

data AuthResult = AuthResult
  { authClaims :: KM.KeyMap JSON.Value
  , authRole   :: Text
  }

Now, this line is covered, too.

WDYT?

[wolfgangwalther]

The CI failure tells me that this would bump the minimum required GHC version to 9.2. OverloadedRecordDot was only introduced in 9.2.0: https://ghc.gitlab.haskell.org/ghc/doc/users_guide/exts/overloaded_record_dot.html

Any specific reason why we still support GHC 8.10?

[steve-chavez]

LGTM!

Any specific reason why we still support GHC 8.10?

Not one from my side.

cc @robx

[robx]

LGTM!

Any specific reason why we still support GHC 8.10?

Not one from my side.

cc @robx

None that I'm aware of either.

[steve-chavez]

Any specific reason why we still support GHC 8.10?

Removed on #2683

[steve-chavez]

There's a blocker for OverloadedRecordDot on #2834.

That will be solved once we offer a static binary for ARM. (or once Ubuntu upgrades GHC)

[wolfgangwalther]

I rebased the first two commits, which were about Auth.hs, and then added two more commits to show how this would look like with AppState.hs. The last commit is only an example for "virtual fields" on appState, which would need to be extended to cover all the getters and setters in that file.

The virtual fields approach would allow to rewrite stuff like this (random example):

AppState.reReadConfig False appState

into

appState.reReadConfig False

i.e. providing both the proper namespacing and less duplication.

---

I don't understand while some of the CI jobs fail to build with an unused import warning while others build just fine (just like it does locally for me).

[wolfgangwalther]

I don't understand while some of the CI jobs fail to build with an unused import warning while others build just fine (just like it does locally for me).

The new .hs-boot file needs to be made known to nix. That should solve it.