Skip to content

Add Prim.Row.Lacks constraint#3305

Merged
garyb merged 7 commits intopurescript:masterfrom
natefaubion:row-lacks
Apr 17, 2018
Merged

Add Prim.Row.Lacks constraint#3305
garyb merged 7 commits intopurescript:masterfrom
natefaubion:row-lacks

Conversation

@natefaubion
Copy link
Copy Markdown
Contributor

@natefaubion natefaubion commented Apr 13, 2018

No description provided.

LiamGoodacre
LiamGoodacre reviewed Apr 13, 2018
View reviewed changes
src/Language/PureScript/Environment.hs Outdated
-- class Lacks (label :: Symbol) (rows :: # Type)
, (primSubName "Row" "Lacks", (makeTypeClassData
[ ("label", Just kindSymbol)
, ("rows", Just (Row kindType))
Copy link
Copy Markdown
Member

@LiamGoodacre LiamGoodacre Apr 13, 2018

Choose a reason for hiding this comment

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

rows - plural?

LiamGoodacre
LiamGoodacre reviewed Apr 13, 2018
View reviewed changes
src/Language/PureScript/TypeChecker/Entailment.hs Outdated
= [ TypeClassDictionaryInScope [] 0 NubInstance [] C.Nub [r, r'] Nothing ]
forClassName _ C.Lacks [TypeLevelString sym, r]
| Just (r', cst) <- rowLacks sym r
= [ TypeClassDictionaryInScope [] 0 LacksInstance [] C.Lacks [(TypeLevelString sym), r'] cst ]
Copy link
Copy Markdown
Member

@LiamGoodacre LiamGoodacre Apr 13, 2018

Choose a reason for hiding this comment

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

Unnecessary parens.

@LiamGoodacre
Copy link
Copy Markdown
Member

LiamGoodacre commented Apr 13, 2018

CI build error is interesting:

/home/travis/build/purescript/purescript/src/Language/PureScript/TypeChecker/Entailment.hs:171:5: warning:
    Pattern match checker exceeded (2000000) iterations in
    an equation for ‘forClassName’. (Use -fmax-pmcheck-iterations=n
    to set the maximun number of iterations to n)
    |
171 |     forClassName ctx [email protected] [msg] =
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...

@natefaubion
Copy link
Copy Markdown
Contributor Author

natefaubion commented Apr 13, 2018

I wonder if matching on TypeLevelString there is making the exhaustiveness checker blow up

@natefaubion
Copy link
Copy Markdown
Contributor Author

natefaubion commented Apr 13, 2018

I'm not sure about the pattern iteration warning. I think it just must be too many cases? I tried rearranging it a bit to no avail. It's nothing to do with the case in particular (removing a different one eliminates the warning).

@natefaubion
Copy link
Copy Markdown
Contributor Author

natefaubion commented Apr 13, 2018

I've just split up the pattern matrix to avoid the explosion of possibilities.

garyb
garyb reviewed Apr 13, 2018
View reviewed changes
src/Language/PureScript/TypeChecker/Entailment.hs Outdated
forClassName _ _ _ = internalError "forClassName: expected qualified class name"
forClassName _ _ _ = forClassNameError

forClassNameError = internalError "forClassName: expected qualified class name"
Copy link
Copy Markdown
Member

@garyb garyb Apr 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure this is the appropriate error for all the other cases - it would have been matching forClassName ctx cn@(Qualified Nothing .... I guess the result should just be []? (No dictionary found when the arguments are incorrect).

Copy link
Copy Markdown
Contributor Author

@natefaubion natefaubion Apr 13, 2018

Choose a reason for hiding this comment

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

Oh good point

@natefaubion
Copy link
Copy Markdown
Contributor Author

natefaubion commented Apr 13, 2018

I've fixed the fallthrough. It's tedious, but avoids the warning. I don't know if that's a good thing.

@natefaubion
Copy link
Copy Markdown
Contributor Author

natefaubion commented Apr 13, 2018

I'll try pulling out the cases into separate functions which will help eliminate some of the brittleness of this approach.

@natefaubion
Copy link
Copy Markdown
Contributor Author

natefaubion commented Apr 14, 2018

OK this is ready to look at again.

garyb
garyb approved these changes Apr 14, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@garyb garyb left a comment

Choose a reason for hiding this comment

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

I like this approach 👍

tests/TestPrimDocs.hs

when (not (null extraNames)) $
error $ "Extra Prim names: " ++ show undocumentedNames
error $ "Extra Prim names: " ++ show extraNames
Copy link
Copy Markdown
Member

@garyb garyb Apr 14, 2018

Choose a reason for hiding this comment

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

😄

@garyb
Copy link
Copy Markdown
Member

garyb commented Apr 17, 2018

@LiamGoodacre any further thoughts/comments?

LiamGoodacre
LiamGoodacre approved these changes Apr 17, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@LiamGoodacre LiamGoodacre left a comment

Choose a reason for hiding this comment

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

Looks great!

@garyb garyb merged commit 2495be5 into purescript:master Apr 17, 2018
@garyb garyb mentioned this pull request Apr 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garyb garyb garyb approved these changes

@LiamGoodacre LiamGoodacre LiamGoodacre approved these changes

Assignees

No one assigned

Labels

status: ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@natefaubion @LiamGoodacre @garyb