Skip to content

Fix #7048: Check for splice stability #8097

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 2 commits into from
Jan 28, 2020

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki self-assigned this Jan 27, 2020
@nicolasstucki nicolasstucki force-pushed the fix-#7048 branch 3 times, most recently from 780038e to c3ec5d3 Compare January 27, 2020 10:27
@nicolasstucki nicolasstucki marked this pull request as ready for review January 27, 2020 13:53
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

{
val s = '{Option.empty[${T}]}
val r = '{identity($s)} // works
val r2 = '{identity(${s: Expr[Option[T]]})} // error // error
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not obvious why this is not accepted by the 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.

32 |      val r2 = '{identity[Option[T]](${s: Expr[Option[T]]})} // error // error
   |                                     ^^^^^^^^^^^^^^^^^^^^^
   |    (Test.this.given_Type_T : => quoted.Type[Test.this.T]) is not stable

All these can be fixed by making the given stable

val getT: Type[T] = T // need this to avoid getting `null`
given getT.type = getT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an extra test for it

Copy link
Contributor

Choose a reason for hiding this comment

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

Why it has to be stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type splices require a stable prefix to be correctly encoded. It also avoids having to deal with side effects in the transformation performed in ReifyQuotes. Most code receives the quoted type as a parameter and hence is stable.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a restriction in TASTy? This is unprecedented in Scala, as usually only the path in types needs to be stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a path in a type. A type that is inferred and then healed.

@nicolasstucki nicolasstucki merged commit e7a0f80 into scala:master Jan 28, 2020
@nicolasstucki nicolasstucki deleted the fix-#7048 branch January 28, 2020 13:13
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.

2 participants