Skip to content
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

Fix incomplete-record-updates warnings #1059

Merged
merged 3 commits into from
Apr 19, 2022

Conversation

jhrcek
Copy link
Contributor

@jhrcek jhrcek commented Apr 16, 2022

Fixing some ghc warnings that show up in CI as discussed in #1056 (comment)

There are some issues in ghc with -fwarn-incomplete-record-updates giving false positives, e.g.
https://gitlab.haskell.org/ghc/ghc/-/issues/5728
https://gitlab.haskell.org/ghc/ghc/-/issues/17783

Some of those should be fixed in recent versions of GHC (9+),
but we can avoid them even on GHC 8 by using RecordWildCards trick mentioned here
The other instances of this warning are fixed by restructuring the code a bit to make it more obvious to GHC that all cases are handled.

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your efforts!

As it stands, I am opposed to code changes just to work around false positives in a GHC warning mechanism. If the warning is unsound, we should switch it off rather than change the code (unless we can change the code without making any sacrifices, but I do not see it here).

So I am +1 on silencing the warning with -Wno-..., ideally in a per-file basis.
I am -1 on these code changes.

chunked
. setHeader "Content-Encoding" "gzip"
. setHeader "Vary" "Accept-Encoding"
. removeResponseHeader "Content-Length"
. removeResponseHeader "Content-MD5"
. alterResponseHeader "ETag" (map modifyETag)
$ r { rsBody = GZip.compressWith compressParams $ rsBody r }
$ Response { rsBody = GZip.compressWith compressParams rsBody, .. }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this version (with the magic dots .. in the record update) harder to comprehend. I consider this a fringe feature of GHC Haskell (I am seeing it for the first time now).
There might be some risks of unintentionally introducing bugs by shadowing one of the record fields introduced by the .. pattern which is then picked up by the .. update, is there?

I think that in the contrary we should strive to get rid of all the .. magic and be more explicit about what is in scope.

So, I'd rather not change code to work around an incorrect GHC warning mechanism. Rather switch off the warning.

setHeader "Content-Length" (show rangeLen)
. setHeaderBS (BS.C8.pack "Content-Range") (contentRange fr to fullLen)
. removeResponseHeader "Content-MD5"
$ r { rsBody = BS.Lazy.take rangeLen $ BS.Lazy.drop fr $ rsBody r
$ Response
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I am opposed.

@jhrcek jhrcek force-pushed the jhrcek/incomplete-record-updates branch 2 times, most recently from 8809e4d to 178dcfd Compare April 16, 2022 08:58
@jhrcek
Copy link
Contributor Author

jhrcek commented Apr 16, 2022

It's not that -fwarn-incomplete-record-updates is unsound.
It actually found bunch of places that could potentially fail at runtime with incomplete pattern marches.
The only issue is that GHCs prior to version 9 can give this warning even for some cases where the failure condition provably cannot happen. I removed the usage of RecordWildCards from my PR. Let me know if you're against even this less invasive vefsion of the fixes and I'll close this PR.
Btw. any idea why CI is failing on 9.0.2?

@jhrcek jhrcek changed the title Fix/work around incomplete-record-updates warnings Fix incomplete-record-updates warnings Apr 16, 2022
@andreasabel
Copy link
Member

Btw. any idea why CI is failing on 9.0.2?

Yes, this is mysterious. I am suspecting that the package-db caching is unsound:

- name: Cache cabal global package db
id: cabal-global
uses: actions/cache@v2
with:
path: |
~/.cabal
key: ${{ runner.os }}-${{ matrix.versions.ghc }}-${{ matrix.versions.cabal }}-cabal-global-${{ hashFiles('cabal.project') }}

I am pushing a new, idiomatic CI from

@andreasabel
Copy link
Member

I am ok with this modified PR. Using \case would be a cosmetical improvement.
If you rebase this PR onto master, I hope the CI will succeed!

@andreasabel andreasabel added the re: code quality Concerning the code quality of the implementation of `hackage-server` label Apr 16, 2022
@jhrcek jhrcek force-pushed the jhrcek/incomplete-record-updates branch from 178dcfd to 6e84bc2 Compare April 16, 2022 13:38
@jhrcek
Copy link
Contributor Author

jhrcek commented Apr 16, 2022

Thank you for updating CI, LambdaCase suggestion applied.

@andreasabel andreasabel merged commit b4ea1c7 into haskell:master Apr 19, 2022
@andreasabel
Copy link
Member

Thanks, @jhrcek !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
re: code quality Concerning the code quality of the implementation of `hackage-server`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants