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

Add reflect TypeLambda.paramVariances #17568

Merged
merged 2 commits into from
Oct 13, 2023
Merged

Conversation

nicolasstucki
Copy link
Contributor

Fixes #16734

@nicolasstucki nicolasstucki requested a review from bishabosha May 24, 2023 12:43
@nicolasstucki nicolasstucki marked this pull request as ready for review May 24, 2023 12:43
@nicolasstucki
Copy link
Contributor Author

@neko-kai could you check if the test example is representative of your use cases?

@neko-kai
Copy link
Contributor

neko-kai commented May 24, 2023

@nicolasstucki
Almost, there's the type lambda case

variances[[A] =>> K1Cov[A]]

I think that's the case that broke in 3.3.0 - before that declaredVariances returned +A for this type lambda, now it doesn't - so we'd need a function that tells the real computed variance of a A inside this type lambda - even if type lambda itself is not variance-annotated. Thank you for helping us here btw!

@neko-kai
Copy link
Contributor

neko-kai commented May 24, 2023

@nicolasstucki
There's also the type param case

variances[[A, F[+_]] =>> F[A]]

@bishabosha
Copy link
Member

bishabosha commented May 24, 2023

// from the test case
variances[[A, F[+B]] =>> F[A]]

currently the result for A is that it is still "declared" invariant, which is true, but would it be also possible to expose how the compiler computes the real variance of A? (which is what #16734 is asking for) or should macro authors compute that themselves?

@nicolasstucki nicolasstucki changed the title Add reflect TypeLambda.declaredVariances Add reflect TypeLambda.paramVariances May 24, 2023
println(variances[KFunky])
println(variances[[A, F[B]] =>> F[A]])
println(variances[[A, F[+B]] =>> F[A]])
println(variances[[A, F[-B]] =>> F[A]])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neko-kai to make these work, I used paramVariance instead of declaredVariances. Is this the functionality you need? See the variances in the check file. If it is what you need then you could also access this method reflectively as a workaround in 3.3.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolasstucki
Yes, thank you! That's what I'll do right now, but it'd be great to have a public API anyway, to stop using reflection for this in the future.

@nicolasstucki nicolasstucki requested a review from smarter May 24, 2023 15:32
@nicolasstucki
Copy link
Contributor Author

nicolasstucki commented May 24, 2023

Currently the result for A is that it is still "declared" invariant, which is true, but would it be also possible to expose how the compiler computes the real variance of A? (which is what #16734 is asking for) or should macro authors compute that themselves?

I changed that when I switched to paramVariances. Now we return the computed variances.

@@ -2212,6 +2212,8 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler
extension (self: TypeLambda)
def param(idx: Int): TypeRepr = self.newParamRef(idx)
def paramBounds: List[TypeBounds] = self.paramInfos
def paramVariances: List[Flags] =
Copy link
Member

Choose a reason for hiding this comment

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

Having this just on TypeLambda is a bit limiting since classes also have parameter with variance annotations, it would be nice if the same API worked for TypeLambda, TypeRef of classes and TypeRef of parameterized type aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added Symbol.paramVariance and added tests for all these cases. I did not add it on TypeRef because it is more consistent if we go to the definition of the class and ask for the variances on it.

/** Variance flags for of this type parameter.
*
* Variance flags can be one of `Flags.{Covariant, Contravariant, EmptyFlags}`.
* If this is not the symbol of a type parameter the result is `Flags.EmptyFlags`.
Copy link
Member

Choose a reason for hiding this comment

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

Could we simply assert that this is a type parameter to prevent misinterpreting EmptyFlags as Invariant on things which aren't type parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not show the distinction between type and term flags in the reflection API.

@nicolasstucki nicolasstucki requested a review from smarter July 6, 2023 08:34
@smarter smarter assigned nicolasstucki and unassigned smarter Oct 6, 2023
@bishabosha
Copy link
Member

needs a rebase

@nicolasstucki nicolasstucki merged commit 40cee39 into scala:main Oct 13, 2023
18 checks passed
@nicolasstucki nicolasstucki deleted the fix-16734 branch October 13, 2023 07:33
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 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.

TASTY: no way to access variance of parameters of a TypeParamRef type constructor
6 participants