Opened 4 years ago

Last modified 6 months ago

#5333 new bug

Arrow command combinators and infixr cause the desugarer to fail

Reported by: peteg Owned by: ross
Priority: low Milestone: 7.12.1
Component: Compiler (Parser) Version: 7.0.3
Keywords: Cc: peteg42@…, ross@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC rejects valid program Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

The following code exhibits the bug:

{-# LANGUAGE Arrows, NoMonomorphismRestriction #-}
module T where

import Prelude hiding ( id, (.) )
import Control.Arrow

cc1 :: Arrow a => a e b -> a e b -> a e b
cc1 = undefined

-- 'g' fails to compile.
-- g = proc (x, y, z) ->
--   ((returnA -< x) &&& (returnA -< y) &&& (returnA -< z))

-- 'f' compiles:
--   - without an infix declaration
--   - with the infixl declaration
-- and fails with the infixr declaration
infixr 6 `cc1`
-- infixl 6 `cc1`

f = proc (x, y, z) ->
  ((returnA -< x) `cc1` (returnA -< y) `cc1` (returnA -< z))

GHC says:

ghc: panic! (the 'impossible' happened)
  (GHC version 7.0.3 for i386-apple-darwin):
	dsSyntaxTable Not found: base:GHC.Desugar.>>>{v 01W}

Change History (13)

comment:1 Changed 4 years ago by simonpj

  • Cc ross@… added

Ross, might you look at this?

comment:2 Changed 4 years ago by ross

The problem is that the renamer attaches a table of rebindable names to each top-level command (body of proc or argument of a form, including infix operators) before it does the fix-up for right associativity. That re-arrangement moves the left argument out of scope of the table.

One fix would be to add these tables in a separate pass after renaming.

But I was wondering whether having a separate table on each argument of a form is particularly useful, since it's not very easy to have different names in scope there than at the proc.

comment:3 Changed 4 years ago by simonpj

Thanks Ross. Adding a whole new pass just to deal with this very special case isn't very attractive.

In fact, I've moved sharply away from these "syntax tables" in the rest of the compiler. As you'll see, the only use of SyntaxTable is on a HsCmdTop, and I'd love to be rid of it. Instead, in (say) monad comprehensions, the necessary operators are attached directly to the HsStmt constructors, see the SyntaxExpr arguments in StmtLR (in HsExpr) for example. This would mean the problem reported here simply would not arise.

This would not as straightforward for arrows, because in effect we hijack a bunch of HsExpr constructors for use in arrow abstractions (see line 600ff of HsExpr), which says

The legal constructors for commands are:

  = HsArrApp ...                -- as above

  | HsArrForm ...               -- as above

  | HsApp       (HsCmd id)
                (HsExpr id)

  | HsLam       (Match  id)     -- kappa

  -- the renamer turns this one into HsArrForm
  | OpApp       (HsExpr id)     -- left operand
                (HsCmd id)      -- operator
                Fixity          -- Renamer adds fixity; bottom until then
                (HsCmd id)      -- right operand

  | HsPar       (HsCmd id)      -- parenthesised command

  | HsCase      (HsExpr id)
                [Match id]      -- bodies are HsCmd's
                SrcLoc

  | HsIf        (Maybe (SyntaxExpr id)) --  cond function
  					 (HsExpr id)     --  predicate
                (HsCmd id)      --  then part
                (HsCmd id)      --  else part
                SrcLoc

  | HsLet       (HsLocalBinds id)       -- let(rec)
                (HsCmd  id)

  | HsDo        (HsStmtContext Name)    -- The parameterisation is unimportant
                                        -- because in this context we never use
                                        -- the PatGuard or ParStmt variant
                [Stmt id]       -- HsExpr's are really HsCmd's
                PostTcType      -- Type of the whole expression
                SrcLoc

I'm not keen on adding extra fields to all of these constructors. Actually I think it might be best simply to make a new data type for arrows, with these constructors, each decorated with appropriate methods as in StmtLR. The desugaring code is separate anyway. There would be some duplication of typechecking code, not not a lot. But I think the clarity would be worth it; code dealing with arrows would, well, deal with arrows, and other code would not need to worry about the case that it was actually (say) typechecking an arrow abstraction. Decoupling the two would simplify both -- admittedly at the cost of a bit more code. But I feel that the code-sharing we get just isn't paying its way.

If you felt able to look at this it would be fantastic. I'd be happy to have a Skype chat about details, if that would help. Frankly I'm out of my depth with the arrow syntax.

comment:4 Changed 4 years ago by ross

  • Owner set to ross

Yes, I noticed a lot of sighing around the rebindable stuff. It sounds like a good idea to have a separate HsCmd type, with a corresponding variant of Stmt. #5045 would have been caught by the type-checker if that had been in place. But duplicating MatchGroup and friends would be a nuisance. That might be saved if they were generalized to something like

type MatchGroup = MatchGroupG HsExp

data MatchGroupG e id
  = MatchGroup
        [LMatchG e id]
        PostTcType

type LMatch = LMatchG HsExp

and so on.

Splitting the types won't fix the current bug, but it's probably best to do that first and then rework the rebinding for arrows properly. My algorithm for the first part would be to copy everything and then simplify the arrow version.

Unfortunately I'll be online only intermittently until September.

comment:5 Changed 4 years ago by simonpj

That sounds good. So the plan that, when you can, you will

  • First split the data types
  • Then fix the rebinding problem

For MatchGroup I suggest abstracting over something of kind *, rather than over the type constructor. Less brain-strain. But let's discuss when you get to it.

When you have a rough cut (say the data types worked out) it would be good to talk.

Thanks Ross.

Simon

comment:6 Changed 4 years ago by igloo

  • Milestone set to 7.4.1

comment:7 Changed 3 years ago by igloo

  • Milestone changed from 7.4.1 to 7.6.1
  • Priority changed from normal to low

comment:8 Changed 3 years ago by igloo

  • Milestone changed from 7.6.1 to 7.6.2

comment:9 Changed 2 years ago by simonpj@…

commit c3ad38d7dc39ef583ddfb586413baa2e57ca3ee8

Author: Simon Peyton Jones <[email protected]>
Date:   Mon Mar 4 09:40:56 2013 +0000

    Rearrange the typechecking of arrows, especially arrow "forms"
    
    The typechecking of arrow forms (in GHC 7.6) is known to be bogus, as
    described in Trac #5609, because it marches down tuple types that may
    not yet be fully worked out, depending on when constraint solving
    happens.  Moreover, coercions are generated and simply discarded.  The
    fact that it works at all is a miracle.
    
    This refactoring is based on a conversation with Ross, where we
    rearranged the typing of the argument stack, so that the arrows
    have the form
       a (env, (arg1, (arg2, ...(argn, ())))) res
    rather than
       a (arg1, (arg2, ...(argn, env))) res
    as it was before.
    
    This is vastly simpler to typecheck; just look at the beautiful,
    simple type checking of arrow forms now!
    
    We need a new HsCmdCast to capture the coercions generated from
    the argument stack.
    
    This leaves us in a better position to tackle the open arrow tickets
     * Trac #5777 still fails.  (I was hoping this patch would cure it.)
     * Trac #5609 is too complicated for me to grok.  Ross?
     * Trac #344
     * Trac #5333

 compiler/deSugar/Coverage.lhs     |    3 +
 compiler/deSugar/DsArrows.lhs     |  486 +++++++++++++++++++++----------------
 compiler/hsSyn/HsExpr.lhs         |   13 +-
 compiler/hsSyn/HsUtils.lhs        |    7 +-
 compiler/parser/Parser.y.pp       |    4 +-
 compiler/parser/RdrHsSyn.lhs      |    4 +-
 compiler/rename/RnExpr.lhs        |    4 +-
 compiler/rename/RnTypes.lhs       |    2 +-
 compiler/typecheck/TcArrows.lhs   |  272 ++++++++++-----------
 compiler/typecheck/TcHsSyn.lhs    |    6 +-
 docs/users_guide/glasgow_exts.xml |   48 +++--
 11 files changed, 461 insertions(+), 388 deletions(-)

comment:10 Changed 2 years ago by simonpj

  • difficulty set to Unknown

The major refactorings are now in place to let us fix this ticket. In particular,

  • #7071 does the data type refactoring
  • #5609 does the type checker refactoring

Completing the move away from "syntax tables" should be straightforward. I'm happy to advise.

Simon

comment:11 Changed 12 months ago by thoughtpolice

  • Milestone changed from 7.6.2 to 7.10.1

Moving to 7.10.1.

comment:12 Changed 7 months ago by thomie

The testcase from the description compiles fine with HEAD (ghc-7.9.20141125).

comment:13 Changed 6 months ago by thoughtpolice

  • Milestone changed from 7.10.1 to 7.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

Note: See TracTickets for help on using tickets.