Skip to content

Typecheck passes for incorrect assignment #21868

Closed
@canonic-epicure

Description

@canonic-epicure

Hello,

This code typechecks fine:
const some : ((arg : string) => boolean) = () => true

In the same time it looks obviously incorrect. Since the "arg" in the type definition is not optional. So the function w/o argument is assigned to the variable, which has a type of function with 1 argument.

I'd understand if this would typechecks (it does)
const some1 : ((arg? : string) => boolean) = () => true
but not the above.

I've tried running it with --strictFunctionTypes and it typechecks as well.

Activity

canonic-epicure

canonic-epicure commented on Feb 11, 2018

@canonic-epicure
Author

Huh, optimizing typechecking for the "Array.forEach()" case is kind of weird. In any well-typed language those types are clearly different.
The note from the FAQ:

But forEach should just mark its parameters as optional! e.g. forEach(callback: (element?: T, index?: number, array?: T[]))

This is indeed how the signature for the "forEach" should have looked like.

Now consider this example:

interface Something {
    doSomething : (a : number, b : string, c : string[]) => string
}

const b : Something = {
    doSomething : () => ''  // TYPECHECKS ??
}

b.doSomething() // DOES NOT TYPECHECKS

Behavior is very inconsistent.

canonic-epicure

canonic-epicure commented on Feb 11, 2018

@canonic-epicure
Author

Check this example as well. Again, different behavior:

interface Something {
    doSomething : (a : number, b : string, c : string[]) => string
}

class SomeClass implements Something {
    doSomething : () => '' // TYPECHECKS ??
}
var b = new SomeClass()

b.doSomething() // TYPECHECKS ??
canonic-epicure

canonic-epicure commented on Feb 11, 2018

@canonic-epicure
Author

The logic from the FAQ entry is not correct:
This is not what an optional callback parameter means. Function signatures are always read from the caller's perspective. If forEach declared that its callback parameters were optional, the meaning of that is "forEach might call the callback with 0 arguments".

Indeed, signatures are read from the caller's perspective. But the caller in this case is not the "forEach" method, but the one that calls "forEach" itself. It just happens that "forEach" will provide all three optional arguments to its callback. The caller of "forEach" may consume arbitrary number of them, or even none.

ahejlsberg

ahejlsberg commented on Feb 11, 2018

@ahejlsberg
Member

This is working as intended and the wording in the FAQ is indeed correct. In your examples above, it is safe to treat a SomeClass as a Something because you can safely ignore arguments you aren't interested in. The implements keyword doesn't mean "must implement with exactly the same signature", it just means "must implement so you can safely be treated as".

canonic-epicure

canonic-epicure commented on Feb 11, 2018

@canonic-epicure
Author

Sad to hear. Obviously wrong from my perspective. It is safe to add arguments, not remove.

canonic-epicure

canonic-epicure commented on Feb 11, 2018

@canonic-epicure
Author

Following your logic "you can safely ignore arguments you aren't interested in" basically means - it is safe to ignore mandatory argument for function. Does not make sense for me.

RyanCavanaugh

RyanCavanaugh commented on Feb 11, 2018

@RyanCavanaugh
Member

There are dozens of issues where people have argued about this (start at #17868); we don't need another. This is the intended behavior.

canonic-epicure

canonic-epicure commented on Feb 11, 2018

@canonic-epicure
Author

It is a very strange way of running a language business - compromise type safety in return of some ad-hoc convenience for few methods of the built-in lib.

If you have some experience with any language with proper type systems (Haskell, Idris) then const some : ((arg : string) => boolean) = () => true is an obvious type contract violation. Type says - this value is of type "function that accept one mandatory argument". Then assignment operator breaks the contract.

It is very strange way to think about the type (arg : string) => boolean as "a function that accept a string argument and return boolean, OR a function w/o arguments that returns boolean". Thats a sum type. If user's intention would been to define such type, s/he would explicitly used "|".

The callbacks for native methods can be defined as sum types instead:

type forEachCallbackType0<T> = () => void
type forEachCallbackType1<T> = (value: T) => void
type forEachCallbackType2<T> = (value: T, index: number) => void
type forEachCallbackType3<T> = (value: T, index: number, array: T[]) => void


type forEachCallbackType<T> = forEachCallbackType0<T> | forEachCallbackType1<T> | forEachCallbackType2<T> | forEachCallbackType3<T>

Now this is a "it-was-always-like-that-and-we-will-not-change-it-because-it-may-break-something" thing and If I'm correct many people made their reputation, advocating for the current state of things. But it does not become correct because of that. Its obviously wrong.

j-oliveras

j-oliveras commented on Feb 11, 2018

@j-oliveras
Contributor

An optional or required parameter is from the perspective of the caller.

RyanCavanaugh

RyanCavanaugh commented on Feb 12, 2018

@RyanCavanaugh
Member

You're begging the question. Ignored extra arguments are both common and safe in JavaScript; it's only "unsafe" if you think ignoring extra arguments is unsafe, and it isn't except by your own fiat.

It makes no sense to require everyone writing a calling-back function to write out N + 1 explicit overloads if they supply N arguments.

canonic-epicure

canonic-epicure commented on Feb 12, 2018

@canonic-epicure
Author

Everyone writing a calling back function and willing to make it flexible in terms of arguments will make them optional, thats all. In fact, an optional arguments is exactly equivalent of the sum type above.

But a hole in the typechecker still remains:
const some : ((arg : string) => boolean) = () => true
And you deliberately ignore it. This has nothing to do with the callbacks. Its a problem on its own. You solved a "callbacks problem" by compromising type-safety.

RyanCavanaugh

RyanCavanaugh commented on Feb 12, 2018

@RyanCavanaugh
Member

Everyone writing a calling back function and willing to make it flexible in terms of arguments will make them optional, thats all

This is covered in the FAQ! You should read it! https://github.com/Microsoft/TypeScript/wiki/FAQ#why-are-functions-with-fewer-parameters-assignable-to-functions-that-take-more-parameters

canonic-epicure

canonic-epicure commented on Feb 13, 2018

@canonic-epicure
Author

The fact that something is in the FAQ does not make it automatically correct.

Let me explain.

The type that represent a function with one required argument, that is a string and return number:

type func1Req = (a : string) => number

The type that represent a function with one optional argument, that is a string and return number:

type func1Opt = (a? : string) => number

The value of func1Opt can be called with 0 arguments as well, so the func1Opt is actually a sum type:

type func1Opt = ((a : string) => number) | (() => number).

This is how the question mark ? on the function argument is resolved in typechecker I'm pretty sure.

Now lets check the forEach signature:

forEach(callbackfn: (value: T, index: number, array: T[]) => void, thisArg?: any): void;

The signature is actually incorrect, since it is valid for the caller of the "forEach" method, to provide a callback without arguments, only 1 argument, 2 args, or all 3 args. So, the correct signature should look like:

    forEach<T>(
        callbackfn: ((value: T, index: number, array: T[]) => void)
            | ((value: T, index: number) => void)
            | ((value: T) => void)
            | (() => void),
        thisArg?: any
    ): void;

We are only interested in the type of the callbackfn here:

type callbackType<T> = ((value: T, index: number, array: T[]) => void)
            | ((value: T, index: number) => void)
            | ((value: T) => void)
            | (() => void)

Now, lets examine what will be the type of the callback fn, if we specify all arguments as optional:

type cbk<T> = ((value?: T, index?: number, array?: T[]) => void)

Lets apply the same resolving rule using the sum type for the last argument:

type cbk1<T> = ((value?: T, index?: number, array?: T[]) => void) | ((value?: T, index?: number) => void)

We've indicated in the types, that since the array? argument is optional, we can possibly omit it and instead provide a function with only 2 arguments.

Applying the rule again 2 times and we get exactly the same signature as we defined previously:

type cbk2<T> = ((value: T, index: number, array: T[]) => void) | ((value: T, index: number) => void) | ((value: T) => void) | (() => void)

What we did, we proved that:

Thing1 = Some
Thing2 = Some

Which by transitivity of the equality means Thing1 = Thing2. Which in turn means - if the intention is, that caller of the method with the callback can omit some of the callback's arguments - it is safe and actually correct to specify the arguments of the callbacks as optional.

If the intention is, that caller of the method with the callback can not omit any of the callback's arguments, then those should not be marked as optional. That's all, no need to compromise type-safety.

RyanCavanaugh

RyanCavanaugh commented on Feb 13, 2018

@RyanCavanaugh
Member

You're just arguing that optionality should have a different definition from the definition that it does have. The language not having the definition you wanted to have doesn't change the definition, or make it wrong.

I can argue that a meter should be 10cm, because then 1cm = 0.1m, which is clearly safe, whereas 1cm = 0.01m is unsafe. The fact that a book says 1m = 100cm is irrelevant because a correct definition of the meter is 1m = 10cm. You can tell it's unsafe because 5cm should be 0.5m but it's not. Please change science.

RyanCavanaugh

RyanCavanaugh commented on Feb 13, 2018

@RyanCavanaugh
Member

Also worth nothing that if optional means "callee may ignore", then there's no syntax for "caller might not provide"!

canonic-epicure

canonic-epicure commented on Feb 13, 2018

@canonic-epicure
Author

No, I'm arguing, that const some : ((arg : string) => boolean) = () => true is invalid typing.

I would not talk about the callbacks at all, if it wouldn't been mentioned in the FAQ, as the reason why the code above is considered valid. It has been artificially made valid, to solve the "callbacks problem".

That was a bad way of solving that problem, especially that it only applies to built-in methods. The better solution would be to use sum types as I mentioned (which is equivalent to marking arguments as optional).

RyanCavanaugh

RyanCavanaugh commented on Feb 13, 2018

@RyanCavanaugh
Member

No, I'm arguing, that const some : ((arg : string) => boolean) = () => true is invalid typing.

OK, let me try this: Why do you think it should be invalid? The circular argument of "because it is unsafe" should be avoided -- as much as possible, unsafe things are indeed made invalid.

It's odd to say "let's not talk about callbacks"; in what other scenario would you alias a function?

canonic-epicure

canonic-epicure commented on Feb 13, 2018

@canonic-epicure
Author

Following your logic, why not make the const some : string = 11 valid? It is always possible to convert a number to string, so its safe, then why bother with extra type checks? Then we are back to old-good-plain-untyped javascript.

Types express some contracts ("invariants") about the code, that user has intention to maintain. If I declare const some : string my intention is to only store string value in the "some". If I declare const some : ((arg : string) => boolean) my intention is clearly to store value of the function with 1 required argument. If I declare const some : ((arg? : string) => boolean) then it can be function with 1 or 0 arguments.

RyanCavanaugh

RyanCavanaugh commented on Feb 13, 2018

@RyanCavanaugh
Member

Following your logic, why not make the const some : string = 11 valid?

Because (11).toUpperCase() crashes but (() => null)("foo") doesn't. And code like the latter is running all the time in JavaScript programs (via indirection) to no ill effect.

If I declare const some : ((arg? : string) => boolean) then it can be function with 1 or 0 arguments.

You say you want to disallow unsafe assignments. In your world, this code is legal:

// Callee may have zero or one arguments
const some: ((arg?: string) => string) = arg => arg.toUpperCase();
// When invoking a function, you don't need to provide optional arguments (by definition)
some();

and it crashes. What's "safe" about this? How do you imagine changing the definition of "optional" in this world?

canonic-epicure

canonic-epicure commented on Feb 13, 2018

@canonic-epicure
Author

This discussion goes to nowhere very quickly.

Thanks for explaining the current quality standards of the typescript typechecker.

RyanCavanaugh

RyanCavanaugh commented on Feb 13, 2018

@RyanCavanaugh
Member

This discussion goes to nowhere very quickly.
Thanks for explaining the current quality standards of the typescript typechecker.

I'm here with actual code examples showing why it works the way it does. Why this attitude?

locked and limited conversation to collaborators on Jul 3, 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

    Working as IntendedThe behavior described is the intended behavior; this is not a bug

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @canonic-epicure@ahejlsberg@RyanCavanaugh@j-oliveras

        Issue actions

          Typecheck passes for incorrect assignment · Issue #21868 · microsoft/TypeScript