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

Attempt to more-frequently use local aliases when printing out types #34778

Open
DanielRosenwasser opened this issue Oct 28, 2019 · 13 comments
Open
Labels
Domain: Type Display Bugs relating to showing types in Quick Info/Tooltips, Signature Help, or Completion Info Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Milestone

Comments

@DanielRosenwasser
Copy link
Member

When a user imports a type alias like

// ./a.ts
export type Foo = number | string | symbol;

We should try to print out Foo in error messages like in

// ./b.ts
import { Foo } from "./a.js"

let x: Foo = true;

and in declaration emit like in

// ./c.ts
import { Foo } from "./a.js"

function f(x: Foo) {
  return x;
}
@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Domain: Type Display Bugs relating to showing types in Quick Info/Tooltips, Signature Help, or Completion Info labels Oct 28, 2019
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 3.8.0 milestone Oct 28, 2019
@aaronjensen
Copy link

aaronjensen commented Oct 29, 2019

I want to call out another case explicitly since it behaves differently right now and that's when a type alias is used in an argument of a lambda:

For example,

import { DeepReadonly } from 'utility-types'

export type Foo = DeepReadonly<{ foo: string }>
export const useFoo = ({ foo }: { foo: Foo }) => {}

Currently emits:

import { DeepReadonly } from 'utility-types';
export declare type Foo = DeepReadonly<{
    foo: string;
}>;
export declare const useFoo: ({ foo }: {
    foo: {
        readonly foo: string;
    };
}) => void;

Ideally it would emit:

import { DeepReadonly } from 'utility-types';
export declare type Foo = DeepReadonly<{
    foo: string;
}>;
export declare const useFoo: ({ foo }: Foo) => void;

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Nov 4, 2019

As long as the Identity<> trick doesn't break, right?

#34556

There are cases where Foo is actually a generic type. And Foo<T> is 100+ lines long because T is 100+ lines long.

But if Foo<T> is "expanded", the expanded type could be like 5 lines.

The Identity<> trick forces TS to always expand the type, at the moment.


Pick and Omit suffer from this problem, too.

Pick<T, K>

If T is 100+ properties and K is 2 properties, the emit can be 100+ lines long. In such cases, I always tell people to write their own version of Pick and Omit using the Identity<> trick.

#34793


I guess I should find/file a separate issue for this, and locate all the relevant issues. (Like a meta issue regarding type display?)

But it seems like TS doesn't really give developers a way to explicitly control how they want a type to "expand" in emit/hover, at the moment.

To TS, as long as the types behave well, then all is good; it doesn't matter if the type looks simple or complicated.

Except, to developers, if the type looks complicated/long/verbose, then it's extra cognitive overhead. That, and hover and error messages tend to truncate long types.


I'm not sure how to phrase this properly.

I think it's fine to have heuristics for TS to automatically determine how to "present" a type. Given a type, there could be a million different ways to express it syntactically.

However, there needs to be a way for a programmer to step in and say, "Your heuristics are bad.", "Expand this type.", "Don't expand this type.", "Use typeof x instead of X", etc.

Because, sometimes, the 100+ line type is more readable than the 5 line type. Mostly, it's the other way around. Sometimes, aliases are easier to understand. Other times, the full expanded type is easier. There's no way an algorithm can display a type such that it'll always be "easiest" to read. It's rather subjective, I've found.


Also relevant,
#32977

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Nov 4, 2019

I bring this up over here because I just saw that this is the proposed output,

// ./c.ts
import { Foo } from "./a.js"

function f(x: Foo) {
  return x;
}

This output would actually be bad for a lot of my use cases involving generic types. Right now, I'm using the Identity<> trick to always force Foo to be expanded in the hover/tooltip.


Also, @aaronjensen said this is undesirable,

import { DeepReadonly } from 'utility-types';
export declare type Foo = DeepReadonly<{
    foo: string;
}>;
export declare const useFoo: ({ foo }: {
    foo: {
        readonly foo: string;
    };
}) => void;

However, there are cases where it is desirable. So, forcing either current display, or proposed display isn't a great idea, unless it comes with official ways for devs to control the exact type display.

This can make the difference between a usable API and an unusable API (or, at least indecipherable when looking at the type display on hover)

@aaronjensen
Copy link

@AnyhowStep That's interesting, I wasn't thinking there would be times where the existing behavior would be desirable. I wonder though, in your examples and when you talk of an ability to expand types, are you imagining it fully expanding the entire type? If so, it seems like you're potentially talking about api authors being able to provide utility types that fully expand (like your Mapped2 example).

