New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't always require type-class instance names #1078

Closed
puffnfresh opened this Issue Apr 22, 2015 · 16 comments

Comments

Projects
None yet
9 participants
@puffnfresh
Copy link
Contributor

puffnfresh commented Apr 22, 2015

Allow leaving off a name if we can provide a simple way to derive them. This would be useful even if just for single-param type-classes.

https://twitter.com/pelotom/status/590767793731166211

@paf31

This comment has been minimized.

Copy link
Member

paf31 commented Apr 22, 2015

The problem obviously is how to generate nice names. MPTCs are difficult enough, but FlexibleInstances and OverlappingInstances also give problems. How would you differentiate instance Show (Foo a) from instance Show (Foo String), or instance Show (Bar String) from instance Show (Bar Int)? You would have to include all type info in the name, in which case they get ugly quickly, or do a global analysis, in which case things aren't predictable.

Sorry, but I vote 👎 . I see why people want this, but unless we have a good approach with predictable, tidy code gen, I don't see it worth the effort to save a few keystrokes.

@puffnfresh

This comment has been minimized.

Copy link
Contributor

puffnfresh commented Apr 22, 2015

I just vote for generating a fresh name. I absolutely don't care about the type-class instance's name in the output JS. Even in the case where I care about calling PureScript code from JS, I doubt I would ever want to write JS referring to the instance.

@paf31

This comment has been minimized.

Copy link
Member

paf31 commented Apr 22, 2015

I generally dislike using fresh name generation at all. Obviously, it's needed, but I like to reduce it where possible.

@jdegoes

This comment has been minimized.

Copy link

jdegoes commented Apr 22, 2015

I think this moves us in the opposite direction of where PS has been going (e.g. with having operators merely be aliases for named functions). Honestly, named instances may be a pain, but it's a tiny insignificant fraction of the time spent programming (it takes a second or two to type those characters and minutes to hours to design / write the code doing the work).

I also dislike fresh names and think PS can do a better job there (it has already improved since the early days, now parameter names are propagated into the JS functions in some cases, though there's room for improvement in other places).

@andyarvanitis

This comment has been minimized.

Copy link
Contributor

andyarvanitis commented Apr 22, 2015

Maybe this has already been mentioned, but a not-so-good thing about those names is that they make you think they are somehow significant on the PS side, rather than really being sort of an FFI thing. I think it's more of a cognitive effort/distraction than a time/keyboard-typing-effort thing for people.

@pelotom

This comment has been minimized.

Copy link
Contributor

pelotom commented Apr 22, 2015

It seems to me that if there's any function you want to access from JS, you can simply declare it as a top-level binding and define the TC instance method in terms of that. Even if you want all the methods for an instance to be available in one structure, you can define them in a record... so I tend to feel that instances don't really need to have intelligible names at all in JS. And it's just very odd to be declaring names for things that you can't actually refer to elsewhere in your PS code.

@hdgarrood

This comment has been minimized.

Copy link
Contributor

hdgarrood commented Apr 22, 2015

Possibly related, #514 - we would have a reason to refer to instance names if that happens.

@garyb

This comment has been minimized.

Copy link
Member

garyb commented Apr 23, 2015

A potential compromise: allow unnamed instances but make them a warning. Convenient for development, but then can go back and name them when done.

@puffnfresh

This comment has been minimized.

Copy link
Contributor

puffnfresh commented Apr 23, 2015

@garyb that's a great compromise! 👍

@paf31

This comment has been minimized.

Copy link
Member

paf31 commented Apr 23, 2015

What would we put into the externs file? Coming up with a globally unique identifier might be a bit dodgy in the presence of partial compilation. It was ok before because we generated a name, so maybe we have to do that again.

@joneshf

This comment has been minimized.

Copy link
Member

joneshf commented Apr 23, 2015

Since the only purpose of instance names is to have a reliable way to reference the instance from the target language, making it optional is not a good idea. Also a warning isn't really a deterrent. I imagine most people simply wont write an instance name, and will just ignore the warning even after making the code publicly available, or wait for someone to add a flag to shut that warning up. If you live entirely in the ps world, you have no incentive to give names to instances. At that point it becomes a question of whether you want to be a good samaritan to the community or not. Not to mention it makes things more complex for the compiler and for users.

It's more complex for users because, if the name generation scheme changes, you have no warning about that from the library level, and have to go to the language to figure out what's different now. Then you may have to update a huge amount of code. Plus different libraries might be compiled from different name generations schemes, etc. Whereas if the instance name is required in ps and a developer changes the name, you only have to be concerned with differences between versions of said library.

If writing instance names is a pain, you can mitigate it a bit by not choosing fancy names. The following compiles just fine, and while it doesn't get rid of the names entirely, it lightens the load for you.

module Wat where

  class Wat a

module Foo where

  import Wat

  data Foo

  instance a :: Wat Foo

module Bar where

  import Wat

  data Bar

  instance a :: Wat Bar

If ps wants to be a better js and support calling ps functions from js well, then I agree with @paf31 and say force the instance name. If ps doesn't care about that, then just get rid of the names entirely. Making the names optional just adds complexity for very little gain.

@pelotom

This comment has been minimized.

Copy link
Contributor

pelotom commented Apr 23, 2015

@joneshf I agree with much of what you said (I really don't care for the idea of optional with a warning), but I still think optional names make sense as long as, in the case where you elect not to choose a name, the instance is not meant to be accessed from JS. Just give it a fully munged name that only the compiler can make sense of. In the event that instance names are part of your "API", you can give them a name, and then they're accessible from JS. I believe that avoids the problems you brought up.

Edit:

I read your post a bit hastily, my comment above really only applies to the user side of the complexity issue you mentioned.

@hdgarrood

This comment has been minimized.

Copy link
Contributor

hdgarrood commented Apr 23, 2015

+1 for what @joneshf said. Contributing to the compiler and other tools that work with purescript ASTs has generally been a pleasure, and I think a large part of that owes to its simplicity. I think it's well worth holding on to.

A clearer error in the case where you forget you're not writing Haskell would be nice (I also do this often), but I'm not in favour of making instance names optional.

@jdegoes

This comment has been minimized.

Copy link

jdegoes commented Apr 23, 2015

Making the names optional just adds complexity for very little gain.
👍

@puffnfresh

This comment has been minimized.

Copy link
Contributor

puffnfresh commented Apr 24, 2015

I had a go at implementing @garyb's suggestion this morning and things got more complicated than I expected pretty fast. I'm happy to leave at that, the cost isn't worth it.

@chexxor

This comment has been minimized.

Copy link
Contributor

chexxor commented Jan 19, 2018

I've been wondering lately why PS doesn't allow omitting an instance's name - I'm glad there's already discussion about it here. The chat room had a discussion about this today and I wanted to reflect from there some more reasons why named instances is necessary.

  • Deterministic names are good and we don't have a good function for that which still produces nice names.
  • Also, consider the combinations of data types and class names and duplicate data types in different modules: instance Functor FooBar Baz -> ?; instance Functor Foo BarBaz -> ?
  • Nice to name an instance differently than class or type: instance refl :: TypeEquals a a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment