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

Destructuring function overloads with strictNullChecks produces invalid .d.ts #10078

Closed
rkirov opened this Issue Aug 1, 2016 · 7 comments

Comments

Projects
None yet
10 participants
@rkirov
Contributor

rkirov commented Aug 1, 2016

TypeScript Version: Version 2.1.0-dev.20160801

Code

class A {
    f({x}: {x?: boolean} = {}) {};
}
class B extends A {
    f({x, y}: {x?: boolean, y?: boolean} = {}) {};
}

with strictNullChecks the code is accepted and the following .d.ts produced

declare class A {
    f({x}?: {
        x?: boolean;
    }): void;
}
declare class B extends A {
    f({x, y}?: {
        x?: boolean;
        y?: boolean;
    }): void;
}

Expected behavior:
Since the original code is accepted, the .d.ts file should also be accepted.

Actual behavior:
When passed to tsc with strictNullChecks the .d.ts in question produces the error

test.d.ts(7,8): error TS2459: Type '{ x?: boolean | undefined; y?: boolean | undefined; } | undefined' has no property 'x' and no string index signature.
test.d.ts(7,11): error TS2459: Type '{ x?: boolean | undefined; y?: boolean | undefined; } | undefined' has no property 'y' and no string index signature.

Confusingly, if you removed the inheritance the signature for B is accepted.

In general, I am surprised that TypeScript keeps any mention of destructuring in the .d.ts. In my mind that is part of the implementation of the function and has no relevance to the function signature. What would happen if TS plainly emits - f(a?: {x?: boolean;}): void;.

@ahejlsberg

This comment has been minimized.

Show comment
Hide comment
@ahejlsberg

ahejlsberg Aug 1, 2016

Member

Definitely looks like a bug.

In general, I am surprised that TypeScript keeps any mention of destructuring in the .d.ts. In my mind that is part of the implementation of the function and has no relevance to the function signature. What would happen if TS plainly emits - f(a?: {x?: boolean;}): void;.

I actually agree, and that was our original intent. However, a key issue is that we have no good parameter name to substitute in place of a destructuring pattern. You'd end up with auto-generated names like _1 or p1 instead of the arguably more informative {x, y}.

Member

ahejlsberg commented Aug 1, 2016

Definitely looks like a bug.

In general, I am surprised that TypeScript keeps any mention of destructuring in the .d.ts. In my mind that is part of the implementation of the function and has no relevance to the function signature. What would happen if TS plainly emits - f(a?: {x?: boolean;}): void;.

I actually agree, and that was our original intent. However, a key issue is that we have no good parameter name to substitute in place of a destructuring pattern. You'd end up with auto-generated names like _1 or p1 instead of the arguably more informative {x, y}.

@ahejlsberg ahejlsberg added the Bug label Aug 1, 2016

@ahejlsberg ahejlsberg added this to the TypeScript 2.0.1 milestone Aug 1, 2016

@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Aug 2, 2016

Member

@sheetalkamat can you take a look? Thanks!

Member

RyanCavanaugh commented Aug 2, 2016

@sheetalkamat can you take a look? Thanks!

sheetalkamat added a commit that referenced this issue Aug 8, 2016

sheetalkamat added a commit that referenced this issue Aug 8, 2016

Cache in the type of binding pattern without optionality to use it wh…
…en deciding type for binding element in it

Fixes #10078

@mhegazy mhegazy modified the milestones: TypeScript 2.1, TypeScript 2.0.1 Aug 22, 2016

rkirov added a commit to rkirov/angular that referenced this issue Sep 2, 2016

Fixes to support strictNullChecks for ngc emit.
Change __private_types fields form foo?: T, to foo: T, otherwise
typeof foo would be T | undefined.

Due to TS bug Microsoft/TypeScript#10078
some types have to be temporary set to any in order to produce
valid .d.ts files.

rkirov added a commit to rkirov/angular that referenced this issue Sep 2, 2016

Fixes needed to support strictNullChecks for ngc emit.
Change __private_types__ fields from `foo?: T`, to `foo: T`, otherwise
typeof foo would be `T | undefined`.

Due to TS bug Microsoft/TypeScript#10078
some types have to be temporary set to `any` in order to produce
valid .d.ts files.

rkirov added a commit to rkirov/angular that referenced this issue Sep 8, 2016

