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

Type alias name (union) is not preserved when using index access/lookup types #32287

Closed
OliverJAsh opened this issue Jul 8, 2019 · 7 comments
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@OliverJAsh
Copy link
Contributor

TypeScript Version: 3.5.2

Search Terms: indexed access lookup types type alias naming

Code

Given:

type MyUnion = 1 | 2 | 3;

// Hover this
type MyObject = { value: MyUnion };

If I hover over MyObject, I see { value: MyUnion; }.

However, given:

type MyRecord = {
    Foo: 1;
    Bar: 2;
    Baz: 3;
}

type MyUnion = MyRecord[keyof MyRecord];

// Hover this
type MyObject = { value: MyUnion };

If I hover over MyObject, I see { value: 1 | 2 | 3; }.

I would expect the same behaviour as we see in my first example ({ value: MyUnion; }). The name of the type alias, MyUnion, should be preserved and its usage/reference inside of MyObject should be shown.

To give some context on my real world use case for this: I am using Unionize to create tagged unions (to avoid boilerplate for constructors, matchers, and type predicates). However, because of the issue described above, the types become very difficult to inspect/read: pelotom/unionize#60.

Under the hood, Unionize does something like this:

type Foo = { foo: number };
type Bar = { bar: number };
type Baz = { baz: number };

type MyUnionRecord = {
    Foo: Foo;
    Bar: Bar;
    Baz: Baz;
};

type MyUnionTaggedRecord = { [K in keyof MyUnionRecord]: { tag: K; value: MyUnionRecord[K] } };
type MyUnion = MyUnionTaggedRecord[keyof MyUnionTaggedRecord];

// Hover this
type MyObject = { value: MyUnion };

If we hover over MyObject here, we see:

{
    value: {
        tag: "Foo";
        value: Foo;
    } | {
        tag: "Bar";
        value: Bar;
    } | {
        tag: "Baz";
        value: Baz;
    };
}

… when we really want to see { value: MyUnion }.

As I demonstrated in pelotom/unionize#60, this scales really badly, e.g. when nesting unions.

@MartinJohns
Copy link
Contributor

Duplicate of #31940.

@OliverJAsh
Copy link
Contributor Author

This issue seems more specific than #31940, so not sure it's a duplicate, but the general idea is the same.

@RyanCavanaugh RyanCavanaugh added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Jul 8, 2019
@RyanCavanaugh
Copy link
Member

The fact that any type alias at all shows up is due to trickery and the limitations of that trickery are apparent in these examples.

The trick is that if the very first time we make a type it's because it's initializing a type alias, then future displays of that type use the alias name. This is a desirable heuristic because you don't want some rando .d.ts file to say

type AcrobaticBackCompatKey = string | number | boolean;

and then have your function

function fn(stringish: string | number | boolean) { }

display back as

fn(stringish: AcrobaticBackCompatKey)

which you didn't write.

In this example, it seems the type 1 | 2 | 3 got created before it got assigned to the type alias, preventing it from being the "given name" for 1 | 2 | 3.

"Why not just make type aliases be a named pointer to the underlying type?"

Nothing's impossible, but this has some performance impacts that aren't immediately obvious. For example

type SN = string | number;
type NB = number | boolean;

declare let q: SN | boolean;
declare let v: string | NB;
q = v;
v = q;

In this example, the assignments can be detected to be correct almost "for free" because their types are literally the same object - there's no way to tell q and v apart. If we have to keep type aliases around all the time, then this operation becomes a lot more expensive.

So it's a trade-off and we'd have to figure out ways to make it not as expensive as it might be, or someone can maybe propose tweaks to our trickery that would make things more palatable while still not messing up existing type display too badly.

@OliverJAsh
Copy link
Contributor Author

Thank you for the explanation. Unfortunately I don't have any ideas how you can resolve this.

@whatisaphone
Copy link

I've never dug into the compiler source, but I have an idea: Keep one type object around for typechecking, and another separate object for diagnostic reporting (only if different than the first).

So for Ryan's example, the AST for q and v might look something like this:

{
  name: "q",
  type: Union([String, Number, Boolean]),
  prettyType: Union([
    TypeAlias("SN", Union([String, Number]),
    Boolean,
  ]),
}
{
  name: "v",
  type: Union([String, Number, Boolean]), // reference-equal to the previous type
  prettyType: Union([
    String,
    TypeAlias("NB", Union([Number, Boolean]),
  ]),
}

type is still the literally the same object, but prettyType is different and would be used when reporting errors. This would probably increase memory footprint a bit, but shouldn't noticeably increase typechecking time. For any declaration where type aliases aren't involved or the distinction isn't helpful, prettyType can be set to null, and reporting would fall back to the actual type. This would hopefully keep memory issues in check.

Of course I'm just this random guy and the devil is always in the details.

@OliverJAsh
Copy link
Contributor Author

This seems to have been fixed. Do we know what changed cause this? Was it intentional?

image

@OliverJAsh OliverJAsh changed the title Type alias name is not preserved when using index access/lookup types Type alias name (union) is not preserved when using index access/lookup types Jan 5, 2021
@Andarist
Copy link
Contributor

@OliverJAsh this has changed between 3.9.0-dev.20200330 and 3.9.0-dev.20200402. It's very likely that the change was introduced in #37444 and some tests there suggest that (like this one). So I would classify this as an intentional change :) The issue can be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants