GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Weak type detection by sandersn · Pull Request #16047 · microsoft/TypeScript

Conversation

@sandersn

@sandersn Nathan Shively-Sanders (sandersn) commented May 23, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jwbay, yuit, RichiCoder1, HerringtonDarkholme, kitsonk, ds300, cartant, zernie, and niieani reacted with thumbs up emoji jwbay, HerringtonDarkholme, federicoce-moravia, kitsonk, and ds300 reacted with hooray emoji jwbay, HerringtonDarkholme, vladimir-tikhonov, pablobirukov, ds300, zernie, and KonanMentor reacted with heart emoji
All reactions
  • 👍 9 reactions
  • 🎉 5 reactions
  • ❤️ 7 reactions
Ryan Cavanaugh (RyanCavanaugh) and others added 9 commits July 9, 2015 21:44
@RyanCavanaugh
Implementation for weak types
bdcc4eb
@RyanCavanaugh
Baselines for weak types
3c76c3e
@sandersn
Merge branch 'master' into weakType
beba5de
It's probably fine!
@sandersn
Do not report weak type errors for intersections
e570775
TODO: Report weak type errors for intersections *correctly*. That part's
not done yet.

Also, fix a couple of errors found by this change.
@sandersn
Merge branch 'master' into weakType
c583c32
@sandersn
Detect weak type correctly for intersection types
396071b
Plus add an intersection test case.
@sandersn
Merge branch 'master' into weakType
49d4aca
@sandersn
Remove mistakenly commited auto-generated file
2433f56
@sandersn
Update tests
c9da705
1. Accept baselines
2. Fix APISample_watcher
3. Switch checker to use JSDOc for isWeak

Some of the baselines seem incorrect or weirder than before.
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label May 23, 2017
@sandersn

Nathan Shively-Sanders (sandersn) commented May 23, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented May 24, 2017

Copy link
Copy Markdown
Member

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

aluanhaddad and SlurpTheo reacted with laugh emoji
All reactions
  • 😄 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Nathan Shively-Sanders (sandersn) added 3 commits May 24, 2017 12:48
@sandersn
Fix react tests w/incorrect weak type assignments
463e385
This even includes react.d.ts itself!
@sandersn
Update baselines
4bab55f
@sandersn
Fix lint
4b1f1b6
@sandersn

Nathan Shively-Sanders (sandersn) commented May 24, 2017

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sandersn
Improve comments in source and tests
0b911d5
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed May 25, 2017
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
result &= related;
}
dynamicDisableWeakTypeErrors = saveDynamicDisableWeakTypeErrors;
if (source !== globalObjectType && getPropertiesOfType(source).length > 0 && every(target.types, isWeak)) {

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) May 25, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sandersn Nathan Shively-Sanders (sandersn) May 25, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sandersn
Improve naming and refactor for readability
d1d487c
@sandersn

Nathan Shively-Sanders (sandersn) commented May 25, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sandersn

Nathan Shively-Sanders (sandersn) commented May 25, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented May 25, 2017

Copy link
Copy Markdown
Member

Fantastic. Those are much better results than I expected 👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sandersn

Nathan Shively-Sanders (sandersn) commented May 25, 2017

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Jessidhia Jessica Franco (Jessidhia) mentioned this pull request May 25, 2017
Preferred way of defining objects with all optional members? DefinitelyTyped/DefinitelyTyped#16738
Closed
1 task done
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request May 26, 2017
Partial<T> accepts arbitrary Function #13132
Closed
Nathan Shively-Sanders (sandersn) added 2 commits May 26, 2017 10:19
@sandersn
Weak types must not have call or construct sigs
548f92a
This changes the definition of weak types
@sandersn
Re-enable weak type check for intersection props
f9a05a1
Previously, intersections disabled the weak type check for their
constituents, and all properties (recursively) of their constituents.

Also add test for this case.
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request May 26, 2017
Design Meeting Notes, 5/26/2017 #16114
Closed
@mihailik

mihailik commented May 30, 2017

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sandersn

Nathan Shively-Sanders (sandersn) commented May 30, 2017

