Skip to content

Instantiate abstraction body during inference#3128

Merged
LiamGoodacre merged 1 commit intopurescript:masterfrom
LiamGoodacre:fix/inferred-constrained-codomain
Oct 28, 2017
Merged

Instantiate abstraction body during inference#3128
LiamGoodacre merged 1 commit intopurescript:masterfrom
LiamGoodacre:fix/inferred-constrained-codomain

Conversation

@LiamGoodacre
Copy link
Copy Markdown
Member

@LiamGoodacre LiamGoodacre commented Oct 22, 2017 •
edited
Loading

Fixes #3125

Currently there is an issue with shadowed type vars in the shifted forall - see my comment on #3125.

Does this seem like a good approach?

@garyb
Copy link
Copy Markdown
Member

garyb commented Oct 22, 2017

This was what I had thought of as a means of fixing it also, I just didn't work on it long enough (> 15 minutes 😉) to figure out how to get it done. So yeah, seems like a good approach to me!

@LiamGoodacre
Copy link
Copy Markdown
Member Author

LiamGoodacre commented Oct 22, 2017 •
edited
Loading

Fixed the shadowing issue by not checking the type of the TypedValue when we need to re-infer.

@garyb
Copy link
Copy Markdown
Member

garyb commented Oct 22, 2017

Should we add the example from the issue as a test case too?

@LiamGoodacre
Copy link
Copy Markdown
Member Author

LiamGoodacre commented Oct 22, 2017

Hmm, if I use TypeValue False, then it doesn't expand things properly anymore. So I'll have to undo that and find a different way.

@LiamGoodacre
Copy link
Copy Markdown
Member Author

LiamGoodacre commented Oct 22, 2017

I don't need to re-infer, I just need to check the expression against the new type.

paf31
paf31 reviewed Oct 22, 2017
View reviewed changes
src/Language/PureScript/TypeChecker/Types.hs Outdated
-- if we shifted some quantifiers/constraints then we'll need to check against the new type
expr' <- if null qs && null cs
then return expr
else check expr ty'
Copy link
Copy Markdown
Contributor

@paf31 paf31 Oct 22, 2017

Choose a reason for hiding this comment

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

Can you explain why we need to check again here? It definitely seems odd to me to have to traverse the same terms twice.

paf31
paf31 reviewed Oct 22, 2017
View reviewed changes
src/Language/PureScript/TypeChecker/Types.hs Outdated
withBindingGroupVisible $ bindLocalVariables [(arg, ty, Defined)] $ do
body@(TypedValue _ _ bodyTy) <- infer' ret
return $ TypedValue True (Abs (VarBinder arg) body) $ function ty bodyTy
let (qs, cs, ty') = moveQuantifiersToFrontOverFunction (function ty bodyTy)
Copy link
Copy Markdown
Contributor

@paf31 paf31 Oct 22, 2017

Choose a reason for hiding this comment

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

Can you not just instantiate the type of the body here?

Copy link
Copy Markdown
Member Author

@LiamGoodacre LiamGoodacre Oct 22, 2017

Choose a reason for hiding this comment

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

Yes, that makes more sense. 😸

@LiamGoodacre LiamGoodacre changed the title Shift quantifiers & constraints from inferred body in abstraction Instantiate abstraction body during inference Oct 22, 2017
@paf31
Copy link
Copy Markdown
Contributor

paf31 commented Oct 23, 2017

Thanks! Looks good, but could you test it with some existing packages as well, please?

@LiamGoodacre
Copy link
Copy Markdown
Member Author

LiamGoodacre commented Oct 24, 2017 •
edited
Loading

I've checked that the following packages compile:

purescript-argonaut             purescript-either               purescript-lists                purescript-profunctor-lenses
purescript-argonaut-codecs      purescript-enums                purescript-maps                 purescript-proxy
purescript-argonaut-core        purescript-exceptions           purescript-math                 purescript-psci-support
purescript-argonaut-traversals  purescript-exists               purescript-maybe                purescript-record
purescript-arraybuffer-types    purescript-filterable           purescript-monoid               purescript-refs
purescript-arrays               purescript-foldable-traversable purescript-newtype              purescript-sets
purescript-assert               purescript-foreign              purescript-node-buffer          purescript-smolder
purescript-avar                 purescript-free                 purescript-node-http            purescript-st
purescript-behaviors            purescript-functions            purescript-node-path            purescript-strings
purescript-bifunctors           purescript-functors             purescript-node-streams         purescript-symbols
purescript-catenable-lists      purescript-gen                  purescript-node-url             purescript-tailrec
purescript-console              purescript-generics             purescript-nonempty             purescript-transformers
purescript-const                purescript-generics-rep         purescript-nullable             purescript-tuples
purescript-contravariant        purescript-globals              purescript-options              purescript-type-equality
purescript-control              purescript-identity             purescript-parallel             purescript-typelevel-prelude
purescript-debug                purescript-integers             purescript-partial              purescript-unfoldable
purescript-distributive         purescript-invariant            purescript-prelude              purescript-unsafe-coerce
purescript-eff                  purescript-lazy                 purescript-profunctor

Tried to check more than this but the removal of Generic caused me to cull them.
I also haven't gone through and checked that they generate the correct code.

Sometime this week I will run the tests of various modules to get a better assurance that things aren't broken.

@garyb
Copy link
Copy Markdown
Member

garyb commented Oct 24, 2017

I think profunctor-lenses is a pretty good test for this, if that worked, I doubt there's anything that will be rejected now that previously did work! Nicely done.

@LiamGoodacre
Copy link
Copy Markdown
Member Author

LiamGoodacre commented Oct 28, 2017 •
edited
Loading

I've checked that the following packages' tests pass: profunctor-lenses, foldable-traversable. I also tried halogen, but it has dependencies on Generic; so that obviously failed.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented Oct 28, 2017

👍 Thanks for testing. I say let's merge then.

@LiamGoodacre LiamGoodacre merged commit c507005 into purescript:master Oct 28, 2017
@LiamGoodacre LiamGoodacre deleted the fix/inferred-constrained-codomain branch October 28, 2017 20:45
@garyb
Copy link
Copy Markdown
Member

garyb commented Oct 28, 2017

Nice, now I can revive my typeclass representation PR... for the 3rd attempt at it 😄

Thanks Liam!

paf31 pushed a commit that referenced this pull request Nov 8, 2017
@LiamGoodacre @paf31
Instantiate abstraction body during inference (#3128)
64d6c5a
@joneshf joneshf mentioned this pull request Dec 20, 2017
Closed
jutaro pushed a commit to mgmeier/purescript that referenced this pull request Mar 12, 2018
@LiamGoodacre @jutaro
Instantiate abstraction body during inference (purescript#3128)
92ced6f
@garyb garyb mentioned this pull request Mar 18, 2019
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@paf31 paf31 paf31 approved these changes

Reviewers whose approvals may not affect merge requirements

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

@LiamGoodacre @garyb @paf31