Spurious warning about incomplete record updates

[kozross]

Given a source file as so:

module RDPBlowup where

data Foo = Foo {
  bar :: Int,
  baz :: Float
  } | Quux {
  quux :: String
  }

And a Cabal file as so:

cabal-version: 2.2
name:          rdp-blowup
version:       2.0.0
build-type:    Simple
tested-with:   GHC ==8.8.3

library
  exposed-modules:    RDPBlowup
  build-depends:
    , base                     >=4.13  && <5
    , record-dot-preprocessor  ^>=0.2.5
    , record-hasfield          ^>=1.0

  ghc-options:
    -Wincomplete-record-updates -fplugin=RecordDotPreprocessor

  hs-source-dirs:     src
  default-extensions:
    DataKinds
    DuplicateRecordFields
    FlexibleContexts
    FlexibleInstances
    MultiParamTypeClasses
    TypeApplications
    TypeSynonymInstances

  default-language:   Haskell2010

Attempting to compile triggers these spurious warnings:

[1 of 1] Compiling RDPBlowup

/home/koz/mlabs/rdp-blowup/RDPBlowup:1:1: warning: [-Wincomplete-record-updates]
    Pattern match(es) are non-exhaustive
    In a record-update construct: Patterns not matched: (Foo _ _)

/home/koz/mlabs/rdp-blowup/RDPBlowup:1:1: warning: [-Wincomplete-record-updates]
    Pattern match(es) are non-exhaustive
    In a record-update construct: Patterns not matched: (Quux _)

/home/koz/mlabs/rdp-blowup/RDPBlowup:1:1: warning: [-Wincomplete-record-updates]
    Pattern match(es) are non-exhaustive
    In a record-update construct: Patterns not matched: (Quux _)

Without enabling the GHC plugin for the record dot syntax, these don't appear. I'd rather not have to silence this (useful) warning or constantly have to filter it for false positives. Am I doing something wrong here?

[ndmitchell]

I'm not sure if these warnings are spurious or not - the record projection x{bar=1} is indeed an incomplete pattern which doesn't match Quux. Ignoring these warnings seems like a possible failure mode, and reporting it doesn't seem unreasonable.

[ndmitchell]

Because of the fix for #31 this went a way because I'm not using record updates at all. Not sure whether that's a good thing or not, but it's what happened.

[kozross]

I'm still getting this issue on the latest version. The only change I made to the above example was setting the version of record-dot-preprocessor to 0.2.6.

[ndmitchell]

I didn't perfectly match the bug, but rewrote it in my test suite - perhaps missing some essential detail. I'll take another look tomorrow.

[ndmitchell]

Ah, I was experimenting with the preprocessor, which triggered the same bug, not the plugin. Annoyingly, the plugin doesn't work on Windows, so I'm mostly relying on the CI, and I had a stray -w to ignore all warnings in the plugin. I've fixed the CI, which should now fail, and will investigate further.

[ndmitchell]

So the plugin currently requires -Wno-incomplete-record-updates. Fixing it will involve a reasonable amount of time, so I'd accept a patch, but currently have no intention to do so myself. In the Plugin code, instead of generating r{selector=x} it would be necessary to generate a full case alternative. But that might error, so it would be necessary to fake up an error message. But then you are approximately replacing an error message the compiler can see and warn on, with one it can't, which leaves me a little uneasy. My recommendation for now would just be to disable that specific warning, since you are likely doing incomplete record updates, albeit through the RecordDotSyntax.

[kozross]

@ndmitchell I understand your reasoning, but our chief issue isn't so much that such an incomplete update triggers a warning, it's where this warning is triggered. Currently, this is at the time (and in the module) where the type is defined, rather than at point of use, where it would actually be useful. Therefore, we have to silence this warning anywhere we want to use RecordDotSyntax and we define a sum type which uses record syntax in any of its 'arms' - even if we don't use the update syntax in that module at all!

[ndmitchell]

@kozross Yep, totally agreed. I actually did a whole PhD on trying to move the warnings about incompleteness from their definition site to their use site! I'd much rather GHC worked that way, but its unlikely to do so in the near or medium term. As such, I'd accept a patch converting us from incomplete record updates to a case match - but don't have the bandwidth to do so myself - and am on the fence about whether it's a good idea (but if someone feels strong enough to implement it, I'll take that as evidence that my weak preferences aren't too relevant and happily merge).