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

Should the compiler api resolve certain equivalent types to the first encountered one? #28197

Closed
dsherret opened this issue Oct 28, 2018 · 5 comments
Labels
Question An issue which isn't directly actionable in code Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@dsherret
Copy link
Contributor

dsherret commented Oct 28, 2018

The question in the title is the current behaviour.

TypeScript Version: 3.2.0-dev.20181027
Relates to: #25731

The compiler API will resolve some, but not all, equivalent types to the first encountered type:

import * as ts from "typescript";

const testFilePath = "/file.ts";
const testFileText = `
export type AvatarSize = 'sm' | 'md' | 'lg' | 'xl';
export type CardAvatarSize = 'sm' | 'md' | 'lg' | 'xl';

let v1: AvatarSize, v2: CardAvatarSize;
`;

// common setup
const testSourceFile = ts.createSourceFile(testFilePath, testFileText, ts.ScriptTarget.Latest);
const variableStatement = testSourceFile.statements.find(ts.isVariableStatement)!;
const variableDeclarations = variableStatement.declarationList.declarations;

// outputs: "AvatarSize", "AvatarSize"
const typeChecker1 = getTypeChecker();
logTypeText(typeChecker1, variableDeclarations[0]);
logTypeText(typeChecker1, variableDeclarations[1]);

// outputs: "CardAvatarSize", "CardAvatarSize"
const typeChecker2 = getTypeChecker();
logTypeText(typeChecker2, variableDeclarations[1]); // note 1, not 0 this time
logTypeText(typeChecker2, variableDeclarations[0]);

function logTypeText(typeChecker: ts.TypeChecker, declaration: ts.VariableDeclaration) {
    const type = typeChecker.getTypeAtLocation(declaration.type!);
    console.log(typeChecker.typeToString(type));
}

function getTypeChecker() {
    const options: ts.CompilerOptions = { target: ts.ScriptTarget.ES5 };
    const host: ts.CompilerHost = {
        fileExists: filePath => filePath === testFilePath,
        directoryExists: dirPath => dirPath === "/",
        getCurrentDirectory: () => "/",
        getDirectories: () => [],
        getCanonicalFileName: fileName => fileName,
        getNewLine: () => "\n",
        getDefaultLibFileName: () => "",
        getSourceFile: filePath => filePath === testFilePath ? testSourceFile : undefined,
        readFile: filePath => filePath === testFilePath ? testFileText : undefined,
        useCaseSensitiveFileNames: () => true,
        writeFile: () => {}
    };
    return ts.createProgram({
        options,
        rootNames: [testFilePath],
        host
    }).getTypeChecker();
}

I found #25731 that says this is a design limitation. What I was wondering is in the context of the compiler API, which people aren't using only for type checking, does it make sense to have this design? When working with the compiler api, I feel like it's valuable to have distinct ts.Type objects whose text will be the name of the type alias used in the situation (ex. when someone does type MyType = string; it would be nice for it to return a MyType ts.Type object instead of the string one). Or does this just increase the complexity too much internally?

Also note that this also happens when object types are aliased...

export type Test = { prop: string; };
export type Test2 = { prop: number; };
export type AvatarSize = Test | Test2;
export type CardAvatarSize = Test | Test2;

...but not when they're inlined, which seems inconsistent?

export type AvatarSize = { prop: string; } | { prop: number; };
export type CardAvatarSize = { prop: string; } | { prop: number; };

Thanks!

@weswigham
Copy link
Member

For performance reasons, we intern types where possible (this way we avoid duplicating work for equivalent types). We do not currently intern anonymous object types, though we've experimented with it before. Unfortunately, interning object types has the side effect of breaking go to definition on the interned types; so we didn't pull it in. The specific types we intern today are indexed accesses, unions, and intersections (also reverse mapped types, but only inference can produce those). This is a tradeoff - origin information is lost on interned types; but we do avoid quite a bit of work most of the time.

@weswigham weswigham added Question An issue which isn't directly actionable in code Working as Intended The behavior described is the intended behavior; this is not a bug labels Oct 29, 2018
@dsherret
Copy link
Contributor Author

Thanks for the explanation, @weswigham! I thought it was probably for performance reasons.

How difficult do you think it would be to have a way to configure the api to disable this behaviour? Perhaps disabling it would be a lot of work because some code other code relies on the types being interned? As you mentioned, there is a tradeoff between speed and information, but in some analysis scenarios people would be more interested in the information.

Overall, I don't view disabling this behaviour as a must have because it's not so bad to live with it, but it seems like there will be limitations when analyzing code that has equivalent type aliases. Perhaps there's a better way of using the compiler api to get around this behaviour though.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@jasonkuhrt
Copy link

How difficult do you think it would be to have a way to configure the api to disable this behaviour?

Hey @weswigham, a few of us are building a documentation generator for TypeScript packages called Tydoc. We've found that this interning behaviour causes issues for some cases of analysis (for example this one). When generating documentation, performance is less important as it is often done in a build/ci step. It would be amazing if TypeScript had an opt-in low-perf mode for analysis use-cases like ours.

What do you think? Can we re-open this issue?

@jasonkuhrt
Copy link

jasonkuhrt commented Nov 11, 2020

For cross-reference posterity here is a StackOverflow issue about a case involving this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants