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

Reject functions with not enough parameters on strict mode #17868

Closed
inad9300 opened this issue Aug 17, 2017 · 28 comments
Closed

Reject functions with not enough parameters on strict mode #17868

inad9300 opened this issue Aug 17, 2017 · 28 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@inad9300
Copy link

As is well explained in the FAQ, "functions with fewer parameters [are] assignable to functions that take more parameters."

This is fine to ease the transition from JavaScript, but limits TypeScript's ability to detect errors at compile time. A compiler flag to switch on this check would be an amazing step towards safer code.

This option may also be enabled by the "strict" flag.

@DanielRosenwasser DanielRosenwasser added the Suggestion An idea for TypeScript label Aug 17, 2017
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 17, 2017

So you would be okay with all your callbacks to map requiring 3 parameters? For example

function squareAll(nums: number[]) {
  return nums.map((v, _index, _arr) => v ** 2);
}

@inad9300
Copy link
Author

inad9300 commented Aug 17, 2017

I understand it's inconvenient for some native functions, but if that's the price to pay for detecting mistakes, I'm absolutely OK with it. The trigger of this request was indeed a hidden mistake that took a while to catch up due to TypeScript being too tolerant.

Besides, for custom functions, you can always define optional parameters, which will be very useful in callbacks. In fact, perhaps this is the way all callbacks of native functions accepting them should be declared.

@antoniovazquezblanco
Copy link

On top of what @inad9300 has said, you're not obligated to rewrite all your map callbacks because you are not obligated to turn that check on.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Aug 17, 2017

you can always define optional parameters,

This isn't what an optional parameter means! If strictly enforced (which we don't because everyone gets it wrong when writing declaration files), an optional parameter in a callback position means "I (the caller) might not provide this parameter", not "You (the callee) are free to ignore this parameter".

you are not obligated to turn that check on.

Of course. The line of discussion here is about whether anyone would turn this flag on in the first place if it caused massive headaches for all built-in array functions. Our hypothesis is "no".

@antoniovazquezblanco
Copy link

antoniovazquezblanco commented Aug 17, 2017

In my case the answer would be yes. I would turn this flag on to ensure my callbacks are correctly defined and called instead of silently failing.

@RyanCavanaugh
Copy link
Member

Can you show an example where a callback was "silently failing" ?

@inad9300 same question - seeing the actual bug you had would be very informative

@inad9300
Copy link
Author

inad9300 commented Aug 17, 2017

This is not best place to pose questions, but since it is relevant for the discussion... Given the following code:

function each<T>(list: T[], callback: (item?: T, index?: number, listReference?: T[]) => void) {
    // ...
}

What do the optional parameters mean here? As I see it, they should mean that I, the caller of each (which is the caller of the callback), am allowed to provide a callback function which does not take any parameter at all, but might also provide a callback that accepts an item, or and item and a number, an so on.

Edit Added missing name for the callback argument.

@RyanCavanaugh
Copy link
Member

What you've typed represents a function that acts like this

function each<T>(list: T[], cb: (item?: T, index?: number, listReference?: T[]) => void) {
    // ...
    cb();
    cb(list[0]);
    cb(list[1], 1);
    // etc
}

@inad9300
Copy link
Author

OK. It's a bit confusing, but I think I understand. Now, what does it mean for the provider of the callback to declare its parameters as optional? It's completely useless and ignored, as I can see quickly in the playground.

function each<T>(list: T[], cb: (item?: T, index?: number, listReference?: T[]) => void) {
    // ...
    cb();
    cb(list[0]);
    cb(list[1], 1);
    // etc
}

// What is the difference between these two callbacks?
each([1, 2, 3], (item: number) => { })
each([1, 2, 3], (item?: number) => { })

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Aug 17, 2017

// What is the difference between these two callbacks?

The first one should be illegal given the definition of each, but isn't. This is basically due to historical reasons - people wrote tons of definitions using ? thinking it meant "you may ignore it", and we didn't detect the error due to a bug. If anything were to change here regarding callback arity under strict, it would be to make this an error.

The second one is a "correct" callback for a declared callback type which has optional parameters.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Aug 18, 2017

A function has an interface and a function with fewer arguments can implement that same interface so long as the types of the arguments it provides are assignable

