Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Fix the code which is stumbled on an old type inference problem of the Kotlin compiler #53

Merged
merged 2 commits into from
Feb 27, 2020

Conversation

petukhovv
Copy link
Contributor

@petukhovv petukhovv commented Feb 27, 2020

Fix the code which is stumbled on an old type inference problem of the Kotlin compiler

Background.
The old type inference has some problems with common super type calculation on Unit-returning lambdas. Namely, in one of the following cases, the old type inference runs "coercion to Unit":

  1. If in the place where common super type calculation is required (e.g. "if", "when", "select" function), the first candidate was a lambda which returns Unit (explicitly or implicitly);
  2. If in any of the branches there was an explicit specifying of the returned functional type, namely, Unit in the return position.

This is the wrong behavior: type inference in such cases should still run common super type calculation. The new type inference behave correctly in such cases.

Problem with the changing code.
The problem appears when someone relies on the inferred type (for example, () -> Unit), although in the new inference, due to run of the common super type calculation, the type could be more common (for example, () -> Any). This is exactly what happened in the changing code: cb(either) returns IOOf<Unit> (and the branch returns (Either<Throwable, A>) -> IOOf<Unit>), but all the remaining branches return (Either<Throwable, A>) -> Unit, so we have CST((Either<Throwable, A>) -> Unit, (Either<Throwable, A>) -> IOOf<Unit>) = (Either<Throwable, A>) -> Any. In IORunLoop.startCancelable signature, we expect (Either<Throwable, A>) -> Unit, because now we have an error in the new type inference.

The new type inference turning on.
We are going to turn on the new type inference in Kotlin 1.4 by default. Therefore, now we are trying to test the compilation of large projects on Kotlin with the new inference. Nevertheless, sometimes errors are detected not in the new inference, but in the old one, which means that if someone uses a code that was erroneously compiled, such code will stop compiling. We try to warn of such cases (like this one).

…e Kotlin compiler

Background.
The old type inference has some problems with common super type calculation on Unit-returning lambdas. Namely, in one of the following cases, the old type inference runs "coercion to Unit":
1) If in the place where common super type calculation is required (e.g. "if", "when", "select" function), the first candidate was a lambda which returns Unit (explicitly or implicitly);
2) If in any of the branches there was an explicit specifying of the returned functional type, namely, Unit in the return position.
This is the wrong behavior: type inference in such cases should still run common super type calculation. The new type inference behave correctly in such cases.

Problem with the changing code.
The problem appears when someone relies on the inferred type (for example, `() -> Unit`), although in the new inference, due to run of the common super type calculation, the type could be more common (for example, `() -> Any`). This is exactly what happened in the changing code: `cb(either)` returns `IOOf<Unit>` (and the branch returns `(Either<Throwable, A>) -> IOOf<Unit>`), but all the remaining branches return `(Either<Throwable, A>) -> Unit`, so we have `CST((Either<Throwable, A>) -> Unit, (Either<Throwable, A>) -> IOOf<Unit>) = (Either<Throwable, A>) -> Any`. In `IORunLoop.startCancelable` signature, we expect `(Either<Throwable, A>) -> Unit`, because now we have an error in the new type inference.

The new type inference turning on.
We are going to turn on the new type inference in Kotlin 1.4 by default. Therefore, now we are trying to test the compilation of large projects on Kotlin with the new inference. Nevertheless, sometimes errors are detected not in the new inference, but in the old one, which means that if someone uses a code that was erroneously compiled, such code will stop compiling. We try to warn of such cases (like this one).
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @PetukhovVictor 👋

Thank you very much for the PR & contribution! Very happy to hear that Arrow was build of this effort to try compilation on large projects with the evolution of the language.

@1Jajen1
Copy link
Member

1Jajen1 commented Feb 27, 2020

This fails because of arrow-kt/arrow-incubator#43. #54 will fix that in a sec.

@petukhovv petukhovv changed the title Fix the code which is stumbled on an old type inference problem Fix the code which is stumbled on an old type inference problem of the Kotlin compiler Feb 27, 2020
@rachelcarmena
Copy link
Member

This fails because of arrow-kt/arrow-incubator#43. #54 will fix that in a sec.

Thanks @1Jajen1 🎉

Thank you so much, @PetukhovVictor 🎉

@rachelcarmena rachelcarmena merged commit ee5b46a into arrow-kt:master Feb 27, 2020
@petukhovv
Copy link
Contributor Author

Thanks for accepting the fix!

We will report if we find any more problems on Arrow source code due to the difference in the behavior of the old type inference and new one.

@nomisRev
Copy link
Member

Awesome! Thank you @PetukhovVictor.

@rachelcarmena
Copy link
Member

It's still no available in OSS repository. It will be rerun when GitHub issues are solved 🙏

@rachelcarmena
Copy link
Member

It's already available in OSS repository 🎉

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

Successfully merging this pull request may close these issues.

5 participants