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

Implement super #313

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Implement super #313

wants to merge 5 commits into from

Conversation

mizchi
Copy link

@mizchi mizchi commented May 14, 2014

It works in most cases but with memberAccessOp doesn't.
I can't guess some specs. So I want to ask here.

@vendethiel
Copy link
Contributor

oh :).

[CS.Super, ({arguments: args, compile, inScope, ancestry}) ->
# TODO: fix me dirty way to catch assignee
classNode = find ancestry, (node) => node.hasOwnProperty('ctor')
functionName = (find ancestry, (node) -> node.assignee).assignee.data
Copy link
Author

Choose a reason for hiding this comment

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

I want to catch member assigment of class from here but how can I decide its order?

@akre54
Copy link

akre54 commented May 14, 2014

Excellent!

@mizchi
Copy link
Author

mizchi commented May 15, 2014

Fix by myself :) but I have no confidence it is correct way... in especially assignee detection.

@michaelficarra please give me advice, or merge if you don't care.

@@ -380,6 +380,12 @@ inlineHelpers =
exp: -> new JS.CallExpression (memberAccess (new JS.Identifier 'Math'), 'pow'), arguments
undef: -> new JS.UnaryExpression 'void', new JS.Literal 0
slice: -> new JS.CallExpression (memberAccess (memberAccess (new JS.ArrayExpression []), 'slice'), 'call'), arguments
super: (className, functionName, args) ->
applied = if args.length > 0 then new JS.ArrayExpression (map args, expr) else new JS.Identifier 'arguments'
Copy link

Choose a reason for hiding this comment

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

You could probably do call for > 0 args, falling back to apply for 0 args

Copy link
Author

Choose a reason for hiding this comment

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

I checked jashkenas/coffeescript now.

    Sub.prototype.func = function() {
      Sub.__super__.func.apply(this, arguments);
      Sub.__super__.func.call(this, 3);
      return Sub.__super__.func.call(this, 3, 5);
    };

Yes, you are right. I'll fix.

@akre54
Copy link

akre54 commented May 15, 2014

There are still a few pending tests that could get squashed by this. The static super ref needs to reference __super__.constructor (CS), and some of the scope lookup stuff may need some streamlining. But awesome to get the main case out of the way on this!

@mizchi
Copy link
Author

mizchi commented May 16, 2014

I will implement static super too. After that I want to reply other peoples fix.

@@ -374,6 +374,7 @@ secondaryExpressionNoImplicitObjectCall = expressionworthy / assignmentExpressio
expressionworthy
= functionLiteral
/ conditional
/ super
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be in primaryExpression? super.a should be allowed. Though it would introduce ambiguity with super a, it should still parse fine.

@michaelficarra
Copy link
Owner

Thanks for the PR, but there's a lot of work still needed here and, more importantly, I am still not sure of the future of super in CoffeeScript to begin with. Many, including myself, have been petitioning the other maintainers for adoption of ES6 semantics.

@akre54
Copy link

akre54 commented May 19, 2014

Shouldn't CSR at least try to match CS super as it stands now, and then fix if CS changes? There hasn't been any progress on super in years and it's preventing many people (myself included) from switching to CSR.

@michaelficarra
Copy link
Owner

Sure. I'd merge a PR that achieves that. We need all those tests I mentioned in place first, though.

@mizchi
Copy link
Author

mizchi commented May 19, 2014

My private goal is that I want to make CSR which has compatibility to CS. (And for my hobby project TypedCoffeeScript that aimed to super set of CS, but you, execept me, don't have to take care about it:)

Anyway, I will fix some point you mentioned first. Thx!

# B = null
# makeClass = ->
# B = class extends this
# func: -> super + ' B'
Copy link
Author

Choose a reason for hiding this comment

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

I tried but I didn't know generating class$ without var name confliction. Need to change on cscodegen?

Copy link
Owner

Choose a reason for hiding this comment

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

No, cscodegen is for generating CoffeeScript source code from a CoffeeScriptRedux AST (and is woefully incomplete). Can you explain the problem in more detail?

@mizchi
Copy link
Author

mizchi commented May 20, 2014

Now it works about super and static method super in class as CS specs.

@@ -589,7 +589,10 @@ leftHandSideExpressionNoImplicitObjectCall = callExpressionNoImplicitObjectCall
/ secondaryExpressionNoImplicitObjectCall

callExpression
= fn:memberExpression accesses:callExpressionAccesses? secondaryArgs:("?"? secondaryArgumentList)? {
= SUPER accesses:callExpressionAccesses? secondaryArgs:secondaryArgumentList? {
Copy link
Owner

Choose a reason for hiding this comment

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

Please copy these lines to the callExpressionNoImplicitObjectCall definition. That rule is used in places where a call is not allowed to be passed an implicit object, such as the B C call in

class A extends B C
  constructor: ->

Then include a test for a super call in that position.

@mizchi
Copy link
Author

mizchi commented May 21, 2014

Oops, I passed only syntax and broke execution. I'll fix...

super('one/').length + string

result = (new ThirdChild).func 'four'
# eq result, '9two/three/four' # can't pass super('one/').length + string
Copy link
Author

Choose a reason for hiding this comment

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

I failed it yet.

@notslang
Copy link

+1 thanks @mizchi, this is a hugely important for me being able to move to CS-redux

@ef4
Copy link
Contributor

ef4 commented May 27, 2015

I incorporated this PR into my ES6 branch, so I can piggyback off the super keyword. It's working pretty well. Demo:

$ cat sample.coffee

class Foo extends Bar
  someValue: 1
  constructor: (a) ->
    super(a)
  doIt: ->
    super("yay")

$ ./bin/coffee --bare --js --input ./sample.coffee --target-es6

// Generated by CoffeeScript 2.0.0-beta9-dev-es6
class Foo extends Bar {
  constructor(a) {
    super(a);
  }
  doIt() {
    return super.doIt('yay');
  }
}
Foo.prototype.someValue = 1;

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

Successfully merging this pull request may close these issues.

6 participants