Saturate higher kinded types in `Coercible` constraints by kl0tl · Pull Request #3893 · purescript/purescript

kl0tl · 2020-05-30T12:41:07Z

This PR implements the type application rule mentioned by the Safe Zero-cost Coercions for Haskell paper in section 2.8 Supporting higher order polymorphism:

► If Coercible t1 t2, where t1, t2 :: k1 → k2, then Coercible (t1 x) (t2 x)

Fix #3889.

lib/purescript-ast/src/Language/PureScript/Environment.hs

kl0tl · 2020-05-30T12:42:29Z

src/Language/PureScript/TypeChecker/Entailment.hs

-      TypeConstructor _ tyName -> do
-        -- If the first argument is a plain newtype (e.g. @newtype T = T U@ and
-        -- the constraint @Coercible T b@), look up the type of its wrapped
-        -- field and yield a new wanted constraint in terms of that type
-        -- (@Coercible U b@ in the example).
-        (_, wrappedTy, _) <- lookupNewtypeConstructor env tyName
-        pure [Constraint nullSourceAnn C.Coercible [] [wrappedTy, b] Nothing]


I took the liberty to simplify coercibleWanteds by removing the case expression because this first case is a simplification of when the newtype has parameters.

lib/purescript-ast/src/Language/PureScript/Types.hs

src/Language/PureScript/TypeChecker/Entailment.hs

hdgarrood · 2020-07-11T18:42:32Z

@kl0tl I think this is probably the next biggest showstopping Coercible bug to fix? What do you think about getting this merged next?

src/Language/PureScript/TypeChecker/Entailment.hs

kl0tl · 2020-07-11T23:33:14Z

Agreed! This is the biggest one left to merge I think.

Would you then prefer that I update #3878 to solve Coercible constraints on rows instead of having an ad-hoc case for records or that I open another pull request?

hdgarrood · 2020-07-12T00:20:53Z

Up to you, I don’t have a preference either way.

…inds

hdgarrood · 2020-08-23T21:24:09Z

src/Language/PureScript/TypeChecker/Entailment.hs

+    coercibleWanteds env a b
+      | (TypeConstructor _ aTyName, _, axs) <- unapplyTypes a
+      , (TypeConstructor _ bTyName, _, bxs) <- unapplyTypes b
+      , Just (aTyKind, _) <- M.lookup aTyName $ types env


I feel like if this lookup fails, an internal compiler error might be more appropriate than a generic "no instance found" error?

You‘re right, both type constructors should have been inserted into the environment at this point. I‘ve thrown "coercibleWanteds: type lookup failed" on failed lookups here and in the next guard.

hdgarrood · 2020-08-23T22:00:41Z

src/Language/PureScript/TypeChecker/Entailment.hs

+      , (aks, kind) <- unapplyKinds aTyKind
+      , (bks, _) <- unapplyKinds bTyKind
+      , length axs < length aks
+      , length bxs < length bks = do


Can I run my understanding of what's going on here past you to check it's accurate? So axs is the list of type arguments that a has, and aks is the list of kinds of those arguments (and likewise for b). We know that length axs <= length aks, because otherwise we would have had a kind unification error by now, so the possibilities are that length axs might be less than length aks (if a isn't fully applied), or otherwise it will be equal to length aks (if it is fully applied). And again, same for b.

I don't think it should be possible that only one of the two arguments is fully applied, right? Otherwise we would have had a kind unification error on line 400? Does it follow that one of these length checks is redundant?

That is, I feel like it should follow from a and b having the same kind that length aks - length axs = length bks - length bxs

You‘re absolutely right and the length bxs < length bks check is indeed redundant now that kinds are checked in solveCoercible. I‘ve removed it and also another redundant not (null bxs) check in the next guard (if both type constructors have the same name and the first one has no arguments then it would be ill-kinded for the second one to have arguments).

hdgarrood · 2020-08-23T22:54:05Z

src/Language/PureScript/TypeChecker/Entailment.hs

+          -- in the constraint @Coercible (D a) (D a')@), yield a new wanted
+          -- constraint in terms of the types saturated with the same variables
+          -- (e.g. @Coercible (D a t0) (D a' t0)@ in the exemple).
+          tys <- traverse freshTypeWithKind aks


Will this work if the type constructors in use in a and b have different numbers of arguments? I'm wondering if this should instead be something like replicate (length aks - length axs) freshTypeWithKind, and then get rid of the drop on the following lines. For example, if we have, say

MkX :: X -> Type -> Type MkYY :: Y -> Y -> Type -> Type SomeX :: X SomeY :: Y

and the two arguments a and b we're dealing with are MkX SomeX and MkYY SomeY SomeY, then I think we will have

axs = [ SomeX ] bxs = [ SomeY, SomeY ] aks = [ X, Type ] bks = [ Y, Y, Type ]

In this case, tys will have the same number of elements as aks, i.e. 2, and so we'll have

drop (length axs) tys = [ t0 ] drop (length bxs) tys = []

so we end up recursing with

a = MkX SomeX t0 b = MkYY SomeY Some Y

which then leads to a kind unification error, even though the user might not have written anything ill-kinded? Let me know if this makes any sense.

Very well spotted! Thank you for taking the time to write such a detailed example 🙇

Sure! I was mainly doing it for my own benefit initially, to try to check I had understood it properly (and because this was too much to keep in my head at once) haha

Would you mind adding a test which would catch this too, when you get a chance?

I added tests/purs/failing/CoercibleHigherKindedData.purs and a case inspired by #3893 (comment) to tests/purs/passing/Coercible.purs.

hdgarrood · 2020-08-23T22:58:43Z

src/Language/PureScript/TypeChecker/Entailment.hs

+          replaceTySyns = replaceAllTypeSynonymsM tySynMap kindMap
+      (a', kind) <- lift $ replaceTySyns a >>= kindOf
+      (b', kind') <- lift $ replaceTySyns b >>= kindOf
+      lift $ unifyKinds kind kind'


I'm sure this is a gap in my own understanding, but it feels a little bit strange to me that we aren't capturing any information as a result of unifying kinds here. Is it possible that we learn something about either kind or kind' that we didn't know until we reached this line? If so, should that information be carried forward?

unifyKinds returns a (MonadError MultipleErrors m, MonadState CheckState m) => m () so I’m not sure how to extract any information from a successful unification 🤔

If unifying kinds extends the current substitution perhaps we should apply it to the kinds we inferred and then refer to [kind, kind'] rather than kinds in the returned type class dictionary? Or perhaps we should rather apply it to the types rewritten by kindOf earlier? I’m completely out of my depth here 😅

Yeah, I’m a bit out of my depth too. @natefaubion I’d be interested to hear your thoughts on this if you have a moment.

unifyKinds does extend the substitution and at some point it must be applied, yes. I believe it's always applied at the top of the solver loop.

Do you think that suggests that it might be worth exporting apply from TypeChecker.Kinds to use here?

Ah, I suppose we should resolve the other discussion (#3893 (comment)) first.

applySubstitution as is used everywhere else is fine. I think there's only one in the kind-checker for convenience or module dependencies or something.

… different kinds

hdgarrood · 2020-09-05T17:23:53Z

Sorry, yes, I mean kind inference. Would you prefer that we investigate and address this first, then?

hdgarrood · 2020-09-05T17:24:24Z

(where "this" = "the issue that an unelaborated type is being constructed somewhere")

kl0tl · 2020-09-05T17:39:20Z

I wanted to add the following test

module Main where

import Safe.Coerce (coerce)

data Unary a
data Binary a b

data Proxy a = Proxy
type role Proxy representational

data Unit

unaryToBinary :: Proxy Unary -> Proxy (Binary Unit)
unaryToBinary = coerce

but it fails with

Error found:
in module Example
at Example.purs:14:17 - 14:23 (line 14, column 17 - line 14, column 23)

  Type variable k is undefined.

while checking that type t2
  has kind t3
while inferring the kind of Unary @t3 t2
while solving type class constraint
                                                  
  Prim.Coerce.Coercible (Unary @t0 t2)            
                        (Binary @Type @t1 Unit t2)
                                                  
while checking that expression coerce
  has type Proxy @(t0 -> Type) (Unary @t0) -> Proxy @(t1 -> Type) (... @t1 Unit)
in value declaration unaryToBinary

where t0 is an unknown type
      t1 is an unknown type
      t3 is an unknown type
      t2 is an unknown type

See https://github.com/purescript/documentation/blob/master/errors/UndefinedTypeVariable.md for more information,
or to contribute content related to this error.

I don’t have figured out why yet but adding kind annotations to Unary and Binary parameters fixes the issue.

natefaubion · 2020-09-05T17:44:04Z

Sorry, yes, I mean kind inference. Would you prefer that we investigate and address this first, then?

I would like to at least know where they are coming from. For example, are the subgoals we are constructing for Coercible lacking kind applications? If that's the only source, it's probably not worrisome, it's just inefficient. I have not looked at the code extensively for how we are constructing our sub goals, but that's something to investigate.

kl0tl · 2020-09-05T18:08:08Z

The kind inference issue can be observed with

module Example where

import Safe.Coerce (coerce)

newtype N f = N (f {})

example :: forall f. N f -> f {}
example = coerce

When unwrapping N we extract the wrapped type from the environment, which lacks a kind application on the empty row:

((f :: Type -> Type) (Record ()))

natefaubion · 2020-09-05T18:39:46Z

If you insert a call to trace debugDataConstructors, I get:

coercibleWanteds
Example.N :: forall (f :: Type -> Type). f (Record (() @Type)) -> N f

Qualified (Just (ModuleName "Example")) (ProperName {runProperName = "N"})
["f"]
f (Record ())

So I think whatever source lookupNewtypeConstructor is using is just not an accurate source.

natefaubion · 2020-09-05T18:41:55Z

The kind checker doesn't elaborate the type in the data declaration spine, which appears to be what lookupNewtypeConstructor is using. It yields elaborated types of constructors.

natefaubion · 2020-09-05T20:15:20Z

Here is where it does not rewrite the ctor

purescript/src/Language/PureScript/TypeChecker/Kinds.hs

Line 605 in 2bd7ca5

fmap ((ctor,) . mkForAll ctorBinders) $ inferDataConstructor tyCtor' ctor

Here is where it checks the fields and returns a type for the constructor:

purescript/src/Language/PureScript/TypeChecker/Kinds.hs

Line 613 in 2bd7ca5

flip checkKind E.kindType . foldr ((E.-:>) . snd) tyCtor . dataCtorFields

You could have it package up new constructor fields as well as returning the type here.

kl0tl · 2020-09-06T11:25:13Z

Thank you for the pointers Nate! I think I managed to do as you suggested and unifying elaborated kinds happens to also fix #3893 (comment) 🎉 There’s two new issues with this though, tests/purs/passing/LetPattern.purs now fails with:

    var v1 = Y.create(25252)("hello, world")(false);
                                            ^

TypeError: Y.create(...)(...) is not a function

and tests/purs/passing/Coercible.purs fails with:

Error found:
in module Main
at tests/purs/passing/Coercible.purs:71:18 - 71:24 (line 71, column 18 - line 71, column 24)

  No type class instance was found for
                                                                                
    Prim.Coerce.Coercible (forall (k1 :: Type) (k2 :: Type). Phantom1 @k2 s0 t4)
                          s0                                                    
                                                                                

while checking that type forall (a :: Type) (b :: Type). Coercible @Type a b => a -> b
  is at least as general as type s0 -> Roles1 @t1 @t2 r3 s0 t4
while checking that expression coerce
  has type s0 -> Roles1 @t1 @t2 r3 s0 t4
in value declaration roles1ToSecond

where r3 is a rigid type variable
        bound at (line 71, column 18 - line 71, column 24)
      s0 is a rigid type variable
        bound at (line 71, column 18 - line 71, column 24)
      t4 is a rigid type variable
        bound at (line 71, column 18 - line 71, column 24)
      t1 is an unknown type
      t2 is an unknown type

See https://github.com/purescript/documentation/blob/master/errors/NoInstanceFound.md for more information,
or to contribute content related to this error.

natefaubion · 2020-09-06T14:44:19Z

src/Language/PureScript/TypeChecker/Kinds.hs

+  -> m (DataConstructorDeclaration, SourceType)
+inferDataConstructor tyCtor DataConstructorDeclaration{..} = do
+  tyCtor' <- checkKind (foldr ((E.-:>) . snd) tyCtor dataCtorFields) E.kindType
+  let (_, _, tys) = unapplyTypes tyCtor'


I don't think unapplyTypes is correct here. unapplyTypes deconstructs applications, but tyCtor' is the signature for the data constructor. That is, given data Foo a = Foo Int a String, then tyCtor' will be Int -> a -> String -> Foo a. unapplyTypes called on this will yield an application to -> with two arguments. Maybe we should do a traversal over dataCtorFields, checking each field against E.kindType, zip this with dataCtorFields, and then assemble the constructor signature after.

Of course 🤦 I‘ve checked each field independently. I attempted to build the constructor type from the checked fields types but this led to issues with rank-n kinds so I built it from the unchecked field types and then checked the whole.

Is there a way to reuse some of the work done checking the fields when building the constructor type?

I'm not sure why you would have rank-n issues. Maybe you constructed the associativity of -> incorrectly? Maybe something like:

inferDataConstructor tyCtor DataConstructorDeclaration{..} = do dataCtorFields' <- traverse (traverse (flip checkKind E.kindType)) dataCtorFields dataCtor <- flip (foldr ((E.-:>) . snd)) dataCtorFields' =<< checkKind tyCtor E.kindType pure (DataConstructorDeclaration { dataCtorFields = dataCtorFields', .. }, dataCtor)

Oh I just forgot to check the constructor return type 😅 I applied your suggestion!

natefaubion · 2020-09-06T14:59:11Z

src/Language/PureScript/TypeChecker/Kinds.hs

      let tyUnks = snd . fromJust $ lookup (mkQualified datName moduleName) tySubs
-          ctors' = fmap (fmap (generalizeUnknowns tyUnks . replaceTypeCtors)) ctors
+          replaceTypeCtorsAndGeneralizeUnknowns = generalizeUnknowns tyUnks . replaceTypeCtors
+          ctors' = fmap (mapDataCtorFields (fmap (fmap replaceTypeCtorsAndGeneralizeUnknowns)) *** replaceTypeCtorsAndGeneralizeUnknowns) ctors


It's not clear to me that you want to generalize unknowns in the data constructor spine like this, as this will introduce foralls inside the data constructor spine for any field with an unknown. These unknowns should be quantified as part of the data declaration, not in each field. That is

data Foo f a = Foo (f a)

This should be quantified as

data Foo :: forall k. (k -> Type) -> k -> Type data Foo f a = Foo (f @k a)

Not as

data Foo f a = Foo (forall k. f @k a)

We should generalize the data declaration as a whole, not just each field type individually.

Oh that’s why I had those unwanted foralls! I’ve replaced unknowns without generalizing them.

natefaubion · 2020-09-06T19:07:29Z

@hdgarrood @kl0tl I'm happy with the changes to the kind checker and the use of elaborateKind. As long as everything passes and y'all are happy with the Coercible changes, I think this is OK to merge.

kl0tl · 2020-09-06T20:14:27Z

The last unresolved issue should be about whether to apply the current substitution to Coercible constraints parameters and/or their kinds before computing subgoals (#3893 (comment)).

As you said, the current substitution is already applied at the top of the solver recursive worker so I think the only risk is to fail to equate the parameters of identical constructors at role nominal here?

We’re unifying elaborated kinds now though, are unknowns still expected in them?

natefaubion · 2020-09-06T20:16:30Z

You need to apply a substitution if you want to observe anything about the type after unifying. You do not want to observe a unification variable that is otherwise solved.

natefaubion · 2020-09-06T20:25:23Z

As you said, the current substitution is already applied at the top of the solver recursive worker so I think the only risk is to fail to equate the parameters of identical constructors at role nominal here?

We’re unifying elaborated kinds now though, are unknowns still expected in them?

I think you said you kept unification because it yielded a better error? Is unification necessary for soundness? If not, then maybe it isn't necessary to apply a substitution.

kl0tl · 2020-09-07T11:50:40Z

The unified kinds are thrown away afterwards but we rely on the constraint arguments to have the same kind when computing subgoals to avoid redundant checks (for instance we only check if the first argument is unsaturated or has a non empty arguments list).

I’d like to find a case where applying the substitution makes a difference. If my understanding is correct this should only happen when one of the constraint arguments has an unknown kind application that would be solved by the unification of both arguments kinds? I’m struggling to observe this though 😅

natefaubion · 2020-09-07T19:46:56Z

If my understanding is correct this should only happen when one of the constraint arguments has an unknown kind application that would be solved by the unification of both arguments kinds? I’m struggling to observe this though

I'm not sure how a kind like this (though it sounds like it's in paradox territory 😆) would be unknown at this point since we are past type-checking.

hdgarrood · 2020-09-08T16:49:26Z

Since this issue is difficult (potentially impossible) to observe, shall we merge this now and come back to it later if it comes back up?

@type

Given the following declaration ```purs newtype N f = N (f {}) ``` solving a `Coercible (N f) (f {})` constraints yields a `Coercible (f {}) (f {})` subgoal by the unwraping rule. This constraint seems trivial but if constructors fields are not elaborated the actual subgoal is `Coercible (f (Record ())) (f (Record (() @type)))`, which isn’t solvable because of the missing kind application on the left! Inferring kinds and comparing the rewritten terms fixes the issue at the expense of redundant work (kinds are already inferred during type checking) but then invalid coercions between unsaturated higher kinded types with polymorphic parameters fail to type check with an `UndefinedTypeVariable` error instead of the expected `NoInstanceFound`.

kl0tl · 2020-09-08T16:57:13Z

Sounds good to me! I‘ve squashed the last commit and the one it fixes.

hdgarrood · 2020-09-09T11:58:54Z

Amazing - thanks for your work on this!

kl0tl commented May 30, 2020

View reviewed changes

lib/purescript-ast/src/Language/PureScript/Environment.hs Outdated Show resolved Hide resolved

kl0tl commented May 30, 2020

View reviewed changes

kl0tl force-pushed the polykinded-coercible branch 3 times, most recently from 896006f to 0c42d95 Compare June 1, 2020 14:15

This was referenced Jun 3, 2020

Role inference for foreign imported types without roles declaration is wrong #3895

Closed

Recurse on the right of arrows and under foralls when inferring nominal roles from kinds #3896

Merged

kl0tl force-pushed the polykinded-coercible branch from 0c42d95 to 11ff2ad Compare June 26, 2020 11:45

kl0tl commented Jun 26, 2020

View reviewed changes

lib/purescript-ast/src/Language/PureScript/Types.hs Outdated Show resolved Hide resolved

natefaubion reviewed Jul 3, 2020

View reviewed changes

src/Language/PureScript/TypeChecker/Entailment.hs Outdated Show resolved Hide resolved

kl0tl force-pushed the polykinded-coercible branch 5 times, most recently from 9311cde to fdc2b40 Compare July 5, 2020 14:35

natefaubion reviewed Jul 11, 2020

View reviewed changes

src/Language/PureScript/TypeChecker/Entailment.hs Outdated Show resolved Hide resolved

kl0tl added 3 commits July 12, 2020 00:41

Saturate higher kinded types in Coercible constraints

560a1b7

Apply the polymorphic kind of Coercible constraints when solving them

e4c5304

Forbid heterogenously kinded Coercible constraints arguments

a465784

kl0tl force-pushed the polykinded-coercible branch from 8798f00 to a465784 Compare July 11, 2020 22:59

kl0tl added 2 commits July 12, 2020 13:17

Don’t rewrite thrown away types when unifying Coercible arguments k…

d2405fe

…inds

Compare Coercible arguments rewritten with their inferred kinds

8552334

hdgarrood reviewed Aug 23, 2020

View reviewed changes

kl0tl added 3 commits August 25, 2020 19:38

Throw on failed type lookups when solving Coercible constraints

d9b8da8

Remove redundant checks from coercibleWanteds guards

cf6d84e

Support Coercible constraints on unsaturated type constructors with…

eea4f60

… different kinds

natefaubion reviewed Sep 6, 2020

View reviewed changes

kl0tl force-pushed the polykinded-coercible branch 2 times, most recently from 068db0e to bcdf8c8 Compare September 6, 2020 19:03

hdgarrood approved these changes Sep 8, 2020

View reviewed changes

kl0tl force-pushed the polykinded-coercible branch from bcdf8c8 to 7e8c171 Compare September 8, 2020 16:56

Fix CoercibleKindMismatch golden test

e1b50bc

hdgarrood merged commit 3a1ac10 into purescript:master Sep 9, 2020

kl0tl deleted the polykinded-coercible branch September 18, 2020 17:25

kl0tl mentioned this pull request Nov 15, 2020

Interaction solver for Coercible constraints #3955

Merged

JordanMartinez mentioned this pull request Jan 18, 2022

Update version to v0.15.0 working-group-purescript-es/purescript#4

Closed

Conversation

kl0tl commented May 30, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hdgarrood commented Jul 11, 2020

Uh oh!

Uh oh!

kl0tl commented Jul 11, 2020

Uh oh!

hdgarrood commented Jul 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood commented Sep 5, 2020

Uh oh!

hdgarrood commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kl0tl commented Sep 5, 2020

Uh oh!

natefaubion commented Sep 5, 2020

Uh oh!

kl0tl commented Sep 5, 2020

Uh oh!

natefaubion commented Sep 5, 2020

Uh oh!

natefaubion commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natefaubion commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kl0tl commented Sep 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natefaubion Sep 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood commented Sep 5, 2020 •

edited

Loading

natefaubion commented Sep 5, 2020 •

edited

Loading

natefaubion commented Sep 5, 2020 •

edited

Loading

natefaubion Sep 6, 2020 •

edited

Loading

natefaubion Sep 6, 2020 •

edited

Loading

natefaubion commented Sep 6, 2020 •

edited

Loading