Fixes needed to support strictNullChecks for ngc emit.
Change __private_types__ fields from `foo?: T`, to `foo: T`, otherwise
typeof foo would be `T | undefined`.

Due to TS bug Microsoft/TypeScript#10078
some types have to be temporary set to `any` in order to produce
valid .d.ts files.

@mhegazy mhegazy modified the milestones: TypeScript 2.1, TypeScript 2.1.2, Future Oct 27, 2016

@feng-xiao

This comment has been minimized.

Show comment
Hide comment
@feng-xiao

feng-xiao Mar 1, 2017

I'm using TS 2.21, when StrickNullChecks turned on, same thing happened to angular 4.0rc1 in file @angular\forms\typings\src\model.d.ts. TypeScript generates following code that causes compile error TS2459 (has no property 'x' and no string index signature)
reset(formState?: any, {onlySelf, emitEvent}?: { onlySelf?: boolean; emitEvent?: boolean; }):

feng-xiao commented Mar 1, 2017

I'm using TS 2.21, when StrickNullChecks turned on, same thing happened to angular 4.0rc1 in file @angular\forms\typings\src\model.d.ts. TypeScript generates following code that causes compile error TS2459 (has no property 'x' and no string index signature)
reset(formState?: any, {onlySelf, emitEvent}?: { onlySelf?: boolean; emitEvent?: boolean; }):

matsko added a commit to angular/angular that referenced this issue May 4, 2017

@mhegazy mhegazy modified the milestones: TypeScript 2.4, TypeScript 2.5 Jun 5, 2017

alxhub added a commit to alxhub/angular that referenced this issue Jun 9, 2017

fix(http): move destructuring inside {Request,Response}Options ctor
Previously the RequestOptions/ResponseOptions classes had constructors
with a destructured argument hash (represented by the
{Request,Response}OptionsArgs type). This type consists entirely of
optional members.

This produces a .d.ts file which includes the constructor declaration:

constructor({param, otherParam}?: OptionsArgs);

However, this declaration doesn't type-check properly. TypeScript
determines the actual type of the hash parameter to be OptionsArgs | undefined,
which it then concludes does not have a `param` or `otherParam` member.

