Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typed structural equality #154

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Typed structural equality #154

wants to merge 7 commits into from

Conversation

kquick
Copy link
Member

@kquick kquick commented Jun 7, 2023

The structuralEquality TH implementation does not witness the equality of the types involved, which can make it difficult to use for parameterized GADT types. It also cannot correctly generate the comparison slices for parameterized GADTs because there may not be wildcard cases admissable due to type equality, even if there are other constructors with other resulting types.

The impact should be minimal, although it may require the introduction of the ScopedTypeParameters pragma in sources importing and using this code.

kquick added 5 commits June 6, 2023 12:10
Without the change to structuralEquality, the new tests would result in errors
like the following:

test/Test/TH.hs:51:11: error:
    • Couldn't match type ‘a1’ with ‘(:~:) @{k0} a0 a0’
      Expected: Maybe a1
        Actual: Maybe ((:~:) @{k0} a0 a0)
    • ‘a1’ is untouchable
        inside the constraints: "int" ~ "int"
        bound by a pattern with constructor: T3_Int :: Int -> T3 "int",
                 in a case alternative
        at test/Test/TH.hs:51:11-41
    • In the expression: Just Refl
      In the expression:
        if (x1_ab5y == y1_ab5z) then Just Refl else Nothing
      In a case alternative:
          T3_Int y1_ab5z
            -> if (x1_ab5y == y1_ab5z) then Just Refl else Nothing
   |
51 |   (==) = $(structuralEquality [t|T3|] [])
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Older GHC versions do not allow unary tuples.
When generating `structuralEquality` or `structuralTypeEquality`, the former is
used in an `Eq` scenario for `(==) :: a -> a -> Bool` and so if there is a GADT
definition like the following:]

    data T1 a where
       T1_Int :: Int -> T1 Int
       T1_Char :: Char -> T1 Char

then the `(==)` function will only admit `T1_Int` constructors or `T1_Char`
constructors but not both.  However, `TestEquality` will parameterize over the
GADT type parameter and does not require them to match.  As a result, when
generating the splice for comparison:

    \x y ->
      case x of
        T1_Int ->
          case y of
            T1_Int -> ...
            _ -> False    -- << HERE
        T1_Char ->
          case y of
            T1_Char -> ...
            _ -> False    -- << AND HERE

that splice is fine for `TestEquality`, but because the types must match for `Eq`
then the two wildcard clauses marked by HERE comments above are superfluous and
cause warnings/errors.  There may be other explicit matchers (e.g. if T1 also had
a `T1_Nat :: Nat -> T1 Int` constructor) which will be needed, but the wildcard
should only be present for testEquality.

This patch makes the adjustment to allow for the differences in the
structuralEquality and structuralTypeEquality definitions, along with some tests
to demonstrate/exercise those changes.
Copy link
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the PR description, it wasn't obvious to me why this change was necessary in the first place. Here is my attempt to describe the issue, after looking at the -ddump-splices output a bit.

When you use structuralTypeEquality, like in this example:

data T a where
  MkT1 :: T Int
  MkT2 :: T Bool

instance TestEquality T where
  testEquality :: T a -> T b -> Maybe (a :~: b)
  testEquality = $(structuralTypeEquality [t|T|] []) 

Then you will generate code involving GADT pattern-matching. This can interact poorly with type inference in general, but because the type of testEquality mentions Maybe (a :~: b) in its return type, this provides enough information to allow the GADT pattern matches to type-check.

structuralEquality is a minor variation of structuralTypeEquality. If you write this:

instance TestEquality T where
  testEquality :: T a -> T a -> Bool
  testEquality = $(structuralEquality [t|T|] []) 

It is equivalent to writing this:

instance TestEquality T where
  testEquality :: T a -> T a -> Bool
  testEquality = isJust $(structuralTypeEquality [t|T|] [])

This is very similar to the previous example, but with a key difference: the return type is now Bool, which no longer mentions any of the type variables. As a result, GHC's type inference has no way to know what the overall type of the generated GADT pattern matches should be, and therefore the generated code will fail to type-check. (Concretely, GHC will complain that that the a in T a is untouchable.)

One way to fix this issue is to add an explicit type ascription to the generated case expression that makes the return type of the case clear. This requires binding type variables, as they may not yet be in scope:

instance TestEquality T where
  testEquality :: T a -> T a -> Bool
  testEquality = isJust (\arg1 arg2 -> (\(x :: T a) (y :: T b) -> case (x, y) of { ... } :: Maybe (a :~: b)) arg1 arg2)

This is the approach taken in this PR.

Changelog.md Outdated

