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

Provide a disciminated union of all available Node types #13634

Closed
ajafff opened this issue Jan 23, 2017 · 6 comments
Closed

Provide a disciminated union of all available Node types #13634

ajafff opened this issue Jan 23, 2017 · 6 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Infrastructure Issue relates to TypeScript team infrastructure Suggestion An idea for TypeScript

Comments

@ajafff
Copy link
Contributor

ajafff commented Jan 23, 2017

TypeScript Version: 2.1.5

Recently in tslint development there was the idea to create a discriminated union of all ts.Nodes available in the AST. That would make it easier to switch on Node.kind and avoid the need of type assertions. It would also simplify some of TypeScript's own code like forEachChild in parser.ts.

Before we start to create and maintain such a union ourselves, the question is if typescript can provide anything for this use case? Given there are already unions for things like BlockLike, CallLikeExpression, etc., would it be possible to have more of them, maybe even auto generated?

Unfortunately that would ultimately result in the demand for more unions for Expression, Statement, ...

Code

ts.forEachChild(someNode, visitNode);
function visitNode(node: ts.Node) {
  switch (node.kind) {
    case ts.SyntaxKind.Identifier:
      doStuff((node as ts.Identifier).text);
    case ts.SyntaxKind.PropertyAssignment:
      if ((node as ts.PropertyAssignment).initializer.kind === ts.SyntaxKind.Identifier)
        doStuff(((node as ts.PropertyAssignment).initializer as ts.Identifier).text);
     // ... to be continued
  }
}

Expected behavior:

ts.forEachChild(someNode, visitNode);
function visitNode(node: ts.AllNodes) { // discriminated union of all subtypes of ts.Node
  switch (node.kind) {
    case ts.SyntaxKind.Identifier:
      doStuff(node.text); // type can be inferred
    case ts.SyntaxKind.PropertyAssignment:
      if (node.initializer.kind === ts.SyntaxKind.Identifier) // node is inferred as ts.PropertyAssignment
        doStuff(node.initializer.text); // node.initializer could be inferred to be ts.Identifier, would require another union for all subtypes of ts.Expression
  }
}

//cc @nchen63

@mhegazy mhegazy added Suggestion An idea for TypeScript Infrastructure Issue relates to TypeScript team infrastructure labels Jan 23, 2017
@jwbay
Copy link
Contributor

jwbay commented Jan 24, 2017

Thanks for making this issue. I meant to and kept forgetting. When I'm writing lint rules, dealing with type assertions is always the most annoying part of the process. If discriminated unions won't work somehow, I'd also gladly take type guard functions for each type of node that can hopefully be generated. Babel does something similar - see https://github.com/babel/babel/tree/master/packages/babel-types and ctrl-f the page for 't.is'

@adidahiya
Copy link
Contributor

In tslint-react we have a generic type guard for this, but it is just a little verbose:

export function nodeIsKind<T extends ts.Node>(node: ts.Node, kind: ts.SyntaxKind): node is T {
    return node.kind === kind;
}

@ajafff
Copy link
Contributor Author

ajafff commented Feb 3, 2017

@adidahiya I like this idea, but it doesn't check if the type parameter matches kind. So you can accidentally write the following:

nodeIsKind<ts.StringLiteral>(node, ts.SyntaxKind.ClassDeclaration);

@rozzzly
Copy link

rozzzly commented Aug 21, 2017

This is the pattern I use for my redux actions and it works splendidly.

export interface Action<T> {
    type: T;
    payload: any;
    meta?: any;
}

export type CREATE_POST_ActionID = 'CREATE_POST';
export const CREATE_POST: CREATE_POST_ActionID = 'CREATE_POST';
export interface CREATE_POST_Action extends Action<CREATE_POST_ActionID> {
    payload: {
        title: string;
        content: string
    }
}
export const createPost = (title: string, content: string): CREATE_POST_Action => ({
    type: CREATE_POST,
    payload: { title, content }
});

