Skip to content

[ES7] Exponentiation #4914

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

Merged
merged 42 commits into from
Oct 12, 2015
Merged

[ES7] Exponentiation #4914

merged 42 commits into from
Oct 12, 2015

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Sep 21, 2015

Issues #4812

// ** operator is right-assocative
break;
}
else if (token !== SyntaxKind.AsteriskAsteriskToken && newPrecedence <= precedence) {
Copy link
Member

Choose a reason for hiding this comment

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

You can just keep this if (newPrecedence <= precedence)

Alternatively you could make an inner if:

// Check the precedence to see if we can "take" the operator.
// When the precedence is too high, we'll need to build up a right-heavy 
// expression by calling ourselves recursively below.
// Otherwise, we'll return the current node to our caller so that they can
// build a left-heavy expression using the current node as a right operand.
if (newPrecedence <= precedence) {
    // For left-associative operators, a caller can only accumulate left-heavy expressions
    // like this if the new precedence is less than or equal to than the current one.
    // However, for right-associative operators (i.e. the ** operator), the new operator's
    // precedence needs to be strictly less, since something like  a ** b ** c needs to be
    // parsed as the right-heavy expression a ** (b ** c).
    //
    // Here we ensure that only if the new operator is right-associative do we
    // check for strict equality on precedence.
    if (token !== SyntaxKind.AsteriskAsteriskToken || newPrecedence < precedence) {
        break;
    }
}

Comments optional, though I think they'd be useful 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the above check is the same as

if(newPrecenden <= precendence) {
    break;
}

you want to also break if the operator has lower precedence not just equal.

a * b - c

will be parsed incorrectly as a * (b-c)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, as we discusssed offline, this should be

token !== SyntaxKind.AsteriskAsteriskToken || newPrecedence < precedence

Amended the above

Copy link
Member

Choose a reason for hiding this comment

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

Let's just do

const canTakeNextOperator = token === SyntaxKind.AsteriskAsteriskToken ? 
    newPrecedence < precedence :
    newPrecedence <= precedence;

Thanks @weswigham 😄

@DanielRosenwasser
Copy link
Member

Can you test the emit for the following?

var a, b, c;
new a ** b ** c;
new a ** new b ** c;
new (a ** b ** c);

@DanielRosenwasser
Copy link
Member

Also, can you add a test for the following?

(<number><any>(1,2)) ** (<any>3,4)

@DanielRosenwasser
Copy link
Member

Also maybe an ES5/6 test with the following:

// Object literal binding assignment
// Causes an early error, but it is useful to see the emit.
({ toExponential } **= 1 ** 2);

@yuit
Copy link
Contributor Author

yuit commented Sep 24, 2015

what are you trying to test with this

var a, b, c;
new a ** b ** c;
new a ** new b ** c;
new (a ** b ** c);

@jeffreymorlan
Copy link
Contributor

Pre-emptive bug report: downlevel emit for **= duplicates expressions with side effects, e.g. a[i++] **= 2; becomes a[i++] = Math.pow(a[i++], 2). In general, an LHS of the form x.prop or x[y] may require x and/or y to be saved to a temp variable, if they are not simple variables/constants already.

There should also be an error if the global Math is masked by a local variable of the same name.

@yuit
Copy link
Contributor Author

yuit commented Sep 24, 2015

@jeffreymorlan good catch. Thanks! 💃

@yuit
Copy link
Contributor Author

yuit commented Sep 25, 2015

thanks @rbuckton for updating me about yet another discussion regarding the precedence of the operator (here is the link http://jsbin.com/bihilaveda/1/edit?output). So there may be a change on the precedence. Full discussion (https://esdiscuss.org/topic/exponentiation-operator-precedence)

@@ -3015,7 +3015,31 @@ namespace ts {
let newPrecedence = getBinaryOperatorPrecedence();

// Check the precedence to see if we should "take" this operator
if (newPrecedence <= precedence) {
// - For left associative operator (all operator but **), only consume the operator
// , recursively call the function below and parse binaryExpression as a rightOperand
Copy link
Member

Choose a reason for hiding this comment

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

Move the comma to the preceding line, add a comma after "below"

// a[0] **= a[0] **= 2
// _a = a, _a[0] = Math.pow(_a[0], (_b = a, _b[0] = Math.pow(_b[0], 2)));
// ^ -> required extra parenthesis controlled by shouldEmitParenthesis
// exponentiation compount in variable declaration
Copy link
Member

Choose a reason for hiding this comment

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

Compound exponentiation

@@ -204,7 +204,7 @@ namespace ts {
{
name: "target",
shortName: "t",
type: { "es3": ScriptTarget.ES3, "es5": ScriptTarget.ES5, "es6": ScriptTarget.ES6 },
type: { "es3": ScriptTarget.ES3, "es5": ScriptTarget.ES5, "es6": ScriptTarget.ES6, "es7": ScriptTarget.ES7 },
Copy link
Contributor

Choose a reason for hiding this comment

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

i would remove this.

@yuit
Copy link
Contributor Author

yuit commented Oct 12, 2015

@mhegazy and @DanielRosenwasser let me know if you have anymore comments


synthesizedLHS = <ElementAccessExpression>createSynthesizedNode(SyntaxKind.ElementAccessExpression, /*startsOnNewLine*/ false);

let identifier = emitTempVariableAssignment(leftHandSideExpression.expression, /*canDefinedTempVariablesInPlaces*/ false, /*shouldemitCommaBeforeAssignment*/ false);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldEmitCommaBeforeAssignment

yuit added a commit that referenced this pull request Oct 12, 2015
@yuit yuit merged commit 77eaf04 into master Oct 12, 2015
@yuit yuit deleted the exponentiation branch October 12, 2015 23:37
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants