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

Premature narrowing causing type error #33597

Closed
kolodny opened this issue Sep 25, 2019 · 12 comments
Closed

Premature narrowing causing type error #33597

kolodny opened this issue Sep 25, 2019 · 12 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@kolodny
Copy link

kolodny commented Sep 25, 2019

TypeScript Version: 3.6

Search Terms: narrowing generic

When inferring/extracting the array type from an inferred rest arg, it seems that the rest arg is widened which causes the widened type to get passed down to the type and may be too wide to match the declared type.

In the repro case below, the rest type is being inferred as [number] instead of [1] this causes the ValueFromArray to return number and when attempting to assign number to Num fails since number is much larger than Num

Code

class Observable<T> {
  value!: T
}

type ValueFromArray<T extends any[]> = T extends Array<infer U> ? U : never

declare function of<A extends Array<any>>(
  ...args: A
): Observable<ValueFromArray<A>>;

type Num = 1|2;

const working: Observable<Num> = of(1 as const); // Works
const broken: Observable<Num> = of(1); // Error: Type 'Observable<number>' is not assignable to type 'Observable<Num>'.

Expected behavior:

const broken: Observable<Num> = of(1) should compile correctly

Actual behavior:

Type error: Type 'Observable<number>' is not assignable to type 'Observable<Num>'.

Playground Link

Related Issues: ReactiveX/rxjs#5027

@RyanCavanaugh
Copy link
Member

The declaration of of should be:

declare function of<A>(...args: A[]): Observable<ValueFromArray<A[]>>;

I think at one point we were going to make the form as written illegal since it doesn't really make any sense (the rest arg type is not truly specializable)

@RyanCavanaugh RyanCavanaugh added the Question An issue which isn't directly actionable in code label Sep 25, 2019
@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 25, 2019

I think at one point we were going to make the form as written illegal since it doesn't really make any sense (the rest arg type is not truly specializable)

Please don't =(

Sometimes, we do want it to infer a tuple type for the type parameter! The order of the input arguments is important.

For example, a personal use case, SELECT statements in my query builder.

The order of items selected is important and I want A to be inferred as a tuple type (and not an unordered union of stuff selected) because order is important to UNION/INTERSECT/EXCEPT compound query statements.

Order and length of the tuple*

Like, you can't union two SELECT statements if they select a different number of columns!


The A constraint is any[] but could be specialized to a specific tuple type [1,2,3]

@kolodny
Copy link
Author

kolodny commented Sep 25, 2019

While that works, it would be nice to get the specific args from the invocation, in RxJS this would allow the piped map() to have a callback type of

of(true, 'test').pipe(
  map(callback)
)

// callback has type
type Callback = <T>(elem: Boolean, index: 0): T | <T>(elem: string, index: 1): T

It would also allow us to keep track of how large the Observable is so that something like

of(1, 2).pipe(take(4))

Can fail at compile time.

Is there a way to get that behavior and not have the widening issue mentioned above?

@kolodny
Copy link
Author

kolodny commented Sep 27, 2019

This isn't really a question and is more of a bug report / feature request (or removal of the "feature" request) since at the moment having rest args extending any[] is supported but isn't properly inferred when extracting the Array's generic

@fatcerberus
Copy link

Sometimes, we do want it to infer a tuple type for the type parameter! The order of the input arguments is important.

Agreed. It’s also the only way to capture parameter names when writing a function whose sole purpose is to forward a call, a la Function.prototype.call.

@benlesh
Copy link

benlesh commented Sep 30, 2019

@RyanCavanaugh I guess I'm confused a little by your responses in here. This next release of RxJS is a bid to try to fix all of our current typings woes for our users. I really want to get this right.

Would it be possible to collaborate directly with the TS team to shore this up and make sure we don't have regressions when there are new versions of TypeScript? I think that RxJS just happens to trip all of your edge cases. Google catches a lot of those, but the feedback loop is slow: 1) TypeScript releases new major, 2) Google syncs TS into google3 and sees breaking things 3) RxJS team hears about it and investigates 4) Feedback given to TS team.

