-
Notifications
You must be signed in to change notification settings - Fork 2k
[CS2]: Fix #4591: multiple accesses after super #4592
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
[CS2]: Fix #4591: multiple accesses after super #4592
Conversation
@@ -1808,3 +1808,19 @@ test 'Bound method of immediately instantiated class with expression base class | |||
|
|||
{derivedBound} = a | |||
eq derivedBound(), 3 | |||
|
|||
test "#4591: super.x.y, super['x'].y", -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually super.foo
refers to a method named foo
, as in super.foo()
. We should add another test that tests super.x.y()
and super['x'].y()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also super.x['y']
and super.x['y']()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, super.foo
still has the implicit call there – it means super(arguments...).foo
, so why'd that case be special?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vendethiel that changed in 2
to support ES-style super
invocations. Compare bare super, super with parens, super with access, and super with both.
Deeper accesses against a super
field could be useful if you're trying to bind
/call
/apply
it, I guess.
class A
f: -> super.f.apply(this, arguments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I didn't follow that. That's good to know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@helixbass do you mind adding these additional tests, unless there’s some reason they don’t make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeoffreyBooth added tests for the cases you outlined
@connec that's a more realistic use case than anything I thought of, I just noticed the hole in the grammar
@helixbass if you don’t mind merging and resolving conflicts, I think this is ready. @lydell, @vendethiel or @connec, any notes? |
|
Also, that error is pretty surprising (it's not from this PR, this happens as well on
|
WRT #4601:
This PR hasn't been rebased yet, so the same error happens. Once rebased I think it should work, but we might want to add a test. |
@GeoffreyBooth merged @vendethiel none of those seem directly relevant to this PR (or #4601). I don't know if |
@connec or @vendethiel or @lydell, any other notes? |
Nope. If needed we can revisit the rest later. |
Surprisingly, just wrapping a
new Value()
around theSuper
in the grammar didn't seem to break anything and madesuper.x.y
andsuper['x'].y
workJust looking through references to
Super
innodes.coffee
, I didn't see anything that looked like it would need updating, but there could be gotchas I'm not aware of