Skip to content

Instantiate types in record literals as necessary#2625

Merged
paf31 merged 4 commits intomasterfrom
phil/1110
Feb 11, 2017
Merged

Instantiate types in record literals as necessary#2625
paf31 merged 4 commits intomasterfrom
phil/1110

Conversation

@paf31
Copy link
Copy Markdown
Contributor

@paf31 paf31 commented Feb 5, 2017

Fixes #1110 and #2312. Replaces #2367.

I'm not really very happy with this fix, but I've been stuck thinking about this for a long time now, and it's been difficult to fix this and keep Halogen compiling as-is. So this is a compromise, and I've added a note in a comment to explain the fix.

let fields = zipWith (\name (TypedValue _ _ t) -> (Label name, t)) (map fst ps) ts
ty = TypeApp tyRecord $ rowFromList (fields, REmpty)
return $ TypedValue True (Literal (ObjectLiteral (zip (map fst ps) ts))) ty
-- We make a special case for Vars in record labels, since there are the
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you meant "these".

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.

Thanks!

@paf31
Copy link
Copy Markdown
Contributor Author

paf31 commented Feb 8, 2017

@LiamGoodacre Would you mind reviewing this one, please?

@LiamGoodacre
Copy link
Copy Markdown
Member

Will do, I'll have a look through this tonight :)

@LiamGoodacre
Copy link
Copy Markdown
Member

What would you expect to happen in this code:

type Z t = forall x. t x -> (forall a. t a) -> t x
class C t where c :: Z t
instance cA :: C Array where c x _ = x
test2 :: forall m. Monad m => m { ccc :: Z Array }
test2 = pure { ccc: c }

@paf31
Copy link
Copy Markdown
Contributor Author

paf31 commented Feb 10, 2017

I'm not sure, but I could try it. Why do you ask about that snippet specifically?

@LiamGoodacre
Copy link
Copy Markdown
Member

Sorry, I forgot to add an explanation. I tried added the above code to your test file. The idea was to also test a polymorphic var with a constraint and also a quantified argument. I probably shouldn't have lumped them together, but anyway: the test failed with an escaped skolem. I would have expected it to succeed, so I was wondering if you thought the same.

@paf31
Copy link
Copy Markdown
Contributor Author

paf31 commented Feb 10, 2017

Constrained functions in record literals are still a bit of a pain. Does it work if you annotate the property?

@LiamGoodacre
Copy link
Copy Markdown
Member

With an annotation it still fails in the same way:

test2 :: forall m. Monad m => m { ccc :: Z Array }
test2 = pure { ccc: (c :: Z Array) }
  1) Passing examples '1110.purs' should compile and run without error
       Error found:
       in module Main
       at /Users/liamgoodacre/git/purescript/examples/passing/1110.purs line 25, column 1 - line 25, column 51

         A type variable has escaped its scope.

       in the expression { ccc: c cA
                         }
       in value declaration test2

@paf31
Copy link
Copy Markdown
Contributor Author

paf31 commented Feb 10, 2017

Oh, then that's a bug. Thanks.

@paf31
Copy link
Copy Markdown
Contributor Author

paf31 commented Feb 10, 2017

I think it's easily fixed, thankfully. I'll add a test for this.

@paf31 paf31 changed the base branch from master to phil/2567-2 February 10, 2017 19:34
@paf31 paf31 changed the base branch from phil/2567-2 to master February 10, 2017 19:35
@paf31
Copy link
Copy Markdown
Contributor Author

paf31 commented Feb 11, 2017

@LiamGoodacre How's this?

@paf31
Copy link
Copy Markdown
Contributor Author

paf31 commented Feb 11, 2017

The whole package set continues to compile with this change, so it seems safe enough.

@LiamGoodacre
Copy link
Copy Markdown
Member

This looks good :) - although I'm not an expert on this part of the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inferred types of record fields should not be polymorphic

3 participants