Rxjs: catchError with `throw` degrades type inference to empty type, causing unsafe code

[mprobst]

RxJS version: 6

Code to reproduce:

Consider this code that tries to convert an error thrown by an Observable into its own error class:

class SomeClass {
  private x: number;
  someClassField: string;
}
class UnrelatedClass {
  private y: number;
  unrelatedField: string;
}

declare const obs: Observable<SomeClass>;
// Working as expected.
obs.pipe(map(v => v.someClassField));
// Broken!
obs.pipe(
    catchError(e => {
      throw new Error(e);
    }),
    // Ouch, v cannot be of type UnrelatedClass here, should be SomeClass!
    map((v: UnrelatedClass) => 1));

The problem here is in the signature of catchError. Because the error handling function given doesn't return any value, its return type is inferred as never. catchError is defined as:

function catchError<T, R>(
    selector: (err: any, caught: Observable<T>) =>
        Observable<R>): OperatorFunction<T, T|R>;

So if you pass in a selector that's (e: any) => never, you end up with R being inferred as {} (the empty type), because there is no return. That in turn leads to the pipe chain degrading to {}. And now because in TypeScript callbacks are bivariant, that means users can pass any arbitrary type in place of the v passed down, e.g. UnrelatedClass above.

Expected behavior:

Should give a compile error that v is not of type UnrelatedClass.

Actual behavior:

Code is silently accepted, generic types are a lie, fails at runtime and is super confusing for newer users.

[mprobst]

I was going to suggest adding an overload to catchError like so:

function catchError<T>(
    selector: (err: any, caught: Observable<T>) =>
        never): OperatorFunction<T, T>;

That'll make sure that if selector never returns a value, we don't end up with an Observable<{} | T> as the result, but just an Observable<T>. That's an improvement because it fixes the type inference decision here, so that we know the result above is an Observable<SomeClass>.

However testing this out, it turns out there's another more fundamental problem: because TypeScript considers callback functions bivariant, we can specify any type we want in the map function, and TS will accept it silent. That's independent of catchError:

declare const obs2: Observable<SomeClass>;
obs.pipe(
  map(v => v.someClassField),
  // v must be string here, but bivariance ruins the day :-(
  map((v: UnrelatedClass) => v.unrelatedField));

I've created a playground for this here, to make it a bit easier to throw around ideas.

[benlesh]

It seems this is more of an issue with TypeScript and type inference within the pipe method. Currently, you can type things however you want inside of pipe and TypeScript seems to let you do it. I'm not sure there's anything we can do on the RxJS side.

[benlesh]

I'm labeling this but leaving it open. I don't think we can fix it, but I want to track it.

[felixfbecker]

However testing this out, it turns out there's another more fundamental problem: because TypeScript considers callback functions bivariant,

Are you sure this is happening even with strictFunctionTypes?

[benlesh]

Okay, after more thought, I'm going to close this one. The reason you get a loss of type safety here is because the function doesn't return anything TypeScript can infer a type from. This isn't anything rxjs can fix.

[felixfbecker]

@benlesh a function that never returns and throws in all cases is correctly inferred as return type never.

What speaks against overloading catchError to return Observable<never> if the catcher returns never?

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.