Skip to content

Remove splice internal representation from public API #6073

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 3 commits into from
Mar 21, 2019

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Mar 11, 2019

This removes the $splice public member from Expr that was only for compiler internal representation.

sealed abstract class Expr[+T] {
-  final def `$splice`: T = throw new Error("splice should have been compiled away")
  ...
}

And remove the asInstanceOfs added around each splice to make pickling/unpickling retain phase consistency on types.

With this change, we also start hitting an instance of #4867 that disallow us to write splices of expressions of union types. If #4867 is fixed we would not have this issue.

@nicolasstucki nicolasstucki self-assigned this Mar 11, 2019
@nicolasstucki nicolasstucki force-pushed the remove-splice-private-method branch 6 times, most recently from 4674ae6 to 4fa9ada Compare March 12, 2019 16:37
@nicolasstucki nicolasstucki force-pushed the remove-splice-private-method branch 4 times, most recently from bc9fd8f to a3c32a7 Compare March 15, 2019 09:03
@nicolasstucki nicolasstucki marked this pull request as ready for review March 15, 2019 10:22
@nicolasstucki nicolasstucki force-pushed the remove-splice-private-method branch from 16c186c to a946148 Compare March 15, 2019 10:26
@nicolasstucki nicolasstucki requested a review from odersky March 15, 2019 10:28
@nicolasstucki nicolasstucki force-pushed the remove-splice-private-method branch 2 times, most recently from fae3de1 to 1a22b7c Compare March 15, 2019 11:05
@@ -203,7 +203,7 @@ object StagedTuple {
if (!specialize) '{dynamic_++[Self, That]($self, $that)}
else {
def genericConcat(xs: Expr[Tuple], ys: Expr[Tuple]): Expr[Tuple] =
fromArrayStaged[Tuple]('{${ toArrayStaged(xs, None) } ++ ${ toArrayStaged(ys, None) }}, None)
fromArrayStaged[Tuple]('{${ toArrayStaged(xs, None) } ++ (${ toArrayStaged(ys, None) }: Array[Object])}, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the ascription needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the desugaring of ${...} to def exprSplice[T](x: Expr[T]): T we get now need to infer T as well as the types of ++.

The acctual error is

[error] 206 |        fromArrayStaged[Tuple]('{${ toArrayStaged(xs, None) } ++ ${ toArrayStaged(ys, None) }}, None)
[error]     |                                                                    ^^^^^^^^^^^^^^^^^^^^^^^
[error]     |    Found:    quoted.Expr[Array[Object]]
[error]     |    Required: quoted.Expr[scala.collection.GenTraversableOnce[Object]]

Copy link
Contributor

Choose a reason for hiding this comment

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

That's problematic. We should avoid type ascriptions that need to be added for obscure reasons. Should we change the desugaring instead?

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 is an instance of #6126 which is quite specific to ++ with Arrays. If fixed this would not be an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll have a look at #6126.

`scala.internal.Quoted.exprSplice` can be used for the same purpose by explicily setting
the type parameter of `exprSplice` to a phase correct type alias.
Currently type inference for `Expr` of union types is not precise enough as reported in scala#4867.
This implies that currently we cannot have macros of the form

```scala
inline def foo(x: T) <: X | Y = ${ ... }
```

but we can always default back to the less informative alternative

```scala
inline def foo(x: T) <: Any = ${ ... }
```

Both form will refine the type to the type of the expression returned by the macro.
@nicolasstucki nicolasstucki force-pushed the remove-splice-private-method branch from d1b2bab to 59a170e Compare March 20, 2019 16:32
@odersky odersky merged commit 036096a into scala:master Mar 21, 2019
@ghost ghost removed the stat:needs review label Mar 21, 2019
@allanrenucci allanrenucci deleted the remove-splice-private-method branch March 21, 2019 19:27
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