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

Fix bug super ref check doesn’t honor spec evaluation order #5801

Merged
merged 2 commits into from
Jun 26, 2017

Conversation

buunguyen
Copy link
Member

Q A
Patch: Bug Fix? yes
Major: Breaking Change? no
Minor: New Feature? no
Deprecations? no
Spec Compliancy? yes
Tests Added/Pass? yes
Fixed Tickets Fixes #5800
License MIT
Doc PR no
Dependency Changes no

Per spec, the following is valid code but Babel throws 'super.*' is not allowed before super() error.

class Foo extends Bar {
  constructor() {
    super[super().method]();
  }
}

Fixed by performing validation in exit handler of MemberExpression whose object is super. Also added a related test for the case of super.foo(super()); (should throw).

@mention-bot
Copy link

@buunguyen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @existentialism, @hzoo and @loganfsmyth to be potential reviewers.

@@ -0,0 +1,3 @@
{
"throws": "'super.*' is not allowed before super()"
Copy link
Member

Choose a reason for hiding this comment

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

Is this is valid, why are we asserting it throws?

Copy link
Member Author

@buunguyen buunguyen Jun 1, 2017

Choose a reason for hiding this comment

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

I think it's not valid because super.foo is being accessed before the call to super(). (See rule SuperProperty:super.IdentifierName in spec).

That's consistent with behavior in Firefox. This will throw:

class Bar {
  foo() {
    console.log('out');
  }
}
class Foo extends Bar {
  constructor() {
    super.foo(super());
  }
}
new Foo()

=> ReferenceError: |this| used uninitialized in Foo class constructor

But the following would run and print "out" in Firefox.

class Bar {
  foo() {
    console.log('out');
  }
  get method() {
    return 'foo';
  }
}
class Foo extends Bar {
  constructor() {
    super[super().method]();
  }
}
new Foo()

Copy link
Member

Choose a reason for hiding this comment

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

Lol, I got this confused with the valid case below. Ignore me.

@codecov
Copy link

codecov bot commented Jun 1, 2017

Codecov Report

Merging #5801 into 7.0 will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              7.0    #5801      +/-   ##
==========================================
- Coverage   84.91%   84.87%   -0.04%     
==========================================
  Files         284      284              
  Lines        9928     9927       -1     
  Branches     2788     2786       -2     
==========================================
- Hits         8430     8426       -4     
- Misses        988      991       +3     
  Partials      510      510
Impacted Files Coverage Δ
...bel-plugin-transform-es2015-classes/src/vanilla.js 90.59% <100%> (-0.05%) ⬇️
packages/babel-traverse/src/path/context.js 84.48% <0%> (-1.73%) ⬇️
packages/babel-traverse/src/visitors.js 85.71% <0%> (-0.96%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e739f9...6e31d0e. Read the comment docs.

@existentialism existentialism added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jun 1, 2017
# Conflicts:
#	packages/babel-plugin-transform-es2015-classes/src/vanilla.js
@buunguyen
Copy link
Member Author

Just resolved conflict from #5802 that was just merged.

@buunguyen
Copy link
Member Author

Any further feedback on this? Thanks!

@jridgewell jridgewell merged commit 033bad3 into babel:7.0 Jun 26, 2017
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants