Opened 6 years ago

Closed 6 years ago

#9156 closed bug (fixed)

Duplicate record field

Reported by: wojteknar Owned by: gintas
Priority: low Milestone:
Component: Compiler Version: 7.8.2
Keywords: Cc: nomeata
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC accepts invalid program Test Case: T9156
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D87
Wiki Page:

Description

This compiles.

data D = D1 { f1 :: Int }
       | D2 { f1, f1 :: Int } -- Oops, duplicate

Probably harmless, but still.

Attachments (4)

Change History (19)

comment:1 Changed 6 years ago by wojteknar

Type of failure: None/UnknownGHC accepts invalid program

comment:2 Changed 6 years ago by nomeata

Owner: set to gintas

comment:3 Changed 6 years ago by gintas

Test Case: T9156

comment:4 Changed 6 years ago by gintas

Wait, some performance tests are failing, I don't think they're flukes.

comment:5 Changed 6 years ago by gintas

Status: newpatch

OK, the failures do look like a fluke, although I'm not quite sure. Could someone take a look?

comment:6 Changed 6 years ago by gintas

Cc: nomeata added

comment:7 Changed 6 years ago by gintas

The 0002-Refactored-record-field-duplicate-code-to-use-nested.patch is broken, I accidentally uploaded a stale version. Sorry about that. Use 0002-fixed-refactor.patch instead.

Changed 6 years ago by gintas

Attachment: 0002-fixed-refactor.patch added

comment:8 Changed 6 years ago by gintas

The patch should be good to apply. Could someone take a closer look?

comment:9 Changed 6 years ago by nomeata

Sorry for the delay.

The refactoring patch lost your comment, and I believe the code could use some comments.

Also, your dropOneUnloc (unloc v) could be a deleteBy (\v' -> unloc v == unloc v') or deleteBy ((==) on unloc), unless I’m mistaken. This would make the code a tad smaller and easier to read (assuming you add a comment that states that it is important here that deleteBy deletes only the first occurence).

Very minor, but I slightly tripped over (L loc name : r') ++ go remSeen' rs, and would not have tripped over (L loc name) : r' ++ go remSeen' rs.

Changed 6 years ago by gintas

Updated patch

comment:10 Changed 6 years ago by gintas

Re-added the comment and fixed the cosmetic issues you mentioned, please take a look. Thanks.

comment:11 Changed 6 years ago by nomeata

Looks good to me. I think I’ll try to create a Pharicator code review thingy tomorrow, just to get used to the new tool.

comment:12 Changed 6 years ago by nomeata

Differential Rev(s): Phab:D87

comment:13 Changed 6 years ago by Joachim Breitner <mail@…>

In d2942184c8cc53cb3b50f78a7ecff930c3e5861f/ghc:

Fixed issue with detection of duplicate record fields

Duplicate record fields would not be detected when given a type
with multiple data constructors, and the first data constructor
had a record field r1 and any consecutive data constructors
had multiple fields named r1.

This fixes #9156 and was reviewed in https://phabricator.haskell.org/D87

comment:14 Changed 6 years ago by nomeata

Applied. Sorry, Gintautas, for the delay and thanks for your second contribution to GHC! I hope there will be more to come :-)

comment:15 Changed 6 years ago by nomeata

Resolution: fixed
Status: patchclosed
Note: See TracTickets for help on using tickets.