* Updates Data.Parameterized.TH.GADT.structuralEquality to add type assertions
to cover all type parameters. This change may require the addition of the
@ScopedTypeVariables@ pragma to modules importing this code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses markdown rather than Haddock formatting, so use backticks instead:

Suggested change
@ScopedTypeVariables@ pragma to modules importing this code.
`ScopedTypeVariables` pragma to modules importing this code.

Changelog.md Outdated
@@ -2,6 +2,12 @@

## next -- *TBA*

## 2.1.6.0.100

* Updates Data.Parameterized.TH.GADT.structuralEquality to add type assertions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Updates Data.Parameterized.TH.GADT.structuralEquality to add type assertions
* Updates `Data.Parameterized.TH.GADT.structuralEquality` to add type assertions

Comment on lines +151 to +156
-- However, that result presumes a `TestEquality f where testEquality :: f a ->
-- f b -> Maybe (a :~: b)`. If the GADT has a single type parameter, those
-- types align and there is no problem. If the GADT has multiple type
-- variables, GHC is unsure of which we are making the TestEquality assertion
-- about and we need to help. We actually want to make that assertion over
-- _all_ of the parameters, so given:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the issue here is the fact that the GADT has multiple type variables, but rather it is a type inference issue. Would you be willing to rewrite this comment to explain this aspect of the problem? (The issue description that I left elsewhere in my review could serve as a template for the new comment.)

Also, this comment is pretty long. It might be helpful to split this out into a separate Note so that we don't break up the code with a large number of comment lines.

Comment on lines +181 to +203
gadtParams <- return $ datatypeInstTypes d
arg1Params <- fmap varT <$> newNames "xTy" (length gadtParams)
arg2Params <- fmap varT <$> newNames "yTy" (length gadtParams)
let arg1Ty = foldl appT (conT $ datatypeName d) arg1Params
let arg2Ty = foldl appT (conT $ datatypeName d) arg2Params
#if MIN_VERSION_base(4,14,0)
let mkSuperTy tyList = foldl appT (promotedTupleT (length tyList)) tyList
#else
let mkSuperTy tyList =
if length tyList < 2
then if length tyList == 0
then error "Expected at least one type in structuralEquality"
else head tyList
else foldl appT (promotedTupleT (length tyList)) tyList
#endif
let arg1AllParamTy = mkSuperTy arg1Params
let arg2AllParamTy = mkSuperTy arg2Params

[| \(x :: $(arg1Ty)) (y :: $(arg2Ty)) ->
isJust ($(structuralTypeEquality_ True tpq pats) x y
:: Maybe ($(arg1AllParamTy) :~: $(arg2AllParamTy))
)
|]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit saddened by how complex this function has become, as structuralEquality really ought to be as simple as calling isJust $(structuralTypeEquality ...). Is there any reason that we couldn't just keep the implementation of structuralEquality as-is, but move the explicit type ascriptions to structuralTypeEquality? The type ascriptions aren't strictly required for structuralTypeEquality, but it wouldn't do any harm to add them, I think.

Comment on lines +160 to +169
-- the template haskell here should generate:
--
-- \ (x :: D xt1 xt2 xt3) (y :: D yt1 yt2 yt3) ->
-- isJust ( ($(structuralTypeEquality ... x y))
-- :: Maybe ( '(xt1, xt2, xt3) :~: '(yt1, yt2, yt3) )
-- )
--
-- This will perform the equality check in a way that obtains proof of equality
-- for all of the type parameters. This will require the ScopedTypeVariables
-- pragma, but GHC will happily suggest that if it's missing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm quite skeptical of equating all of the type parameters. Consider this data type:

data S a b where
  MkS1 :: S a Bool
  MkS2 :: S a Double

This use of structuralTypeEquality will typecheck:

f :: S a1 b1 -> S a2 b2 -> Maybe (b1 :~: b2)
f = $(structuralTypeEquality [t|S|] [])

But this would not:

f :: S a1 b1 -> S a2 b2 -> Maybe ('(a1, b1) :~: '(a2, b2))
f = $(structuralTypeEquality [t|S|] [])

And indeed, it's not clear how this could typecheck, as matching on S's data constructors don't provide any way to scrutinize a1 or a2.

Comment on lines 259 to 263
let otherMatchingCons =
let sameContext = (==) `on` constructorContext
in if argsSameType
then filter (sameContext con) multipleCases
else multipleCases
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit subtle. If I understand this correctly, this is an attempt to avoid generating cases that are unreachable because of GADT return types? If so, it would definitely be worth leaving a comment here explaining this subtlety, ideally with an accompanying example.

Also, I doubt that this would be enough to catch everything, since it's possible for types to differ in silly ways (e.g., a ~ Int versus Int ~ a). It would be worth mentioning that this approach is not complete.

