TSMappedType - TSTypeParameter.name issue

[JamesHenry]

Considering the following source:

type C = {
    [/* commentC */ c in C]: string
}

Prior to v8 we didn't have any test coverage for mapped types.

@armano2 added good test coverage, and at the same time explicit conversion logic (rather than using the deeplyCopy() fallback which had implicitly been used up until that point) in the PR which would become v8.

Because of not having the test coverage in there prior to the code change, we didn't explicitly point out a change which is probably worthy of discussion:

The lowercase c (TSTypeParameter.name) was previously an Identifier - now it is just a string value (i.e. it has no node in the AST).

I discovered this when trying to update Prettier here to the latest: https://github.com/prettier/prettier/pull/5728

The final failing test case is around prettier being unable to attach the comment /* commentC */ in the way it expects. Naturally this is because it is trying to attached it to the TSTypeParameter.name, which used to be a node, and no longer is.

Without this being reinstated to an Identifier node, I'm not sure how we could preserve Prettier's behaviour here?

We are not ignoring the new mapped type test sources in the ast-alignment-tests, so we must be aligned with the babel AST here, but maybe they both need to change in this case?

Would love to know your thoughts: @uniqueiniquity @armano2 @ikatyang

Full ASTs in each case:

Before v8 (c is an Identifier)

```json { "body": [ { "id": { "loc": { "end": { "column": 6, "line": 1 }, "start": { "column": 5, "line": 1 } }, "name": "C", "range": [ 5, 6 ], "type": "Identifier" }, "loc": { "end": { "column": 1, "line": 3 }, "start": { "column": 0, "line": 1 } }, "range": [ 0, 48 ], "type": "TSTypeAliasDeclaration", "typeAnnotation": { "loc": { "end": { "column": 1, "line": 3 }, "start": { "column": 9, "line": 1 } }, "range": [ 9, 48 ], "type": "TSMappedType", "typeAnnotation": { "loc": { "end": { "column": 35, "line": 2 }, "start": { "column": 27, "line": 2 } }, "range": [ 38, 46 ], "type": "TSTypeAnnotation", "typeAnnotation": { "loc": { "end": { "column": 35, "line": 2 }, "start": { "column": 29, "line": 2 } }, "range": [ 40, 46 ], "type": "TSStringKeyword" } }, "typeParameter": { "constraint": { "loc": { "end": { "column": 26, "line": 2 }, "start": { "column": 25, "line": 2 } }, "range": [ 36, 37 ], "type": "TSTypeReference", "typeName": { "loc": { "end": { "column": 26, "line": 2 }, "start": { "column": 25, "line": 2 } }, "name": "C", "range": [ 36, 37 ], "type": "Identifier" } }, "loc": { "end": { "column": 26, "line": 2 }, "start": { "column": 20, "line": 2 } }, "name": { "loc": { "end": { "column": 21, "line": 2 }, "start": { "column": 20, "line": 2 } }, "name": "c", "range": [ 31, 32 ], "type": "Identifier" }, "range": [ 31, 37 ], "type": "TSTypeParameter" } } } ], "loc": { "end": { "column": 1, "line": 3 }, "start": { "column": 0, "line": 1 } }, "range": [ 0, 48 ], "sourceType": "script", "tokens": [ { "loc": { "end": { "column": 4, "line": 1 }, "start": { "column": 0, "line": 1 } }, "range": [ 0, 4 ], "type": "Identifier", "value": "type" }, { "loc": { "end": { "column": 6, "line": 1 }, "start": { "column": 5, "line": 1 } }, "range": [ 5, 6 ], "type": "Identifier", "value": "C" }, { "loc": { "end": { "column": 8, "line": 1 }, "start": { "column": 7, "line": 1 } }, "range": [ 7, 8 ], "type": "Punctuator", "value": "=" }, { "loc": { "end": { "column": 10, "line": 1 }, "start": { "column": 9, "line": 1 } }, "range": [ 9, 10 ], "type": "Punctuator", "value": "{" }, { "loc": { "end": { "column": 5, "line": 2 }, "start": { "column": 4, "line": 2 } }, "range": [ 15, 16 ], "type": "Punctuator", "value": "[" }, { "loc": { "end": { "column": 21, "line": 2 }, "start": { "column": 20, "line": 2 } }, "range": [ 31, 32 ], "type": "Identifier", "value": "c" }, { "loc": { "end": { "column": 24, "line": 2 }, "start": { "column": 22, "line": 2 } }, "range": [ 33, 35 ], "type": "Keyword", "value": "in" }, { "loc": { "end": { "column": 26, "line": 2 }, "start": { "column": 25, "line": 2 } }, "range": [ 36, 37 ], "type": "Identifier", "value": "C" }, { "loc": { "end": { "column": 27, "line": 2 }, "start": { "column": 26, "line": 2 } }, "range": [ 37, 38 ], "type": "Punctuator", "value": "]" }, { "loc": { "end": { "column": 28, "line": 2 }, "start": { "column": 27, "line": 2 } }, "range": [ 38, 39 ], "type": "Punctuator", "value": ":" }, { "loc": { "end": { "column": 35, "line": 2 }, "start": { "column": 29, "line": 2 } }, "range": [ 40, 46 ], "type": "Identifier", "value": "string" }, { "loc": { "end": { "column": 1, "line": 3 }, "start": { "column": 0, "line": 3 } }, "range": [ 47, 48 ], "type": "Punctuator", "value": "}" } ], "type": "Program" } ```