Copy link
Copy Markdown
Member Author

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

interface C extends Optional {}
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost ghost mentioned this pull request Jun 28, 2017
TypeScript 2.4.1 - TS2345 - "Property 'push' is missing in type #16802
Closed
@xogeny

Michael Tiller (xogeny) commented Jun 28, 2017

Copy link
Copy Markdown

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...?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sandersn

Nathan Shively-Sanders (sandersn) commented Jun 28, 2017

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sandersn

Nathan Shively-Sanders (sandersn) commented Jun 28, 2017

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sandersn Nathan Shively-Sanders (sandersn) mentioned this pull request Jun 28, 2017
Fix collapse.tsx for TS 2.4 palantir/blueprint#1296
Merged
@levenleven

Aleksey Levenstein (levenleven) commented Jul 4, 2017

Copy link
Copy Markdown

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"?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kitsonk Kitson Kelly (kitsonk) mentioned this pull request Jul 5, 2017
ProxyHandler no longer implementable in TS 2.4 #16933
Closed
@sandersn

Nathan Shively-Sanders (sandersn) commented Jul 6, 2017

Copy link
Copy Markdown
Member Author

Aleksey Levenstein (@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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@luke-john Luke John (luke-john) mentioned this pull request Jul 7, 2017
Partials accept functions #17003
Closed
@kitsonk Kitson Kelly (kitsonk) mentioned this pull request Jul 7, 2017
TS 2.4.1 flags legal code as invalid? #17000
Closed
@levenleven

Aleksey Levenstein (levenleven) commented Jul 7, 2017

Copy link
Copy Markdown

Nathan Shively-Sanders (@sandersn) Ok, thanks. Tried in playground, not sure which version is running there.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sandersn

Nathan Shively-Sanders (sandersn) commented Jul 7, 2017

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) mentioned this pull request Jul 7, 2017
type check issue #16986
Closed
@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) mentioned this pull request Aug 7, 2017
Weak type detection only works with object literals being passed #17656
Closed
This was referenced Aug 21, 2017
knuddels-userapps-api: Fix test DefinitelyTyped/DefinitelyTyped#19192
Merged
plotly.js: Fix test DefinitelyTyped/DefinitelyTyped#19193
Merged
react-virtualized-select: Fix compile errors DefinitelyTyped/DefinitelyTyped#19194
Merged
baidumap-web-sdk: Fix weak type errors DefinitelyTyped/DefinitelyTyped#19196
Merged
merge2: Fix compile error, clean up types DefinitelyTyped/DefinitelyTyped#19207
Merged
leaflet-draw: Fix test DefinitelyTyped/DefinitelyTyped#19273
Closed
kendo-ui: Fix compile errors DefinitelyTyped/DefinitelyTyped#19274
Merged
i18next/v2: Fix test DefinitelyTyped/DefinitelyTyped#19282
Merged
@ghost ghost mentioned this pull request Aug 30, 2017
Excess properties are not checked e.g. in callbacks #7547
Closed
@ghost ghost mentioned this pull request Sep 7, 2017
react-navigation: Fix compile errors DefinitelyTyped/DefinitelyTyped#19610
Merged
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Jan 4, 2018
Partial<T> where T has only optional members should not yield ... has no properties in common with type 'Partial<...>'error #20999
Closed
@masaeedu

Asad Saeeduddin (masaeedu) commented Apr 11, 2018

Copy link
Copy Markdown
Contributor

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented Apr 11, 2018

Copy link
Copy Markdown
Member

Asad Saeeduddin (@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 };
xogeny and SlurpTheo reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@masaeedu

Asad Saeeduddin (masaeedu) commented Apr 11, 2018

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented Apr 11, 2018

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser left review comments
@ahejlsberg Anders Hejlsberg (ahejlsberg) ahejlsberg left review comments
+1 more reviewer
@yuit Yui (yuit) yuit left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

All Optional Object Interface Converts to any

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@sandersn @RyanCavanaugh @mihailik @xogeny @kitsonk @levenleven @masaeedu @yuit @DanielRosenwasser @ahejlsberg @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.