Skip to content
This repository has been archived by the owner on Jan 28, 2023. It is now read-only.

Add grammar flags to disallow mixing ?? with || and &&. #39

Merged
merged 4 commits into from
Jul 13, 2019

Conversation

DanielRosenwasser
Copy link
Member

Alternative to #38. Instead of using early errors, this change disallows combining the nullish coalescing operator (??) with both the logical AND (&&) and logical OR (||) operators directly through the grammar.

@DanielRosenwasser
Copy link
Member Author

@DanielRosenwasser
Copy link
Member Author

The current changes were adapted from https://gist.github.com/bakkot/332ddeb26978c41613abc092120448b9, but I think there might have been a mistake in that grammar. Should it be the following?

 NullishExpression[Nullish]:
   LogicalORExpression[?Nullish]
-  LogicalORExpression[+Nullish] ?? NullishExpression[+Nullish]
+  NullishExpression[+Nullish] ?? LogicalORExpression[+Nullish]

@rkirsling
Copy link
Member

rkirsling commented Jul 12, 2019

(Note: This is spill-over from #38. Sorry in advance for the full-throttle teacher mode. 😅)


I believe this change is motivated by a view of the differing precedence of || and && as arbitrary, but I want to encourage us to remember why this isn't so (at least, not any more than PEMDAS itself is 😄).

|| is an additive operation while && is a multiplicative operation—they act on true and false in the same way as + and * act on 0 and 1 (as long as we clamp anything greater than 1 to be just 1).

So we've got stuff like:

  • false && false || true = true just as 0 * 0 + 1 = 1
  • false || true && true = true just as 0 + 1 * 1 = 1

But more importantly, we have short-circuiting:

  • false && x = false just as 0 * x = 0
  • false || x = x just as 0 + x = x

And thankfully, that fact doesn't become less useful when we're dealing with truthiness—that's why folks have been rightfully able to write stuff like the following up 'til now:

const isValid =
  options.alreadyValidated ||
  hasValidFoo() && hasValidBar() ||
  GLOBAL_OVERRIDE;

I understand that people make mistakes and that bugs can be costly. It's important that we have lint rules and even compiler warnings for such things. But I don't think a && b || c is more deserving to be a syntax error than a * b + c, and I think this same rationale applies to a && b ?? c.

@ljharb
Copy link
Member

ljharb commented Jul 12, 2019

There is a significant difference in that nearly everyone is taught some form of PEMDAS in school, but many people have never seen && or || prior to programming - it’s why the airbnb style guide requires parens when mixing && and ||, but not when mixing * + / -.

@claudepache
Copy link

claudepache commented Jul 12, 2019

Note that, if we do want to disallow mixing ?? and other operators without the use of parentheses, we could very well include everything between (in term of precedence level) comparison operators (<=, ==, etc.) and logical operators (&&, ||) , since some languages has found more convenient to give ?? a precedence level higher than comparisons (per #26 (comment)).

Anyway, introducing such rules at the language level is not common. (Not that I am against that.)

PR #40 (giving ?? a precedence level just below ||) solves the problem raised in #26 in a more traditional way, while respecting what I believe to be the original intent of giving ?? approximately the same precedence level as ||, namely: Code patterns using a || b, where a is guaranteed to never be falsy-but-not-nullish, could be safely upgraded to a ?? b, that is to say, without being trapped by the differing precedence level of other surrounding operators.

@bakkot
Copy link

bakkot commented Jul 12, 2019

@rkirsling beyond what @ljharb says about || not being common in elementary school, it's also the case that

|| is an additive operation while && is a multiplicative operation

is a fact not frequently not taught to or understood by even to people who have university degrees in computer science (who themselves not that large a fraction of JS programmers).

@rkirsling
Copy link
Member

rkirsling commented Jul 12, 2019

@bakkot That is fair, and I'm not in the least suggesting that people "should know better" or something (though I do get excited to share it with others 'cause I think it's really nifty 😂); I only mean to argue that syntax errors are not something to impose lightly.

(Now, if we wanted to introduce syntax warnings into the spec... 😉)

@ljharb I actually don't take your comment as a point of disagreement—I've been an enthusiastic advocate of the Airbnb style guide at my workplace for years. 😄

spec.html Show resolved Hide resolved
@DanielRosenwasser
Copy link
Member Author

I've included updates to uses or LogicalORExpression and ConditionalExpression, and included NullishExpression in the appropriate places. The spec diff isn't as tiny as it was before, but by-and-large it's not too bad when rendered.

@DanielRosenwasser
Copy link
Member Author

Hey all, I've removed the redundant clauses now that spec.html reflects additions/removals from the actual spec text. Please give it a final look, as I'll be merging the changes in later tonight.

@DanielRosenwasser DanielRosenwasser merged commit 99ff664 into tc39:master Jul 13, 2019
@DanielRosenwasser DanielRosenwasser deleted the nullish-grammar-flag branch July 13, 2019 05:50
facebook-github-bot pushed a commit to facebook/flow that referenced this pull request Aug 6, 2019
Summary:
updates parsing of the nullish coalescing operator (`??`) based on the updated stage 3 draft (tc39/ecma262#1644). this specific change happened in tc39/proposal-nullish-coalescing#39.

Changelog: fix(parser): Updated parsing of the experimental nullish coalescing `??` operator. It now has a lower precedence than `||` and `&&`, and parentheses are required to nest it with them.

Reviewed By: gabelevi

Differential Revision: D16561382

fbshipit-source-id: 099ef58bc6cfa90ff3a3153636363986aab2f158
@hax
Copy link
Member

hax commented Nov 1, 2019

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