Skip to content

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

Closed

Description

@Raynos

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.

Activity

  1. RyanCavanaugh commented on Sep 28, 2016

    @RyanCavanaugh
    Member

    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.

  2. Raynos commented on Sep 29, 2016

    @Raynos
    Author

    Ryan Cavanaugh (@RyanCavanaugh)

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

  3. HolgerJeromin commented on Oct 19, 2016

    @HolgerJeromin
    Contributor

    Ryan Cavanaugh (@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);
    
  4. RyanCavanaugh commented on Oct 19, 2016

    @RyanCavanaugh
    Member

    Jake Verbaten (@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

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

  5. vknez commented on Oct 21, 2016

    @vknez

    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!

  6. added
    Design LimitationConstraints of the existing architecture prevent this from being fixed
    and removed
    SuggestionAn idea for TypeScript
    on Oct 31, 2016
  7. RyanCavanaugh commented on Oct 31, 2016

    @RyanCavanaugh
    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.

  8. Raynos commented on Oct 31, 2016

    @Raynos
    Author

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

  9. Igorbek commented on Nov 4, 2016

    @Igorbek
    Contributor

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

  10. locked and limited conversation to collaborators on Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Design LimitationConstraints of the existing architecture prevent this from being fixed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions