Skip to content

Fix #5965: Make scala.quoted.Type poly-kinded #5988

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 5 commits into from
Mar 8, 2019

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Feb 26, 2019

No description provided.

@nicolasstucki nicolasstucki force-pushed the fix-#5965 branch 2 times, most recently from 535d9fc to 8484b2f Compare February 26, 2019 17:02
@nicolasstucki
Copy link
Contributor Author

To make this work we also need to bootstrap scala.tasty._. #5990 is the first step towards this goal.

@milessabin
Copy link
Contributor

Very interested to see where this goes.

@nicolasstucki
Copy link
Contributor Author

@liufengyun maybe we should wait until #5990 is merged to merge this one. That PR will simplify the changes that are currently in QuotedOpsImpl.scala and QuotedOps.scala

@nicolasstucki
Copy link
Contributor Author

Also, rebasing #5990 on this one would be more complex than the other way around.

import scala.reflect.ClassTag
import scala.runtime.quoted.Unpickler.Pickled

sealed abstract class Type[T <: AnyKind] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I only added <: AnyKind and src-non-bootstrapped/scala/quoted/Type.scala contatins the current version without the bound.

liufengyun
liufengyun previously approved these changes Mar 7, 2019
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM

tree.tpe.stripTypeVar match {
case tp: AppliedType =>
val splicedType = tp.tycon.asInstanceOf[TypeRef].prefix.termSymbol
AppliedTypeTree(transformSplice(ref(splicedType).select(tpnme.splice)), tp.args.map(TypeTree(_))).withSpan(tree.span)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not obvious why there is no need to transformSplice on the type arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should call transform on the arguments. I will double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this whole branch is useless. This is already handled by the transformation done in the Staging phase.

@@ -0,0 +1,15 @@
{
val y: collection.immutable.List[scala.Int] = scala.List.apply[scala.Int](1, 2, 3)
(y: collection.immutable.List[evidence$1$_~$1])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

evidence$1$_~$1 was leaking here

@nicolasstucki nicolasstucki dismissed liufengyun’s stale review March 8, 2019 10:38

Need to review last commit

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

Nice simplification 👍

Enable kind polimorphism by default by replacing `-Ykind-polymorphism` with `-Yno-kind-polymorphism`.
This branch should have been removed whe the logic in the staging Staging phase was split
@liufengyun liufengyun merged commit 33deb72 into scala:master Mar 8, 2019
@ghost ghost removed the stat:needs review label Mar 8, 2019
@liufengyun liufengyun deleted the fix-#5965 branch March 8, 2019 14:12
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