#13644 closed bug (fixed)

overloaded name used in record pattern matching leads to panic! (the 'impossible' happened) in ghc

Reported by: pjljvdlaar Owned by:
Priority: normal Milestone: 8.4.1
Component: Compiler Version: 8.0.2
Keywords: Cc: adamgundry
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Compile-time crash or panic Test Case: rename/should_fail/T13644
Blocked By: Blocking:
Related Tickets: #13840, #13973, #14087 Differential Rev(s): Phab:D3988
Wiki Page:

Description

In Haskell,

  1. The scope of definitions can be controled.
  2. The same name can be used to define both a function and a field of record.
  3. The user can use that name in record pattern matching when only the function is within scope. For example
    FuncId{ name = nm }
    

resulting in the following bug

[38 of 39] Compiling TxsUtils         ( src\TxsUtils.hs, .stack-work\dist\1f7101f2\build\Txs
Utils.o )
ghc.EXE: panic! (the 'impossible' happened)
  (GHC version 8.0.2 for x86_64-unknown-mingw32):
        translateConPatVec: lookup

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

Attachments (2)

defs.zip (33.0 KB) - added by pjljvdlaar 22 months ago.
Example to reproduce the bug: use stack build
repo.zip (1.2 KB) - added by mpickering 22 months ago.

Download all attachments as: .zip

Change History (23)

Changed 22 months ago by pjljvdlaar

Attachment: defs.zip added

Example to reproduce the bug: use stack build

comment:1 Changed 22 months ago by simonpj

I bet this is a dup of #12158, which needs attention from someone.

comment:2 Changed 22 months ago by pjljvdlaar

The issue occurs on line 157 of TxsUtils.hs

@simonpj. I doubt it: the scoping is important, otherwise a nice warning is generated.

C:\Users\pilaar\Desktop\TheMa\Experiments\Haskell\defs\src\TxsUtils.hs:157:37: error:
    Ambiguous occurrence `name'
    It could refer to either `TxsDefs.name',
                             imported from `TxsDefs' at src\TxsUtils.hs:26:1-14
                             (and originally defined in `Ident')
                          or `FuncId.name',
                             imported from `FuncId' at src\TxsUtils.hs:28:1-13

comment:3 Changed 22 months ago by mpickering

I reduced the example to something with 4 files and no dependencies.

To reproduce ghc Repo.hs.

Changed 22 months ago by mpickering

Attachment: repo.zip added

comment:4 Changed 22 months ago by mpickering

This comment was garbage.

Last edited 22 months ago by mpickering (previous) (diff)

comment:5 Changed 22 months ago by mpickering

Owner: set to mpickering

comment:6 Changed 22 months ago by mpickering

I put the test up on phab. https://phabricator.haskell.org/D3535

The problem is in TcPat.find_field_ty but I can't see how to cleanly fix it.

In the renamer we don't care yet whether a record selector is the right one for the data constructor. When we check in the type checker, all the verify is that the OccName of the selector is one of the field labels of the right data constructor. We don't compare Names at all like we should do because of something to do with ORF.

comment:7 Changed 22 months ago by mpickering

Owner: mpickering deleted

comment:8 Changed 22 months ago by mpickering

I think this is exactly #12158 as Simon suggested.

comment:9 Changed 22 months ago by Ben Gamari <ben@…>

In c5b28e06/ghc:

Add a failing test for T13644

The problem originates in TcPat.find_field_ty but I don't know how to
clearnly fix it.

Reviewers: austin, bgamari

Reviewed By: bgamari

Subscribers: rwbarton, thomie

GHC Trac Issues: #13644

Differential Revision: https://phabricator.haskell.org/D3535

comment:10 Changed 21 months ago by Phyx-

Operating System: WindowsUnknown/Multiple

comment:11 Changed 20 months ago by RyanGlScott

#13840 is another occurrence of this.

comment:12 Changed 20 months ago by RyanGlScott

So is #13973.

