Opened 3 years ago

Closed 2 years ago

#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 3 years ago.
Example to reproduce the bug: use stack build
repo.zip (1.2 KB) - added by mpickering 3 years ago.

Download all attachments as: .zip

Change History (23)

Changed 3 years ago by pjljvdlaar

Attachment: defs.zip added

Example to reproduce the bug: use stack build

comment:1 Changed 3 years ago by simonpj

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

comment:2 Changed 3 years 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 3 years ago by mpickering

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

To reproduce ghc Repo.hs.

Changed 3 years ago by mpickering

Attachment: repo.zip added

comment:4 Changed 3 years ago by mpickering

This comment was garbage.

Last edited 3 years ago by mpickering (previous) (diff)

comment:5 Changed 3 years ago by mpickering

Owner: set to mpickering

comment:6 Changed 3 years 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 3 years ago by mpickering

Owner: mpickering deleted

comment:8 Changed 3 years ago by mpickering

I think this is exactly #12158 as Simon suggested.

comment:9 Changed 3 years 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 3 years ago by Phyx-

Operating System: WindowsUnknown/Multiple

comment:11 Changed 2 years ago by RyanGlScott

#13840 is another occurrence of this.

comment:12 Changed 2 years ago by RyanGlScott

So is #13973.

comment:13 Changed 2 years ago by RyanGlScott

Milestone: 8.4.1

And #14087.

comment:14 Changed 2 years 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 2 years ago by simonpj

Cc: adamgundry added

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

comment:16 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by bgamari

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