The problem I see with that, is if the consumer of that type doesn't want the type expanded. I certainly wouldn't, if I did something like Mapped2<MyReduxState>.

So, I don't think the choice could be made at the level of the type alias definition.

That makes me wonder if the missing piece is the ability to expand types in dev tools (maybe this isn't missing, I'm not sure). If I could cursor over a type and then expand it one step at a time, I could learn about the type as needed. This would be similar to a macro expand in lisp languages.

Do you have any examples where a fully aliased type is "at least indecipherable when looking at the type display on hover"? That seems to only apply to expanded types in my mind.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Nov 4, 2019

when you talk of an ability to expand types, are you imagining it fully expanding the entire type?

Not necessarily so. And it isn't always expansion I'm interested in. Sometimes, just using typeof x (where x is of type X) is better than the full type of X. And this is like the opposite of expansion.

Sometimes, keeping the alias is better than expanding.

Sometimes, forcing the alias is better than expanding. It's all super subjective because humans are involved.

you're potentially talking about api authors being able to provide utility types that fully expand (like your Mapped2 example).

Also not necessarily so. They're not always utility types. By "utility type", I mean very generic type aliases that can be applied to a variety of situations. A lot of the time, they're one-off types for a particular function/method.

That makes me wonder if the missing piece is the ability to expand types in dev tools (maybe this isn't missing, I'm not sure).

This is definitely missing. And would make the problem a little less painful. I believe there's an issue somewhere asking for this...

Even so, this wouldn't solve the problem 100%

This will let you work around the problem in your tooltip/hover. But not during emit, where it will probably go back to the "default" type display, which may be 100+ lines long, and ugly, and impossible to read, and cause your emit time to be 40-80+s

Do you have any examples where a fully aliased type is "at least indecipherable when looking at the type display on hover"? That seems to only apply to expanded types in my mind.

Imagine T is 100+ lines. Now, Pick<T, "hi"> is 100+ lines.

Pick<{
//Snip 100+ lines
}, "hi">

Whereas the fully expanded type would be something like,

{ hi : U }

There are many other less contrived examples in my projects but this is the easiest to explain.


Also consider,

Omit<{
//Snip 100+ lines
}, "hi"|"bye">

Which becomes,

Pick<{
//Snip 100+ lines
}, "key0"|"key1"|"key2"|...|100+ more|...|"keyN">

So, now your type is 200+ lines long.

Whereas this would be better,

{
  key0 : U0,
  key1 : U1,
  key2 : U2,
  //snip 100+ lines
  keyN : UN,
}

Only 100+ lines, vs 200+.