comment:13 Changed 19 months ago by RyanGlScott

Milestone: 8.4.1

And #14087.

comment:14 Changed 18 months ago by sighingnow

Maybe we should throw a type error when pattern name mismatch, rather than just panic.

Use the case from #13973 as an example,

-- Record.hs
module Record (Record(..)) where

data Record = Record { field1 :: Int, field2 :: Int }

-- Test.hs
module Test (foo) where

import qualified Record as Rec

field2 :: ()
field2 = ()

foo :: Rec.Record -> Int
foo Rec.Record{field2 = field2} = field2

In function translateConPatVec (in compiler/deSugar/Check.hs, during the desugar pass), we can't know whether the pattern label comes from the record fields, or just another ordinary symbol.

When pattern name lookup (lookup name zipped) fails, we could reach a conclusion that name must not be one of the record fields. Then the compiler can throw an error like

Test.hs:11:16: error:
    Record field not in scope: ‘field2’
    Perhaps you meant one of these:
      ‘Rec.field1’ (imported from Record),
      ‘Rec.field2’ (imported from Record),

(Noticing that Record field not in scope has point out that the missing item is a record field label, rather than other ordinary variable, without causing any confusion.)

comment:15 Changed 18 months ago by simonpj

Cc: adamgundry added

Adam, might you look at this, since it seems to be in ORF-land?

comment:16 Changed 18 months ago by adamgundry

@simonpj thanks for pointing me to this ticket.

The problem is in TcPat.find_field_ty but I can't see how to cleanly fix it.

@mpickering part of the fix is surely to compare the selector names (which we have available). I've implemented this in 52110a7966848538583acb65f6e064aadc751260 and it fixes the original test case.

However, the related bug #13847 remains: this patch is too liberal as it accepts examples where the field label is in scope only qualified, but it is used unqualified, and DisambiguateRecordFields is not enabled. Several of the other related bugs demonstrate this, so my patch causes them to be accepted (without a panic, but without an error either).

I'm out of time to investigate further for now, unfortunately.

comment:17 Changed 18 months ago by simonpj

Thanks Adam!

I think you are saying

Right? If so, let's add regression tests for the others; and then close this one.

comment:18 in reply to:  16 Changed 18 months ago by mpickering

Replying to adamgundry:

@simonpj thanks for pointing me to this ticket.

The problem is in TcPat.find_field_ty but I can't see how to cleanly fix it.

@mpickering part of the fix is surely to compare the selector names (which we have available). I've implemented this in 52110a7966848538583acb65f6e064aadc751260 and it fixes the original test case.

However, the related bug #13847 remains: this patch is too liberal as it accepts examples where the field label is in scope only qualified, but it is used unqualified, and DisambiguateRecordFields is not enabled. Several of the other related bugs demonstrate this, so my patch causes them to be accepted (without a panic, but without an error either).

I'm out of time to investigate further for now, unfortunately.

Is it not currently implemented like it is to enabled ORF to work properly? I think your patch changes it back to how it used to work? I can possibly try to create a test case tomorrow.

comment:19 Changed 17 months ago by adamgundry

Differential Rev(s): Phab:D3988
Status: newpatch
Test Case: rename/should_fail/T13644

Looking at this again, I realised that the fix to #13847 is entirely analogous. Diff away!

@mpickering I probably broke this when implementing ORF, but AFAICS there is no ORF-related reason for us to compare field labels here, because we already know the selector names we want.

comment:20 Changed 17 months ago by Ben Gamari <ben@…>

In 72b00c34/ghc:

Identify fields by selector when type-checking (fixes #13644)

Test Plan: new test for #13847, and the test for #13644 now passes

Reviewers: mpickering, austin, bgamari, simonpj

Reviewed By: mpickering, simonpj

Subscribers: simonpj, rwbarton, thomie

GHC Trac Issues: #13644, #13847

Differential Revision: https://phabricator.haskell.org/D3988

comment:21 Changed 17 months ago by bgamari

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