Prevent calls to super outside of a constructor#2452
Prevent calls to super outside of a constructor#2452ChadKillingsworth wants to merge 2 commits intogoogle:masterfrom
Conversation
brad4d
left a comment
There was a problem hiding this comment.
Thanks for taking this on.
Just one small requested change.
| "JSC_CANNOT_CONVERT_YET", | ||
| "ES6 transpilation of ''{0}'' is not yet implemented."); | ||
|
|
||
| static final DiagnosticType INVALID_SUPER = |
There was a problem hiding this comment.
Could you rename this to INVALID_SUPER_CALL?
cc8e8c3 to
077ab55
Compare
|
@ChadKillingsworth sorry, but @MatrixFrog refactored class transpilation out of |
|
I thought I'd checked that this wouldn't cause conflicts. Sorry!
|
077ab55 to
960f150
Compare
|
All fixed - it barely conflicted |
960f150 to
c3eed57
Compare
|
Sent for internal testing and review. |
|
|
||
| if (exprRoot.getParent().isNew()) { | ||
| compiler.report(JSError.make(node, INVALID_SUPER_CALL)); | ||
| } else if (NodeUtil.isCallOrNewTarget(exprRoot) |
There was a problem hiding this comment.
why isCallOrNewTarget() here?
It's redundant with the previous if.
| potentialCallee = parent; | ||
| Node exprRoot = node; | ||
|
|
||
| if (exprRoot.getParent().isGetElem() || exprRoot.getParent().isGetProp()) { |
There was a problem hiding this comment.
An internal test system found that changing this to be if (true) { didn't cause any test failures.
This implies a missing test and made me realize it's not as clear as it should be what's happening here.
Could this logic be clarified to something like this?
private void visitSuper(Node superNode, Node parent) {
checkState(superNode.isSuper());
if (parent.isCall()) {
// super(...)
visitBareSuperCall(superNode, parent);
} else if (parent.isGetProp()) {
checkState(parent.getFirstChild() == superNode);
// super.something
if (parent.getParent().isCall()) {
// super.something(...)
visitSuperDotSomethingCall(superNode, parent);
} else {
// super.something used in some other way
// report not supported yet error
}
} else if (parent.isGetElem()) {
checkState(superNode == parent.getFirstChild());
// super[...]
// report not supported yet error
} else if (parent.isNew()) {
// new super(...)
// invalid use of super
} else {
// some other use of super we don't support yet
}
}|
ping @ChadKillingsworth |
|
@brad4d I've been slammed - I hope to get back to this soon. |
|
@ChadKillingsworth |
|
@brad4d Changes made. |
|
@ChadKillingsworth |
|
test results look good |
Closes google#2452 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=157253710
Prerequisite for #2418
Emit errors for calls to
superoutside of a constructor.