In this very specialized case, a developer may say "Hmm, I want to force this 100+ line type to be a single line type alias". (In this case, no generics are involved, so it's a hard coded type and we can just alias it. But if it's a generic, life gets harder and we just have to keep the 100+ line type because it's better than the 200+ line alternative)

And it would be useful if one can write,

type Blah = //that 100+ line type

And force TS to never expand the type.

At the end of the day, it's super subjective. Sometimes, the 100+ line type is readable. Sometimes it isn't.

I like it when I can control what the emit and hover display by default, so that the type is as easy to understand as possible, with as little work as possible for downstream users.


Here's an example from one of my use cases,

type SomeDelegate<TableT extends ITable> =
  (columns : SomeMappedType<TableT>) => OtherComplexType<TableT>
;

function foo<TableT extends ITable> (table : TableT, d : SomeDelegate<TableT>) : //snip

TableT will almost always be a super long type (100+ lines). It is a generic type that describes a SQL table.

SomeMappedType<TableT>, when unexpanded, will always be 100+ lines long, too. Since TableT is 100+ lines long.

However, it will always be more readable, when fully expanded. Considerably fewer lines.

When users (and even myself) use the function, they write something like,

foo(myTable, (columns) => // snip

Now, when they hover over columns, because they're unsure what properties and types are on columns, they would expect to see the properties of columns.

However, because it isn't naturally expanded all the time, they end up seeing,

SomeMappedType<{ /* properties of myTable that we don't care about */ }>

Definitely not a good user experience. In this case, a fully expanded type is more readable. And so I use the Identity<> trick on SomeMappedType<> to get that behavior.

@aaronjensen
Copy link

Does TableT ever get an alias? Like type CustomerTable = ...? There's lots of things that can cause a type alias to be expanded currently, but if all/most of those were gone, would your user actually end up seeing: columns: SomeMappedType<CustomerTable>? That's readable, though arguably less informative than {foo: string} if that's what it were to expand to. One could then go investigate those two types.

Anyway, I hear you that there are times when more control is desired. This issue is in response to a serious performance impact of the current implementation. My guess is that aliasing more types by default will be a better experience for most, though I can see how a way to opt out could be helpful. I'll be curious to see what the TS team ultimately comes up with.

Thank you for the examples and the discussion!

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Nov 4, 2019

Does TableT ever get an alias? Like type CustomerTable = ...?

Not at the moment. A simple table looks like this,

const myTable = tsql.table("myTable")
    .addColumns({
        testId : tm.mysql.bigIntUnsigned(),
        otherVal : tm.mysql.bigIntUnsigned(),
    })
    .setPrimaryKey(columns => [columns.testId]);

Then, you just use myTable directly and pass it around as an object to other generic functions that take an ITable.

Having type CustomerTable = typeof customerTable wouldn't be helpful in 99% of the cases because that type alias gets ignored when passing customerTable as an object around.


Also, I'm pretty sure TS will expand the type, even if I aliased it, anyway.

const x = {
    a: "",
    b: "",
    c: "",
    d: "",
    e: "",
};

type X = typeof x;

declare function foo<T>(t: T): T;

/*
const y0: {
    a: string;
    b: string;
    c: string;
    d: string;
    e: string;
}
*/
const y0 = foo(x);

/*
const y1: {
    a: string;
    b: string;
    c: string;
    d: string;
    e: string;
}
*/
const y1 = foo<typeof x>(x);

/*
const y2: {
    a: string;
    b: string;
    c: string;
    d: string;
    e: string;
}

Would be nice to have,
const y2 : X;

or something. I don't know.
Even if we did have that, it would require me to explicitly 
pass in `X` as a type parameter, thus losing type inference
and degrading usability.
*/
const y2 = foo<X>(x);

Playground

@aaronjensen
Copy link

Then, you just use myTable directly and pass it around as an object to other generic functions that take an ITable

Got it, thanks. I see similar things with io-ts since you're defining validators at runtime and extracting the types, but you typically pass around the runtime implement.

Also, I'm pretty sure TS will expand the type, even if I aliased it, anyway.

If I understand you correctly, that's part of what this issue is about—preventing the expansion in your last case.

@OliverJAsh
Copy link
Contributor

@DanielRosenwasser Does this cover the use cases I outlined in #32287

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Nov 8, 2019

If I understand you correctly, that's part of what this issue is about—preventing the expansion in your last case.

It is.

However, I'm just throwing it out there that they shouldn't be so aggressive with fixing this issue, that they break my Identity<> hack. Because I have use cases where expanding the type is extremely beneficial to me.

If using the alias Foo of Foo<T> is 100+ lines long, and expanding Foo<T> is 5 lines long, you can bet I'll want to use the Identity<> hack to force TS to expand Foo<T> every time.


The best thing that could happen would be an official way to tell the compiler,

  • "Yes, expand this type. I don't want to see the alias."
  • "No, don't expand this type, use the alias."

@fvilante
Copy link

fvilante commented Dec 20, 2019

@AnyhowStep

I agree with you:

...it's fine to have heuristics to automatically determine how to "present" a type.

However, there needs to be a way for a programmer to step in ...

What do you think about this solution-space:

  1. Programmer mark source-code with special comments which works as instructions to a IDE-plugin specialized in 'type presentation' (a mechanism similar to actual linter tools).
  2. While TS-Server enables this plugin to do his work providing an suitable API.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Dec 20, 2019

The problem with that solution is that it does not help for .d.ts emit and error message elaboration.

Those 2 cases are the main motivators, for me.

See,

#35654

#34556

When .d.ts emit goes wrong and it emits too much, the emitted files become unreadable, or cause emit times to blow up

When it emits too little, it can also impact readability negatively, the compiler needs to put in more work to resolve the type for downstream consumers, increasing check times, causing earlier max depth errors, etc

Error elaborations are also affected by emit being too small, or too large

@DanKaplanSES
Copy link

I'm not understanding the original post so I created a bug workbench with the same code.

Here's a screenshot, including the error:

image

What's the expected result?

//cc @DanielRosenwasser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Type Display Bugs relating to showing types in Quick Info/Tooltips, Signature Help, or Completion Info 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

8 participants