---

Current (c is a string value)

```json { "body": [ { "id": { "loc": { "end": { "column": 6, "line": 1 }, "start": { "column": 5, "line": 1 } }, "name": "C", "range": [ 5, 6 ], "type": "Identifier" }, "loc": { "end": { "column": 1, "line": 3 }, "start": { "column": 0, "line": 1 } }, "range": [ 0, 48 ], "type": "TSTypeAliasDeclaration", "typeAnnotation": { "loc": { "end": { "column": 1, "line": 3 }, "start": { "column": 9, "line": 1 } }, "range": [ 9, 48 ], "type": "TSMappedType", "typeAnnotation": { "loc": { "end": { "column": 35, "line": 2 }, "start": { "column": 29, "line": 2 } }, "range": [ 40, 46 ], "type": "TSStringKeyword" }, "typeParameter": { "constraint": { "loc": { "end": { "column": 26, "line": 2 }, "start": { "column": 25, "line": 2 } }, "range": [ 36, 37 ], "type": "TSTypeReference", "typeName": { "loc": { "end": { "column": 26, "line": 2 }, "start": { "column": 25, "line": 2 } }, "name": "C", "range": [ 36, 37 ], "type": "Identifier" } }, "loc": { "end": { "column": 26, "line": 2 }, "start": { "column": 20, "line": 2 } }, "name": "c", "range": [ 31, 37 ], "type": "TSTypeParameter" } } } ], "loc": { "end": { "column": 1, "line": 3 }, "start": { "column": 0, "line": 1 } }, "range": [ 0, 48 ], "sourceType": "script", "tokens": [ { "loc": { "end": { "column": 4, "line": 1 }, "start": { "column": 0, "line": 1 } }, "range": [ 0, 4 ], "type": "Identifier", "value": "type" }, { "loc": { "end": { "column": 6, "line": 1 }, "start": { "column": 5, "line": 1 } }, "range": [ 5, 6 ], "type": "Identifier", "value": "C" }, { "loc": { "end": { "column": 8, "line": 1 }, "start": { "column": 7, "line": 1 } }, "range": [ 7, 8 ], "type": "Punctuator", "value": "=" }, { "loc": { "end": { "column": 10, "line": 1 }, "start": { "column": 9, "line": 1 } }, "range": [ 9, 10 ], "type": "Punctuator", "value": "{" }, { "loc": { "end": { "column": 5, "line": 2 }, "start": { "column": 4, "line": 2 } }, "range": [ 15, 16 ], "type": "Punctuator", "value": "[" }, { "loc": { "end": { "column": 21, "line": 2 }, "start": { "column": 20, "line": 2 } }, "range": [ 31, 32 ], "type": "Identifier", "value": "c" }, { "loc": { "end": { "column": 24, "line": 2 }, "start": { "column": 22, "line": 2 } }, "range": [ 33, 35 ], "type": "Keyword", "value": "in" }, { "loc": { "end": { "column": 26, "line": 2 }, "start": { "column": 25, "line": 2 } }, "range": [ 36, 37 ], "type": "Identifier", "value": "C" }, { "loc": { "end": { "column": 27, "line": 2 }, "start": { "column": 26, "line": 2 } }, "range": [ 37, 38 ], "type": "Punctuator", "value": "]" }, { "loc": { "end": { "column": 28, "line": 2 }, "start": { "column": 27, "line": 2 } }, "range": [ 38, 39 ], "type": "Punctuator", "value": ":" }, { "loc": { "end": { "column": 35, "line": 2 }, "start": { "column": 29, "line": 2 } }, "range": [ 40, 46 ], "type": "Identifier", "value": "string" }, { "loc": { "end": { "column": 1, "line": 3 }, "start": { "column": 0, "line": 3 } }, "range": [ 47, 48 ], "type": "Punctuator", "value": "}" } ], "type": "Program" } ```

[armano2]

it has to be identifier, i missed this case, we can change that.

[ikatyang]

Identifier looks more reasonable to me, but it does not look like a TSTypeParameter-only issue but for all Identifier/TypeParameter:

// current:
//         v string (TSTypeParameter#name)
//                   v TSTypeReference (TSTypeParameter#constraint)
//         vvvvvvvvvvv TSTypeParameter
function a<A extends B>(b: c) {}
//                      ^^^^ Identifier
//                         ^ TypeAnnotation (Identifier#typeAnnotation)
//                      ^ string (Identifier#name)

// IMO, it should be:
//         v Identifier (TSTypeParameter#name)
//                   v TSTypeReference (TSTypeParameter#constraint)
//         vvvvvvvvvvv TypeParameter
function a<A extends B>(b: c) {}
//                      ^^^^ Parameter
//                         ^ TypeAnnotation (Parameter#typeAnnotation)
//                      ^ Identifier (Parameter#name)

FWIW, I tracked down the history in babylon:

• TSTypeParameter was introduced in babel/babylon#523, which followed the existing format from Flow.
• (Flow)TypeParameter was introduced in babel/babylon#25, which was modified from Identifier
  • Identifier { name: string, typeAnnotation?: TypeAnnotation }

[armano2]

@JamesHenry besides normal test cases from this project i'm running tests over popular libraries and typescript test cases in https://github.com/armano2/typescript-estree-test (~19k files)

but sometimes its hard to spot all potential issues 🦊

i even added d.ts generator with all possible outputs to do some comparison between my changes.

estree-13.5.2...estree-5.3

TSTypeParameter: compare

export interface TSTypeParameter extends BaseNode {
  type: 'TSTypeParameter';
+ name: string;
- name: string | Identifier;
  default?:
    | TSFunctionType
    | TSIndexedAccessType
@@ -2237,6 +2166,7 @@ export interface TSTypeParameter extends BaseNode {
    | TSObjectKeyword
    | TSStringKeyword;
  constraint?:
-   | Literal
    | TSArrayType
    | TSConstructorType
    | TSFunctionType

[armano2]

to change this we have to just disable some tests from babel and change this line

- name: node.name.text,
+ name: convertChildType(node.name),

https://github.com/JamesHenry/typescript-estree/blob/master/src/convert.ts#L2240

[JamesHenry]

Yep! I won’t get chance to do a PR until tomorrow so if you have time feel free to submit, otherwise I’m very happy to do it

[armano2]

31 tests is failing :(