Opened 7 weeks ago

Closed 6 weeks ago

#15815 closed bug (fixed)

problem with splicing type into constraint

Reported by: int-e Owned by: RyanGlScott
Priority: highest Milestone: 8.6.2
Component: Template Haskell Version: 8.6.1
Keywords: Cc: int-index
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC rejects valid program Test Case: th/T15815
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D5274
Wiki Page:

Description (last modified by int-e)

Consider the following two-module example. (as gist: https://gist.github.com/int-e/a666991423c10150bd99dd0e874d6150)

{-# LANGUAGE TemplateHaskell #-}
module A where

mkFoo tyQ = [d|
    foo :: a ~ $(tyQ) => a
    foo = undefined
|]
{-# LANGUAGE TemplateHaskell, GADTs #-}
module B where

import A

mkFoo [t| Int -> Int |]

This loads fine in ghc-8.4.2, but with ghc-8.6.1 and current head (commit 23956b2ada690c78a134fe6d149940c777c7efcc), the splice goes wrong:

$ ghci B.hs
GHCi, version 8.6.1: http://www.haskell.org/ghc/  :? for help
[1 of 2] Compiling A                ( A.hs, interpreted )
[2 of 2] Compiling B                ( B.hs, interpreted )

B.hs:7:1: error:
    • Expected a type, but ‘a_a4uD ~ Int’ has kind ‘Constraint’
    • In the type signature: foo :: a_a4uD ~ Int -> Int => a_a4uD
  |
7 | mkFoo [t| Int -> Int |]
  | ^^^^^^^^^^^^^^^^^^^^^^^
Failed, one module loaded.
*A> 

As a workaround one can define a type alias for the Int -> Int type.

Change History (13)

comment:1 Changed 7 weeks ago by int-e

Description: modified (diff)

comment:2 Changed 7 weeks ago by RyanGlScott

Owner: set to RyanGlScott
Priority: normalhighest

This appears to be my fault, since this regression was introduced in commit b9483981d128f55d8dae3f434f49fa6b5b30c779 (Remove HsEqTy and XEqTy). I'll take a look.

comment:3 Changed 7 weeks ago by RyanGlScott

Simpler example that only requires one module:

{-# LANGUAGE GADTs #-}
{-# LANGUAGE TemplateHaskell #-}
module Bug where

$([d| foo :: a ~ (Int -> Int) => a
      foo = undefined |])
$ /opt/ghc/8.4.4/bin/ghc Bug.hs
[1 of 1] Compiling Bug              ( Bug.hs, Bug.o )

$ /opt/ghc/8.6.1/bin/ghc Bug.hs
[1 of 1] Compiling Bug              ( Bug.hs, Bug.o )

Bug.hs:5:3: error:
    • Expected a type, but ‘a_a44c ~ Int’ has kind ‘Constraint’
    • In the type signature: foo :: a_a44c ~ Int -> Int => a_a44c
  |
5 | $([d| foo :: a ~ (Int -> Int) => a
  |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...

Or, using fewer quotes:

{-# LANGUAGE GADTs #-}
{-# LANGUAGE TemplateHaskell #-}
module Bug where

import Language.Haskell.TH

$(do foo <- newName "foo"
     a   <- newName "a"
     pure [ SigD foo (ForallT [] [AppT (AppT EqualityT (VarT a)) (AppT (AppT ArrowT (ConT ''Int)) (ConT ''Int))] (VarT a))
          , ValD (VarP foo) (NormalB (VarE 'undefined)) []
          ])
$ /opt/ghc/8.4.4/bin/ghc Bug.hs
[1 of 1] Compiling Bug              ( Bug.hs, Bug.o )

$ /opt/ghc/8.6.1/bin/ghc Bug.hs
[1 of 1] Compiling Bug              ( Bug.hs, Bug.o )

Bug.hs:7:3: error:
    • Expected a type, but ‘a_a452 ~ Int’ has kind ‘Constraint’
    • In the type signature: foo :: a_a452 ~ Int -> Int => a_a452
  |
7 | $(do foo <- newName "foo"
  |   ^^^^^^^^^^^^^^^^^^^^^^^...

comment:4 Changed 7 weeks ago by RyanGlScott

Cc: goldfire added

I believe I understand what is happening here. The problem is that when you roundtrip the following declaration through Template Haskell:

$([d| foo :: a ~ (Int -> Int) => a
      foo = undefined |])

Then all parentheses are stripped away when converting this to the GHC AST. This has important consequences when processing foo's type signature. Before the offending commit (that I linked to in comment:2), the context of foo's type signature would become:

HsEqTy a (HsOpTy Int (->) Int)

But after the offending commit, this becomes:

HsOpTy a (~) (HsOpTy Int (->) Int)

Upon first glance, these would appear to be identical. But as it turns out, HsEqTy actually had special treatment in the renamer, which means that it was renamed as though one had implicitly added HsParTys around both of its arguments. On the other hand, when GHC renaming sequences of HsOpTys (as in the second example), no such thing happens. In essence, that AST corresponds to:

a ~ Int -> Int

GHC thinks that should be parenthesized as:

(a ~ Int) -> Int

Which leads to the error that we see in this ticket.

A quick-and-dirty way to fix this would be to change cvtTypeKind such that when we convert an EqualityT, we parenthesize ~'s arguments if they aren't parenthesized. In other words, apply this patch:

  • compiler/hsSyn/Convert.hs

    diff --git a/compiler/hsSyn/Convert.hs b/compiler/hsSyn/Convert.hs
    index 8b12a78..74a6011 100644
    a b cvtTypeKind ty_str ty 
    14371437
    14381438           EqualityT
    14391439             | [x',y'] <- tys' ->
    1440                    returnL (HsOpTy noExt x' (noLoc eqTyCon_RDR) y')
     1440                   let px = parenthesizeHsType opPrec x'
     1441                       py = parenthesizeHsType opPrec y'
     1442                   in returnL (HsOpTy noExt px (noLoc eqTyCon_RDR) py)
    14411443             | otherwise ->
    14421444                   mk_apps (HsTyVar noExt NotPromoted
    14431445                            (noLoc eqTyCon_RDR)) tys'

That makes the original program compile again, although it's just applying a bandage over a deeper wound—namely, that TH conversion strips away parentheses in the first place.

goldfire, weren't you looking into fixing the parentheses issue in #15760? If so, perhaps your patch there would make for a more elegant fix for this ticket. On the other hand, if that still needs some work, I could whip up a stopgap solution now (based on the code above) so that this could be backported to a patch release if necessary.

comment:5 Changed 6 weeks ago by int-index

Cc: int-index added

Ryan, why do you believe that preserving parentheses would solve this issue? The original code does not have any parentheses (unlike your minimised version).

The issue here appears to be manyfold

  1. We reassociate type operators according to their fixities, so with a ~ $(tyQ) and [t| Int -> Int |] from the original report we get a ~ Int -> Int (no parens!) which gets correctly associated as (a ~ Int) -> Int. That's not the behaviour I would expect, but I've just learned that it's documented in Note [Converting UInfix].
  2. However, we map all three of (~) a b, a ~ b, and (a ~ b) to AppT (AppT EqualityT a) b. So we discard both parenthesization information (as you noted) and whether equality was applied in an infix or a prefix fashion.
  3. When we convert back to HsType, we use HsOpTy if there are two operands, even if (~) was prefix in the original code. This means that even if we fix #15760, we will still get incorrect behaviour in the (~) a b case.

For expressions, we have a distinction between UInfixE and InfixE:

  • UInfixE gets reassociated as necessary (documented in Note [Operator association])
  • InfixE gets parenthesised as necessary (documented in Note [Converting UInfix])

In types we have a similar distinction – UInfixT and InfixT. The former must get reassociated, and the latter parenthesised.

NB. Looking at the code that handles InfixT it appears broken to me (because unlike code for InfixE, it does not seem to parenthesise). But this is either a bug or I'm missing something. For the sake of the argument let's assume InfixT is treated the same way as InfixE – by parenthesising arguments as necessary.

The question is: do we treat AppT (AppT (EqualityT a)) b as a moral equivalent of UInfixT or InfixT? The bug seems to be that we used to treat it as InfixT (or at least as InfixT is supposed to work), now we treat it as UInfixT. Your patch with adding parenthesizeHsType seems to make it treated like InfixT again, but ideally the fix should be to

  • stop confusing (~) a b and a ~ b: map the former to AppT (AppT (EqualityT a)) b and the latter to InfixT a EqualityT b
  • fix the treatment of InfixT in cvtTypeKind – parenthesise as necessary in expressions
  • drop this silly special case:
               | [x',y'] <- tys' ->
                     returnL (HsOpTy noExt x' (noLoc eqTyCon_RDR) y')
    
Last edited 6 weeks ago by int-index (previous) (diff)

comment:6 Changed 6 weeks ago by RyanGlScott

Cc: goldfire removed
Milestone: 8.6.2

You're right, I misspoke about #15760 fixing this issue. Forget I said anything about that.

Your fix smells correct to me, and if there weren't any other extenuating circumstances, I'd endorse it as the one true solution. Unfortunately, there's a bit of a thorny issue in that this code no longer works on GHC 8.6, and I don't see a simple way to work around the issue at the moment. We need to backport something to fix this, but the question is what.

Alas, changing the way we desugar infix types to use InfixT would constitute a breaking change in practice, so I'm reluctant to backport that. There is quite a bit of code in the wild which, for better or worse, assumes that InfixT only ever appears in user written code (i.e., InfixT never appears in desugared or reified TH ASTs). See this function from th-abstraction as one example. I'm not sure what exactly would happen if we started feeding that function InfixTs, but I imagine that something or another would change at runtime, which would be awful for a point release.

Therefore, I'm inclined to adopt the patch in comment:4 as a crude (and ideally, temporary) fix for this issue in an 8.6 point release (hopefully 8.6.2), and work towards implementing your more robust solution for 8.8. Does that sound reasonable?

comment:7 in reply to:  6 ; Changed 6 weeks ago by int-index

Sounds reasonable to me, although I'm unsure if we should target 8.8 for the more robust fix rather than 8.10.

When #15760 is fixed, decomposeType in th-abstraction that you linked would have to start to care about parentheses as well, so I believe it's better to do both changes at once. And since the merge window for 8.8 closes in a couple of days, and these are breaking changes, then a more realistic target is probably 8.10 – what do you think?

comment:8 in reply to:  7 Changed 6 weeks ago by RyanGlScott

Replying to int-index:

When #15760 is fixed, decomposeType in th-abstraction that you linked would have to start to care about parentheses as well

Egads, I hadn't thought about that. That's a very good point, and I agree that it would be best to try and ship these fixes together.

And since the merge window for 8.8 closes in a couple of days

Wow. Really? That deadline crept up faster than I ever thought possible...

then a more realistic target is probably 8.10 – what do you think?

Sure. I'd be fine with putting this off later than 8.8 if need be (be it 8.10, 8.12, or whenever).

comment:9 Changed 6 weeks ago by RyanGlScott

Differential Rev(s): Phab:D5274
Status: newpatch

I've submitted Phab:D5274 with the short-term fix from comment:4 (that ought to be backported to 8.6.2).

int-index, can you open a new ticket tracking the long-term goal of desugaring/reifying uses of infix types to InfixT?

comment:10 Changed 6 weeks ago by int-index

Wow. Really? That deadline crept up faster than I ever thought possible...

Sorry to mislead – I thought "cut release branch in November 2018" meant November 1st, turns out it's November 18th, so there are three weeks left.

int-index, can you open a new ticket tracking the long-term goal of desugaring/reifying uses of infix types to InfixT?

I created #15824.

comment:11 Changed 6 weeks ago by Ben Gamari <ben@…>

In b8a797e/ghc:

Fix #15815 by parenthesizing the arguments to infix ~

An unfortunate consequence of commit
b9483981d128f55d8dae3f434f49fa6b5b30c779 (`Remove HsEqTy and XEqTy`)
is infix uses of `~` in TH quotes now desugar differently than
before. In particular, we have that:

```haskell
a ~ (Int -> Int)
```

Now desugars to:

```haskell
HsOpTy a (~) (HsOpTy Int (->) Int)
```

Which GHC interprets as being:

```haskell
a ~ Int -> Int
```

Or, equivalently:

```haskell
(a ~ Int) -> Int
```

Which is different than what was intended! This is the cause
of #15815.

All of this has revealed that we likely need to renovate the way we
desugar infix type operators to be more consistent with the treatment
for infix expressions (see
https://ghc.haskell.org/trac/ghc/ticket/15815#comment:5 for more on
this.) Doing so would constitute a breaking change, however, so we
will likely want to wait until another major GHC release to do this.

In the meantime, this patch offers a non-invasive change to the way
that infix uses of `~` are desugared. This makes the program
in #15815 compile again by inserting extra `HsParTy`s around the
arguments to `~` if they are lacking them.

Test Plan: make test TEST=T15815

Reviewers: int-index, goldfire, bgamari

Reviewed By: int-index

Subscribers: int-e, rwbarton, carter

GHC Trac Issues: #15815

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

comment:12 Changed 6 weeks ago by RyanGlScott

Status: patchmerge
Test Case: th/T15815

comment:13 Changed 6 weeks ago by bgamari

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