export type DELETE_POST_ActionID = 'DELETE_POST';
export const DELETE_POST: DELETE_POST_ActionID = 'DELETE_POST';
export interface DELETE_POST_Action extends Action<DELETE_POST_ActionID> {
    payload: {
        id: number;
    }
}
export const deletePost = (id: number): DELETE_POST_Action => ({
    type: DELETE_POST,
    payload: { id }
});

export type KnownActionIDs = (
    | CREATE_POST_ActionID
    | DELETE_POST_ActionID
    // ...
);

export type KnownActions = (
    | CREATE_POST_Action
    | DELETE_POST_Action
    // ...
);

Holy sheit thats a lot of boilerplate... welcome to redux. Anyway, this makes things super easy to use:

import { KnownActions, CREATE_POST, DELETE_POST } from './actions';

export interface Post {
    id: number;
    title: string;
    content: string;
}

export interface State {
    posts: Post[];
    // ...
}

const initialState: State = {
    posts: []
};

export default function Reducer(state: State = initialState, action: KnownActions): State {
    switch(action.type) {
        case CREATE_POST: 
            // action.payload._____ (completions for `title` and `content`, but not `id`
            return {
                ...state,
                posts: [
                    ...state.posts,
                    {
                        id: Math.random(),
                        title: action.payload.title,
                        content: action.payload.content
                    }
                ]
             };
        case DELETE_POST:
             return {
                 ...state,
                 posts: state.posts.filter(post => post.id !== action.payload.id) // completions for `id` on `action.payload`
             };
        default:
            return state;
    }
}

I think this matches up with the relationship between ts.Node and ts.SyntaxKind quite nicely. Oh, the refactor would not be nice, but a lot of that could be handled with codemods.

I've been trying to think of a way to write my own typeguard that would do this implicitly, unlike @adidahiya's method which requires the explicit declaration of the the desired generic.

Ideally, something like the following could be achieved.

import * as path from 'path';
import * as ts from 'typescript';

type KnownNodes = (
    | ts.ComputedPropertyName
    | ts.StringLiteral
    | ts.VariableStatement
    // ...
);

// not sure how this could be typed...
export function ofKind(nodes: KnownNodes[], kind: ts.SyntaxKind): ????;
    return nodes.filter(node => node.kind === kind);
}

const file = path.join(__dirname, 'someFile.ts');

const program = ts.createProgram([file], {
    target: ts.ScriptTarget.ESNext,
    module: ts.ModuleKind.ESNext
});
  
const chk = program.getTypeChecker();
const src = program.getSourceFile(file);

const stmts = ofKind(src.statements as any, ts.SyntaxKind.VariableStatement); // ideally inferred as `ts.VariableStatement[]`
stmts.forEach(stmt => stmt.________ // completions for a `ts.VariableStatement`  node.

I can't think of a way to write a typeguard for filtering an array right now, even for my redux example, let alone for ts.Node + ts.SyntaxKind. Perhaps it's possible that it could be hacked together under current language semantics using mapped types. ...paging the geniuses over at [gcanti/typelevel-ts] @gcanti @tycho01. Hmmmm let's add some more smart people: @RyanCavanaugh @mhegazy

Anyone got any input? Anyone think this an issue worth solving?

@KiaraGrouwstra
Copy link
Contributor

For the original switch case I think #2214 might go a long way.
As to the ofKind one, since it's not hooking into the traditional type guards that sounds like it would need general type subtraction (#4183), which I believe to be a use-case of #6606 (comment).

@RyanCavanaugh RyanCavanaugh added the Declined The issue was declined as something which matches the TypeScript vision label Aug 15, 2018
@RyanCavanaugh
Copy link
Member

We tried this at one point (changing Node to be a union) but it was actually a fairly substantial perf hit on our own codebase because any single test would produce a new large union. It's a nice-to-have for tool authors but not something we want to maintain as a separate deliverable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision Infrastructure Issue relates to TypeScript team infrastructure Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants