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.

Sign up for GitHub

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

Jump to bottom

Plug many variance holes (in higher-kinded types, refined types and private inner classes) #8545

Merged
merged 4 commits into from Mar 19, 2020
Merged

Plug many variance holes (in higher-kinded types, refined types and private inner classes) #8545

merged 4 commits into from Mar 19, 2020

Conversation

joroKr21
Copy link
Member

@joroKr21 joroKr21 commented Nov 17, 2019 •
edited

Most examples are fixed in dotty.

The variance checks root in RefChecks for TypeTrees is modified
to check only the variance of PolyTypes (this includes type lambdas).
Checking the definition of the PolyTypes' owners is not correct,
because relativeVariance doesn't work for types in arbitrary position.

To check the variance of PolyTypes we introduce a new type map:
PolyTypeVarianceMap. This is similar to the way variance is checked
for type lambdas in dotty, where type lambdas are encoded explicitly.

We also add explicit tracking for lower bounds of PolyTypes.
For a type parameter of a PolyType, the variance should not be
flipped (or equivalently, flipped twice) in the lower bound.
This applies to both ValidateVarianceMap and PolyTypeVarianceMap.
It fixes variance bugs for types appering in the bounds of HKTs.

Finally, we don't skip local definitions entirely (this is unsound),
but instead cutoff relativeVariance at local boundaries.

Fixes scala/bug#11789, fixes scala/bug#9911, fixes scala/bug#9725

@scala-jenkins scala-jenkins added this to the 2.13.2 milestone Nov 17, 2019
@Blaisorblade
Copy link
Contributor

Fwiw: Dotty does traverses bodies in PostTyper to find refinements and type lambdas (and guess RefChecks is a sensible phase for that). But those lambdas should not be checked for occurrences of class type params, only occurrences of their type params.

@joroKr21
Copy link
Member Author

Fwiw: Dotty does traverses bodies in PostTyper to find refinements and type lambdas (and guess RefChecks is a sensible phase for that). But those lambdas should not be checked for occurrences of class type params, only occurrences of their type params.

Cool, that's what I'm doing here as well. Now need to replicate the fix for private[this].

@joroKr21 joroKr21 force-pushed the variance-holes branch 4 times, most recently from 5699c5e to 83e4ef1 Compare December 4, 2019 03:16
@joroKr21 joroKr21 marked this pull request as ready for review December 4, 2019 04:09
@joroKr21

This comment has been minimized.

Sign in to view
@SethTisue

This comment has been minimized.

Sign in to view
@joroKr21

This comment has been minimized.

Sign in to view
@SethTisue

This comment has been minimized.

Sign in to view
@joroKr21

This comment has been minimized.

Sign in to view
@joroKr21 joroKr21 force-pushed the variance-holes branch 2 times, most recently from ee6c01c to cab4555 Compare December 9, 2019 15:16
@SethTisue
Copy link
Member

SethTisue commented Dec 9, 2019 •
edited

@joroKr21 agree akka-stream and squants failures are known and unrelated.

but what about the monix failure? https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/2855/artifact/logs/monix-build.log

@joroKr21

This comment has been minimized.

Sign in to view
@SethTisue

This comment has been minimized.

Sign in to view
@joroKr21
Copy link
Member Author

I pushed a commit to remove the check for escaping locals which was the cause of the Monix failure.

@SethTisue

This comment has been minimized.

Sign in to view
@SethTisue

This comment has been minimized.

Sign in to view
@SethTisue

This comment has been minimized.

Sign in to view
@joroKr21 joroKr21 force-pushed the variance-holes branch 2 times, most recently from 3224c66 to 370506e Compare December 17, 2019 10:54
@joroKr21 joroKr21 force-pushed the variance-holes branch 2 times, most recently from dd04a28 to 45eb099 Compare January 27, 2020 21:31
@SethTisue
Copy link
Member

@joroKr21 is this ready for review?

@joroKr21
Copy link
Member Author

joroKr21 commented Feb 7, 2020

Yes it has been ready for a while, sorry if I didn't make it clear.

@dwijnand
Copy link
Member

dwijnand commented Feb 8, 2020

Looks like test/files/neg/variance-holes.check differences (only looked at the first commit results).

@joroKr21
Copy link
Member Author

joroKr21 commented Feb 8, 2020 •
edited

Uh, I rebased to resolve conflicts and updated the check. It worked locally. Could that be some difference between bootstrapped and non-bootstrapped compiler?

EDIT: That was another check file.

@joroKr21
Plug many variance holes (pos and neg)
a13cee2 
The variance checks root in `RefChecks` for `TypeTree`s is modified
to check only the variance of `PolyType`s (this includes type lambdas).
Checking the definition of the `PolyType`s' owners is not correct,
because `relativeVariance` doesn't work for types in arbitrary position.

To check the variance of `PolyType`s we introduce a new type map:
`PolyTypeVarianceMap`. This is similar to the way variance is checked
for type lambdas in dotty, where type lambdas are encoded explicitly.

We also add explicit tracking for lower bounds of `PolyType`s.
For a type parameter of a `PolyType`, the variance should not be
flipped (or equivalently, flipped twice) in the lower bound.
This applies to both `ValidateVarianceMap` and `PolyTypeVarianceMap`.
It fixes variance bugs for types appering in the bounds of HKTs.

Finally, we don't skip local definitions entirely (this is unsound),
but instead cutoff `relativeVariance` at local boundaries.
@joroKr21
Add missing pattern match case in varianceInType
d7b572e
@joroKr21
Remove variance check for escaping locals
0765176
@joroKr21
Variances: replace lowerBoundStack with a single flag
a29da6a 
Add documentation to the two different variance validation methods:
`validateDefinition` and `validateVarianceOfPolyTypesIn`.
@SethTisue
Copy link
Member

@adriaanm will you be doing the final review on this, or do we need to find another reviewer?

@adriaanm
Copy link
Contributor

adriaanm commented Feb 17, 2020 •
edited

I won't have time to dig deeper than I already have. It all looked good on my initial pass. Glad to see the additional comments and cleanups since then! It would still be good to get another pair of 👀 on this IMO.

@SethTisue
Copy link
Member

@lrytz you want to be the eyes, or assign someone?

@SethTisue SethTisue assigned lrytz and unassigned adriaanm Feb 18, 2020
@SethTisue
Copy link
Member

SethTisue commented Feb 25, 2020 •
edited

@joroKr21 unless it gets left on the editing-room floor, there will be a brief shout-out in the next Scala Love episode to you and @milessabin for your bravery in tackling changes to Scala 2 type-checking

@SethTisue
Copy link
Member

@joroKr21 I have the same question as I had on your other PR: whether this should get the "release-notes" label

@joroKr21
Copy link
Member Author

joroKr21 commented Feb 26, 2020 •
edited

Perhaps it's worth mentioning that together with #8651 there are a lot of bugs fixed in the variance check department for higher-kinded types, type aliases, type lambdas and type refinements.
And that this is another inch closer to Dotty (which obviously already fixed all the bugs here).

I'm not sure what's required for PR titles and descriptions.

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Feb 26, 2020
lrytz
lrytz approved these changes Mar 19, 2020
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

LGTM to the best of my knowledge

@lrytz lrytz merged commit ae10be4 into scala:2.13.x Mar 19, 2020
@lrytz
Copy link
Member

lrytz commented Mar 19, 2020

Thank you, @joroKr21, for your continuous efforts!

@neko-kai
Copy link
Contributor

@joroKr21 Wow, thank you! 👏

@joroKr21 joroKr21 deleted the variance-holes branch March 19, 2020 17:17
@joroKr21 joroKr21 changed the title Plug many variance holes (pos and neg) Plug many variance holes (in higher-kinded types, refined types and private inner classes) Apr 23, 2020
@SethTisue SethTisue mentioned this pull request Apr 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriaanm adriaanm adriaanm left review comments

@Blaisorblade Blaisorblade Blaisorblade left review comments

@lrytz lrytz lrytz approved these changes

Assignees

@lrytz lrytz

Labels
release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.13.2
8 participants
@joroKr21 @Blaisorblade @SethTisue @dwijnand @adriaanm @lrytz @neko-kai @scala-jenkins