@@ -133,10 +135,72 @@ typeVars :: TypeSubstitution a => a -> Set Name
typeVars = Set.fromList . freeVariables


-- | @structuralEquality@ declares a structural equality predicate.
-- | @structuralEquality@ declares a structural equality predicate for a GADT.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, but this comment could use some expansion. For instance, IIUC, this is mostly used to generate == for Eq instances, if so, the Haddock should probably say that.

Comment on lines +151 to +152
-- However, that result presumes a `TestEquality f where testEquality :: f a ->
-- f b -> Maybe (a :~: b)`. If the GADT has a single type parameter, those
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me like structuralEquality calls structuralTypeEquality, which generates (the syntax of) a function of type f a -> f b -> Maybe (a :~: b), meaning that using structuralEquality would not require that tpq have an instance of TestEquality.

-- instance Eq (D a) where
-- (==) = $(structuralEquality [t|D|] []
--
-- Again, this will fail without the template haskell assertion of the target
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, "assertion" implies "checked (and may fail) at run-time". I could see "ascription" or "annotation" here.

Suggested change
-- Again, this will fail without the template haskell assertion of the target
-- Again, this will fail without the template haskell ascription of the target

#else
let mkSuperTy tyList =
if length tyList < 2
then if length tyList == 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how much computation is really done, i.e., how lazy length is, but this form makes it abundantly clear that it is not at all necessary to compute the length:

Suggested change
then if length tyList == 0
then if null tyList

let mkSuperTy tyList =
if length tyList < 2
then if length tyList == 0
then error "Expected at least one type in structuralEquality"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error would be for cases like $(structuralTypeEquality [t|Bool]), right? Perhaps this would be more clear?

Suggested change
then error "Expected at least one type in structuralEquality"
then error "Expected at least one type parameter in structuralEquality"

Comment on lines +191 to +193
then if length tyList == 0
then error "Expected at least one type in structuralEquality"
else head tyList
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternate way of phrasing this that avoids use of the partial function head:

Suggested change
then if length tyList == 0
then error "Expected at least one type in structuralEquality"
else head tyList
then
if case tyList of
[] -> error "Expected at least one type in structuralEquality"
(ty:_) -> tyList

when (isJust (testEquality a b)) $ assertFailure $ show a ++ " == " ++ show b
when (isJust (testEquality a b))
$ assertFailure
$ show a <> " == " <> show b <> " but should not be!"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Looks like these are still strings, so I'd prefer keeping ++ rather than <>, as it's less polymorphic, and so lets the reader know the more specific types involved.

Comment on lines -16 to +25
import Test.Tasty
import Test.Tasty.HUnit
import Test.Tasty
import Test.Tasty.HUnit

import Control.Monad (when)
import Data.Parameterized.Classes
import Data.Parameterized.NatRepr
import Data.Parameterized.TH.GADT
import GHC.TypeNats
import Control.Monad (when)
import Data.Parameterized.Classes
import Data.Parameterized.NatRepr
import Data.Parameterized.SymbolRepr
import Data.Parameterized.TH.GADT
import GHC.TypeNats
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Why did this formatting change? Would be good to have it in a separate commit, if necessary.

Comment on lines +95 to +114
, testCase "Instance tests" $ do
assertEqual "T3_Int values" (T3_Int 5) (T3_Int 5)
assertNotEqual "T3_Int values" (T3_Int 5) (T3_Int 54)
assertEqual "T3_Bool values" (T3_Bool True) (T3_Bool True)
assertNotEqual "T3_Bool values" (T3_Bool True) (T3_Bool False)

-- n.b. the following is not possible: 'T3 "int"' is not a 'T3 "bool"'
-- assertEqual "T3_Int/T3_Bool values" (T3_Int 1) (T3_Bool True)

T3_Int 1 `eqTest` T3_Int 1
T3_Int 1 `neqTest` T3_Int 3
T3_Int 1 `neqTest` T3_Bool True
T3_Bool False `neqTest` T3_Bool True
T3_Bool True `eqTest` T3_Bool True

assertEqual "T4_Int values" (T4_Int @String 5) (T4_Int @String 5)
assertNotEqual "T4_Int values" (T4_Int @String 5) (T4_Int @String 54)

T4_Int @String 1 `eqTest` T4_Int @String 1
T4_Int @String 1 `neqTest` T4_Int @String 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could maybe get more coverage with some property tests:

forall (i j :: Int). (T3_Int i == T3_Int j) == (i == j)
[same for Bool]
forall (i j :: Int). (T3_Int i == T3_Int j) == isJust (testEquality (T3_Int i) (T3_Int j))
...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants