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

A slightly more conservative version of #14218 #18352

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 7, 2023

Two changes to #14218

  • Fix hasUpperBound to work correctly for higher-kinded types

  • A more conservative fix in IsFullyDefinedAccumulator. We now maintain the symmetry that

    • if variance < 0, we maximize
    • if variance > 0 (and Nothing is admissible) we minimize
    • only if variance = 0 we use the upper bound as a tie breaker

    Previously, we maximized even at variance > 0 if there was an upper but no lower bound. But that was asymmetric since there is no corresponding case where we minimize at variance < 0 if there is a lower but no upper bound.

Two changes

 - Fix `hasUpperBound` to work correctly for higher-kinded types
 - A more conservative fix in `IsFullyDefinedAccumulator`. We now maintain
   the symmetry that

     - if variance < 0, we maximize
     - if variance > 0 (and Nothing is admissible) we minimize
     - only if variance = 0, we use the upper bound as a tie breaker

   Previously, we maximized even if variance > 0 if there was an upper but
   no lower bound. But that was asymmetric since there is no corresponding
   case where we minimize at variance < 0 if there is a lower but no upper
   bound.
@odersky
Copy link
Contributor Author

odersky commented Aug 7, 2023

This still fails #18163. But I am now not sure that #18163 needs fixing at all. It's such a weird corner case for type inference, and I believe it worked only by accident before.

@dwijnand dwijnand changed the title A slightly more conservative version of #14128 A slightly more conservative version of #14218 Aug 7, 2023
@odersky odersky requested a review from dwijnand August 7, 2023 21:34
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Looks good and makes sense. Thanks, Martin.

@dwijnand dwijnand merged commit 5759bb8 into scala:main Aug 8, 2023
17 checks passed
@dwijnand dwijnand deleted the fix-14128-alt branch August 8, 2023 07:37
@Kordyjan Kordyjan added this to the 3.3.1 milestone Aug 10, 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