type F = (key: string, value: string) => object;

An example implementation of F could be

const f: F = (key, value) => ({[key]: value});

Another implementation of F is

const g: F = key => f(key, key);

Why is this a problem?

Trying to enforce that a provided callback binds each possible argument feels like trying to depend on the implementation of an interface.

@inad9300
Copy link
Author

Thanks for the clarification, @RyanCavanaugh. In my opinion, the separation between normal and strict modes should allow to overcome this kind of history-provoked limitations. That bug should definitely be fixed in strict mode.

@aluanhaddad, for me, it is precisely when I see functions as interfaces when it makes the less sense. If you accept { a: number, b: number }, you cannot provide { a: 2 }. You can however do it when the interface is { a: number, b?: number }. Now we move over to functions and the same doesn't hold. In your example, g is not a valid value of type F the same way { a: 2 } is not a valid value of type { a: number, b: number }. I find totally predictable that "everyone gets it wrong", as @RyanCavanaugh pointed out. It's just inconsistent, isn't it?

@RyanCavanaugh
Copy link
Member

I don't understand the claim that g is not a valid F. People write code like let n = arr.map(x => x - 1) all the time. There's no way you can say that code is wrong in any meaningful sense of the word; it is simply refuted by experience.

Again I'd like to see the root-cause bugs here to understand how you had a bug caused by wrong function arity. The only case I can think of is something like writing a comparator function, where it's somewhat implausible that you could write a correct 1-arity function for Array#sort (though you could still write a correct implementation using the arguments object!).

@inad9300
Copy link
Author

inad9300 commented Aug 18, 2017

Of course in the current system g is a valid F, but I find it similar to { a: 2 } being a valid { a: number, b: number }, which is obviously not correct. All I was saying is that this is at least counterintuitive. As you initially implied, ? is tricky to get; I'm still confused by it.

As for the issue @antoniovazquezblanco and me had, we have been trying to reproduce it, without success unfortunately. Our scenario is a bit complex and we simply didn't manage to return things to the state they were in when we had the problem.

But your example is very good already, and I'm certain there are many analogous cases. If you are Array.prototype.sort, you expect a function with the following interface:

type CompareFn<T> = (a: T, b: T) => -1 | 0 | 1

And you better don't pass to it a function that takes only one argument, because it is not going to work. And all the people using ? wrong are most likely relying on the compiler to catch that.

Actually, Array.prototype.map is a similar case. You are not going to map a lot if you provide a 0-argument function, are you? And yet, this is (and should not be, if in strict mode) valid TypeScript code:

[1, 2, 3].map(() => 42)

Please, don't hesitate to refute my arguments. I might be still missing the point, and I'm happy to be proven wrong.

@mhegazy mhegazy added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Suggestion An idea for TypeScript labels Aug 18, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Aug 18, 2017

The rational here is simple, a callback that declares less parameters than its caller is the same as a callback that declares the parameters and never uses them.

@inad9300 can you share an example where having a callback with less parameters than the declaration would be considered a bug.

@inad9300
Copy link
Author

inad9300 commented Aug 18, 2017

I'm sorry, but didn't I just provide two examples of that? (One as a continuation of @RyanCavanaugh's case).

Iif in strict mode, passing sort a callback with 0 or 1 argument should be considered a bug. Passing map or forEach a callback with 0 arguments should be considered a bug. What are the chances of you calling filter with a 0-argument function intentionally? Or find? I bet they are very low. Just to mention a few native alternatives, but the same could go for any custom function accepting a callback.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 18, 2017

Iif in strict mode, passing sort a callback with 0 or 1 argument should be considered a bug. Passing map or forEach a callback with 0 arguments should be considered a bug. What are the chances of you calling filter with a 0-argument function intentionally? Or find? I bet they are very low. Just to mention a few native alternatives, but the same could go for any custom function accepting a callback.

So is [1, 2, 3].map((_a,_b,_c) => 42). and [].sort((_a, _b) => -1); so not sure why this is OK, but dropping the parameters are not. the more common case in fewer arguments is users are using the first, and ignoring the additional ones, like map.

Moreover, --strict is not meant to be a mode for all possible checks to happen. we would still like it to be usable.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Aug 18, 2017

A zero-parameter map / forEach callback is totally fine: I want a new parallel array with some initial contents. Or I want to do some operation n times based on the length of the array.

Even sort and filter don't make a great case for this, because you have to make two completely orthogonal mistakes at the same time for this to come up: You have to forget to declare the parameters, and you have to write a function body that does something (what?) without actually trying to refer to the parameters that should be there.

It's not even clear to me how this rule helps you in that case. If you forgot to use the parameters that should have been there in the function body, how is declaring them going to help? Under this system you're going to routinely have to write a bunch of stand-in _1 _2 _3 parameters any time you have a callback, which isn't going to clue you in to the fact that you should have been using them -- your code will be littered with (_a, _b, _c) => ... function expressions and the fact that you should have used _a is no more apparent than it was before.

We could imagine some special syntax for "This parameter must be declared for this callback", but I think that'd be useful for sort, filter, and almost nothing else, which doesn't really clear the bar in terms of cost vs value.

@inad9300
Copy link
Author

@mhegazy, what you are saying sounds to me like not checking a function's use against its declaration because the function is not guaranteed to be correct anyway, so why bother. I think you are mixing what are two very different things: type checks and executable code. Getting the types right is only a prerequisite to getting the code right.

I understand you don't want to accept every suggestion you get, nor to pollute the compiler's flag set. At the end of the day, there are many users to satisfy, and you are the real experts, so you decide. All I'm asking is for you to take the proposal into consideration, not just spit off counterarguments without having spend proper time to consider.

Maybe it should not be under the strict flag, but under a special flag. That's not for me to decide, of course. I can only suggest and give my point of view.

@RyanCavanaugh
Copy link
Member

We've been explaining this behavior to users for five years now. We understand why it works the way it does, and we understand why there isn't a flag. If you think it's just that we haven't thought about it, you are very much mistaken.

@inad9300
Copy link
Author

inad9300 commented Aug 18, 2017

@RyanCavanaugh, I think the first part of your comment is a too-narrowed view. Of course it is unlikely that you fail in simple cases, but once you start to have more complex ones, where (e.g.) you pass one of a set of callbacks dynamically, it is then when you need a compiler to back you up the most.

Edit Besides, I would argue that using map as a means to loop a number of times is not "totally fine."

The second part is a totally valid one. It would imply clutter. I personally value correctness over everything else. But again, I'm not the one to take the decision. Other opinions would be helpful to better evaluate the cost vs. value.

@inad9300
Copy link
Author

@RyanCavanaugh I should have imagined you have already thought about this many times. I'm sorry to have insinuated otherwise. Regardless of when, I just wanted to argue in favor of taking the proper time to consider suggestions in general.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Aug 19, 2017

This also impacts assignability.

async function get(url: string) {
  const response = await fetch(url, {method: "GET"});
  return response.json();
}

const endpoints = [
    "api/users",
    "api/products",
    "api/orders"
];

const [users, products, orders] = await Promise.all(endpoints.map(get));

That code is perfectly fine and making it an error would not only be inconvenient but it would severely limit code reuse.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 19, 2017

@inad9300 the main goal of TypeScript is to increase developer productivity; this means we have to base many of our decisions on usability. As noted by @RyanCavanaugh and @DanielRosenwasser this issue has been discussed in the past, and JS coding patterns were clear in favor of the current behavior.

@inad9300
Copy link
Author

OK, got it. I have never seen TypeScript as a productivity tool, to be honest, but rather as a help to write correct programs as your code gets bigger and bigger (hence "JavaScript that scales").

Someone should have pointed out the existence of previous discussions in the beginning, linking to them, in order to minimize the time I make you waste.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 19, 2017

You found the FAQ which you linked to. There is a search tool which can lead you to all sorts of conversation on the topic, including #13043, #16871, #9825, and #9765 which all seem to be in this same space. I am not sure why it is incumbent on others to point these out to you.

@inad9300
Copy link
Author

Let me rephrase: I would have appreciated –and think it would have been on the benefit of everyone– that someone had kindly pointed out to existing issues discussing the topic.

Thanks for pointing out to some.

@ORESoftware
Copy link

+1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

8 participants