Skip to content

Tighten Ycheck of type ascriptions #12513

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

Merged
merged 1 commit into from
May 25, 2021

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 18, 2021

We previously did not check that Typed nodes are type-correct,
i.e. that the type of the expression conforms to the given type.
Now we do. This uncovered problems in three different places:

  • for internal gadt ascriptions, where a cast should be used instead of
    a type ascription.
  • for avoidance, where two tests fail
  • for PCPCheckAndHeal, where at least one test fails

For now, the logic in avoidance is changed so that we use a cast if an ascription
is incorrect. But this should be fixed so that the cast is never needed, if possible.
After the inlining phase, we fall back to the old logic for typedTyped that
does not check conformance. This is because of PCPCheckAndHeal. This problem should also be
followed up.

We previously did not check that Typed nodes are type-correct,
i.e. that the type of the expression conforms to the given type.
Now we do. This uncovered problems in three different places:

 - for internal gadt ascriptions, where a cast should be used instead of
a type ascription.
 - for avoidance, where two tests fail
 - for PCPCheckAndHeal, where at least one test fails

For now, the logic in avoidance is changed so that we use a cast if an ascription
is incorrect. But this should be fixed so that the cast is never needed, if possible.
After the inlining phase, we fall back to the old logic for typedTyped that
does not check conformance. This is because of PCPCheckAndHeal. This problem should also be
followed up.

We also had to disable one FromTasty test to pass the stricter requirements
Copy link
Contributor

@abgruszecki abgruszecki left a comment

Choose a reason for hiding this comment

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

LGTM. I took a quick look at the problem with GADTs and it seems that this is another of those cases where we want to "reinstall" the GADT constraints we previously inferred.

@abgruszecki abgruszecki assigned odersky and unassigned abgruszecki May 25, 2021
@abgruszecki abgruszecki merged commit 1f4d125 into scala:master May 25, 2021
@abgruszecki abgruszecki deleted the refactor-typed branch May 25, 2021 13:34
@Kordyjan Kordyjan added this to the 3.0.1 milestone Aug 2, 2023
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

Successfully merging this pull request may close these issues.

3 participants