Skip to content

Callback parameter type is any for union of arrays #52748

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

Closed
steverep opened this issue Feb 13, 2023 · 26 comments
Closed

Callback parameter type is any for union of arrays #52748

steverep opened this issue Feb 13, 2023 · 26 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@steverep
Copy link

steverep commented Feb 13, 2023

Bug Report

πŸ”Ž Search Terms

array, union, callback, any, contextual

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed all FAQ

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

let myCondition = true;

const arr1 = ["a", "b", "c"];
const arr2 = [1, 2, 3];

// Correctly inferred as string[] | number[]
const myArr = myCondition ? arr1 : arr2;

for (const val of myArr) {
    // val correctly inferred to string | number
  console.log(val);
}

myArr.forEach((val) => {
    // val incorrectly inferred as any
  console.log(val);
});

πŸ™ Actual behavior

The for..of loop correctly infers the type, but array methods (e.g. forEach, map, filter, etc.) infer the array item type to be any.

πŸ™‚ Expected behavior

Given a type which is the union of arrays and/or tuples, T1[] | T2[] | ... Tn[], array methods which take functions should infer the item parameter's type as T1[number] | T2[number] | ... Tn[number].

@steverep
Copy link
Author

Note #37453 is nearly the same issue, but for a union of a set and an array. It was closed saying:

We're only capable of combining contextual types when the source signatures come from the same declaration

I don't claim to understand the details of that limitation, but it seems like it should not apply here since the source declaration is just the array prototype for all members of the union.

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Feb 13, 2023
@RyanCavanaugh
Copy link
Member

arr1.forEach((val) => {
  // val incorrectly inferred as any
  console.log(val);
});
  1. It seems like you meant to write myArr instead of arr1 here?
  2. If you do, it's string | number as expected
  3. Please make your Playground link and your sample match.

@steverep
Copy link
Author

Sorry, pasted the wrong code. I corrected to match the playground.

However, even if I do just correct that empty array version to use myArr, it still seems to infer any (at least in 4.9.5).

@steverep
Copy link
Author

Ugh... finally corrected code and playground not to be arr1.foreach...

I don't know how to see the ^? inspections in the playground. They don't seem to be accessible using a screen reader. I'm simply putting that in vscode and inspecting the types there, and it's clearly any.

@fatcerberus
Copy link

fatcerberus commented Feb 13, 2023

It's most definitely not any. Something must be wrong with your local configuration (screenshot taken directly from your playground):

image

