Solve `Coercible` constraints on rows by kl0tl · Pull Request #3878 · purescript/purescript

kl0tl · 2020-05-16T22:42:01Z

natefaubion · 2020-05-22T14:20:10Z

src/Language/PureScript/TypeChecker/Entailment.hs

+          unless (null (fst rl1) && null (fst rl2)) $
+            throwError . errorMessage $ TypesDoNotUnify (rowFromList rl1) (rowFromList rl2)


I think we should add tests for coercing polymorphic rows.

test :: forall r. { a :: Int | r } -> { a :: Id1 Int | r } test = coerce

I don't think there's a reason this shouldn't coerce since the tails unify.

test :: forall r s. { a :: Int | r } -> { a :: Id1 Int | s } test = coerce

I'd think this needs to introduce a subgoal for Coercible r s, but Coercible isn't polykinded, so I assume we'll need to reject this with a unification error.

Should we consider having special casing for records so that it introduces a subgoal Coercible (Record r) (Record s)?

I think a unification error is fine, since it's likely an error anyway. For example you couldn't write:

test :: forall r s. { a :: Int | r } -> { a :: Int | s } test = identity

The way to solve that would be a type equality that r ~ s.

Right, that makes sense.

Actually, maybe a unification error isn't right, and we should just punt the whole constraint? It's possible that it could be solved at the call site.

Given

test :: forall r s. { a :: Int | r } -> { a :: Id1 Int | s } test = coerce

We are solving the instantiation of coerce. With the provided signature, the constraint is not solvable because r does not unify with s. We shouldn't throw a unification error because it's possible that it could be solved, given that r does unify with s. That is, this is potentially solvable:

test :: forall r s. Coercible { a :: Int | r } { a :: Id1 Int | s } => { a :: Int | r } -> { a :: Id1 Int | s } test = coerce test2 = test { a: 42 } :: { a :: Id1 Int }

because now r and s unify with (). We should not throw a unification error, we should just defer the constraint.

Note that I think it's fine to throw a unification error for the existing case as written, because we know for a fact there is no solution, since there are concrete rows that differ. I'm only talking about polymorphic tails.

Right, I see, thanks. That approach seems a bit odd to me because then test effectively has an invisible Coercible constraint, right? If I wrote a type annotation forall r s. { a :: Int | r } -> { a :: Id1 Int | s } then I would want the compiler to tell me if that function can’t actually be used for all possible choices of r and s; this feels like a parametricity violation to me.

I'm not sure what you mean by "an invisible Coercible constraint". I'm just saying that the compiler should not fail with a unification error, it should fail saying that it can't solve the constraint, and so the user should add the constraint if they want it to solve.

Oh! I see now. That sounds reasonable to me, yes.

kl0tl · 2020-05-24T10:08:58Z

I wasn’t considering rows tails so open rows were coercible regardless of whether their tails unified 🙃

I rethrown a NoInstanceFound error instead of a TypesDoNotUnify error when rows tails don’t unify and added tests for open rows, as suggested by @natefaubion.

Also I special cased records instead of rows because Coercible isn’t polykinded so mentioning coercible constraints on rows such as

Prim.Coerce.Coercible ( x :: Int
                      )
                      ( y :: String
                      )

in error messages is misleading: the compiler throws a KindsDoNotUnify error when writing such constraint. Should we check the kinds of type constructors arguments so we don’t try to solve coercible constraints on types of kind other than Type?

This means that we can’t solve coercible constraints on user written types parametrized by rows though, such as Data.Variant.Variant for instance. I don’t know the consequences of making Coercible polykinded but we could maybe introduce another class for coercing rows?

natefaubion · 2020-05-24T13:53:21Z

I think the case for records is fine for now. I don't think it should hold up a release of it, and we can continue to discuss solutions in a separate issue.

hdgarrood · 2020-05-24T16:46:06Z

This looks good to me too, but I'd quite like to get #3873 in first since I think it addresses a more fundamental problem with how we're dealing with roles inside the compiler, if that sounds ok?

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

Should we check the kinds of type constructors arguments so we don’t try to solve coercible constraints on types of kind other than Type?

This means that we can’t solve coercible constraints on user written types parametrized by rows though, such as Data.Variant.Variant for instance. I don’t know the consequences of making Coercible polykinded but we could maybe introduce another class for coercing rows?

