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

Speed up inference by providing more type annotations #69

Closed
sledorze opened this issue May 12, 2017 · 8 comments
Closed

Speed up inference by providing more type annotations #69

sledorze opened this issue May 12, 2017 · 8 comments

Comments

@sledorze
Copy link
Collaborator

sledorze commented May 12, 2017

The motivation here is to follow @sandersn advices (maybe @ahejlsberg will also give us some guidance on that matter)

microsoft/TypeScript#15443 (comment)

@OliverJAsh
Copy link
Collaborator

Do we have any ideas where we should add the type annotations?

My project which uses fp-ts does not compile because of microsoft/TypeScript#15443. I'm very keen to unblock this, so I can continue using fp-ts in my project.

To get us started, it would be good to take a look at this simple test case, and figure out which type annotations are needed to get it compiling again.

@gcanti
Copy link
Owner

gcanti commented May 13, 2017

@OliverJAsh The major problem in the test case is mixing Either and Validation

If you start from helpers/ip.ts

-task.of(validation.success('foo'))
+task.of(validation.success<any[], string>('foo'))

then in routes you get an error in getUserTwitterCredentials: ApiResponseNew<UserTwitterCredentialsT> is a Validation but t.validate returns a Either. If you remove the second map

export const getUserTwitterCredentials = (userId: string) => {
    const task = new fp.task.Task(() => Promise.resolve('{}'));
    return task
        .map(result => t.validate(result, UserTwitterCredentials))
        // .map((validation): ApiResponseNew<UserTwitterCredentialsT> => (
        //     validation.mapLeft(validationErrors => (
        //         [{ }]
        //     ))
        // ));
};

then you get 2 errors:

  • session.userId is not defined
  • req.ip is not defined

If you fake session and req

const session = {
    userId: '678'
}
const req = {
    ip: '387'
}

then you get the last error

credentialsM.chain(credentials => {
    // requestIpM :: Task<string>
    const requestIpM = normalizeRequestIp(req.ip);
    // y :: task.Task<ApiResponseNew<string>>
    const y = requestIpM.chain(a => getIanaTimeZoneFromIp(a))

    // this is not valid: y is a Validation but EitherT expects a Either
    // const ianaTimeZoneM = new fp.eitherT.EitherT(fp.task, y);
    // return ianaTimeZoneM
})

y is a Validation but EitherT expects a Either. Changing to

    const y = requestIpM.chain(a => getIanaTimeZoneFromIp(a))
        .map(v => v.toEither())

will let you compile the project

@OliverJAsh
Copy link
Collaborator

@gcanti That's really helpful. My project compiles now.

How did you deduce that the memory issue was because of this? I knew the project had type errors, I just didn't know what they were because of the memory/compilation issue.

@gcanti
Copy link
Owner

gcanti commented May 13, 2017

The deduction comes from the following helpful comment by @sandersn

From poking at it with the debugger, it looks like type inference starts comparing various failure Monads like Left | Right and Succcess | Failure and gets caught in structural comparison of their various methods

Unfortunately always provide type parameters seems necessary (to get fail fast errors like in helpers/ip.ts) but not sufficient (to avoid crashes): if you put by mistake a wrong type (for example a Validation for a Either or a Either for a Option) canches are that you get stuck. As noticed by @sandersn I scrupulously provided phantom types (like _tag and _URI) to make assignability fail fast, however they don't help with the type inference issue:

There's no way of failing quickly out of an inference. It just fails to find any inferences for _tag: 'Failure' → _tag: 'Right' and moves on to the next property

@sandersn
Copy link
Contributor

Update from microsoft/TypeScript#15443: It looks like we might have a fix on the typescript side so being explicit about type parameters probably won't be needed.

@sledorze
Copy link
Collaborator Author

thanks @sandersn

@sledorze
Copy link
Collaborator Author

Verified so I close

@gcanti
Copy link
Owner

gcanti commented May 17, 2017

Just verified as well, amazing. Many thanks to the TypeScript team! You rock

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

No branches or pull requests

4 participants