Opened 4 months ago

Closed 5 weeks ago

#15645 closed bug (fixed)

TypeChecking of Monad patterns incorrect with RebindableSyntax and OverloadedStrings

Reported by: NeilMitchell Owned by: shayne-fletcher-da
Priority: normal Milestone: 8.8.1
Component: Compiler Version: 8.4.3
Keywords: GHCProposal Cc: ndmitchell@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC rejects valid program Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D5251
Wiki Page:

Description

Using GHC 8.4.3, I'd expect the following to work:

{-# LANGUAGE RebindableSyntax, OverloadedStrings #-}

module Fail where

import Prelude hiding (fail)

foo x = do
    Just y <- x
    return y

newtype Text = Text String

fail :: Text -> a
fail (Text x) = error x

fromString :: String -> Text
fromString = Text

But it fails with:

Fail.hs:8:5-15: error:
    * Couldn't match expected type `[Char]' with actual type `Text'
    * In a stmt of a 'do' block: Just y <- x
      In the expression:
        do Just y <- x
           return y
      In an equation for `foo':
          foo x
            = do Just y <- x
                 return y
  |
8 |     Just y <- x
  |     ^^^^^^^^^^^

Given the enabled extensions, I'd expect foo to desugar as:

foo x = x >>= \v -> case v of
    Just y -> return y
    _ -> fail (fromString "pattern match error")

But looking at TcMatches.tcMonadFailOp it checks the fail operation (which is literally fail) takes an argument of type tyString (e.g. [Char]). One way around that would be to make the "fail-op" being passed around be fail . fromString if the appropriate extensions are enabled.

Change History (16)

comment:1 Changed 4 months ago by simonpj

Could be. But would it not be simpler for you to provide a fail with type String -> m a? It's not hard to do! I'm not seeing a strong motivation for doing this in the compiler.

comment:2 Changed 4 months ago by NeilMitchell

The use case is for Prelude replacement modules that seek to switch the type of [Char] to something like Text. A standard way is to define your own Monad/MonadFail class which has fail :: Text -> m a in it. If that class is forced to have fail :: [Char] -> m a instead (as it is now) then all your users have to implement a function working on [Char], even though for everything else in your custom library, they never see [Char] and your custom Prelude has no other [Char] related functions.

In the particular example I'm working on, the Char type has been eliminated entirely, aside from the fail function.

comment:3 Changed 4 months ago by simonpj

But all you have to do is define fail = myFail . fromString and you are done.

I suppose you are going to say that you don't want two variants of fail in scope. That is a bit more convincing. But now you may get new mysterious messages about missing IsString instances arising from invisible code.

Make a GHC proposal!

comment:4 Changed 4 months ago by NeilMitchell

Indeed, you can't use the name fail, which is unfortunate. The mysterious messages about things already happens - I get an error about [Char] vs Text - so the IsString thing is just the same consequence.

I'd view this as a bug fix (overloaded strings should imply even generated strings are overloaded, if they are passed to user-controlled functions), but happy to go through GHC proposal process.

comment:5 Changed 4 months ago by simonpj

I'd view this as a bug fix (overloaded strings should imply even generated strings are overloaded, if they are passed to user-controlled functions), but happy to go through GHC proposal process.

Ah, now that is a much solider point! Thanks.

comment:6 Changed 4 months ago by NeilMitchell

BTW, I'm happy to get the work done, if there's acceptance that it really is a bug, and what it should do. Implementation notes follow (only read them if we're agreed it is a bug, if not I'll transfer them to a proposal).

There's still an open question of whether the fromString is injected only when you have OverloadedStrings and RebindableSyntax (and thus a user-controlled fail), or always for OverloadedStrings - my view is likely only when both are enabled. This would then be consistent with not desugaring pattern matches to fromString, since patError isn't user-controlled.

My fix would be that getFailFunction and failFunction in RnExpr.hs should be changed so that if OverloadedStrings is enabled then the fail_op would be fail . fromString rather than just fail as it is now.

comment:7 Changed 4 months ago by simonpj

The notes look ok to me. I do think a (small) proposal is the right way to proceed.

comment:9 Changed 4 months ago by RyanGlScott

Keywords: GHCProposal added

comment:10 Changed 4 months ago by shayne-fletcher-da

Owner: set to shayne-fletcher-da

comment:11 Changed 3 months ago by NeilMitchell

Differential Rev(s): D5251

Proposal was accepted. Patch submitted at https://phabricator.haskell.org/D5251

comment:12 Changed 3 months ago by potato44

Differential Rev(s): D5251Phab:D5251

comment:13 Changed 3 months ago by shayne-fletcher-da

Updated revision to Diff 18486.

comment:14 Changed 7 weeks ago by NeilMitchell

Status: newpatch

comment:15 Changed 5 weeks ago by Ben Gamari <ben@…>

In 8a4edd1/ghc:

Enable rebindable fail with overloaded strings

Summary: enable rebindable fail with overloaded strings

Reviewers: bgamari, simonpj

Reviewed By: bgamari, simonpj

Subscribers: simonpj, ndmitchell, rwbarton, carter

GHC Trac Issues: #15645

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

comment:16 Changed 5 weeks ago by bgamari

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