Skip to content

Commit

Permalink
Fix printing edge cases in Nullish Coalescing and Optional Chaining
Browse files Browse the repository at this point in the history
This is a mix of changes, most importantly:

- Always print parens when mixing nullish coalescing and another logical
- Fixes assignment in the callee of an optional chain, which now blocks babel#11248

Also, cleans up a bit of code, and removes an unnecessary parens around `class A extends new B {}`
  • Loading branch information
jridgewell committed Mar 13, 2020
1 parent f9d4b79 commit 519cdff
Show file tree
Hide file tree
Showing 9 changed files with 73 additions and 35 deletions.
22 changes: 1 addition & 21 deletions packages/babel-generator/src/node/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,7 @@ function isOrHasCallExpression(node) {
return true;
}

if (t.isMemberExpression(node)) {
return (
isOrHasCallExpression(node.object) ||
(!node.computed && isOrHasCallExpression(node.property))
);
} else {
return false;
}
return t.isMemberExpression(node) && isOrHasCallExpression(node.object);
}

export function needsWhitespace(node, parent, type) {
Expand Down Expand Up @@ -97,18 +90,5 @@ export function needsParens(node, parent, printStack) {
if (isOrHasCallExpression(node)) return true;
}

/* this check is for NullishCoalescing being used with LogicalOperators like && and ||
* For example when someone creates an ast programmaticaly like this
* t.logicalExpression(
* "??",
* t.logicalExpression("||", t.identifier("a"), t.identifier("b")),
* t.identifier("c"),
* );
* In the example above the AST is equivalent to writing a || b ?? c
* This is incorrect because NullishCoalescing when used with LogicalExpressions should have parenthesis
* The correct syntax is (a || b) ?? c, that is why we need parenthesis in this case
*/
if (t.isLogicalExpression(node) && parent.operator === "??") return true;

return find(expandedParens, node, parent, printStack);
}
25 changes: 18 additions & 7 deletions packages/babel-generator/src/node/parentheses.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,6 @@ export function Binary(node: Object, parent: Object): boolean {
return true;
}
}

return false;
}

export function UnionTypeAnnotation(node: Object, parent: Object): boolean {
Expand Down Expand Up @@ -244,7 +242,8 @@ export function ConditionalExpression(node: Object, parent: Object): boolean {
t.isBinary(parent) ||
t.isConditionalExpression(parent, { test: node }) ||
t.isAwaitExpression(parent) ||
t.isOptionalMemberExpression(parent) ||
t.isOptionalMemberExpression(parent, { object: node }) ||
t.isOptionalCallExpression(parent, { callee: node }) ||
t.isTaggedTemplateExpression(parent) ||
t.isTSTypeAssertion(parent) ||
t.isTSAsExpression(parent)
Expand Down Expand Up @@ -272,16 +271,28 @@ export function OptionalCallExpression(node: Object, parent: Object): boolean {
);
}

export function AssignmentExpression(node: Object): boolean {
export function AssignmentExpression(
node: Object,
parent: Object,
printStack: Array<Object>,
): boolean {
if (t.isObjectPattern(node.left)) {
return true;
} else {
return ConditionalExpression(...arguments);
return ConditionalExpression(node, parent, printStack);
}
}

export function NewExpression(node: Object, parent: Object): boolean {
return isClassExtendsClause(node, parent);
export function LogicalExpression(node: Object, parent: Object): boolean {
switch (node.operator) {
case "||":
if (!t.isLogicalExpression(parent)) return false;
return parent.operator === "??" || parent.operator === "&&";
case "&&":
return t.isLogicalExpression(parent, { operator: "??" });
case "??":
return t.isLogicalExpression(parent) && parent.operator !== "??";
}
}

// Walk up the print stack to determine if our node can come first
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
1 + (a = 2);
1 + (a += 2);
a = a || (a = {});

(a = b)();
(a = b)?.();
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
1 + (a = 2);
1 + (a += 2);
a = a || (a = {});
a = a || (a = {});
(a = b)();
(a = b)?.();
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class A6 extends class {} {}

class A7 extends (B ? C : D) {}

class A8 extends (new B()) {}
class A8 extends new B() {}

class A9 extends (B, C) {}

Expand All @@ -32,4 +32,4 @@ async function f1() {

function* f2() {
class A16 extends (yield 1) {}
}
}
Original file line number Diff line number Diff line change
@@ -1,2 +1,17 @@
const foo = 'test'
console.log((foo ?? '') == '')
console.log((foo ?? '') == '')


a ?? (b ?? c);
(a ?? b) ?? c;

a ?? (b || c);
a || (b ?? c);
(a ?? b) || c;
(a || b) ?? c;

a ?? (b && c);
a && (b ?? c);
(a ?? b) && c;
(a && b) ?? c;

Original file line number Diff line number Diff line change
@@ -1,2 +1,12 @@
const foo = 'test';
console.log((foo ?? '') == '');
console.log((foo ?? '') == '');
a ?? b ?? c;
a ?? b ?? c;
a ?? (b || c);
a || (b ?? c);
(a ?? b) || c;
(a || b) ?? c;
a ?? (b && c);
a && (b ?? c);
(a ?? b) && c;
(a && b) ?? c;
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,14 @@ foo ||bar;
(x => x)|| bar;
(function a(x){return x;})|| 2;
0||(function(){return alpha;});
a ?? (b || c);

a && (b && c);
(a && b) && c;

a || (b || c);
(a || b) || c;

a || (b && c);
a && (b || c);
(a || b) && c;
(a && b) || c;
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,11 @@ foo || bar;
return alpha;
};

a ?? (b || c);
a && b && c;
a && b && c;
a || b || c;
a || b || c;
a || b && c;
a && (b || c);
(a || b) && c;
a && b || c;

0 comments on commit 519cdff

Please sign in to comment.