Skip to content

Conversation

@no-longer-on-githu-b
Copy link
Contributor

@no-longer-on-githu-b no-longer-on-githu-b commented Apr 15, 2017

Fixes #2839.


This change is Reviewable

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use kindOf here instead, possibly.

Copy link
Contributor

Choose a reason for hiding this comment

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

TypedValue True here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't matter too much, but I use this boolean to indicate "this should be checked", and then "this was checked", so it'd be good to be consistent is all.

@paf31
Copy link
Contributor

paf31 commented Apr 15, 2017

Thanks!

A few comments:

  • I think you need a new clause in unifyTypes.
  • I would prefer to not overload the type constructor notation, since it's not a regular type constructor, and to use something like a keyword instead, such as proxy T.

@no-longer-on-githu-b
Copy link
Contributor Author

  • Ok
  • I agree

@no-longer-on-githu-b no-longer-on-githu-b changed the title Add Proxy syntax Add proxies Apr 15, 2017
Copy link
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.

Copy link
Member

Choose a reason for hiding this comment

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

If we've switched from Proxy to proxy then we don't need this change?

Copy link
Contributor Author

@no-longer-on-githu-b no-longer-on-githu-b Apr 15, 2017

Choose a reason for hiding this comment

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

Indeed. :) Will change later, working on something else atm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this go under reservedTypeNames?

Copy link
Contributor

@paf31 paf31 left a comment

Choose a reason for hiding this comment

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

I'll merge this before the next major release if there are no objections between now and then.

@paf31
Copy link
Contributor

paf31 commented Apr 23, 2017

A couple more thoughts:

  • Don't forget to update the types pretty printer in Docs/Publish, otherwise Pursuit will crash on any modules containing proxy types.
  • Perhaps RowCons can use proxy to simulate poly kinds now, like @natefaubion has done with FProxy in his polymorphic variants implementation.

@natefaubion
Copy link
Contributor

Perhaps RowCons can use proxy to simulate poly kinds now, like @natefaubion has done with FProxy in his polymorphic variants implementation.

That was one question I had, whether I could write something like this:

prj
    sym f a r1 r2
  . RowCons sym (proxy f) r1 r2
   IsSymbol sym
   proxy sym
   VariantF r2 a
   Maybe (f a)
prj p = on p Just (const Nothing)

Note the use of proxy in RowCons.

@paf31
Copy link
Contributor

paf31 commented Apr 25, 2017

That was one question I had, whether I could write something like this:

I think so, because it's not like RowCons uses any value with that type, it's a purely type-level function. So there doesn't need to be an actual polykinded type anywhere.

@garyb
Copy link
Member

garyb commented May 18, 2017

What was the reason for not using the symbol at the type level too?

@kritzcreek kritzcreek closed this May 18, 2017
@kritzcreek kritzcreek reopened this May 18, 2017
@paf31
Copy link
Contributor

paf31 commented May 18, 2017

Sorry, which symbol again? :)

@garyb
Copy link
Member

garyb commented May 18, 2017

So it's @"foo" at the value level, but proxy "foo" at the type level currently?

@garyb
Copy link
Member

garyb commented May 28, 2017

Bump for the @ vs proxy question, I know at least @kritzcreek, @natefaubion, and me don't love that it's different between types and values. 😄

@hdgarrood
Copy link
Contributor

I'm also a bit confused about that, I think it would be nice to use the same syntax for the type and the value if possible.

@paf31
Copy link
Contributor

paf31 commented May 28, 2017

I'd be fine with @ in types.

@no-longer-on-githu-b
Copy link
Contributor Author

no-longer-on-githu-b commented Jun 3, 2017

I'm fineish with @a being the type syntax for "proxy of a". However, usually, the syntax of a value and the syntax of the type of that value do not coincide. Different things look different. Consistently, @a would be a type of kind proxy a.

I'll change it due to popular demand. We can always change it back. Fun fact: with this change, this PR will no longer be a breaking change.

@paf31
Copy link
Contributor

paf31 commented Jun 3, 2017

