Skip to content
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

Implement optional chaining and nullish coalescing #461

Closed
a-x- opened this issue Jun 5, 2019 · 12 comments
Closed

Implement optional chaining and nullish coalescing #461

a-x- opened this issue Jun 5, 2019 · 12 comments
Assignees

Comments

@a-x-
Copy link

a-x- commented Jun 5, 2019

a?.ba && a.b
Ok or fork?

@alangpierce
Copy link
Owner

alangpierce commented Jun 6, 2019

@a-x- sure, I don't have any problem with anyone forking the project and adding any features they want to their own fork.

In this case, I wouldn't accept this transformation as a PR because it's not correct in all cases, and more generally I think a correct optional chaining implementation is likely difficult enough that I'd prefer to call it out of scope for Sucrase. But if there's an implementation that's correct and isn't too complicated, I'd be happy to consider it.

Babel compiles it as:

var _a;
(_a = a) === null || _a === void 0 ? void 0 : _a.b;

which also requires finding a good place to declare _a.

Some issues with the a?.ba && a.b transformation you have:

  • If a is a function call, like a()?.b, then it will be called twice when it's only supposed to be called once.
  • The expression a && a.b uses a truthy/falsy check rather than a null/undefined check, so the evaluation behavior will be different if a is 0, false, NaN, or ''.
  • The expression a && a.b returns a if the check fails, but a?.b (I believe) returns undefined if the check fails.

@a-x-
Copy link
Author

a-x- commented Jun 6, 2019

Thank you!

but a?.b (I believe) returns undefined if the check fails.

I think it should return null or undefined according to a's value.

@alangpierce
Copy link
Owner

Hmm, the README from https://github.com/tc39/proposal-optional-chaining says it would be undefined if a is null:

a?.b                          // undefined if `a` is null/undefined, `a.b` otherwise.
a == null ? undefined : a.b

But I know it has been a topic of discussion and they've gone back and forth on the exact behavior there.

@villesau
Copy link

villesau commented Jul 2, 2019

@alangpierce
Copy link
Owner

alangpierce commented Aug 26, 2019

Optional Chaining is now Stage 3, and currently my hope is that Chrome/Node will get optional chaining soon enough that it's ok to skip in Sucrase. It's being tracked here and looks like it just got an initial implementation:

https://www.chromestatus.com/feature/5748330720133120
https://bugs.chromium.org/p/v8/issues/detail?id=9553

It definitely seems like the most significant recent JS feature, so it might be high priority enough to go into Sucrase even if the implementation is really complex, but for now I'd prefer to wait and see, so closing this out for now.

@ansballard
Copy link

Figured it's worth bumping this now that typescript has released optional chaining (and nullish coalescing, but that isn't in any browsers yet) in 3.7. Curious to know what might push this one over the edge. For me keeping parity with stable typescript features would be ideal since I'm running both over the same code, but I don't know how common that use case is. Thanks for the hard work!

Also, what would be the first step implementing, just to pull in the way babel does it and tweak from there?

@alangpierce
Copy link
Owner

Yep, agreed! I've been thinking about this more over the last few days since the TS release, and I'll reopen this issue and repurpose it to built-in support in Sucrase. I'll add some more detail when I get a chance.

Chrome Canary now has optional chaining available by default, and in Chrome Beta it's available behind the "Experimental JavaScript" flag, so it's already reasonable to use it in development with Sucrase (which will emit it unchanged) for running code in the browser. My team runs tests in Node, though, and it looks like Node 14 (to be released next year) will be the first version with support. We also lag behind the latest Node, so it will be a while before we have native support for the syntax in tests. It seems like a high-enough value syntax that it can be an exception to the "Sucrase is hesitant to implement upcoming JS features" rule.

Sucrase transforms require a lot more thought and care than Babel transforms since Sucrase makes the parser as minimal as possible and transforms code in a left-to-right scan, so it's tricky, but I think there will be a way to make it work.

@alangpierce alangpierce reopened this Nov 11, 2019
@alangpierce alangpierce changed the title I want to add optional chaining for js sources Implement optional chaining and nullish coalescing Nov 11, 2019
@alangpierce alangpierce self-assigned this Nov 11, 2019
@alangpierce
Copy link
Owner

Ooh, looks like optional chaining is actually available in node 13 behind a flag and there's a backport that will make it available in node 12 behind a flag: nodejs/node#30109 . Still seems useful to have in Sucrase, but hopefully runtime support will come relatively soon.

@Whobeu
Copy link

Whobeu commented Nov 12, 2019

I have tested Optional Chaining and Nullish Coalescing in Node 13 using the --harmony-optional-chaining and --harmony-nullish flags respectively. No issues with either. However, I tested out this function from https://node.green validation for Nullish Coalescing with Node 13 and it returns false:

function nullishCoalescingOperator() {
  return (null ?? 42 === 42) &&
    (undefined ?? 42 === 42) &&
    (false ?? 42 === false) &&
    ('' ?? 42 === '') &&
    (0 ?? 42 === 0);
}

