[ts] Allow keywords in tuple labels

[Harpica]

Q	A
Fixed Issues?	Fixes #13702, Closes #13743
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link	No
Any Dependency Changes?	No
License	MIT

Fixes Typescript compiler failure while the name (label) in tuple is a keyword:

• add keyword check before parsing first token
• if it is keyworkd - treat it as Identifier
• else - keep the logic

Can't fix another problem mentioned in the issue:

type FuncWithDescription = [
  function: (...args: any[]) => any, // fixed
  string: string // not fixed, throws error 
]

It seems to be called by this line of code (1116 line)

 this.raise(TSErrors.InvalidTupleMemberLabel, { at: type });

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/54112/

[JLHwung]

Thanks for the PR!

• For third-party parser test failure, please update the allowlist:

make bootstrap-typescript && make test-typescript-update-allowlist

• For Babel 8 breaking changes test:

Please duplicate the test to tuple-keyword-labeled-babel-7, set "BABEL_8_BREAKING": "false" in options.json of the duplicated Babel 7 test.
Update the current test options with "BABEL_8_BREAKING": true.
Re-run OVERWRITE=1 yarn jest and BABEL_8_BREAKING=1 OVERWRITE=1 yarn jest.

[liuxingbaoyu]

For third-party parser test failure, please update the allowlist:

Does the failure of third-party parser tests seem unexpected?

[Harpica]

@JLHwung, thank you for your advice!

I fixed the second issue (on Babel 8), but I can't perform these 2 commands for 3d parties issue:

(My OS is Windows)

[liuxingbaoyu]

This command does not support windows yet, do you have wsl?
If not, try
git clone --single-branch --shallow-since=2022-04-01 https://github.com/microsoft/TypeScript.git ./build/typescript
then
git checkout -q ce85d647ef88183c019588bcf398320ce29b625a

[Harpica]

@liuxingbaoyu Thank you! WSL helped c:
Waiting for review

[nicolo-ribaudo]

Thank you!

I just realized I had the main diff hidden when reviewing 😅

[nicolo-ribaudo]

There is a possible alternative solution that would also fix the string: case.

Instead of checking if tokenIsKeyword, we can check if we have a "word" followed by a colon:

const isLabeled = tokenIsKeywordOrIdentifier(this.state.type) && this.lookaheadCharCode() === charCodes.colon;

and then if isLabeled is true we parse the label:

let label;
if (isLabeled) {
  label = this.parseIdentifier(true);
  this.expect(tt.colon);
}

const type = this.tsParseType();

[Harpica]

@nicolo-ribaudo Hello!
Thank you for suggestion, currenty I'm trying to implement it and it seems that it doesn't help with the case with question mark (labeled optional tuple): [identifier?: type]
Is there a way to look for more than one token ahead? Also, why does string: string fail check for

 if (
      type.type === "TSTypeReference" &&
      !type.typeParameters &&
      type.typeName.type === "Identifier"
    ) 

If this is the way to find types that could be converted to identifiers

[nicolo-ribaudo]

The reason is that it's parsed as a TSStringKeyword, by the keywordTypeFromName function.

Maybe this would work, trying to parse the identifier both as a type and as a label at the same time if it's followed by ?:

let label;
let optional;
let type;

const isWord = tokenIsKeywordOrIdentifier(this.state.type);
if (isWord && this.lookaheadCharCode() === charCodes.colon) {
  label = this.parseIdentifier(true);
  this.expect(tt.colon);
  type = this.tsParseType();
} else if (isWord && this.lookaheadCharCode() === charCodes.questionMark) {
  const startLoc = this.state.loc;
  const wordName = this.state.value;

  optional = true;

  const typeOrLabel = this.tsParseNonArrayType();

  if (this.lookaheadCharCode() === charCodes.colon) {
    label = this.createIdentifier(this.startNodeAt(startLoc), wordName);
    this.expect(tt.questionMark);
    this.expect(tt.colon);
    type = this.tsParseType();
  } else {
    type = typeOrLabel;
    this.expect(tt.questionMark);
  }
} else {
  type = this.tsParseType();
}

Or if you prefer, you could update the current type.type === "TSTypeReference" check to compare it with all the possible types introduced by the keywordTypeFromName function.

[nicolo-ribaudo]

When can this be followed by a colon?

[Harpica]

In case we have invalid label in tuple followed by colon, which fails tokenIsKeywordOrIdentifier check?

I'm sorry to bother you, this issue seemed easier than it really is for me

[Harpica]

I was thinking about the case described in "tuple-invalid-label-1" test: type T = [x.y: A]; which should throw error TSErrors.InvalidTupleMemberLabel to be passed

[nicolo-ribaudo]

Ok 👍 Maybe add a comment in the code with that example, explaining that it can only happen with invalid input.
Also, probably we can move the TSErrors.InvalidTupleMemberLabel inside this block so that it's throw in the same place as where we parse the invalid code.

[liuxingbaoyu]

Friendly ping. :)
Don't worry if you run out of time! You can modify the branch to be non-main (so that I can push this PR).

[Harpica]

@liuxingbaoyu
I've fetched fresh commits and seems like tests are passed :)
I'm sorry, but what do you mean by modifying branch? I can change base branch in PR (babel:main), but I cannot change my branch (without closing PR and opening new, which doesn't seem to be good)

[liuxingbaoyu]

Ah, sorry! I have not tried this. For forked/main I can't make edits to help finish if you don't have time anymore. It looks like you are continuing please ignore it. :)

[Harpica]

@nicolo-ribaudo Hello!
Tests seem to be passed now, could you please review pr once again? :)

[JLHwung]

For future PRs: we should recover from this case:

type t = [label: string?]

which may happen when users are migrating labelless tuple to a labeled one.

Currently TS throws "A labeled tuple element is declared as optional with a question mark after the name and before the colon, rather than after the type." and Babel throws unrecoverable errors.

[JLHwung]

Can we cache the next char code when isWord is true? So we don't have to look ahead twice.

[JLHwung]

Can you augment the test case to include optional keyword labeled tuple? such as void?: string