Travis says

  tests/TestCompiler.hs:218: 
  1) Passing examples 'DuplicateProperties.purs' should compile and run without error
       Error found:
       in module Main
       at /home/travis/build/purescript/purescript/examples/passing/DuplicateProperties.purs line 6, column 1 - line 6, column 55
       
         Type variable proxy is undefined.
       
       while checking the kind of proxy     
                                    ( x :: a
                                    | r     
                                    )       
                                  -> proxy r
       in value declaration subtractX
       
       See https://github.com/purescript/documentation/blob/master/errors/UndefinedTypeVariable.md for more information,
       or to contribute content related to this error.
       
  tests/TestCompiler.hs:218: 
  2) Passing examples 'FunctionalDependencies.purs' should compile and run without error
       Error found:
       in module Main
       at /home/travis/build/purescript/purescript/examples/passing/FunctionalDependencies.purs line 14, column 1 - line 14, column 75
       
         Type variable proxy is undefined.
       
       while checking the kind of Append a b c => proxy a -> proxy b -> proxy c
       in value declaration appendProxy
       
       See https://github.com/purescript/documentation/blob/master/errors/UndefinedTypeVariable.md for more information,
       or to contribute content related to this error.

@paf31
Copy link
Contributor

paf31 commented Jun 3, 2017

with this change, this PR will no longer be a breaking change.

I think we should include this in 0.11.5 then, once the tests pass of course.

@natefaubion
Copy link
Contributor

Isn't it still a breaking change because @ can't be used as an operator? FWIW I think it was always dubious due to @ in patterns.

@paf31
Copy link
Contributor

paf31 commented Jun 4, 2017

Yes, actually, that's true. This currently works, for example:

data Test a b = Test a b

infix 4 type Test as @

x :: forall a. a @ a -> a
x (Test a _) = a

@paf31
Copy link
Contributor

paf31 commented Jun 4, 2017

Now I guess the question is, would we rather reserve @ as a type operator, or proxy as a type argument name? I would actually lean towards reserving proxy, since we can always pick some other name, but @ seems like it might be handy as a type operator for some things.

@garyb
Copy link
Member

garyb commented Jun 4, 2017

Using proxy is "more breaking" than @ given there were numerous examples of its existing usage up there ^, but I don't feel strongly about the syntax for it either way... although I think maybe the fact that a single @ is actually allowed as an operator currently is a bit weird, given that it doesn't work in patterns:

data Test a b = Test a b

infix 4 type Test as @
infix 4 Test as @

x :: forall a. a @ a -> a
x (a @ _) = a -- Type error, Test a0 a0 /~ a0, the compiler sees this as an @ binding, not `Test a _`

@garyb
Copy link
Member

garyb commented Jun 4, 2017

Actually, given that, I think I do prefer @. It would be weirder that there's a single reserved lname in the types, vs a magic prefix type operator that is already somewhat-reserved at the value level.

@paf31
Copy link
Contributor

paf31 commented Jun 4, 2017

Alright then, @ it is. Thanks!

@garyb
Copy link
Member

garyb commented Jun 4, 2017

I guess the above parse does mean you can't use a proxy literal in a pattern, but then you don't really need to since they're singly-inhabited.

@no-longer-on-githu-b
Copy link
Contributor Author

@garyb the list of uses of proxy above are values, not types. proxy would only be a keyword in types.

Copy link
Contributor

Choose a reason for hiding this comment

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

"@"?

@paf31 paf31 merged commit 74bc4a9 into purescript:master Sep 9, 2017
@paf31
Copy link
Contributor

paf31 commented Sep 9, 2017

🎆 Thanks!

hdgarrood added a commit that referenced this pull request Dec 30, 2017
@hdgarrood hdgarrood mentioned this pull request Dec 30, 2017
hdgarrood added a commit that referenced this pull request Jan 3, 2018
* Revert "pretty print proxy types when rendering docs (#3144)"

This reverts commit 14227c7.

* Revert "Print proxy type as an operator (#3124)"

This reverts commit cfd1db3.

* Revert "Fix proxies: synonyms, inference, traversals, instances (#3095)"

This reverts commit fe0aa0d.

* Revert "Add proxies (#2846)"

This reverts commit 74bc4a9.

* Remove one last occurrence of the ProxyType constructor

* Update tests to not use new proxies
matthewleon pushed a commit to matthewleon/purescript that referenced this pull request Jan 6, 2018
* Revert "pretty print proxy types when rendering docs (purescript#3144)"

This reverts commit 14227c7.

* Revert "Print proxy type as an operator (purescript#3124)"

This reverts commit cfd1db3.

* Revert "Fix proxies: synonyms, inference, traversals, instances (purescript#3095)"

This reverts commit fe0aa0d.

* Revert "Add proxies (purescript#2846)"

This reverts commit 74bc4a9.

* Remove one last occurrence of the ProxyType constructor

* Update tests to not use new proxies
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.

8 participants