Opened 20 months ago

Closed 20 months ago

Last modified 19 months ago

#11167 closed bug (fixed)

Fixity of field-deconstructors incorrect

Reported by: hvr Owned by: kanetw
Priority: highest Milestone: 8.0.1
Component: Compiler Version: 7.11
Keywords: ORF Cc: adamgundry
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): phab:D1585
Wiki Page:

Description

The example below

module Foo where

data SomeException

newtype ContT r m a = ContT {runContT :: (a -> m r) -> m r}

runContT' :: ContT r m a -> (a -> m r) -> m r
runContT' = runContT

catch_ :: IO a -> (SomeException -> IO a) -> IO a
catch_ = undefined

-- has type error
foo :: IO ()
foo = (undefined :: ContT () IO a) `runContT` (undefined :: a -> IO ()) `catch_` (undefined :: SomeException -> IO ()) 

-- typechecks
foo' :: IO ()
foo' = (undefined :: ContT () IO a) `runContT'` (undefined :: a -> IO ()) `catch_` (undefined :: SomeException -> IO ())

works with GHC 7.10 but breaks with GHC HEAD currently with:

foo.hs:15:47: error:
    • Couldn't match expected type ‘a0 -> IO ()’
                  with actual type ‘IO ()’
    • In the second argument of ‘runContT’, namely
        ‘(undefined :: a -> IO ())
         `catch_` (undefined :: SomeException -> IO ())’
      In the expression:
        runContT
          (undefined :: ContT () IO a)
          (undefined :: a -> IO ())
          `catch_` (undefined :: SomeException -> IO ())
      In an equation for ‘foo’:
          foo
            = runContT
                (undefined :: ContT () IO a)
                (undefined :: a -> IO ())
                `catch_` (undefined :: SomeException -> IO ())

foo.hs:15:48: error:
    • Couldn't match expected type ‘IO ()’
                  with actual type ‘a1 -> IO ()’
    • In the first argument of ‘catch_’, namely
        ‘(undefined :: a -> IO ())’
      In the second argument of ‘runContT’, namely
        ‘(undefined :: a -> IO ())
         `catch_` (undefined :: SomeException -> IO ())’
      In the expression:
        runContT
          (undefined :: ContT () IO a)
          (undefined :: a -> IO ())
          `catch_` (undefined :: SomeException -> IO ())

Change History (18)

comment:1 Changed 20 months ago by hvr

Version: 7.10.27.11

comment:2 Changed 20 months ago by kanetw

Owner: set to kanetw

comment:3 Changed 20 months ago by kanetw

Duplicate record fields (phab:D761, b1884b0e62f62e3c0859515c4137124ab0c9560e) broke this. Currently trying to find what exactly broke it.

Last edited 20 months ago by kanetw (previous) (diff)

comment:4 Changed 20 months ago by hvr

Cc: adamgundry added

cc'ing Adam as his patch seems to have caused this regression

comment:5 Changed 20 months ago by kanetw

Ok, so the source of the problem lies in RnExpr.hs:rnExpr (OpApp ...), there's a missing case for when we have a HsRecFld when getting fixity.

But now we've got a problem. We're running this during renaming, so we don't have the corresponding Id yet, only a Name. If the record field is Unambiguous, that's not a problem -- PostRn Name Name = Name. But if it's Ambiguous, PostTc Name Name = PlaceHolder, i.e. we have no way of getting the name.

The Unambiguous case is easy, and that'd solve this ticket. But we need to deal with the Ambiguous case, too.

Last edited 20 months ago by kanetw (previous) (diff)

comment:6 Changed 20 months ago by kanetw

I see a few possible solutions for the ambig. case:

  • Warn and default fixity to defaultFixity = infixl 9
  • Disallow infix usage of ambiguous record fields completely (abort with an error).
  • As long the fixity of each overloaded record is identical, use it. Otherwise do one of the above.

I don't like any of these.

comment:7 Changed 20 months ago by ezyang

Another possibility is if the fixity is ambiguous, allow a user to locally override the fixity (and thus disambiguating), adding some sort of "local fixity" declaration.

comment:8 Changed 20 months ago by adamgundry

Argh, this is awkward. Thanks for tracking this down. Given the proximity of the RC, I'm inclined not to add a new declaration form for the time being. Rather, my vote would be to use the common fixity if there is one, or fail with an error if there isn't. After all, it isn't possible to declare duplicate selectors with different fixities in a single module, and for imported ones the user can always disambiguate using the module system.

Last edited 20 months ago by adamgundry (previous) (diff)

comment:9 Changed 20 months ago by simonpj

Owner: changed from kanetw to adamgundry

I'm puzzled. We only use Ambiguous to defer to the type checker if the occurrence is, well, ambiguous. But here it isn't: there is only one runContT in scope. Ah: it's because, as kanetw points out, there's a missing case in the OpApp case of rnExpr. So we could fix that at least, and then this example would work. Let's do that anyway. That will fix the regression.

Now we'd only have a problem if (a) we have -XOverloadedRecordFields and (b) the selector occurrence really was ambiguous.

And then I suppose that the right thing to do is to fail (at least if the fixities differ) saying "Ambiguous fixity for runContT" or something like that.

Stuff needs to be fixed in the user manual too, to explain this.

Simon

comment:10 Changed 20 months ago by kanetw

Adam, are you going to handle this or should I finish?

comment:11 Changed 20 months ago by adamgundry

@kanetw since you've made a start, if you're happy to finish this off I'd greatly appreciate it! I'm happy to review, update the user manual or otherwise help out.

comment:12 Changed 20 months ago by kanetw

Owner: changed from adamgundry to kanetw

Ok, I'll handle it then.

comment:13 Changed 20 months ago by kanetw

While writing tests I've noticed that infix declarations for record fields are broken (another regression to 7.10.2). I'll make a new ticket for that.

E: See #11173

Last edited 20 months ago by kanetw (previous) (diff)

comment:14 Changed 20 months ago by kanetw

Differential Rev(s): phab:D1585

Made a diff. I'll update the user manual later.

comment:15 Changed 20 months ago by hvr

Status: newpatch

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

In 6e56ac58/ghc:

Fix infix record field fixity (#11167 and #11173).

This extends D1585 with proper support for infix duplicate record
fields.  In particular, it is now possible to declare record fields as
infix in a module for which `DuplicateRecordFields` is enabled, fixity
is looked up correctly and a readable (although unpleasant) error
message is generated if multiple fields with different fixities are in
scope.

As a bonus, `DEPRECATED` and `WARNING` pragmas now work for
duplicate record fields. The pragma applies to all fields with the
given label.

In addition, a couple of minor `DuplicateRecordFields` bugs, which were
pinpointed by the `T11167_ambig` test case, are fixed by this patch:

  - Ambiguous infix fields can now be disambiguated by putting a type
    signature on the first argument

  - Polymorphic type constructor signatures (such as `ContT () IO a` in
    `T11167_ambig`) now work for disambiguation

Parts of this patch are from D1585 authored by @KaneTW.

Test Plan: New tests added.

Reviewers: KaneTW, bgamari, austin

Reviewed By: bgamari

Subscribers: thomie, hvr

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

GHC Trac Issues: #11167, #11173

comment:17 Changed 20 months ago by bgamari

Resolution: fixed
Status: patchclosed

comment:18 Changed 19 months ago by adamgundry

Keywords: ORF added
Note: See TracTickets for help on using tickets.