Putting parentheses around the last three tests makes it work correctly:

function nullishCoalescingOperator() {
  return (null ?? 42 === 42) &&
    (undefined ?? 42 === 42) &&
    ((false ?? 42) === false) &&
    (('' ?? 42) === '') &&
    ((0 ?? 42) === 0);
}

I did not look at the Nullish Coalescing spec to see how the implementation differs in terms of parentheses requirements from when this test had been created.

@alangpierce
Copy link
Owner

I worked through the details and different options by writing up a technical plan at https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan . I'll get started on the implementation soon.

alangpierce added a commit that referenced this issue Dec 27, 2019
Progress toward #461

Tech plan at https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

Some details:
* In the parser, operator parsing now keeps track of the start token index so
  that when we observe a `??` operator, we can mark the first token as being the
  start of an optional chain.
* The token format now has fields for the number of optional chain starts and
  ends for each token, which the transformer will use to inject code.
* Optional chaining code needs to use the HelperManager system, so I refactored
  it to be on the RootTransformer instead of just on one of them.
* The actual start/end code injection is done by the token system so that it
  works alongside all existing transforms.
* Since TokenProcessor now needs a HelperManager, I had to break the dependency
  cycle by making NameManager no longer depend on TokenProcessor, since it was
  only barely using it anyway.

Some quick perf benchmarks show this as having potentially a 4% slowdown even
for code not using nullish coalescing, though I wasn't able to track down any
specific area of code responsible, and it may have been partially due to
variability or other factors. Except for more egregious cases, I'll leave
performance work as a follow-up.
alangpierce added a commit that referenced this issue Dec 27, 2019
Progress toward #461

Tech plan at https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

Some details:
* In the parser, operator parsing now keeps track of the start token index so
  that when we observe a `??` operator, we can mark the first token as being the
  start of an optional chain.
* The token format now has fields for the number of optional chain starts and
  ends for each token, which the transformer will use to inject code.
* Optional chaining code needs to use the HelperManager system, so I refactored
  it to be on the RootTransformer instead of just on one of them.
* The actual start/end code injection is done by the token system so that it
  works alongside all existing transforms.
* Since TokenProcessor now needs a HelperManager, I had to break the dependency
  cycle by making NameManager no longer depend on TokenProcessor, since it was
  only barely using it anyway.

Some quick perf benchmarks show this as having potentially a 4% slowdown even
for code not using nullish coalescing, though I wasn't able to track down any
specific area of code responsible, and it may have been partially due to
variability or other factors. Except for more egregious cases, I'll leave
performance work as a follow-up.
alangpierce added a commit that referenced this issue Dec 28, 2019
Progress toward #461

Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

The parser now associates each access operation with the start token for the
chain, and marks each chain containing an optional chain operation so that we
can wrap it in a function call.

This does not yet handle optional deletion, and there are a number of follow-up
tests that would be good to write, but the feature seems to be working with this
implementation.
alangpierce added a commit that referenced this issue Dec 29, 2019
Progress toward #461

Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

This ended up being a relatively small tweak to the optional chaining
implementation:
* There's a new helper that wraps the optionalChain helper, and I needed to add
  a way for helpers to depend on each other.
* In most cases, we condition on the delete and non-delete cases based on
  whether there's a delete token just before the start of the optional chain.
* To determine whether a subscript is the last of its chain (and therefore needs
  a delete operation), we can walk the tokens forward tracking depth until we
  get to either another subscript or the end of the chain. This means there was
  no need for any changes to the parser. Since it only walks between two
adjacent subscripts, it takes linear extra time unless there's nesting, and only
ever does the more expensive operation when processing an optional delete.
alangpierce added a commit that referenced this issue Dec 29, 2019
Progress toward #461

Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

This ended up being a relatively small tweak to the optional chaining
implementation:
* There's a new helper that wraps the optionalChain helper, and I needed to add
  a way for helpers to depend on each other.
* In most cases, we condition on the delete and non-delete cases based on
  whether there's a delete token just before the start of the optional chain.
* To determine whether a subscript is the last of its chain (and therefore needs
  a delete operation), we can walk the tokens forward tracking depth until we
  get to either another subscript or the end of the chain. This means there was
  no need for any changes to the parser. Since it only walks between two
adjacent subscripts, it takes linear extra time unless there's nesting, and only
ever does the more expensive operation when processing an optional delete.
alangpierce added a commit that referenced this issue Dec 29, 2019
Progress toward #461

Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

This ended up being a relatively small tweak to the optional chaining
implementation:
* There's a new helper that wraps the optionalChain helper, and I needed to add
  a way for helpers to depend on each other.
* In most cases, we condition on the delete and non-delete cases based on
  whether there's a delete token just before the start of the optional chain.
* To determine whether a subscript is the last of its chain (and therefore needs
  a delete operation), we can walk the tokens forward tracking depth until we
  get to either another subscript or the end of the chain. This means there was
  no need for any changes to the parser. Since it only walks between two