(If you can't see the screenshot - hovering over val in your playground example shows it being typed as string | number in both locations.)

@fatcerberus
Copy link

fatcerberus commented Feb 13, 2023

FWIW I can reproduce the any inference by setting the playground to v4.2.3 (which leads to an "implicit any" error). All later versions exhibit the expected typing.

@steverep
Copy link
Author

So after some investigation it seems the problem only occurs when noImplicitAny is turned off. Here's a new playground with the loops trying to log val.length. The for...of loop correctly throws an error, but the forEach does not since it is typed as any.

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Needs More Info The issue still hasn't been fully clarified labels Feb 14, 2023
@RyanCavanaugh
Copy link
Member

This is the intended behavior. The forEach call used to be any with noImplicitAny off, and an error otherwise (due to the implicit any). Since it'd be a breaking change to an any to anything else, that behavior hasn't changed.

@steverep
Copy link
Author

That's very confusing indeed...

  • Nothing in the documentation for noImplicitAny suggests that inferred types would change based on the setting, only that either any will be inferred when the type cannot be determined or an error will occur.
  • Why should the intended behavior be different when the array is a union of array types? If the type is string[], then forEach gives the parameter a string type regardless of noImplicitAny.

I also don't understand why fixing this would be avoided as a breaking change. If anyone is relying on any as the parameter type in this situation, then TS should expose that as a bug to fix.

Please consider altering this intention at least for version 5.

@fatcerberus
Copy link

fatcerberus commented Feb 14, 2023

@steverep AFAIK noImplicitAny doesn't change inferences, only whether an implicit any is an error or not. Something else really weird is going on here. In the playground link from your OP, val in the forEach call is string | number. I've double-checked this several times. In the most recent one you posted above, it's any. Regardless of toggling noImplicitAny. The code is exactly the same in both playgrounds.

@RyanCavanaugh What's going on here?

@RyanCavanaugh
Copy link
Member

This was really done on purpose, not sure what else to say. See #42620

AFAIK noImplicitAny doesn't change inferences, only whether an implicit any is an error or not

This isn't true.

The code is exactly the same in both playgrounds.

Probably the problem is that you still have strict checked in your Playground settings? The Playground UI is confusing; unchecked means "not set", not "false".

@fatcerberus
Copy link

So all told, this ends up being a duplicate of #47294

Probably the problem is that you still have strict checked in your Playground settings? The Playground UI is confusing; unchecked means "not set", not "false".

The implicit any error goes away after clearing the checkbox, so that's not it. For posterity, it turns out the problem was that toggling noImplicitAny in the playground apparently doesn't change the inferences that were already made. Refreshing the playground after clearing the checkbox fixes it. So the option toggles are just buggy.

@steverep
Copy link
Author

I agree this is very similar to #47294, but that issue claims things changed between 4.2 and 4.3, which doesn't seem to be the case here. I went back as far as 3.9 and get the same any inference when noImplicitAny is unchecked. That seems to also suggest the above referenced PR may have nothing to do with this.

Also, that issue was closed without a suitable resolution IMHO. The author also points to the documentation not being aligned with the actual behavior. It's documented as a "Type Checking" option and says:.

In some cases where no type annotations are present, TypeScript will fall back to a type of any for a variable when it cannot infer the type. This can cause some errors to be missed, for example: ...

Reading that, there's no way a user should glean that it should be altering inferences. In fact it says the opposite.

I guess I don't know what else to say either, except that this should be addressed one way or another. Inferring any when the contextual type is clearly available is confusing, inconsistent, and has no benefit (that I can see at least).

@fatcerberus
Copy link

I get the sense that we're all talking past each other.

I agree this is very similar to #47294, but that issue claims things changed between 4.2 and 4.3

It's the same issue - prior to 4.3, it always inferred any in your example. #42620 implemented the inference that allows it to infer string | number in the OP, but this only happens when noImplicitAny is enabled (which you yourself observed), and #47294 is calling out the discrepancy. However, that turns out to be intentional--for whatever reason--per #42620:

With the caveat that this only changes our behavior when noImplicitAny is set

@steverep
Copy link
Author

Ah... I missed that subtlety - thanks for explaining. For my edification, what do you mean by "OP"?

And I don't want to seem like I'm jumping on or over anyone. I'm just trying to make a good case for this not to be written off as "intended behavior". Certainly not all intentions have only positive consequences. πŸ˜‰

@fatcerberus
Copy link

"OP" means Original Post, and/or Original Poster, depending on context. In this case it's referring to the main text of your issue at the top of the page, prior to all the replies.

@steverep
Copy link
Author

@weswigham could you comment on why #42620 only improved type inferences when noImplicitAny is enabled?

@RyanCavanaugh is that PR the only case where noImplicitAny is affecting type inferences?

@RyanCavanaugh
Copy link
Member

We try not to introduce new errors into existing code, especially if that code has strict off

@steverep
Copy link
Author

We try not to introduce new errors into existing code, especially if that code has strict off

Um, isn't that exactly what the PR in question does - strengthens contextual types to expose errors? It doesn't require strict mode.

If this is intentional only in the name of backwards compatibility, it seems to me that #42620 should have left the behavior of noImplicitAny alone, and introduced a new backwards compatibility option instead. To me, that seems like a suitable way to address this.

Please note also that my actual use case results in a situation where noImplicitAny would produce no error (as expected), but turning it off does throw an error, which is just another point of confusion I guess.

@RyanCavanaugh
Copy link
Member

Um, isn't that exactly what the PR in question does - strengthens contextual types to expose errors?

Changing an any to a string | number could absolutely introduce new check-time errors.

If every change we did had an associated back-compat flag, there'd be thousands of them instead of hundreds. The approach isn't really scalable.

it seems to me that #42620 should have left the behavior of noImplicitAny alone

But that's exactly what happened. People with noImplicitAny off had the previous behavior retained, and people with it on went from having a check-time error to having the "expected" type.

Please note also that my actual use case results in a situation where noImplicitAny would produce no error (as expected), but turning it off does throw an error, which is just another point of confusion I guess.

I don't see a check-time error in either case, so I'm not sure what you're referring to.

@steverep
Copy link
Author

steverep commented Feb 14, 2023

I don't see a check-time error in either case, so I'm not sure what you're referring to.

See this playground code instead, which is more aligned with the current use case that prompted me to open the issue. Having noImplicitAny turned off results in an error, and turning it on results in no error, which is counterintuitive.

But that's exactly what happened. People with noImplicitAny off had the previous behavior retained, and people with it on went from having a check-time error to having the "expected" type.

I disagree. I don't see that as leaving the purpose of noImplicitAny alone. It went from behaving as the docs explain to changing the behavior of inferred types. Furthermore, it left users who don't use noImplicitAny completely incapable of benefiting from better contextual types without probably a lot of unrelated work to turn it on.

@fatcerberus
Copy link

Argument of type 'category-${any}' is not assignable to parameter of type '"category-a" | "category-b" | "category-x" | "category-y"'.

Hmm, this error probably shouldn't happen. any is the "bypass type checking" type and the rest of the string matches.

@RyanCavanaugh
Copy link
Member

It went from behaving as the docs explain to changing the behavior of inferred types

noImplicitAny has always had subtle effects outside the strict domain of unannotated variables/parameters. The documentation is necessarily going to not cover every single edge case in the interests of being something that provides information that you need to know rather than information that exists. A 100.0% precise definition of all behavior in TypeScript is 50,000 lines of checker.ts, which is also available for reading.

Hmm, this error probably shouldn't happen

Yeah, though I'm not 100% sure how to make that work. I think it'd be ridiculous for "x-${any}" to match "y-z" for the same reason that { start: "x", end: any } still doesn't match { start: "y", end: "z" }. But the template string relational algorithm doesn't have any logic for relating an infinite string to a finite string (since, apart from any, that is always going to fail). If it comes up again we can think about it more.

@fatcerberus
Copy link

I think it'd be ridiculous for "x-${any}" to match "y-z"

You say that, and yet T & any is any for any T. πŸ˜‰ (I actually do agree with you though)

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

4 participants