Fix entailment issues with skolems and matches#3121
Merged
LiamGoodacre merged 1 commit intopurescript:masterfrom Oct 21, 2017
LiamGoodacre:fix/skolem-apart
Merged
Fix entailment issues with skolems and matches#3121LiamGoodacre merged 1 commit intopurescript:masterfrom LiamGoodacre:fix/skolem-apart
LiamGoodacre merged 1 commit intopurescript:masterfrom
LiamGoodacre:fix/skolem-apart
Conversation
paf31
reviewed
Oct 20, 2017
| module InstanceChains.BothUnknownAndMatch where | ||
|
|
||
| class Same l r o | l r -> o | ||
| instance sameY :: Same t t @"Y" else instance sameN :: Same l r @"N" |
Contributor
There was a problem hiding this comment.
Hmm, well there's the kind error again :(
Contributor
There was a problem hiding this comment.
Oh wait, no, I didn't see the proxy :)
paf31
reviewed
Oct 20, 2017
| -- for label `m`, `Int ~ Int` should be a match | ||
| -- together they should be Unknown | ||
| example :: forall t. @t -> @_ | ||
| example _ = same @(u :: t, m :: Int) @(u :: Int, m :: Int) |
Contributor
There was a problem hiding this comment.
But this is a kind error, surely?
Member
Author
There was a problem hiding this comment.
Note to readers: when this comment was made, same had type forall l r o. Same l r o => @l -> @r -> @o.
paf31
reviewed
Oct 20, 2017
| pairwiseAll _ [] = True | ||
| pairwiseAll _ [_] = True | ||
| pairwiseAll p (x : xs) = all (p x) xs && pairwiseAll p xs | ||
| pairwiseAll :: (a -> a -> Matched ()) -> [a] -> Matched () |
Contributor
There was a problem hiding this comment.
This could use any monoid as the result, right?
paf31
reviewed
Oct 20, 2017
| meet ts | pairwiseAll typesAreEqual ts = Just ts | ||
| | otherwise = Nothing | ||
| verifySubstitution :: Matching [Type] -> Matched (Matching [Type]) | ||
| verifySubstitution mts = foldMap meet mts $> mts where |
paf31
approved these changes
Oct 20, 2017
Contributor
paf31
left a comment
paf31
left a comment
There was a problem hiding this comment.
Aside from fixing up the future kind error, this looks great to me. Thanks for working on this!
When verifying a substitution, if we see a skolem (that hasn't been matched with itself) then it is unknown whether or not we might have a match or be apart. Combining a match with an unknown should be an unknown not a match. I.e: knowing that one of two things match doesn't make the other one now a match. Added failing examples to catch these cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When verifying a substitution, if we see a skolem (that hasn't been
matched with itself) then it is unknown whether or not we might have a
match or be apart.
Combining a match with an unknown should be an unknown not a match.
I.e: knowing that one of two things match doesn't make the other one now
a match.
Added failing examples to catch these cases.