adjacent subscripts, it takes linear extra time unless there's nesting, and only
ever does the more expensive operation when processing an optional delete.
alangpierce added a commit that referenced this issue Dec 29, 2019
Progress toward #461

Tech plan: https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

This ended up being a relatively small tweak to the optional chaining
implementation:
* There's a new helper that wraps the optionalChain helper, and I needed to add
  a way for helpers to depend on each other.
* In most cases, we condition on the delete and non-delete cases based on
  whether there's a delete token just before the start of the optional chain.
* To determine whether a subscript is the last of its chain (and therefore needs
  a delete operation), we can walk the tokens forward tracking depth until we
  get to either another subscript or the end of the chain. This means there was
  no need for any changes to the parser. Since it only walks between two
adjacent subscripts, it takes linear extra time unless there's nesting, and only
ever does the more expensive operation when processing an optional delete.
alangpierce added a commit that referenced this issue Dec 29, 2019
Progress toward #461

This doesn't update it to latest, but the current revision had a number of
usages that can be handled by Sucrase now.
alangpierce added a commit that referenced this issue Dec 29, 2019
Progress toward #461

This doesn't update it to latest, but the current revision had a number of
usages that can be handled by Sucrase now.
alangpierce added a commit that referenced this issue Dec 30, 2019
Progress toward #461

For now, we only run two subdirectories and ignore various tests (sloppy mode,
tests expecting syntax error, and TCO tests). This reveals a few issues with the
current implementation, which will be addressed later. Since it's not a passing
build yet, I'll leave it out of CI for now.

Also drop the Travis build for Node 8 since one of the new dependencies requires
Node 10. I won't officially drop Node 8 support just yet, but will soon, and
it's at end-of-life now.
alangpierce added a commit that referenced this issue Dec 31, 2019
Progress toward #461

The previous implemented worked for normal JS syntax, but `f?.<T>()` needs a
special case when working with tokens. Also, regular type argument syntax wasn't
marking the open-paren correctly, so this fixes that as well.
alangpierce added a commit that referenced this issue Dec 31, 2019
Progress toward #461

The previous implemented worked for normal JS syntax, but `f?.<T>()` needs a
special case when working with tokens. Also, regular type argument syntax wasn't
marking the open-paren correctly, so this fixes that as well.
alangpierce added a commit that referenced this issue Jan 1, 2020
Progress toward #461

Tech plan:
https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

We now scan for an await keyword to indicate that the nullish coalescing or
optional chaining operation is an async one, and in that case, call a different
helper and emit async arrow functions instead of regular ones.

This needed a few extra pieces of information in the token structure:
* We now keep track of the scope depth to distinguish awaits inside inner async
  functions.
* We use a token field to mark that the start token is an async operation.
* We now track the start token for each nullish coalescing operation so that we
  can reference it to determine whether to emit an async arrow function.

The extra token state and the duplicated helpers are all a bit ugly, but
hopefully there won't be much more complexity beyond this, and I may investigate
ways to store the token state in a more concise way. And, of course, this
transform will go away eventually when optional chaining is available in node.
alangpierce added a commit that referenced this issue Jan 1, 2020
…497)

Progress toward #461

Tech plan:
https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

We now scan for an await keyword to indicate that the nullish coalescing or
optional chaining operation is an async one, and in that case, call a different
helper and emit async arrow functions instead of regular ones.

This needed a few extra pieces of information in the token structure:
* We now keep track of the scope depth to distinguish awaits inside inner async
  functions.
* We use a token field to mark that the start token is an async operation.
* We now track the start token for each nullish coalescing operation so that we
  can reference it to determine whether to emit an async arrow function.

The extra token state and the duplicated helpers are all a bit ugly, but
hopefully there won't be much more complexity beyond this, and I may investigate
ways to store the token state in a more concise way. And, of course, this
transform will go away eventually when optional chaining is available in node.
alangpierce added a commit that referenced this issue Jan 2, 2020
Progress toward #461

Tech plan:
https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

To allow super, we mostly just need to skip the code transform on the first
subscript. We also need to add a `.bind(this)` when transforming the second
subscript, which can be determined through a relatively straightforward token
scan.
alangpierce added a commit that referenced this issue Jan 2, 2020
Progress toward #461

Tech plan:
https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan

To allow super, we mostly just need to skip the code transform on the first
subscript. We also need to add a `.bind(this)` when transforming the second
subscript, which can be determined through a relatively straightforward token
scan.
alangpierce added a commit that referenced this issue Jan 2, 2020
alangpierce added a commit that referenced this issue Jan 2, 2020
alangpierce added a commit that referenced this issue Jan 2, 2020
@alangpierce
Copy link
Owner

I just published optional chaining and nullish coalescing support as 3.12.0, so closing this!

@ansballard
Copy link

@alangpierce just want to thank you for knocking this out so quick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants