microsoft / microsoft/TypeScript

Replace Node interface with discriminated unions

Aperta
#56,275 4 commenti 2 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

Experimentation Needed Suggestion
Lingua principale
Go
Stelle
111k
Fork
14.4k
Merge medio
1g 19h
PR unite (30g)
117

Descrizione

🔍 Search Terms

discrimnated type union node internal source api

✅ Viability Checklist
  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of our Design Goals: https://github.com/Microsoft/TypeScript/wiki/TypeScript-Design-Goals
⭐ Suggestion

Happy Halloween 🎃! In the spirit of the holiday, I'd like to resurrect #13634: using a discriminated unions for Node categories. Roughly paraphrasing #18285's very good description here:

As mentioned in https://github.com/Microsoft/TypeScript/pull/18214#issuecomment-326823448, TypeScript's source can remove a lot of casts by using a discriminated union for Node.
While this should not affect TypeScript's runtime performance (as it just changes its types, mostly) it may cause TypeScript to take longer to compile itself.

#13634 was declined in 2018 for performance concerns (https://github.com/microsoft/TypeScript/issues/13634#issuecomment-413289616) but I hope that the years of performance improvements since then have since made this possible.

📃 Motivating Example

Switching TypeScript's Node from an interface from a discriminated type union would allow .kind checks to narrow types of values declared as Node.

import * as ts from "typescript";

declare const node: ts.Node;

if (node.kind === ts.SyntaxKind.FunctionDeclaration) {
  node.name?.text;
  //   ~~~~
  // Property 'name' does not exist on type 'Node'.
}
💻 Use Cases

In TypeScript itself, a match-case-and-full-word search for /(?:name|node|parent) as \w{5,}/ in src/**/*.ts excluding *test* yields 1879 matches right now. After prototyping it locally and enabling @typescript-eslint/no-unnecessary-type-assertion, I mostly-automatically reduced it to 987 matches. More removals could likely be enacted with manual effort.

In community projects that use the TypeScript AST, including typescript-eslint, we'd similarly be able to have more precise type information on TS AST nodes.

#13634 mentions other consumers too, such as Babel's https://github.com/babel/babel/blob/c446ff85c28e117ebf3cd72cd34ef358f1077aa8/packages/babel-types/src/validators/is.ts.

Guida per i contributori

Apri la guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Direzione di ricerca

Inizia esaminando i tentativi precedenti in #18285, #56274 e #54148, quindi ispeziona i 1879 cast match in src/**/*.ts menzionati nell’issue. Il lavoro sarà completo quando l’interfaccia Node sarà sostituita da unioni discriminate, i cast giustificati saranno rimossi quando il narrowing li rende superflui e il comportamento a runtime sarà preservato senza emettere JavaScript diverso.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
typescript
Ambito
compilers
Tipo di issue
Refactoring
Difficoltà
5/5
Tempo stimato
Più di una settimana
Stato di attività
Ferma
Chiarezza
Abbastanza chiara
Idoneità per principianti
25/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.