Skip to content

Add Prim.Row.Lacks constraint#3305

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

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

Conversation

@natefaubion

@natefaubion natefaubion commented Apr 13, 2018

Copy link
Copy Markdown
Contributor

No description provided.

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

@LiamGoodacre LiamGoodacre Apr 13, 2018

Copy link
Copy Markdown
Member

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
Comment thread 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 ]

@LiamGoodacre LiamGoodacre Apr 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unnecessary parens.

@LiamGoodacre

LiamGoodacre commented Apr 13, 2018

Copy link
Copy Markdown
Member

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 cn@C.Warn [msg] =
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^...

@natefaubion

natefaubion commented Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

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

@natefaubion

natefaubion commented Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

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

natefaubion commented Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

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

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

forClassNameError = internalError "forClassName: expected qualified class name"

@garyb garyb Apr 13, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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).

@natefaubion natefaubion Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh good point

@natefaubion

natefaubion commented Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

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

@natefaubion

natefaubion commented Apr 13, 2018

Copy link
Copy Markdown
Contributor Author

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

@natefaubion

natefaubion commented Apr 14, 2018

Copy link
Copy Markdown
Contributor Author

OK this is ready to look at again.

garyb
garyb approved these changes Apr 14, 2018
View reviewed changes

@garyb garyb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like this approach 👍

Comment thread tests/TestPrimDocs.hs

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

@garyb garyb Apr 14, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

😄

@garyb

garyb commented Apr 17, 2018

Copy link
Copy Markdown
Member

@LiamGoodacre any further thoughts/comments?

LiamGoodacre
LiamGoodacre approved these changes Apr 17, 2018
View reviewed changes

@LiamGoodacre LiamGoodacre left a comment

Copy link
Copy Markdown
Member

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