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 — Covariant checking for callback parameters by ahejlsberg · Pull Request #15104 · microsoft/TypeScript

Conversation

@ahejlsberg

@ahejlsberg Anders Hejlsberg (ahejlsberg) commented Apr 10, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

In order to ensure that any generic type Foo<T> is at least co-variant with respect to T no matter how Foo uses T, TypeScript relates parameters bi-variantly (given that parameters are input positions, they naturally relate only contra-variantly). However, when source and target parameters both have function types with a single call signature, we know we are relating two callback parameters. In that case it is sufficient to only relate the parameters of the signatures co-variantly because, similar to return values, callback parameters are output positions. With this PR we introduce that change. This means that a Promise<T> or Observable<T>, where T is used only in callback parameter positions, will be co-variant (as opposed to bi-variant) with respect to T, which solves a commonly reported issue.

This change finds several issues in our real world code suites that all appear to be legitimate inconsistencies.

Fixes #11022.
Fixes #14770.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zpdDG4gta8XKpMCd, whymarrh, Igorbek, schotime, aluanhaddad, trotyl, alitaheri, dead-claudia, svieira, gcnew, and 17 more reacted with thumbs up emoji zpdDG4gta8XKpMCd, whymarrh, schotime, Jessidhia, aluanhaddad, trotyl, alitaheri, svieira, willisplummer, tsofist, and 10 more reacted with hooray emoji zpdDG4gta8XKpMCd, whymarrh, schotime, aluanhaddad, trotyl, alitaheri, remojansen, sledorze, acutmore, sean-vieira, and 4 more reacted with heart emoji
All reactions
  • 👍 27 reactions
  • 🎉 20 reactions
  • ❤️ 14 reactions
Anders Hejlsberg (ahejlsberg) added 7 commits April 8, 2017 12:53
@ahejlsberg
Treat callback parameters as strictly covariant
8ae3e05
@ahejlsberg
Fix callback types to match covariantly
eebd67f
@ahejlsberg
Accept new baselines
11543d7
@ahejlsberg
Add tests
50f84b1
@ahejlsberg
Check callback return values bi-variantly
189fc51
@ahejlsberg
Accept new baselines
33bc9d1
@ahejlsberg
Add another test case
510bc81
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Apr 10, 2017
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Apr 10, 2017
Proposal: covariance and contravariance generic type arguments annotations #10717
Closed
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) changed the title Covariant callbacks Covariant checking for callback parameters Apr 10, 2017
Anders Hejlsberg (ahejlsberg) added 2 commits April 10, 2017 17:14
@ahejlsberg
Allow callbacks unioned with null and/or undefined
ec35b80
@ahejlsberg
Update tests
3cda0ea
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Apr 15, 2017

Copy link
Copy Markdown
Member Author

Mohamed Hegazy (@mhegazy) Want to take a look?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhegazy
Mohamed Hegazy (mhegazy) approved these changes Apr 18, 2017
View reviewed changes
@mhegazy Mohamed Hegazy (mhegazy) added the Breaking Change Would introduce errors in existing code label Apr 18, 2017
@mhegazy Mohamed Hegazy (mhegazy) added this to the TypeScript 2.3.1 milestone Apr 18, 2017
@ahejlsberg
Anders Hejlsberg (ahejlsberg) merged commit cef42c9 into master Apr 18, 2017
@ahejlsberg
Anders Hejlsberg (ahejlsberg) deleted the covariantCallbacks branch April 18, 2017 18:35
This was referenced Apr 19, 2017
Promises of different types should not be compatible #9953
Closed
selenium-webdriver: Fix Promise errors DefinitelyTyped/DefinitelyTyped#15989
Merged
@mjbvz Matt Bierner (mjbvz) mentioned this pull request Apr 24, 2017
Regression on Promise Type Checking in Latest Insiders #15352
Closed
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Apr 25, 2017
Revert #15104 #15381
Merged
Mohamed Hegazy (mhegazy) added a commit that referenced this pull request Apr 25, 2017
@mhegazy
Merge pull request #15381 from Microsoft/revert15104
9ada915 
Revert #15104
@mhegazy Mohamed Hegazy (mhegazy) modified the milestones: TypeScript 2.4, TypeScript 2.3.1 Apr 26, 2017
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request May 3, 2017
Incorrect typecheck on generic Promise<T> with inheritance #15564
Closed
@acutmore

Ashley Claymore (acutmore) commented May 5, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Shawn Talbert (@ShawnTalbert) If not clear. This PR does not introduce any new syntax. Instead changes type assignability of generics where the type is only user in a function parameter position e.g Promise / Observable

interface Box<T> {
   foo(cb: (t: T => void)): void;
}

Before:

declare const bA: Box<Animal>;
const bC: Box<Cat> = bA; // no error 😿