These are really good questions. I was thinking about them a bit more and this led me to discover #3889. Shall we continue this conversation there?

hdgarrood · 2020-08-30T14:37:58Z

I would like to come back to this once #3893 is in, since that addresses a more fundamental issue with Coercible instances for parameterized data types, and will probably have implications for the approach we want to take here.

kl0tl · 2020-09-09T17:51:15Z

I’ve rebased this branch! The changes to the instance solver are quite minimal now that Coercible constraints are polykinded, that’s nice.

hdgarrood · 2020-09-11T16:49:42Z

src/Language/PureScript/Docs/Prim.hs

  , "This rule may seem puzzling since it is impossible to apply `coerce` to a term"
  , "of type `NT1` but it is necessary to coerce types with higher kinded parameters."
  , ""
+  , "Fifth, every pair of rows can be coerced if the types they contain with"


How about this?

Fifth, every pair of rows can be coerced if they have the same labels and the corresponding types for each label are coercible.

That‘s better indeed, I‘ve applied your suggestion.

hdgarrood · 2020-09-13T01:10:40Z

Thanks! Would you mind merging master so that CI will hopefully pass?

kl0tl · 2020-09-13T10:07:10Z

I rebased on top of master, tests pass again 😌

hdgarrood · 2020-09-13T15:36:18Z

I think I'll leave this open for a little bit in case anyone with better knowledge of the solver wants to weigh in; I'm not particularly familiar with it right now.

hdgarrood · 2020-09-16T14:42:35Z

I'm going to merge this for now so that we can get an RC out.

kl0tl force-pushed the fix-issue-3875 branch 2 times, most recently from 8f5f0b5 to 3ab8a86 Compare May 18, 2020 21:29

natefaubion reviewed May 22, 2020

View reviewed changes

kl0tl force-pushed the fix-issue-3875 branch from 3ab8a86 to 8b6ea59 Compare May 24, 2020 10:08

kl0tl changed the title ~~Solve Coercible constraints on rows~~ Solve Coercible constraints on records May 24, 2020

natefaubion approved these changes May 24, 2020

View reviewed changes

kl0tl mentioned this pull request May 27, 2020

Hygiene issue in Coercible solving #3889

Closed

kl0tl mentioned this pull request Jul 11, 2020

Saturate higher kinded types in Coercible constraints #3893

Merged

kl0tl force-pushed the fix-issue-3875 branch from 8b6ea59 to f1fc5ab Compare September 9, 2020 17:48

kl0tl changed the title ~~Solve Coercible constraints on records~~ Solve Coercible constraints on rows Sep 9, 2020

hdgarrood approved these changes Sep 12, 2020

View reviewed changes

kl0tl force-pushed the fix-issue-3875 branch 2 times, most recently from 8648ea3 to 4530792 Compare September 12, 2020 21:47

Solve Coercible constraints on rows

7897537

kl0tl force-pushed the fix-issue-3875 branch from 4530792 to 7897537 Compare September 13, 2020 09:47

hdgarrood merged commit c808124 into purescript:master Sep 16, 2020

kl0tl deleted the fix-issue-3875 branch September 18, 2020 17:25

JordanMartinez mentioned this pull request Jan 18, 2022

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

Closed

		unless (null (fst rl1) && null (fst rl2)) $
		throwError . errorMessage $ TypesDoNotUnify (rowFromList rl1) (rowFromList rl2)

Conversation

kl0tl commented May 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natefaubion May 22, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natefaubion May 23, 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!

natefaubion May 23, 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!

kl0tl commented May 24, 2020

Uh oh!

natefaubion commented May 24, 2020

Uh oh!

hdgarrood commented May 24, 2020

Uh oh!

hdgarrood commented May 24, 2020

Uh oh!

hdgarrood commented Aug 30, 2020

Uh oh!

kl0tl commented Sep 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood commented Sep 13, 2020

Uh oh!

kl0tl commented Sep 13, 2020

Uh oh!

hdgarrood commented Sep 13, 2020

Uh oh!

hdgarrood commented Sep 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

natefaubion May 22, 2020 •

edited

Loading

natefaubion May 23, 2020 •

edited

Loading

natefaubion May 23, 2020 •

edited

Loading