Weak type detection

[sandersn]

Fixes #7485

Builds on #3842 by correctly handling intersections that contain weak types but may not necessarily be weak themselves. I ran the change on our real-world code corpus and on DefinitelyTyped and verified that it doesn't cause a huge number of breaks.

Weak types are

1. Object types with at least one property
2. Where all properties are optional
3. And that do not have a string index signature, number index signature, call signature or construct signature.

Weak types are common in Javascript, but the compiler allows almost anything to be assigned to them. The compiler misses a lot of obvious errors when you pass values to weakly typed parameters, implement weakly typed interfaces, or even cast to weak types. Here is an example of a weak type:

interface Options {
  flag?: boolean
  maximum?: number
  filename?: string
}

The weak type check occurs when the target is a weak type and the source has at least one property, call signature or construct signature. The weak type check issues an error if the source does not have any of the properties of the target. This is no longer allowed:

declare function setup(options: Options): void
const payload = {
  id = 1
  name = "foo.txt"
  data = JSON.stringify(something)
}
setup(payload) // weak type error!

Intersection types are handled specially as weak type targets. Individual constituents of an intersection are exempt from the weak type check. However, if all constituents of an intersection are weak, then the weak type check uses the combined properties of the intersection as the target.

interface UIOptions {
  greener?: boolean
  green?: number
}
type AllOptions = Options & UIOptions
declare function setupUI(options: AllOptions): void
setup(payload) // still not allowed!

The global object type is handled specially as a weak type source. It is not an error to assign Object to a weak type, even if they don't share any properties. That's because people think of Object as similar to {}, even though it actually includes a number of properties.

This PR also improves errors for JSX attributes, which previously implemented their own weak type detection. It also improves subtype reduction because a weak type is no longer treated as a universal supertype like {} is.

Edit: Update rules after some bug fixes.

[sandersn]

The Travis failure occurs because of an error in react.d.ts, which our tests have a copy of. Component implements a weak type named ComponentLifecycle but fails to include any of the weak type's methods. Its intent is to inherit from ComponentLifecycle so that subclasses of Component will be forced to correctly override these methods. I'll send a PR to definitely typed tomorrow to fix the official version and also update this PR.

[RyanCavanaugh]

Component implements a weak type named ComponentLifecycle but fails to include any of the weak type's methods.

Where's the head-desk emoji when you need it

[sandersn]

The weird test changes in controlFlowInstanceOf, subTypeReduction* and generatorTypeCheck63 are because subtype reduction has changed. { foo?: Foo } was previously a supertype of { bar: Bar } by virtue of being a weak type, and of course {} is the supertype of all object types. With this PR, weak types are only supertypes of object types that share at least one property with them.

So unions produce a different type when asked to do subtype reduction:

{ foo?: Foo } | { bar: Bar }
//   used to reduce to
{ foo?: Foo }
// now it does not!

I don't think it was supposed to work this way according to the spec, so this is an improvement.

[DanielRosenwasser]

Comment on what you're doing here and below - the intent is obvious now, but it won't be to a random team member in 3 months. 😄

[sandersn]

I refactored this into a separate function and a named boolean. I think the two new names should serve as adequate documentation.

[sandersn]

Summary of Real World Code run

There aren't as many errors as I thought -- 9, after the React fix in DefinitelyTyped. They are all very good errors except one questionable __metadata pattern. Overall, the RWC run indicates that this is a good change.

I will look at the errors from DefinitelyTyped next.

Good

Misleading names

There are two kinds of misleading names:

1. Wishful thinking: surely IHeaderViewOptions is assignable to IViewOptions, right?
2. Namespaces: I bet Common.Controls.Legacy.ITreeDataItem is assignable to Common.Controls.Legacy.Grid.ITreeDataItem or at least MyProject.ITreeDataItem, right?

These are really hard to catch without the aid of a compiler. And Typescript wasn't checking assignability at all for weak targets. Now it catches the most egregious errors.

This change found one instance of wishful thinking in VS Code and one instance of the namespace problem in another internal Microsoft project. (Who else would have four layers of legacy controls? (Don't answer that.))

Confusing types

There are plenty of types which aren't assignable but look like they might be at first glance. For example: IJSONSchema | IJSONSchema[] to IJSONSchema.

The compiler now catches more of these. It found 2 in VS Code, and 2 in 2 other internal Microsoft projects.

Misleading implements

See above for discussion of the React ComponentLifecycle error. Two projects had this error, which is now fixed in DefinitelyTyped.

Total Spaceship Guy

There are plenty of incorrect assignments that are obvious if you ever notice them. Things like string[] to IJSONSchema, or Promise<IBranch> to IBranch (both from VS Code).

The compiler found 3 of these in VS Code and 1 in another Microsoft project.

Ugly

Another internal MS project had a nearly-empty type called Target with a single optional property named __metadata. I guess that Target is intended to mean that nearly any type in the project might have a __metadata property with something interesting on it. And before this change, you could cast anything to Target and check whether __metadata actually exists.

Bad

There were no bad errors!

Fixes

• Misleading names and totally wrong assignments are both easy to fix.
• Confusing types can be easy to fix if you know the codebase, but otherwise might lead to hours of changing types here and there until the project compiles again.
• Misleading implements are not obvious to fix, but I think that the fix I ended up using for React will always work: remove the implements from the class and instead merge it with an interface that extends the previously-implemented interface.
• I think the fix for the common __metadata type is similar: lots of classes in the project need to merge with an interface that extends the __metadata type in order to be assignable to it.

[sandersn]

Summary of DefinitelyTyped run

Again, there are not as many errors as I thought. Of 3095 projects total, there were 20 errors in definition files and about 80 errors in test files. I haven't looked at the test files yet, but I didn't find any new error patterns in the definitions. Here's the breakdown:

Good

Misleading names

Wishful thinking: kendo-ui (3 times), react-leaflet, react-native

Misleading implements

bookshelf, mongoose, js-yaml, mapbox-gl, polymer

Totally wrong

react-mdl (10 times). This is result of a class declaration that is extended 10 different times.

[RyanCavanaugh]

Fantastic. Those are much better results than I expected 👍

[sandersn]

Yes, there are few enough errors that I am going to go fix up DefinitelyTyped right now.

Note that the Ugly error pattern is very similar to one in the compiler I had to fix in this PR. SymbolLinks is a weak type so you can cast anything to it. getRootSymbols checks whether a Symbol is a TransientSybmol, then casts Symbol to SymbolLinks. But Symbol doesn't inherit from SymbolLinks. TransientSymbol does, so the fix is to cast to TransientSymbol instead. It's likely that the other MS project has a similar simple fix.

[mihailik]

What is the canonical recommended way of solving React-like false error?

I.E. base class with all methods optional, derived class doesn't implement any of those optional methods.

[sandersn]

@mihailik class-interface merging. Remove the implements clause from the class declaration and add a line preceding it:

interface C extends Optional {}

[xogeny]

So I thought this would get resolved upstream in @blueprintjs/core. But I see that it has been bumped down the list of milestones quite a ways now (palantir/blueprint#1237, https://github.com/palantir/blueprint/milestone/26).

I don't really want to wait a month for this, so I'm trying to figure out a way to deal with this in the meantime. What I don't understand about this myThing as WeakType workaround is how does it apply to existing libraries. When I compile against 1.21.0 of @blueprintjs/core, I get this error:

node_modules/@blueprintjs/core/dist/components/collapse/collapse.d.ts(46,32): error TS2559: Type '{ className: string; s
tyle: { height: string; overflowY: string; transition: string; }; }' has no properties in common with type 'DOMAttribute
s<Element>'.

How does casting help me. This is an error in the typings themselves, not in my code. I'm guessing that in order to get this to compile, I'm going to have to go in and modify collapse.d.ts manually for every install until the Blueprint team fixes this issue and publishes a new version. Somebody please tell me I'm missing something here...?

[sandersn]

This is actually a problem with React.DOMElement. It says that its first type argument must be a DOMAttributes, but in actual usage, people always pass either HTMLAttributes or SVGAttributes. Line 46 in collapse.d.ts passes HTMLAttributes. The fix is to change the type paramter constraint in React.DOMElement. I'll do that and add this test case to @types/react so that after it's merged you can just npm install to pull it in.

[sandersn]

DefinitelyTyped/DefinitelyTyped#17581 will almost fix this. Unfortunately, the improved types expose another bug in collapse.d.ts: its overflowY is just string, but needs to be "auto" | ... to match the actual type in HTMLAttributes.

This is a much simpler fix, though. I'll update palantir/blueprint#1237.

[levenleven]

Shouldn't this also prevent passing primitives as a parameter?

setup(1); // no error

I know I can fix this by defining setup as function setup(options: Options & object) { }, but maybe it should be a "default"?

[sandersn]

@levenleven #16343 should have fixed this, but I don't think it shipped in the RC. Are you running 2.4.1 (the official release) or 2.4.0 (the RC).

[levenleven]

@sandersn Ok, thanks. Tried in playground, not sure which version is running there.

[sandersn]

Oops, I think the playground may still be running 2.3. @DanielRosenwasser did you update to 2.4 yet? Is it possible to add the version in small print when you do?

[masaeedu]

I would like to define a type for "non-thenables". I was assuming the following would work:

type NotThenable = { then?: never };

const x: NotThenable = 10

However I get the "has no properties in common with" type error. After this change, how do I define a type for all values that do not have a then property?

[RyanCavanaugh]

@masaeedu

type NotThenable = { then?: never; [others: string]: any } | number | string | boolean | symbol;

const x: NotThenable = 10;
const x1: NotThenable = {};
const x2: NotThenable = { m: 10 };
const x3: NotThenable = { then: () => null };

[masaeedu]

@RyanCavanaugh I was assuming | number | string | boolean | symbol | etc. is equivalent to {}. Does this mean {} is no longer the top type?

[RyanCavanaugh]

I don't understand the question. What is etc? Clearly {} isn't the same as { then?: never; [others: string]: any }