After:

declare const bA: Box<Animal>;
const bC: Box<Cat> = bA; // error 😺
All reactions

Sorry, something went wrong.

Uh oh!

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

Shawn Talbert (ShawnTalbert) commented May 6, 2017

Copy link
Copy Markdown

Yes, wrong issue, sorry
All reactions

Sorry, something went wrong.

Uh oh!

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

Brian McBarron (bmcbarron) commented May 8, 2017

Copy link
Copy Markdown

It appears that the solution was amended to ignore union with null/undefined. Should that be dependent on --strictNullChecks? At the moment, this does not seem to address #13513.
All reactions

Sorry, something went wrong.

Uh oh!

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

Anders Hejlsberg (ahejlsberg) commented May 8, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Brian McBarron (@bmcbarron) #13513 is fixed by this PR in --strictNullChecks mode. Without --strictNullChecks, undefined is assignable to any type and is effectively ignored in union types, but that has always been the case and isn't going to change.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request May 9, 2017
Weird checking errors when using async function, and generic typings #15702
Closed
@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) mentioned this pull request May 15, 2017
Async function not type checking correctly #15830
Closed
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request May 18, 2017
Typescript fails to throw error when a generic is assigned to another generic of whom it's generic type is a superset #13970
Closed
This was referenced May 30, 2017
Add flag to allow bivariant callback parameter comparison #16150
Closed
Complain expected when assigning Promise<A | B> to Promise<A> #16202
Closed
This was referenced Jun 8, 2017
d3-zoom: Add needed type arguments DefinitelyTyped/DefinitelyTyped#17053
Merged
observe-js: Fix type of ArrayObserver_instance DefinitelyTyped/DefinitelyTyped#17056
Merged
passport-local-mongoose: Fix error in typescript@next DefinitelyTyped/DefinitelyTyped#17058
Merged
restify: Fix error in typescript@next DefinitelyTyped/DefinitelyTyped#17061
Merged
stale-lru-cache: Fix error in typescript@next DefinitelyTyped/DefinitelyTyped#17063
Merged
@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) mentioned this pull request Jun 8, 2017
Type checking is not working properly with promises. #16369
Closed
@SlurpTheo SlurpTheo mentioned this pull request Jun 15, 2017
'Discarding Parameters' plus 'Duck Typing' makes every parameter optional? #9352
Closed
This was referenced Jun 16, 2017
core-js: Fix tests that fail due to callback covariance DefinitelyTyped/DefinitelyTyped#17246
Merged
es6-shim: Fix tests that fail due to callback covariance DefinitelyTyped/DefinitelyTyped#17247
Merged
kefir: Use never instead of void for Observable type arguments DefinitelyTyped/DefinitelyTyped#17248
Merged
bittorrent-protocol: Use same default event handler as stream.Readable DefinitelyTyped/DefinitelyTyped#17262
Merged
@HerringtonDarkholme Herrington Darkholme (HerringtonDarkholme) mentioned this pull request Jun 28, 2017
Generic type parameter in plain function is checked contravariantly. #16797
Closed
@kitsonk Kitson Kelly (kitsonk) mentioned this pull request Jun 28, 2017
TS 2.4.1: ReadonlyArray<Identifiable> not assignable to type ReadonlyArray<T> even though <T extends Identifiable> #16805
Closed
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Jul 3, 2017
Possible bug when typing a function that receives a function and its parameters #16871
Closed
@ghost ghost mentioned this pull request Jul 7, 2017
Fix TS 2.4 PromiseLike Errors With Async in VSCode Codebase microsoft/vscode#30216
Closed
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Aug 15, 2017
Generic with extends and return will fail compilation #17794
Closed
@schotime

Adam Schroder (schotime) commented Aug 17, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

This doesn't fix this problem unfortunately and this is usually the case if the callbacks are stored in different files and imported.

class Animal {}
class Cat extends Animal {
    public meow() {}
}

let promise: Promise<Animal> = null;
promise.then((cat: Cat) => { //Should error here but does not
  cat.meow();
});
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost ghost mentioned this pull request Sep 22, 2017
[ts] Cannot find module '@angular/core' microsoft/vscode#34681
Closed
This was referenced Oct 5, 2017
strictFunctionTypes has different behavior with parameter types and return types #18963
Closed
Strictly check callback parameters #18976
Merged
Fix checking of recursive callback types #19107
Merged
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Feb 2, 2018
polymorphic arguments validation error #20268
Closed
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@mhegazy Mohamed Hegazy (mhegazy) mhegazy approved these changes
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

Breaking Change Would introduce errors in existing code

Projects

None yet

Milestone

TypeScript 2.4

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

7 participants

@ahejlsberg @acutmore @ShawnTalbert @bmcbarron @schotime @mhegazy @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.