It would be nice to get things straightened out together if possible, even in an advisory capacity, and then work on something where maybe TS team can run dtslint tests that match features we're using so we prevent regressions or at least know more about breaking changes up front.

@AnyhowStep
Copy link
Contributor

Somewhat related question, how big/widely used must a library be before they receive special attention?

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Sep 30, 2019

@benlesh RxJS is already a part of our user test suite, so we're basically aware of the changes at the point that they happen (assuming they affect compilation of the library itself). Conversely, our nightlies are available on a nightly basis, so if you'd like to field things from your side, that avenue is available. A bug report with "xxx change breaks RxJs in yyyy scenario" is something we'd treat as very actionable as long as the relevant definitions from the .d.ts were inlined so we can comprehend what's happening without hours of digging.

Edit to add: Do we need some representative RxJS-using project in the suite? Is one available?

For this specific case, we could work from a more demonstrative example to figure out what your use cases are and determine the best path going forward. Striking the balance between a representative repro and something simplified to the point that it looks like a question/misunderstanding is difficult, so I appreciate the patience and engagement on that front.

how big/widely used must a library be before they receive special attention?

"Special attention" is probably overly broad, but any notable project that is something we can build as part of our user test suite is welcome to add themselves. The line for inclusion there is basically "yeah, we've heard of that one".

When we do encounter a potential break in those suites, the question is more about representativeness (i.e. is this form of break going to be commonly observed) than popularity, though popularity itself is a de facto form of representation due to usage.

@cartant
Copy link

cartant commented Oct 1, 2019

For this specific case, we could work from a more demonstrative example to figure out what your use cases are and determine the best path going forward.

RxJS includes a number of APIs - of, startWith and endWith - that can be used to declare observables that emit multiple values of different types. One use case would be streams that emit multiple Redux actions:

// A pretend Observable
class Observable<T> {
  constructor(public value: T) {}
}

const init = { type: "init" as const };
const nav = { type: "navigate" as const };

If of is declared like this, actions of different types cannot be specified (see #30824):

declare function of<T>(...args: T[]): Observable<T>;
const actions = of(init, nav); // ERROR: '"navigate"' is not assignable to type '"init"'

So of is instead declared like this:

type ValueFromArray<T extends any[]> = T extends Array<infer U> ? U : never;
declare function of<T extends any[]>(
  ...args: T
): Observable<ValueFromArray<T>>;
const actions = of(init, nav);

Which works fine, except in the situation mentioned above, in which literal arguments are widened.

For me, there are two issues:

  • the widening of literal arguments - which is what this issue was initially about; and
  • whether or not "the form as written" (in the second of declaration in this comment) is something that is encouraged, supported or is likely to change.

The latter of these is what concerns me most, as all of the N-args support (i.e. a single, rest-parameter signature instead of multiple overload signatures, each with a different number of parameters) in RxJS is predicated upon this form being supported.

@typescript-bot
Copy link
Collaborator

This issue has been marked as 'Question' and has seen no recent activity. It has been automatically closed for house-keeping purposes. If you're still waiting on a response, questions are usually better suited to stackoverflow.

@benlesh
Copy link

benlesh commented Oct 11, 2019

Seems this was closed prematurely, it was a little heavier than a simple question, and I don't see a resolution. cc/ @RyanCavanaugh

We're still uncertain what the recommended solution should be for these scenarios.

@kolodny
Copy link
Author

kolodny commented Oct 30, 2019

@RyanCavanaugh Friendly ping on this issue. My teammate ran into this specific issue when trying to create a custom createSelector function

Demo

function createSelector<T extends readonly any[], R>(
  selectors: T,
  mapper: (...ts: T) => R
) {
  return mapper(...selectors);
}

const works = [1, 'str', true] as const;
createSelector(works, (one, str, bool) => { return {one, str, bool} })

const broken = [1, 'str', true];
createSelector(broken, (one, str, bool) => { return {one, str, bool} })

// also broken
createSelector([1, 'str', true], (one, str, bool) => { return {one, str, bool} })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

7 participants