Navigation Menu

Skip to content
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

#2599 is too strict ("cant return from a constructor") #2728

Closed
jldailey opened this issue Feb 26, 2013 · 39 comments
Closed

#2599 is too strict ("cant return from a constructor") #2728

jldailey opened this issue Feb 26, 2013 · 39 comments

Comments

@jldailey
Copy link

The way #2599 is implemented breaks lots of existing code, and breaks two powerful patterns: Singleton classes and Decorator classes.

class MySingleton then constructor: -> return masterInstance

class MyDecorator then constructor: (simpler) -> return decorated(simpler)

Singletons and Decorators are top-tier development patterns (not just "hacks", as suggested by the original discussion).

Not that the whole idea is wrong, "class MyError extends Error" should work smoothly, it's just the part where it now throws a Syntax Error at compilation time to prevent an explicit return; this should just be a Warning, or an optional Syntax Error (with a command line flag?).

If the developer typed r-e-t-u-r-n in the body of their constructor, then they meant it (as opposed to what happens with default constructors such as in the "extends Error" case).

This change makes CS 1.5 unusable for me (I have a whole framework that I use in production every day based around a Decorator pattern).

@gr2m
Copy link

gr2m commented Feb 26, 2013

Same here, I use the the Decorator pattern extensively and was quite surprised that it's forbidden now.
Is there any workaround?

@gr2m
Copy link

gr2m commented Feb 26, 2013

A common use case for me is to allow an instance to be called directly:

class ShareModule
  constructor : ->
    api = @open
    $.extend api, this

    return api

This way I can do things like

share = new ShareModule

share.add( 'name', options )
share( 'name' ).findAll()

Is there any workaround to support such an API? It breaks tons of code of mine.

@epidemian
Copy link
Contributor

Ho man, i though i had left singletons behind when i moved from the Java world...

The most important design principle of CoffeeScript is "it's just JavaScript". JavaScript supports object literals, so there's absolutely no need for defining a whole class and then adding some protection mechanism to it to prevent it from instantiating more than once. Use a global (or top-level in your file) object and reference it wherever you need it.

That being said, if you still think that having a constructor and calling new even when it won't instantiate anything, you can still totally do it:

MySingleton = -> masterInstance

s1 = new MySingleton
s2 = new MySingleton
s1 is s2 # -> true; yay! totally intuitive!

Same thing for that example of the Decorator pattern.

@gr2m, i don't understand what the example of ShareModule is supposed to do, sorry.

Edit: oh, i see, new ShareModule is returning a function instead of a ShareModule. Still replaceable by a (simpler) function call i think.

@gr2m
Copy link

gr2m commented Feb 26, 2013

I would like to make instances of classes callable directly.

instance = new Something
instance.callInstanceMethod()
instance() # call directly

makes sense?

@epidemian
Copy link
Contributor

Yes, i realized what it was doing afterwards. But, if you do that, then they are not really instances of that class, aren't they?

# In CoffeeScript 1.4.0
class Something
  constructor: ->
    return -> console.log "yeah, i'm callable" 

(new Something) instanceof Something # -> false; Oh s#$*...

These are the kind of ugly corner-cases that we want to avoid by having a sane default behaviour for constructors. But it's just that: a default behaviour. If you need some other behaviour for whatever reason, you can still totally use JS-style constructors like the example on my previous message.

@gr2m
Copy link

gr2m commented Feb 26, 2013

ay, cheers. Thanks for aiming for clarity & simplicity.

@jldailey
Copy link
Author

Right, "it's just JavaScript", and JavaScript very intentionally allows returning from a constructor, it's not accidentally in the language. I would argue that throwing the Syntax Error makes the classes way more Java-like than does supporting singleton classes (which I don't personally use, it was just an example of how many serious usage patterns [as opposed to "hacks"] are left out by this change).

Also, one problem with MySingleton = -> masterInstance is that MySingleton is now an anonymous function (which has lots of consequences that CS ignores, like all-anonymous stack traces from CS-compiled code if you don't use classes; I wish there was a second way to set a function's name but that is a separate topic).

Also, it's not the default behavior, it is now the only behavior of a "class" (which CS has made different from simply a function). If I want a class, it has to work this way, which is why the ticket simply says it is "too strict", rather than incorrect; it should be optional/suggested.

