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

Pick right type parameters for opaqueToBounds in TreeUnpickler #13206

Merged
merged 10 commits into from
Jul 30, 2021
Merged

Pick right type parameters for opaqueToBounds in TreeUnpickler #13206

merged 10 commits into from
Jul 30, 2021

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jul 29, 2021 •
edited by griggt
Loading

We passed the TypeParamRefs instead of the type param symbols when unpickling (but not in Namer).

Fixes #12945
Fixes #13001
Fixes #13190

Also:

Fixes #12950
Fixes #13128
Fixes #12927

@odersky odersky changed the title Pick right type parameters in for opaqueToBounds in TreeUnpickler Pick right type parameters for opaqueToBounds in TreeUnpickler Jul 29, 2021
dwijnand
dwijnand approved these changes Jul 29, 2021
View reviewed changes
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.

The 3-issue wombo!

@griggt
Copy link
Contributor

griggt commented Jul 29, 2021

Seems like this also fixes #12950 and #13128

@smarter
Copy link
Member

smarter commented Jul 29, 2021

Should this be backported to the release branch given the number of issues it fixes?

@smarter smarter added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 29, 2021
@griggt
Copy link
Contributor

griggt commented Jul 29, 2021

Also fixes #12927 (I believe this one needs an sbt scripted test)

@soronpo
Copy link
Contributor

soronpo commented Jul 29, 2021

Should this be backported to the release branch given the number of issues it fixes?

Silly me opening all those issues 😜
Well, I had no guarantee that they are related.

@dwijnand
Copy link
Member

Seems like this also fixes #12950 and #13128
Also fixes #12927 (I believe this one needs an sbt scripted test)

@odersky are you in the process of pulling in these test cases, or shall we merge this as is?

@odersky
Copy link
Contributor Author

odersky commented Jul 30, 2021

I am still trying to figure out the failure in onnx-scala. I'll also add tests that work.

odersky added 5 commits July 30, 2021 17:33
@odersky
Pretty-print LambdaParams
882b141 
Customized printer instead of falling back to toString
@odersky
Refactorings for clarity and to avoid unnecessary computation
eadee84
@odersky
Fix definition of Tensor in onnx-Scala
510aefc 
Type parameter Ax needs to be declared explicitly as covariant since Tensor is opaque.

This fell through the cracks before since under separate compilation type parameters
of opaque types got assigned structural variances. But this is incorrect. Under joint
compilation there would be an error.
@odersky
Test case for scala#12950
565c5aa
@odersky
Test case for scala#13128
f89ccf4
@odersky
Copy link
Contributor Author

odersky commented Jul 30, 2021

Also:

Fixes #12950
Fixes #13128

@odersky
Copy link
Contributor Author

odersky commented Jul 30, 2021

I have added tests for everything except #12927. I leave adding a test for that to someone who has more sbt foo than I.

@odersky odersky mentioned this pull request Jul 30, 2021
Closed
@odersky
Copy link
Contributor Author

odersky commented Jul 30, 2021 •
edited
Loading

onnx-scala was frustrating. I first could not get it to test separately. Once I managed that, I thought that it was a problem in unpickler. Once I fixed that with great effort I found out that all other fixed issues broke again. So I finally figured out that the original version of unpickler was correct and the fault was in onnx-scala.

The fault was this:

The type parameters of a regular type alias get variances as determined by the right hand side. For instance in

type L[X] = List[X]

X would be implicitly covariant. But for opaque types that's not true; the variance has to be given explicitly:

opaque type L[+X] = List[X]

Before this PR, and only under separate compilation, the type parameters of opaque types would get the variances from the right hand side. That's what made onnx-scala compile before.

@griggt
Copy link
Contributor

griggt commented Jul 30, 2021

I'll push a test for #12927

@odersky
Copy link
Contributor Author

odersky commented Jul 30, 2021

@griggt Great. Thanks!

@odersky
Copy link
Contributor Author

odersky commented Jul 30, 2021

What is the branch on which one has to rebase the backport?

@odersky odersky merged commit 798faa6 into scala:master Jul 30, 2021
@odersky odersky deleted the fix-13190 branch July 30, 2021 18:38
@smarter
Copy link
Member

smarter commented Jul 30, 2021

It's release-3.0.2: https://github.com/lampepfl/dotty/tree/release-3.0.2

@dwijnand
Copy link
Member

Hepta-kill!

@smarter smarter added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Jul 31, 2021
@Daenyth
Copy link

Daenyth commented Aug 2, 2021

So this should be in the 3.0.3 release?

@dwijnand
Copy link
Member

dwijnand commented Aug 2, 2021

The backport is #13219, and it needs work to land.

@smarter smarter mentioned this pull request Aug 11, 2021
Merged
@dwijnand dwijnand removed the backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" label Sep 28, 2021
@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwijnand dwijnand dwijnand approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.1.0
7 participants
@odersky @griggt @smarter @soronpo @dwijnand @Daenyth @Kordyjan