This is a bug in TypeScript ( Microsoft/TypeScript#10078 ).
As a workaround, destructuring is moved inside the method, where it does not produce
broken artifacts in the .d.ts.

Fixes #16663.

alxhub added a commit to angular/angular that referenced this issue Jun 9, 2017

fix(http): move destructuring inside {Request,Response}Options ctor
Previously the RequestOptions/ResponseOptions classes had constructors
with a destructured argument hash (represented by the
{Request,Response}OptionsArgs type). This type consists entirely of
optional members.

This produces a .d.ts file which includes the constructor declaration:

constructor({param, otherParam}?: OptionsArgs);

However, this declaration doesn't type-check properly. TypeScript
determines the actual type of the hash parameter to be OptionsArgs | undefined,
which it then concludes does not have a `param` or `otherParam` member.

This is a bug in TypeScript ( Microsoft/TypeScript#10078 ).
As a workaround, destructuring is moved inside the method, where it does not produce
broken artifacts in the .d.ts.

Fixes #16663.

alxhub added a commit to alxhub/angular that referenced this issue Jun 12, 2017

fix: argument destructuring sometimes breaks strictNullChecks
Destructuring of the form:

function foo({a, b}: {a?, b?} = {})

breaks strictNullChecks, due to the TypeScript bug Microsoft/TypeScript#10078.
This change eliminates usage of destructuring in function argument lists in cases where it would leak
into the public API .d.ts.

alxhub added a commit to alxhub/angular that referenced this issue Jun 16, 2017

fix: argument destructuring sometimes breaks strictNullChecks
Destructuring of the form:

function foo({a, b}: {a?, b?} = {})

breaks strictNullChecks, due to the TypeScript bug Microsoft/TypeScript#10078.
This change eliminates usage of destructuring in function argument lists in cases where it would leak
into the public API .d.ts.

hansl added a commit to angular/angular that referenced this issue Jun 20, 2017

fix: argument destructuring sometimes breaks strictNullChecks
Destructuring of the form:

function foo({a, b}: {a?, b?} = {})

breaks strictNullChecks, due to the TypeScript bug Microsoft/TypeScript#10078.
This change eliminates usage of destructuring in function argument lists in cases where it would leak
into the public API .d.ts.

hansl added a commit to angular/angular that referenced this issue Jun 20, 2017

fix: argument destructuring sometimes breaks strictNullChecks
Destructuring of the form:

function foo({a, b}: {a?, b?} = {})

breaks strictNullChecks, due to the TypeScript bug Microsoft/TypeScript#10078.
This change eliminates usage of destructuring in function argument lists in cases where it would leak
into the public API .d.ts.

asnowwolf added a commit to asnowwolf/angular that referenced this issue Aug 11, 2017

asnowwolf added a commit to asnowwolf/angular that referenced this issue Aug 11, 2017

fix(http): move destructuring inside {Request,Response}Options ctor
Previously the RequestOptions/ResponseOptions classes had constructors
with a destructured argument hash (represented by the
{Request,Response}OptionsArgs type). This type consists entirely of
optional members.

This produces a .d.ts file which includes the constructor declaration:

constructor({param, otherParam}?: OptionsArgs);

However, this declaration doesn't type-check properly. TypeScript
determines the actual type of the hash parameter to be OptionsArgs | undefined,
which it then concludes does not have a `param` or `otherParam` member.

This is a bug in TypeScript ( Microsoft/TypeScript#10078 ).
As a workaround, destructuring is moved inside the method, where it does not produce
broken artifacts in the .d.ts.

Fixes #16663.

asnowwolf added a commit to asnowwolf/angular that referenced this issue Aug 11, 2017

fix: argument destructuring sometimes breaks strictNullChecks
Destructuring of the form:

function foo({a, b}: {a?, b?} = {})

breaks strictNullChecks, due to the TypeScript bug Microsoft/TypeScript#10078.
This change eliminates usage of destructuring in function argument lists in cases where it would leak
into the public API .d.ts.

juleskremer added a commit to juleskremer/angular that referenced this issue Aug 28, 2017

juleskremer added a commit to juleskremer/angular that referenced this issue Aug 28, 2017

fix(http): move destructuring inside {Request,Response}Options ctor
Previously the RequestOptions/ResponseOptions classes had constructors
with a destructured argument hash (represented by the
{Request,Response}OptionsArgs type). This type consists entirely of
optional members.

This produces a .d.ts file which includes the constructor declaration:

constructor({param, otherParam}?: OptionsArgs);

However, this declaration doesn't type-check properly. TypeScript
determines the actual type of the hash parameter to be OptionsArgs | undefined,
which it then concludes does not have a `param` or `otherParam` member.

This is a bug in TypeScript ( Microsoft/TypeScript#10078 ).
As a workaround, destructuring is moved inside the method, where it does not produce
broken artifacts in the .d.ts.

Fixes #16663.

juleskremer added a commit to juleskremer/angular that referenced this issue Aug 28, 2017

fix: argument destructuring sometimes breaks strictNullChecks
Destructuring of the form:

function foo({a, b}: {a?, b?} = {})

breaks strictNullChecks, due to the TypeScript bug Microsoft/TypeScript#10078.
This change eliminates usage of destructuring in function argument lists in cases where it would leak
into the public API .d.ts.

@mhegazy mhegazy modified the milestones: TypeScript 2.6, TypeScript 2.7 Oct 9, 2017

@Phoenixmatrix

This comment has been minimized.

Show comment
Hide comment
@Phoenixmatrix

Phoenixmatrix Jan 7, 2018

Just hit this as per apollographql/apollo-link-state#158

Workaround for now is to just destructure inside the function body instead of in the function arguments, but that essentially means that any library that exposes typings and use destructuring + defaults in function arguments will break strict nulls. That's kind of serious, especially since it seems to happen in typings that are very difficult to override in an app's custom .d.ts files.

Phoenixmatrix commented Jan 7, 2018

Just hit this as per apollographql/apollo-link-state#158

Workaround for now is to just destructure inside the function body instead of in the function arguments, but that essentially means that any library that exposes typings and use destructuring + defaults in function arguments will break strict nulls. That's kind of serious, especially since it seems to happen in typings that are very difficult to override in an app's custom .d.ts files.

@sheetalkamat

This comment has been minimized.

Show comment
Hide comment
@sheetalkamat

sheetalkamat Feb 9, 2018

Member

This seems to be fixed in latest build

Member

sheetalkamat commented Feb 9, 2018

This seems to be fixed in latest build

@Microsoft Microsoft locked and limited conversation to collaborators Jul 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.