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

"strictFunctionTypes" compiler option leads to error #39

Closed
supermacro opened this issue May 21, 2020 · 8 comments
Closed

"strictFunctionTypes" compiler option leads to error #39

supermacro opened this issue May 21, 2020 · 8 comments

Comments

@supermacro
Copy link
Owner

supermacro commented May 21, 2020

This is a continuation from this comment from #36 (comment)

Looks like the strictFunctionTypes compiler option leads to a rather cumbersome type error which looks to have merit.

This SO thread and the official docs on the subject are great resources for explaining what strictFunctionTypes is, but I personally have not sat down to really think this through to know what the implications for neverthrow are.

Below is a demonstration where there are type errors when the strictFunctionTypes is turned on.

Code example: https://repl.it/repls/SparseSaltyPdf

Screenshot:

Screen Shot 2020-05-21 at 10 16 01 AM

@supermacro
Copy link
Owner Author

cc @paduc

@paduc
Copy link
Contributor

paduc commented May 21, 2020

I agree on the merit of the flag and error.

I haven't had the time to reproduce and troubleshoot locally but I definitely will look into to it asap.

@jsmith
Copy link
Contributor

jsmith commented May 22, 2020

Ok so I quickly looked through the official docs and noticed that stricter type checking applies to functions.

The stricter checking applies to all function types, except those originating in method or constructor declarations. Methods are excluded specifically to ensure generic classes and interfaces (such as Array) continue to mostly relate covariantly.

I changed the declaration to use methods and... it type checks!

import { Result, Ok, Err } from "neverthrow";

export declare const errAsync: <T, E>(err: E) => ResultAsync<T, E>;

export declare class ResultAsync<T, E> {
  private _promise;
  constructor(res: Promise<Result<T, E>>);
  static fromPromise<T, E>(promise: Promise<T>, errorFn?: (e: unknown) => E): ResultAsync<T, E>;
  map<A>(f: (t: T) => A | Promise<A>): ResultAsync<A, E>;
  mapErr<U>(f: (e: E) => U | Promise<U>): ResultAsync<T, U>;
  andThen<U>(f: (t: T) => Ok<U, E> | Err<U, E> | ResultAsync<U, E>): ResultAsync<U, E>;
  match<A>(ok: (t: T) => A, _err: (e: E) => A): Promise<A>;
  then<A>(successCallback: (res: Result<T, E>) => A): Promise<A>;
}

const err1: ResultAsync<{}, { a: "a" }> = errAsync({ a: "a" }); // cat
const err2: ResultAsync<{}, { a: "a" } | { b: "b" }> = err1; // animal

Could the type declarations be changed to methods for this class? I think this is related to contravariant method parameter types but I'm honestly not sure. I can't think of anything type unsafe someone could do with the current code. Anyway the codesandbox can be found here 😃

@paduc
Copy link
Contributor

paduc commented May 22, 2020

@jsmith I changed the function declarations to methods in ResultAsync in a new PR #41

I didn't change errAsync and okAsync, is it necessary ?

@jsmith
Copy link
Contributor

jsmith commented May 22, 2020

I don't think that's necessary. Thank you for creating the PR :)

@supermacro
Copy link
Owner Author

supermacro commented May 22, 2020

Thanks for the help @paduc!

This issue will remain open even after #41 is merged because I want to grok the subtleties of this strictFunctionTypes flag. I am not sure if the flag doesn't apply to methods because of a limit in the compiler, or whether there's something fundamentally different between methods and functions such that the statement "parameter positions are checked contravariantly instead of bivariantly." (from here) doesn't apply to methods (which it seems to me that it should).

@jsmith
Copy link
Contributor

jsmith commented May 23, 2020

I agree, I don't think there is really much of a difference – just that there would be a lot more breaking changes if TypeScript removed method parameter bivarience. There is a proposal about making method parameters contravariant but it's four years old! Using my simple example from #36:

export declare class ResultAsync<T, E> {
   andThen: <U>(f: (t: T) => Ok<U, E>) => ResultAsync<U, E>;
}

const err1: ResultAsync<{}, { a: "a" }> = errAsync({ a: "a" });
const err2: ResultAsync<{}, { a: "a" } | { b: "b" }> = err1;

err1 expects the caller of andThen to pass in an argument of type () => Ok<{}, { a: "a" }>. When we assign err1 to err2, we can now call andThen with () => Ok<{}, { a: "a" } | { b: "b" }> which certainly could be unsafe in other situations but I don't see any potential issues with this library.

@supermacro
Copy link
Owner Author

Update: 2.3.2 has been released which includes @paduc's patch:

#41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants