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

Support TypeScript 4.0 and logical assignment operators #550

Open
andreubotella opened this issue Aug 21, 2020 · 3 comments
Open

Support TypeScript 4.0 and logical assignment operators #550

andreubotella opened this issue Aug 21, 2020 · 3 comments

Comments

@andreubotella
Copy link

TypeScript 4.0 was released yesterday, and it incorporates a number of additions, some of which Sucrase does not support:

Labeled tuple elements

type Range = [start: number, end: number];

type Foo = [first: number, second?: string, ...rest: any[]];

function foo(x: [first: string, second: number]) {
    // ...

    // note: we didn't need to name these 'first' and 'second'
    const [a, b] = x;
    a
//  ^ = const a: string
    b
//  ^ = const b: number
}
SyntaxError: Unexpected token (1:20)

unknown on catch clause bindings

try {
  // ...
} catch (e: unknown) {
  if (typeof e === "string") {
    // We've narrowed 'e' down to the type 'string'.
    console.log(e.toUpperCase());
  }
}
SyntaxError: Unexpected token, expected ")" (3:11)

Logical assignment operators

This is, of course, not specific to TypeScript, but a TC39 proposal that became Stage 4 in July and is due for inclusion in ECMAScript 2021.

// These must not compile to the same code!
// (If obj.prop has a setter, it's only called in the logical assignment
// versions if the logical operation doesn't short-circuit.)

obj.prop = obj.prop && foo();
obj.prop &&= foo();

obj.prop = obj.prop || foo();
obj.prop ||= foo();

obj.prop = obj.prop ?? foo();
obj.prop ??= foo();

Sucrase outputs:

 function _nullishCoalesce(lhs, rhsFn) { if (lhs != null) { return lhs; } else { return rhsFn(); } }

obj.prop = obj.prop && foo();
obj.prop &&= foo();

obj.prop = obj.prop || foo();
obj.prop ||= foo();

obj.prop = _nullishCoalesce(obj.prop, () => ( foo()));
obj.prop ??= foo();

tsc and Babel compile these operators down to obj.prop && (obj.prop = foo());

@Rugvip
Copy link
Contributor

Rugvip commented Sep 9, 2020

Happy to help out with any of these if needed!

@alangpierce
Copy link
Owner

alangpierce commented Sep 21, 2020

@Rugvip thanks! Sorry, I haven't had a lot of bandwidth to work on this project, so help would definitely be appreciated! Happy to take a look at a PR and provide any guidance.

Some thoughts on what the best implementation approach is:

Labeled tuple elements and unknown on catch clause bindings

These are both purely type system extensions, so only the parser needs to change here. As long as parsing completes successfully any type-related tokens have isType set to true, they'll get properly removed.

TypeScript features tend to be implemented in Babel in a timely manner, so the best way to pull in new syntax like this is by looking through the babel-parser changes since the last pass and applying any that are relevant to Sucrase. Sucrase's src/parser directory was forked from Babel, ported to TS, and underwent some significant changes, but the code structure and naming conventions still largely line up.

The last PR where I ported changes over was #523, and you can find others in the history. It would be nice to keep the commit description in the same format as #523: a comprehensive list of commits in the babel-parser package starting from 2020-04-12, with a 🚫 or ✅ for whether they need to be applied.

To list the relevant commits to consider, you can clone the babel repo (or update to latest main on that repo) and list the commits like this:

git log --oneline --first-parent packages/babel-parser

As you can probabbly see from #523, most commits don't need any action, in part due to Sucrase having an intentionally smaller scope. Some examples of commits to skip:

  • Housekeeping specific to the Babel project (version bumps, lint fixes).
  • Any commits that add or modify error-checking, since Sucrase doesn't check errors.
  • Any commits that add or modify tests in the babel-parser test suite. Sucrase currently does not attempt to replicate Babel's test suite, though syntax edge cases are sometimes useful to add as Sucrase tests, preferably as a assertResult code-level test, or sometimes as a parser-specific test in test/tokens-test.ts.
  • Any commits that modify the syntax tree format, since Sucrase doesn't have a syntax tree.
  • Any commits that fix issues in sloppy mode (non-strict mode), since Sucrase assumes strict mode.
  • Any commits related to babel-parser's configuration, since Sucrase's parser has a much simpler configuration.
  • Refactor commits often aren't relevant, but should be considered. If the code has changed enough since the refactor, or if the refactor is in service of goals that don't apply to Sucrase, then they're fine to skip.

To properly apply a PR, I'd recommend reading the actual pull request (e.g. babel/babel#11755 ) to understand the context around the change. For the TS features, you might also want to dig into the git history of the TypeScript repo and read its implementation of the parsing. In any case, having good test coverage in Sucrase (generally with tests in test/typescript-test.ts) is probably the most important thing.

You'll also want to get rid of any of the "[skip ci]" directives in commit titles, since that will cause CI for your PR to get skipped.

If you hit issues, feel free to raise them as questions in the PR, or apply a smaller commit range to keep the PR scope smaller.

Logical assignment operators

Higher-level thought: It may be best to not implement this transform for the sake of simplicity, but ultimately it depends on how hard the transform is. This case falls under "Sucrase is hesitant to implement upcoming JS features" from the README. Just like class fields and optional chaining, logical assignment operators will eventually be widely supported natively and not needed as a Sucrase transform. My impression is that logical assignment operators aren't nearly as high-value as class fields or optional chaining, so the value may not be enough to justify the implementation/complexity cost. But if you or others feel strongly about the value, or if the implementation actually isn't so bad, it seems reasonable to include.

Assuming it makes sense to implement, a few thoughts come to mind:

  • It looks like parsing for these operators is already working fine, so it's just a matter of getting the transform right.
  • It probably makes sense to add a new ...Transformer class for all three cases at once and unconditionally add it in RootTransformer.constructor.
  • Even though a.b ||= c; could become a.b || (a.b = c);, this does not work in general since it repeats the access a.b. The code a[b()] ||= c(); is an example of a more general case to think about; it would be incorrect to call b() twice. Babel and TS implement this by extracting the sub-expression into a variable, but this requires backtracking and scope knowledge that would be hard to get right in Sucrase. I ran into this with optional chaining as well and solved by emitting helper functions that take parameters and re-use them. A rough sketch could be to transform to a function invocation like logicalOrAssignProperty(a, b(), () => c()). Note that you'll also need to ensure that the call to c() is skipped if necessary, following short circuiting rules. It might also help to read through https://github.com/alangpierce/sucrase/wiki/Sucrase-Optional-Chaining-and-Nullish-Coalescing-Technical-Plan to better understand the rationale around how it was done for optional chaining and nullish coalescing.
  • To get this all working, you'll need to know where the expression actually starts and ends. Token.rhsEndIndex is a way to get the end of the right-hand side, but it's not populated for the tokens that you need, so you'll need to add it. Detecting the start is harder because you basically need to be able to look arbitrarily far ahead to see if a ||= token is coming up that's relevant to you. Optional chaining did it via a Token.isOptionalChainStart flag to mark the start token, but I'd be hesitant to add more fields to Token.

Hope that all helps! Again, happy to take a look at a PR or answer questions.

@Rugvip
Copy link
Contributor

Rugvip commented Oct 5, 2020

#556 Now has the parser updates required for "Labeled tuple elements" and "unknown on catch clause bindings".

Regarding "Logical assignment operators" I'll dive into looking at a transform for a bit, since the syntax is not gonna be supported in widely used NodeJS versions for some time.

AnnikaCodes added a commit to smogon/pokemon-showdown that referenced this issue Nov 5, 2022
This is not supported by Sucrase and would break Node 14 attempts to build the client: alangpierce/sucrase#550
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

3 participants