Your "edge case" is my cornerstone.

@satyr
Copy link
Collaborator

satyr commented Feb 26, 2013

(new Something) instanceof Something # -> false; Oh s#$*...

.__proto__ to the rescue:

class Callable
  constructor: ->
    me = -> #...
    me.__proto__ = Callable::
    `return me`

@jldailey
Copy link
Author

re: way more Java-like

Felt I should elaborate. The syntax error forces users of classes into a strict Class Hierarchy world like Java, rather than the actual prototype world that JavaScript is.

The .__proto__ example illustrates this well... in "just JavaScript", you can slice and dice your instances' prototype chain on the fly, this new syntax error amounts to an overly strict adherence to a fictional hierarchy (one modeled after Java's).

@vendethiel
Copy link
Collaborator

__proto__ is non-standart, AFAIR. And chrome inspector display "abc" for abc = -> throw 'foo'.

@mikepack
Copy link

+1 on this issue. Breaking my app.

@jldailey
Copy link
Author

Chrome jumps through all sorts of hoops to attempt to 'name' stack frames based on their context, so you get all kinds of funky names that are more-or-less helpful... but it's a heuristic at best, and specific to a single application at that.

abc = -> throw 'foo'
abc.name is '' # empty string, un-assignable

@jldailey
Copy link
Author

Changing this to a warning restores broken functionality, and as far as I can tell, loses none of the fixes that @epidemian wrote. Am I missing something that we actually gain from being strict about this?

@vendethiel
Copy link
Collaborator

You can assign displayName -- not sure where and where not it's supported

@jldailey
Copy link
Author

.displayName has no effect on Chrome, Node, or Firefox.

But, I don't want this to get derailed by a separate topic.

@gampleman
Copy link

I think this boils down to what the class keyword is for. I think it's idea is to express a common need to escape from the power and expressiveness of prototype manipulation into a safe world of fake hierarchies. If I see a class it gives me several assurances: I can inherit from it, I can instantiate it. That's what a class is. Unlike Java we have other facilities at our disposal that we can use if it is not convenient that these properties should hold. That is why I think that it's a good thing that singleton classes aren't supported - they are a hack of what it means to be a class.

@jldailey
Copy link
Author

@gampleman That is thoughtful, and interesting, but it's an ideological argument about [this] language, and what it should/should not be.

I'm asking a super specific question, about throwing an exception when we see this happening:

What do we gain by crashing the compiler in this case?

Some weak assurance that the broken-in-many-ways instanceof is not broken in this one way?

Same question, reversed: What do we lose by issuing a warning and continuing to compile?

Someone else who uses my class is now only guaranteed that it is "just JavaScript"?

@jldailey
Copy link
Author

Even in nodes.coffee itself, half the calls are throw SyntaxError and the other half are throw new SyntaxError, and both forms chug away happily.

This works because SyntaxError returns from it's constructor.

@epidemian
Copy link
Contributor

@jldailey, fair points.

Also, it's not the default behavior, it is now the only behavior of a "class" (which CS has made different from simply a function).

But you still have functions that work exactly as in JS (well, except that they don't have a name property, as you noted). You can think of classes as syntax sugar for constructor functions just like array comprehensions are syntax sugar for loops (and guess what, array comprehensions also have some restrictions that normal loops don't have, like not being able to return from them).

# This...
class Foo extends Bar
  constructor: (@prop) ->
  method: -> console.log @prop

# Is just syntax sugar for:
Foo = (@prop) ->
Foo extends Bar
Foo::method = -> console.log @prop

To be honest, at first i was really doubtful about removing this existing functionality from classes. But all the examples of possible uses for other typed constructors i saw made me more confident that this restriction makes sense. I think there hasn't been a case where the same results couldn't be achieved by using just a function that in turn calls (or not) the constructor (without giving up class syntax).

I'd really like to see a good example of other typed constructors that could change my mind and say "yes, this is the best way of doing this!".

@satyr
Copy link
Collaborator

satyr commented Feb 27, 2013

Note that having to define an extra function is an unignorable cost.

I'd really like to see a good example of other typed constructors

How about this one:

class Wrapper
  constructor: (@it) ->
    return it if it instanceof Wrapper  # avoid double-wrap

