Skip to content
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

--strictNullChecks does not gaurd against optional arguments assignment #9450

Closed
Raynos opened this issue Jun 30, 2016 · 8 comments
Closed
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@Raynos
Copy link

Raynos commented Jun 30, 2016

TypeScript Version: Version 1.9.0-dev.20160610-1.0

Code

var foo: (baz?: string) => string;

foo = function watwat(baz: string): string {
    return baz.toString();
};

foo(undefined);

Expected behavior:

Expected "Cannot call foo() with undefined"

Actual behavior:

No errors output.

When running node wat.js we get an expected uncaught exception

/home/raynos/projects/http-hash-server/lib/wat.js:3
    return baz.toString();
               ^
TypeError: Cannot call method 'toString' of undefined

My actual production use case is more complicated and involves assigning interfaces into a generic container class where the generic container class has an interface with optional arguments and the instance of the class that assign into the container has methods with non-optional arguments.

I expected that the structural equality checker would fail for optional arguments vs non-optional arguments when --strictNullChecks is enabled.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Aug 18, 2016
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Sep 28, 2016

We have an unfortunate rule about parameter arity that says optional parameters can be treated as mandatory during assignability. This is because people wrote bad callback types which specified parameters as optional intending to mean "ignorable" (but in reality all parameters are safely ignorable by the callee) when the correct interpretation is "might not be provided".

We can think about turning this rule off under strictNullChecks but it would be a fairly large breaking change; doing a scrub of DefinitelyTyped to remove these bad optionality annotations would probably be required.

@Raynos
Copy link
Author

Raynos commented Sep 29, 2016

@RyanCavanaugh

I believe I follow your explanation, for clarity can you post an example from DefinitelyTyped about one of those "bad callback types".

@HolgerJeromin
Copy link
Contributor

@RyanCavanaugh does your explanation describes also my problem or should i open a new issue?

let register = function (name: string, callback: (e: string, data: any) => void): boolean{
    // do something with the callback
    return true;
}
let genCallback = function () {
    return (e: string, data: any) => {
        // do something
    };
}

register('works', genCallback());
register('typo works too!', genCallback);

@RyanCavanaugh
Copy link
Member

@Raynos a typical example looks like this

declare function myForEach(arr: Foo[], cb: (elem: Foo, index?: number) => void): void;
// Intended meaning: You "must" accept elem, but "may" ignore index
// Actual meaning: I will always provide elem, but might not provide index

@HolgerJeromin see https://github.com/Microsoft/TypeScript/wiki/FAQ#why-are-functions-with-fewer-parameters-assignable-to-functions-that-take-more-parameters

@vknez
Copy link

vknez commented Oct 21, 2016

I stumbled across the same issue with my React component callback:

<FieldMapper
    onFieldSelected={this.handleFieldSelected}
/>

Types:

FieldMapperProps {
    onFieldSelected(discriminator: string, field: Field | undefined): void;
    ...
}

SomeComponentClass ...
    handleFieldSelected(discriminator: string, field: Field): void;
    ...

Here, onFieldSelected callback can be called with undefined field (clearing field selection), and it is wrong to assign a function that expects a non-nullable field, but TypeScript does not complain. This means I cannot rely on strictNullChecks in any callback, and in React they are used so much.

I am aware of the design decision about parameter arity you made in the past, but I still hope this can be changed for --strictNullChecks to make it usable even with callbacks, as it is really a game changer!

@RyanCavanaugh RyanCavanaugh added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Oct 31, 2016
@RyanCavanaugh
Copy link
Member

It's tempting to make this change, but the gains would be largely illusory due to parameter bivariance (see FAQ). Since (x: T) => void is already assignable in both directions with (x: T | undefined) => void, the only thing we'd gain is more inconsistency between optionality and | undefined, which should in practice behave as similar as possible.

@Raynos
Copy link
Author

Raynos commented Oct 31, 2016

I forgot about parameter bivariance. Optional parameters should behave identical to T | undefined. Good call.

@Igorbek
Copy link
Contributor

Igorbek commented Nov 4, 2016

Just for tracking, it is another issue that can be solved by variance #10717 #1394

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

5 participants