Opened 11 months ago

Last modified 8 months ago

#15656 patch task

Extend -Wall with incomplete-uni-patterns and incomplete-record-updates

Reported by: ckoparkar Owned by: ckoparkar
Priority: normal Milestone: 8.8.1
Component: Compiler Version: 8.4.3
Keywords: GHCProposal Cc: tomjaguarpaw, RolandSenn
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: #16041 Differential Rev(s): Phab:D5415
Wiki Page:

Description

The proposal has been accepted, and could be implemented.

Attachments (2)

incomplete-record-updates.log (12.9 KB) - added by ckoparkar 8 months ago.
incomplete-uni-patterns.log (19.7 KB) - added by ckoparkar 8 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 11 months ago by ckoparkar

Type: feature requesttask

comment:2 Changed 9 months ago by RolandSenn

Owner: set to RolandSenn

comment:3 Changed 9 months ago by RyanGlScott

FYI: ckoparkar is also working on this in Phab:D5415.

Changed 8 months ago by ckoparkar

Changed 8 months ago by ckoparkar

Attachment: incomplete-uni-patterns.log added

comment:4 Changed 8 months ago by ckoparkar

RolandSenn, feel free to commandeer that patch.

Adding the flags to -Wall was the easy part, and that's done. However, we still need to do something about the code in GHC which is unsafe per these two flags. (Attached the build/error log).

comment:5 Changed 8 months ago by RolandSenn

Owner: RolandSenn deleted

@ckoparkar, @RyanGlScott: Ops, this ticket contains more fun than I expected!

Most of these incomplete patterns are already a very long time in the GHC and libraray code, so we can assume, they are nearly never a problem. The new warnings do not make the code safer or more unsafe. GHC now just reminds us, that there is unsafe code.

To fix, we have 3 possibilities:

  1. Add a pragma to skip the warning to every affected module. This has the advantage, that we don't change the semantics of any GHC or library module.
  1. Add the missing patterns, and call panic! This has the advantage, that if we really ever hit such a case, the user gets a nicer error message, and is invited to report the problem.
  1. Add the missing patterns and add (maybe) intelligent code to the RHS of the pattern. This has the disadvantage, that we add a lot of code. Unfortunately will be unable to test this new code. This is very bad!

I think, we should concentrate on option 1 or 2. The difference in the amount of work is marginal, however, I don't really know, which one is the GHC way to solve this issue. Ryan (and others!) please advice!

Both solutions (1 and 2) are not difficult but a fair amount of work. We should only do it, if we know, that the solution will be accepted by code review.

In addition we have to decide, who does the work: I don't mind to do everything, or just a part (libraries or GHC) or nothing.

The decisions are up to you!

PS: Till now I never did something in the libraries, so I'll probably need some guidance, eg how and where to specify the bumped version numbers of the modified libraries.

PSS: Thanks to ckoparkar for the two attachments.

comment:6 Changed 8 months ago by goldfire

Option (2) seems like the obvious choice (at first) -- but then we realize that doing it means that we have to really change a lot of syntax! In particular, incomplete record updates cannot be cleanly refactored.

We also have another option:

  1. Refine GHC's types so that these warnings go away.

For example, if we have

data T = MkT1 { foo :: Int } | MkT2 { bar :: Bool }

and have t { foo = 5 } somewhere, we'll get an error. But perhaps there is enough logic to suggest that t will always be a MkT1. We could refactor to

data T = MkT1 T1 | MkT2 T2
data T1 = Mk1 { foo :: Int }
data T2 = Mk2 { bar :: Bool }

and then our variable t would have type T1 instead of T and all would be well.

Of course, this refactoring introduces new memory allocations at runtime, and it may be hard to make all the types work out. It's thus impractical. However, it suggests a way forward here:

Do (1), but make (4) the end goal. That is, just squash the warnings for now, but have it be an explicit goal (with a ticket to track the goal) to remove the squashings. This would mean, e.g., that we would need a way for the refactoring above not to take extra allocations at runtime. (This would be a huge improvement for Haskell overall!) And we'd likely need dependent types :). But all this would be very nice concrete fodder for why such improvements are called for.

comment:7 Changed 8 months ago by RolandSenn

Cc: RolandSenn added

comment:8 Changed 8 months ago by RolandSenn

@ckoparkar: Just go ahead! Grab the ticket and do the changes outlined by goldfire in comment:6. It's not so much work, as you only have to add one line to every module (not line!) listed in your attachments. The really difficult work will be postponed to the new ticket.

comment:9 Changed 8 months ago by ckoparkar

Owner: set to ckoparkar

Thanks RolandSenn. Updating that patch now.

comment:10 Changed 8 months ago by ckoparkar

Differential Rev(s): Phab:D5415

comment:11 Changed 8 months ago by ckoparkar

Status: newpatch
Note: See TracTickets for help on using tickets.