a = {}
b = new Wrapper a
c = new Wrapper b
b is c  # true

Object works this way too:

$ node -pe '(o = {}) == new Object(o)'
true

@jldailey
Copy link
Author

What is gained from the throw, specifically the throw?

I thought about lining out my code and why I prefer to do it that way (a plugin system, orthogonal to inheritance), but it's quite beside the point. While there are some "hacks" you might want to excise from the domain of possible programs, these are not fringe patterns; Plugins, Decorators, Factories, Proxies, etc.

How can you say that these are all so clearly the wrong way to solve every problem that we must disallow them in all cases? Who could possibly make that judgement with so much conviction that they stand in the face of the many real things in the world that people made in exactly these ways?

You don't prefer to return from your constructors, that's great, and I prefer to avoid the extra function-call per instance, I prefer having functions with names, these are what is important to me.

But the good news, is that we can both have what we want!

@jldailey
Copy link
Author

I'd really like to see a good example of other typed constructors

Also, they don't always have to be other-typed. In my case, I have a list of plugins to run, each decorating (or replacing) the instance, and the last plugin in the list is the 'core' plugin, which I know will always return the proper type, so (new Foo) instanceof Foo would in fact always be true, and still trigger this SyntaxError.

@epidemian
Copy link
Contributor

@satyr,

How about this one:

Interesting. How about?

class Wrapper
  constructor: (@it) ->
  @wrap = (it) ->
    # avoid double-wrap
    if it instanceof Wrapper then it else new Wrapper it

a = {}
b = Wrapper.wrap a
c = Wrapper.wrap b
b is c

For me, the biggest benefit of doing things this way is that it doesn't break the user's expectations. If i read obj = ObjectPool.get param i know the get function can do whatever it wants to give me the object i asked for; but it if read obj = new SomeObject param i expect a new SomeObject to be instantiated (otherwise, why would the keyboard be called new?).

@jldailey

While there are some "hacks" you might want to excise from the domain of possible programs, these are not fringe patterns; Plugins, Decorators, Factories, Proxies, etc.

How can you say that these are all so clearly the wrong way to solve every problem that we must disallow them in all cases?

Wow, wait a second there. I never said that.

Why is the relevance of the design patterns suddenly being questioned here? It's not like design patterns become unimplementable because of removing these other typed constructors. After all, most of those design patterns were published by the GoF with a Java mindset, and Java lacks the ability to "override" new (that's why you see Factory Method implemented with an interface for example).

I'll concede that i may have trashed Singleton a little bit; but that'll mostly be because Singleton is the ugliest anti-pattern ever made, nothing personal against it really :P

I don't really want to expand too much into this design pattern discussion. Just want to say:

CoffeeScript is just JavaScript.

JavaScript is not Java.

Design patterns' implementations may (and will) vary from language to language.

@jldailey
Copy link
Author

i expect a new SomeObject to be instantiated (otherwise, why would the keyboard be called new?).

new means: clone the prototype, and run some code against it, optionally returning that prototype. That just is what it means, so what you expect just is not what happens in JavaScript, and I'm sorry that you want JavaScript to be more like Java and prevent you from replacing the return value, so that you have to create separate factory functions.

Run time strategies like this should be optional, for the sake of expressiveness, or safety, or whatever you care about most, but not required by the compiler. In attempting to make the compiler think for us (by preventing this
"unsafe" usage of just JavaScript), now we have to do one more thing: work around the compiler (with a second function call, or whatever other method someone spends unnecessary time thinking up).

Why is the relevance of the design patterns suddenly being questioned here?

Because you clearly said you reviewed all examples of possible uses (some of which are not crazy, or edge cases, or hacks, or abnormal in anyway), and even though many of them are currently being used, none of them were worthy of preserving this feature.

The basic premise even, that this new code says: "other-typed constructors aren't allowed" is incorrect, as a great many "same-typed" constructors are now errors.


But, I will leave this thread here, I suspect I won't be able to tell you anything you haven't already dismissed, and we are not engaged in productive debate, but rather an argument about style. I do not want Java, I want the flexibility of JavaScript preserved in CoffeeScript.

There is a work-around in @satyr 's post above:

class Foo then constructor: -> `return ...`

The backticks... I'll grumble a little every time, but I'll live.

@epidemian
Copy link
Contributor

@jldailey it's funny how you turned around the argument of not being Java-like. I'll try to change my tone; no need for being rude :)

new means: clone the prototype, and run some code against it, optionally returning that prototype.

Yes, but i was talking about expectations. The same thing could be said for [3, 4] + [5, 6] being the string '3,45,6', or [] == 0 being true: even if that's what the JS says they are, it goes against people's expectations. And i think it's awesome that CS fixes the second one for example.

I'm now curious about your opinion on other JS "features" removed from CS, like JS's == operator, or fallthrough in switch/case clauses, or function name hoisting for example.


I understand that i may seem like a dismissive arrogant; i'm sorry for that, and for this conversation not being constructive. But i really can't see why would you prefer to grumble every time writing

class Foo then constructor: -> `return something`

When you can do

Foo = -> something

No hack needed. No factory function either. I feel like i'm missing something here.

@jldailey
Copy link
Author

I keep asking: what do we gain? and the answer is always: we don't lose much.

You can see why this conversation won't be productive right?

  • == is only useful for == null which ? replaces, so I don't miss it.

  • Case fall-through isn't really missing: when "a","b" then "c".

  • Function name hoisting I just don't miss.

    Foo = -> something

Again, Foo is an anonymous function now. What if I want a name and a return? This is the part where you are saying, "we don't lose much", but not answering what we gain. The burden of proof is with the breaking change, we should not be here having to defend all the programs that we've written this way, or could possibly ever write this way, what a pointless exercise. You should be here defending breaking those programs, by showing what we gain.

@epidemian
Copy link
Contributor

I keep asking: what do we gain? and the answer is always: we don't lose much.

Ah, OK. I though the answer to that question was rather obvious, but it seems it wasn't. What do we gain?: A simpler language.

The previous rules for when constructors returned a value were rather complex: They were something like this (i hope i get them right):

  1. If the constructor is defined as a function expression after constructor: then it doesn't follow the "return the last expression" rule but rather needs an explicit return to return a value.
  2. If the constructor is not defined, then an auto-generated constructor is created and it returns a value only if the super-class constructor does (this is what caused the error in Fix default constructor for subclasses of native objects #2359, and was not always like this... before 1.3.1 auto-generated constructors did not return a value). Notice that constructor: -> super is not equivalent to not defining a constructor, because of rule 1.
  3. If the constructor is a reference to another function (an "external constructor") then the constructor will return a value if that function returns a value (thus following the "return the last expression" rule in this case).

The new rule for when class constructors return a value are much simpler: they never do.

I hope simpler rules and less corner cases are a gain that you consider worthy.

In any case, "we don't lose much" could be a reason not to keep (or not to add) a language feature. Every feature has a cost, and if you don't gain much from that feature you're probably losing more.

@jldailey
Copy link
Author

Points 1 and 2, and all the use cases you have presented, can be addressed by fixing the default constructor, they do not require that you throw a Syntax Error, which is all I'm talking about, those five lines of code that search for a Return token in the constructor and throw a Syntax Error.

And Point 3, this fix just creates even more complicated rules: the exact same function now behaves completely differently, and is possibly now invalid syntax, based on how someone [else] assigned it, later in the program!?

But, only if that function was written in this same file... and in CoffeeScript... and didn't use backticks... or any prototype tricks. So now we have all the complexity of both rules! Now I call new Foo; was Foo written in CS or JS? Was it written using class syntax or just an anonymous constructor? My expectations now depend on all these factors. The inability to return from an array comprehension is not "leaky" like this.

Returning from constructors is not an "edge case"; it has nothing to do with "other-typed" constructors, it's not an afterthought, or an accident like [] == 0. It's a powerful language feature, and it makes me really sad that you only see that power as "too complicated"... so complicated that it should crash when anyone touches it.

@jashkenas jashkenas reopened this Feb 28, 2013
@davidchambers
Copy link
Contributor

My expectations now depend on all these factors.

My expectations haven't changed, so I feel I'm missing something. It seems to me that…

  • before the change I needed to look at Foo to know what new Foo would do, and
  • after the change I still need to look at Foo to know what new Foo will do.

@epidemian
Copy link
Contributor

My expectations haven't changed, so I feel I'm missing something. It seems to me that…

There is a change, though...

  • before the change I needed to look at Foo to know what new Foo would do, and
    • And if Foo is a class i need to look at the logic in its constructor; or at its super-class' constructor if it doesn't have one; or at the external function that is defined as its constructor.
  • after the change I still need to look at Foo to know what new Foo will do.
    • And if Foo is a class, i know it won't return something else.

@davidchambers
Copy link
Contributor

Right. What I'm keen to understand is why @jldailey believes this change increases the cognitive overhead.

@paulmillr
Copy link

-10 on reverting 1.5.0 change

es6 classes don’t allow you to make some weird edge-case-decorator-pattern-classes

+10 for following es6 way

@jashkenas
Copy link
Owner

I think it's best to revert it, and simply leave it as best practice to never return an "other type" from a constructor. Cases like this are just too heartbreaking otherwise:

http://blog.quickui.org/2013/02/27/quickui-breaks-with-coffeescript-1-5/

@epidemian
Copy link
Contributor

Well, it kinda sucks to do the wrong thing for the "better" of the community.

But yeah, the case of QuickUI is very sad... the real PITA is that the QuickUI creator had to go thought so much trouble just to subclass jQuery. But it's understandable, for his use case, to want to let users subclass QuickUI components using CS's class syntax while the the parent class does all sorts of magic in the constructor to return a proper subclass of jQuery (@JanMiksovsky explained that on #2359). I wonder, however, if the (inherited) other typed constructor in the sub-class is totally necessary, as it seems QuickUI then uses a class function, called create, to instantiate QuickUI components.

Thanks for the link to Twitter, @paulmillr, the reference to the XKCD comic really made my day xD

At least the story of breakage in the CS Google Group had a better ending.

Well, let's discuss how to revert this at #2599, OK?

@jldailey
Copy link
Author

What I'm keen to understand is why @jldailey believes this change increases the cognitive overhead.

after the change I still need to look at Foo to know what new Foo will do.

And if Foo is a class, i know it won't return something else.

But, only if Foo was written in CS, in the same file, with class syntax, without backticks, and without prototype tricks.

Otherwise (frequently) all the old rules still apply anyway.

The only substantive gain I get is that after new Foo, if all the new AND old conditions hold, then I can trust that (new Foo) instanceof Foo will always be true. A very weak gain, IMO, when weighed against what it breaks.

@davidchambers
Copy link
Contributor

Otherwise (frequently) all the old rules still apply anyway.

Right. From a cognitive overhead standpoint we're no better off than we were before, but neither are we worse off. (An earlier comment implied there are now more factors at play, making it more difficult than before to reason about new Foo.)

@jashkenas
Copy link
Owner

Thanks to the tireless @epidemian, this is now back the way it was. Once things seems stable, we'll push it out as a 1.5.1 release. Sorry for the inconvenience.

@JanMiksovsky
Copy link

Just following up on an email thread I've been exchanging on this topic with @epidemian.

First, I did manage to find a workaround for QuickUI's needs that appears to work as long as devs instantiate control classes with "new" (even though vanilla jQuery doesn't require it). I found one situation in which jQuery invokes its own "other typed" constructor without "new": in its pushStack() function. Since CS 1.5-based control subclasses will end up invoking pushStack(), those classes break, but it's not too difficult to patch pushStack (it's short and simple) to add the "new". With that patch, and by requiring the use of "new" generally, I would be able to workaround the constructor limitations of CS 1.5.

That said, I'm grateful to see this change undone. I'm really impressed by the responsiveness of the CoffeeScript community generally, and in particular appreciate @epidemian's careful investigation of QuickUI's problem here.

I want to be clear that, even if this particular change hadn't been rolled back, I still would have harbored no complaints. It seems impossible to move a complex project like CoffeeScript forward without being forced to occasionally break things. While I was dismayed by the problems the CS 1.5 release presented for QuickUI, I view such problems as part of the price to pay for using an actively-maintained programming language. Last year I ported QuickUI from JS to CS, and overall am extremely happy with that decision. The response on this particular issue only serves to reinforce my conviction that that was the right move.

@epidemian
Copy link
Contributor

Awesome news, @JanMiksovsky. Thanks for sharing 😃

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

No branches or pull requests