Repository: prettier/prettier (JavaScript)

Issue:
Parentheses around nullish coalescing operator within conditional expression
**Prettier 1.13.5**
[Playground link](https://prettier.io/playground/#N4Igxg9gdgLgprEAuc0DOMAEA3AhgGwFc5MBeTSKAEwEsYbpMB+TACikP31wCN84AagWLMWAMwL4euMAGshROAEpMSTFGgA5Ttz6DhcANwgANCAgAHeumShcAJ3sQA7gAUHCNMhC5sEGlSmIDz2MrJwMADKFjI0UADmyDD2xGZxaHD2MK6h8QC2uMgS+BlmAFZoAB4AQqFyEZG4eXAAMnFwRQSlIDH2Gfbe0jwAnvjQQRb2cTAA6gEwABbIABwADGaTEBkzoRbek3D92B1m9nAAjoQ0Zzm4+YVIxd0ZeTRJKXBmaHHx-ACKhAg8E6JU+IBgvDmVEWyAATGZkrgaPgfgBhCB5AreDRQE4gQgZAAqvC8jy6cAAvhSgA)
```sh
--parser babylon
```

**Input:**
```jsx
const value = condition ? (nullableValue ?? fallbackValue) : nonNullableValue;
```

**Output:**
```jsx
const value = condition ? nullableValue ?? fallbackValue : nonNullableValue;

```

**Expected behavior:**

I think it would be better if Prettier preserved the parentheses around the arguments of the nullish coalescing operator if nested within a conditional expression:

```jsx
const value = condition ? (nullableValue ?? fallbackValue) : nonNullableValue;
```

My concern is that the precedence of `??` isn't immediately obvious, and I think that parentheses would improve readability here.


Hints:
We don't do this for other operators, like `||`. I'm not sure it makes sense to do this for `??`.
I've considered this argument as well, but I come to a different conclusion: I do think it's valuable to make the precedence clearer by adding explicit parentheses here — clarity trumps brevity.
IMO this case is a little different because the operator contains a `?`, which could be confused with a nested ternary, which parens would clarify. However, we also add parens for actual ternaries, so my point may be moot:

**Prettier 1.13.7**
[Playground link](https://prettier.io/playground/#N4Igxg9gdgLgprEAuc0DOMAEA3AhgGwFc5MBeTSKAEwEsYbpMB+TKZ1zJTAMwPwCNcYANYA1AsU6toAOUL58ufvjjiicANwgANCAgAHeumShcAJzMQA7gAVzCNMhC5sEGlR0h+ZocLgwAZX0hGigAc2QYM2JdULQ4MxgbHzCAW1xkXnx43QArNAAPACEfEX8A3FS4ABlQuEyCHJBgs3izJ0F+AE98aE99M1CYAHV3GAALZAAOAAZdAYh44Z99JwG4Nux63TM4AEdCGl3k3DSMpCym+NSaSOi4XTRQsJUARUIIeAbsh5AYJVGVAmyAATLoorgaPhngBhCCpdJOKDQbYgQjxAAqSkcF0acAAvvigA)
```sh
--parser babylon
```

**Input:**
```jsx
const value = condition ? n ? n : fallbackValue : nonNullableValue;
```

**Output:**
```jsx
const value = condition ? (n ? n : fallbackValue) : nonNullableValue;

```
@vjeux @cpojer I know facebook started using the nullish coalescing operator recently, do you have any insight into people's reactions with reading it?
It’s likely a very niche use case, so whichever decision we make isn’t going to impact a lot of code.

I’m not against adding parenthesis there personally.
To expand on my comment. This is only an issue if the entire line fits in 80 column. Otherwise it would break at the beginning of the `?` and the indentation would make it obvious what precedence is.

It’s unlikely that you have an assignment, condition, fallback and else value that all fits.
I can see there's some visual effort needed to separate the alternatives of a ternary but I don't think the specific operator makes much difference:

```js
const value = condition ? nullableValue ?? fallbackValue : nonNullableValue;
```

```js
const value = condition ? nullableValue || fallbackValue : nonNullableValue;
```

```js
const value = condition ? nullableValue % fallbackValue : nonNullableValue;
```

IMO the debate should be wether or not to use parenthesis in binary expressions inside ternaries rather than making an exception for this one. This change was [already done for some mathematical expressions](https://prettier.io/blog/2018/05/27/1.13.0.html#insert-more-parentheses-in-math-expressions-4413-and-4407-by-duailibe), so it might make sense to revisit this case too.
I agree, it's not so much about the specific operator. IMO it would be a good idea to add/preserve parentheses around binary expressions within conditional expressions. 👍
I'm fine adding them as long as we're consistent across binary expressions
I helped debug some code today where the dev had written something like this:
```
const value = valueOrNull ?? condition ? value1 : value2;
```
javascript sees this as
```
const value = (valueOrNull ?? condition) ? value1 : value2;
```
but the dev thought it would mean this:
```
const value = valueOrNull ?? (condition ? value1 : value2);
```

If prettier would insert the paranthesis there, then the dev could have seen his error earlier. (with format on save)
A somewhat similar problem came up

```typescript
if (maybeValue ?? defaultValue > 2)
```

this was assumed to work like 
```typescript
if ((maybeValue ?? defaultValue) > 2)
```
but it actually works like
```typescript
if (maybeValue ?? (defaultValue > 2))
```

My suggestion for code style here would be to always use parens with nullish coalescing since there is no consistent instinct around how tightly it binds, but prettier will remove the parens in the latter case when that behavior is desired
I can't tell if this issue is supported or not. I've been burned many times by the precedence problem, and would _love_ if prettier added explicit parentheses to help indicate the problem for me.
I also hit this recently, with a `??` operator in the second branch of a ternary. Would love to see Prettier add parens here!

**Input:**
```
const alwaysConfusing  = someCondition ? val1 : val2 ?? val3;
const goodBeforePrettier = someCondition ? val1 : (val2 ?? val3);
const alwaysFine = (someCondition ? val1 : val2) ?? val3;
```

**Output:**
```
const alwaysConfusing = someCondition ? val1 : val2 ?? val3
const goodBeforePrettier = someCondition ? val1 : val2 ?? val3
const alwaysFine = (someCondition ? val1 : val2) ?? val3
```
This issue is nearly 5 years old, but still relevant, and it's actually a bug… Let me know if I should open a separate issue.

Parentheses around nullish coalescing operators aren't merely cosmetic or for readability alone.

In this example, the parentheses help TypeScript with narrowing the type of the variable that's being assigned:

```ts
export function myFunc(
  config: {
    throughDate?: string
  } & (
    | { baseDate: string; trackedStartDate?: never }
    | { baseDate?: never; trackedStartDate: string }
    | { baseDate: string; trackedStartDate: string }
  ),
) {
  const { baseDate, throughDate, trackedStartDate } = config

  const wrong = throughDate ?? baseDate ?? trackedStartDate // --> string | undefined
  const right = throughDate ?? (baseDate ?? trackedStartDate) // --> string
}
```
[Try it on TS Playground](https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBAMwK4DsDGMCWEVwLYCeAYqmgBQCwAUHHGjgpgOYBccA3tbbTABZQQkTXgBEAhjGAB+NgGcYUTCiZc4AXzgAyOJRrcAPhzgAjMbODjJchUqYBuOArFoA1sAAmAZRhjYl6WwowABuwFDqqrSG7CZmFhIBcEGhUA5Orh7evjD+1orKEXpRRqbmuXDy+faOUM5uXj5+CXm2hbQAlAA01O0cqvQo8iVx-p2O-ILCozV1mY05CepwALx0DMzU-ThDAO4CBat8AkKii1JSsWVnF+n1WU2ScAD0T3AAtG8AfBU2BYao7mAjCC7i2g3gimE8EOExO-jg5x0pXij0Rtzm2X8vRe7y+Pyq1DUmyoQA)

But, prettier is removing the parentheses, which then breaks the code.

I'd say parentheses should be left alone unless they're extraneous, like unnecessary double parentheses:

This is ok:

```diff
- const right = throughDate ?? ((baseDate ?? trackedStartDate))
+ const right = throughDate ?? (baseDate ?? trackedStartDate)
```

This is NOT ok:

```diff
- const right = throughDate ?? (baseDate ?? trackedStartDate)
+ const right = throughDate ?? baseDate ?? trackedStartDate
```
That feels like a TypeScript bug, @Merott — I confirmed that Prettier does seem to intentionally change the AST here (as it does with every other logical expression, including `&&` and `||`). If you swap the types in your function from `string` (which can be falsy) to `"a"` (which will always be truthy), and then swap the operator from `??` to `||`, [you’ll find that both `wrong` and `right` are typed as `"a"`](https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBAMwK4DsDGMCWEVwLYCeAYqmgBQCwAUHHGjgpgOYBccA3tbbTABZQQkTXgBEAhjGAB+NgCIxsrnAC+cAGRxKNbgB8OcAEZiAzsHGS5CgNxwYUMWgDWwACYBlGGNjnpbFMAA3YCgVJVo9dkMTMwlfOH8gqBs7B2d3T29Yy1lQ7XD9I1MfbOT7J1cPLxhiuHkc5SUASgAaakaOJXoUY3hIwpjJZtt+QWEfIZTy9KqfFTgAXjoGZmpOnB64AHcBFCYF4YEhUVi4HT1+2bPbMrTKzMk4AHpHuABaV4A+OB6oTF217rwX7CeCLPiHMYnK5kC5QvSTW4ZaqxdrPN6fb52P5MagNKjUIA). Additionally, I tried out all the possible valid parameters to that function that I could think of and neither `wrong` nor `right` ever returned `undefined` with your original code, so I’m fairly confident this is a deficiency in the type checker. Logically, there’s no behavior difference between `(a ?? b) ?? c` (which is what you get if you don’t use parens) and `a ?? (b ?? c)`.

Patch:
diff --git a/src/language-js/needs-parens.js b/src/language-js/needs-parens.js
--- a/src/language-js/needs-parens.js
+++ b/src/language-js/needs-parens.js
@@ -11,6 +11,7 @@ import {
   isBitwiseOperator,
   isCallExpression,
   isMemberExpression,
+  isNullishCoalescing,
   isObjectOrRecordExpression,
   isObjectProperty,
   shouldFlatten,
@@ -366,7 +367,7 @@ function needsParens(path, options) {
           return !isBinaryCastExpression(node);
 
         case "ConditionalExpression":
-          return isBinaryCastExpression(node);
+          return isBinaryCastExpression(node) || isNullishCoalescing(node);
 
         case "CallExpression":
         case "NewExpression":
diff --git a/src/language-js/utils/index.js b/src/language-js/utils/index.js
--- a/src/language-js/utils/index.js
+++ b/src/language-js/utils/index.js
@@ -140,6 +140,14 @@ const isObjectOrRecordExpression = createTypeCheckFunction([
   "RecordExpression",
 ]);
 
+/**
+ * @param {Node} node
+ * @returns {boolean}
+ */
+function isNullishCoalescing(node) {
+  return node.type === "LogicalExpression" && node.operator === "??";
+}
+
 /**
  * @param {Node} node
  * @returns {boolean}
@@ -1143,6 +1151,7 @@ export {
   isMemberish,
   isMethod,
   isNextLineEmpty,
+  isNullishCoalescing,
   isNumericLiteral,
   isObjectOrRecordExpression,
   isObjectProperty,


Test patch:
diff --git a/tests/format/js/nullish-coalescing/__snapshots__/format.test.js.snap b/tests/format/js/nullish-coalescing/__snapshots__/format.test.js.snap
--- a/tests/format/js/nullish-coalescing/__snapshots__/format.test.js.snap
+++ b/tests/format/js/nullish-coalescing/__snapshots__/format.test.js.snap
@@ -29,12 +29,16 @@ foo || (baz ?? baz);
 (foo && baz) ?? baz;
 foo && (baz ?? baz);
 
+foo ?? bar ? a : b;
+a ? foo ?? bar : b;
+a ? b : foo ?? bar;
+
 =====================================output=====================================
 obj.foo ?? "default";
 
 const x = (foo, bar = foo ?? bar) => {};
 
-foo ? bar ?? foo : baz;
+foo ? (bar ?? foo) : baz;
 
 foo ?? bar ?? baz;
 foo ?? bar ?? baz;
@@ -53,5 +57,9 @@ foo || (baz ?? baz);
 (foo && baz) ?? baz;
 foo && (baz ?? baz);
 
+(foo ?? bar) ? a : b;
+a ? (foo ?? bar) : b;
+a ? b : (foo ?? bar);
+
 ================================================================================
 `;
diff --git a/tests/format/js/nullish-coalescing/nullish_coalesing_operator.js b/tests/format/js/nullish-coalescing/nullish_coalesing_operator.js
--- a/tests/format/js/nullish-coalescing/nullish_coalesing_operator.js
+++ b/tests/format/js/nullish-coalescing/nullish_coalesing_operator.js
@@ -20,3 +20,7 @@ foo || (baz ?? baz);
 
 (foo && baz) ?? baz;
 foo && (baz ?? baz);
+
+foo ?? bar ? a : b;
+a ? foo ?? bar : b;
+a ? b : foo ?? bar;