Use the width of a type to add newlines in type assignment error messages

orta commented 3 years ago

Today we ship a one-type-fits-all style for printing type is not assignable to type messages. We'd like to improve this in a pretty simple manner: by occasionally adding newlines. For example with this arbitrary comparison:

let a = { b: { c: { e: { f: 123 } } } };
let b = { b: { c: { e: { f: "123" } } } };
a = b;

Looks like this today:

src/vendor/ata/index.ts(12,1): error TS2322: Type '{ b: { c: { e: { f: string; }; }; }; }' is not assignable to type '{ b: { c: { e: { f: number; }; }; }; }'.
The types of 'b.c.e.f' are incompatible between these types.
    Type 'string' is not assignable to type 'number'.

I propose that because both of the printed types are longer than 20-30 chars, then we switch to:

src/vendor/ata/index.ts(12,1): error TS2322: Type: 
{ b: { c: { e: { f: string; }; }; }; }

is not assignable to type:
{ b: { c: { e: { f: number; }; }; }; }

The types of 'b.c.e.f' are incompatible between these types.
    Type 'string' is not assignable to type 'number'.

Key details

Strip the quotes around the type
Add colons and newlines if the type is over a certain size. Let's call it 30 characters for now, and make it easy to change and we can experiment at the final stages of review.

Some Examples

No changes

let a = 123
let b = "abc"
a = b

Before: src/index.ts(21,1): error TS2322: Type 'string' is not assignable to type 'number'.
After: src/index.ts(21,1): error TS2322: Type 'string' is not assignable to type 'number'.

No change! (Though a part of me does really want to drop the quotes for primitives)

window = {}

Type '{}' is not assignable to type 'Window & typeof globalThis'.
  Type '{}' is missing the following properties from type 'Window': clientInformation, closed, customElements, devicePixelRatio, and 189 more. (2322)

No change! {} and Window & typeof globalThis are too small.

Changing one

window = addEventListener

src/index.ts(1,1): error TS2322: Type '{ <K extends keyof WindowEventMap>(type: K, listener: (this: Window, ev: WindowEventMap[K]) => any, options?: boolean | AddEventListenerOptions | undefined): void; (type: string, listener: EventListenerOrEventListenerObject, options?: boolean | ... 1 more ... | undefined): void; }' is not assignable to type 'Window & typeof globalThis'.
   Type '{ <K extends keyof WindowEventMap>(type: K, listener: (this: Window, ev: WindowEventMap[K]) => any, options?: boolean | AddEventListenerOptions | undefined): void; (type: string, listener: EventListenerOrEventListenerObject, options?: boolean | ... 1 more ... | undefined): void; }' is missing the following properties from type 'Window': HTMLDocument, closed, customElements, devicePixelRatio, and 187 more.

src/vendor/ata/index.ts(12,1): error TS2322: Type: 
{ <K extends keyof WindowEventMap>(type: K, listener: (this: Window, ev: WindowEventMap[K]) => any, options?: boolean | AddEventListenerOptions | undefined): void; (type: string, listener: EventListenerOrEventListenerObject, options?: boolean | ... 1 more ... | undefined): void; }

is not assignable to type:
Window & typeof globalThis

  Type '{ <K extends keyof WindowEventMap>(type: K, listener: (this: Window, ev: WindowEventMap[K]) => any, options?: boolean | AddEventListenerOptions | undefined): void; (type: string, listener: EventListenerOrEventListenerObject, options?: boolean | ... 1 more ... | undefined): void; }' is missing the following properties from type 'Window': HTMLDocument, closed, customElements, devicePixelRatio, and 187 more.

Changing both

Animation = AnalyserNode

index.ts(12,1) Type '{ new (context: BaseAudioContext, options?: AnalyserOptions | undefined): AnalyserNode; prototype: AnalyserNode; }' is not assignable to type '{ new (effect?: AnimationEffect | null | undefined, timeline?: AnimationTimeline | null | undefined): Animation; prototype: Animation; }'.
  Types of property 'prototype' are incompatible.
    Type 'AnalyserNode' is missing the following properties from type 'Animation': currentTime, effect, finished, id, and 18 more.

index.ts(12,1) Type:
{ new (context: BaseAudioContext, options?: AnalyserOptions | undefined): AnalyserNode; prototype: AnalyserNode; }

is not assignable to type:
{ new (effect?: AnimationEffect | null | undefined, timeline?: AnimationTimeline | null | undefined): Animation; prototype: Animation; }

  Types of property 'prototype' are incompatible.
    Type 'AnalyserNode' is missing the following properties from type 'Animation': currentTime, effect, finished, id, and 18 more.

heyAyushh commented 3 years ago

Hey @orta, I am a typescript beginner and want to contribute here, could you guide me where to start?

orta commented 3 years ago

Sure, first read the READMEs/contributing guides - I think all of the changes will probably live inside reportRelationError(message: DiagnosticMessage | undefined, source: Type, target: Type in checker.ts - somewhere around lines 17790.

sourceType and targetType are the strings, you need to use the length of to determine what to do

fatcerberus commented 3 years ago

Though a part of me does really want to drop the quotes for primitives

Then it becomes a garden path sentence:

Type string is not assignable to type number

"What's a type string? Does that mean literal string types? Oh wait, it means the actual type 'string'."

It's better with the quotes.

orta commented 3 years ago

Yeah, exactly, I don't disagree - just pining for a better way to visually distinguish between type and message e.g.

prabhatmishra33 commented 2 years ago

Hello ! @heyAyushh Please let me know if you are not working on this anymore so that I can take this forward 😊

heyAyushh commented 2 years ago

@prabhatmishra33 I think @cytler is already working on it,

ben-m-j-taylor commented 2 years ago

Hi @DanielRosenwasser, I'm just looking at how I might implement a change for this issue and noticed your comment on the PR that was previously opened for this issue.

I wanted to clarify your thoughts on one point that you made.

I think the right fix here is actually to create new diagnostics for specific well-known ones and swap them out; especially since a diagnostic might mention the same type multiple times.

I agree that this seems like a sensible option, however, with the way that the diagnostic message generation works (at least from my current understanding) to introduce a second "pretty" message it would need to be created with a new error code as the generation script will error out if it finds duplicate codes.

Would you be comfortable with introducing a new error code for the "pretty" messages? Personally, that doesn't feel like something that should be done for the sake of introducing some nicer formatting, but I'd be interested to hear your thoughts on this as I might be missing some context/domain knowledge that makes the answer clearer.

DanielRosenwasser commented 2 years ago

Sorry for the delay - yup, adding a new error code is fine.

vansharora03 commented 1 year ago

Hey guys, first time contributor here! Was wondering if I could tackle this issue? It seems stagnant and I am eager to help!

mwaibel-go commented 2 months ago

While this issue is still open, I’d like to point out that the keys in structured types are also unsorted. For instance, I receive the error (formatting by me)

Type
{
  tariffId: FormControl<TariffId>;
  tariffChoiceIds: FormControl<TariffChoiceId[]>;
  startDate: FormControl<ISODate>;
  endDate: FormControl<ISODate | null>;
// lots more
}
is not assignable to type
{
  startDate: FormControl<ISODate>;
  endDate: FormControl<ISODate | null>;
  overlappingPeriod: FormControl<null>;
  tariffId: FormControl<TariffId>;
// Lots more
}

It would help if the keys were in the same order.

RaghavBhat02 commented 2 months ago

is this issue still open?

microsoft / TypeScript