enable rebindable fail with overloaded strings
ClosedPublic

Authored by shayne-fletcher-da on Oct 22 2018, 9:38 AM.

Details

Summary

enable rebindable fail with overloaded strings

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Some suggestions. Generally fine!

compiler/rename/RnExpr.hs
2131

Better

Given the code
  foo x = do { Just y <- x; return y }

we expect it to desugar as
  foo x = x >>= \r -> case r of
                        Just y  -> return y
                        Nothing -> fail "Patterm match error"

But with RebindableSyntax and OverloadedStrings, we really want
it to desugar thus:
  foo x = x >>= \r -> case r of
                        Just y  -> return y
                        Nothing -> fail (fromString "Patterm match error")

So, in this case, we synthesize the function
  \x -> fail (fromString x)

(rather than plain 'fail') for the 'fail' operation. This is done in
'getMonadFailOp'.
2154

use HsUtils.nlHsApp which does the noLoc, and noExr and HsPar stuff for you.

2161

Use HsUtils.mkHsLam.

2176

Use mkRnSyntaxExpr. Well, not quite, since it takes a Name not an expression. So make a helper-function in HsExpr that does take an expression, and redefined mkRnSyntaxExpr in terms of it.

compiler/typecheck/TcExpr.hs
1485

Just use this case for all cases (i.e. not matching HsLam), replacing the previous code for HsVar.

compiler/typecheck/TcMatches.hs
958 ↗(On Diff #18405)

This is extremely hacky -- it's totally unclear, at this point, what significance of HsLam is.

Better: just use SynAny for both cases. I think that'll be fine.

Can we add the test case from the Trac ticket, to ensure we don't regress?

simonpj requested changes to this revision.Oct 25 2018, 6:49 AM

Oh yes, we absolutely need a regression test!

This revision now requires changes to proceed.Oct 25 2018, 6:49 AM
compiler/rename/RnExpr.hs
2161

Unfortunately, HsUtils.mkHsLam is fixed on GHcPs whereas here we need GHcRn. I’ll generailze mkHsLam to GhcPass id.

  • fix-trac-15645 : enable rebindable fail with overloaded strings
shayne-fletcher-da added a comment.EditedOct 28 2018, 5:54 AM

It seems this patch has given rise to a test failure in tests/rebindable/rebindable1.hs. I will investigate.

*** Core Lint errors : in result of Desugar (before optimization) ***
<no location info>: warning:
    In the expression: fail
                         @ (Any -> Any)
                         (unpackCString#
                            "Pattern match failure in do expression at testsuite/tests/rebindable/rebindable1.hs:39:17-22"#)
    Argument value doesn't match argument type:
    Fun type: Any -> Any
    Arg type: [Char]
    Arg:
        unpackCString#
          "Pattern match failure in do expression at testsuite/tests/rebindable/rebindable1.hs:39:17-22"#
bgamari requested changes to this revision.Oct 28 2018, 12:37 PM

Bumping out of merge queue while @shayne-fletcher-da investigates.

This revision now requires changes to proceed.Oct 28 2018, 12:37 PM
  • fix-trac-15645 : argument shape for fail op is always stringTy

@bgamari Diff 18525 submitted fixing the regression so we should be unblocked now!

@simonpj Regarding your note "Better: just use SynAny for both cases. I think that'll be fine.", it turns out that I was just being silly. It's not wrong to pass stringTy in all cases there so that change is reverted and the original, unmodified code remains. However, do please note that if SynAny is passed instead, it gives rise to a Core Lint error on tests/rebindable/rebindable1.hs

*** Core Lint errors : in result of Desugar (before optimization) ***
<no location info>: warning:
    In the expression: fail
                         @ (Any -> Any)
                         (unpackCString#
                            "Pattern match failure in do expression at testsuite/tests/rebindable/rebindable1.hs:39:17-22"#)
    Argument value doesn't match argument type:
    Fun type: Any -> Any
    Arg type: [Char]
    Arg:
        unpackCString#
          "Pattern match failure in do expression at testsuite/tests/rebindable/rebindable1.hs:39:17-22"#

I mention this just in case its hinting at a latent problem!

ndmitchell added inline comments.Oct 29 2018, 12:11 PM
compiler/typecheck/TcExpr.hs
1475

Very minor: Redundant brackets here.

  • fix-trac-15645 : remove redundant parens
shayne-fletcher-da marked an inline comment as done.Oct 29 2018, 12:29 PM
shayne-fletcher-da marked 6 inline comments as done.Oct 29 2018, 12:51 PM
Harbormaster edited this Differential Revision.Oct 29 2018, 1:04 PM
Harbormaster failed remote builds in B25027: Diff 18526!

OK, I'm content, thank you

Oh: one other thing: should we add anything to the user manual for rebindable syntax? I suspect 'yes'

Oh: one other thing: should we add anything to the user manual for rebindable syntax? I suspect 'yes'

I would have said not as a result of this patch. I view this as a bug fix, not a new feature, it's just considering generated string literals to be "string literals".

That said, the section of the manual on rebindable syntax forgot to mention fromString entirely, so that deserves remedying.

  • fix-trac-15645 : update user guide

As far as I know, this is good to go (@bgamari - perhaps this can go back into the merge queue now?). Please let me know if there's any way I can be of further help!

@bgamari - what else needs to be done to get this code merged? @simonpj - can you rereview with the test included (I think all your comments have been addressed).

simonpj accepted this revision.Mon, Nov 26, 5:37 AM

Yes, fine with me, thank you.

bgamari accepted this revision.Mon, Nov 26, 8:48 AM

Looks reasonable to me. Thanks for the ping @ndmitchell and for your work, @shayne-fletcher-da. My apologies for the long silence!

This revision is now accepted and ready to land.Mon, Nov 26, 8:48 AM
This revision was automatically updated to reflect the committed changes.