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 toExprOfTuple in scala.quoted #7037

Merged
merged 3 commits into from Aug 19, 2019

Conversation

@nicolasstucki
Copy link
Contributor

commented Aug 13, 2019

No description provided.

@nicolasstucki nicolasstucki self-assigned this Aug 13, 2019

@milessabin

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

LGTM!

I think we also need something to construct a tuple type given a Seq[Type].

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Still need to add at least 23 test

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

What would be the signature of the Seq[Type]?

@milessabin

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

What would be the signature of the Seq[Type]?

How about,

def tupleType(tpes: Seq[reflect.Type]): reflect.Type

Or should that be quoted.Type? I have to be honest, I don't yet have a clear mental model of when I should be working with quoted vs tasty.reflect types.

@milessabin

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

The context is, I want to be able to generate a trait implementation of the form,

class FooImpl extends Foo {
  type ElemTypes = <tuple type here>
  val Elems: ElemTypes = <tuple term here>
}

where the tuple elements are typed tasty trees (from an annots call) and the tuple element types are the tasty types recovered from those trees.

I'm open to changing the approach so long as I can produce a term of that form from whatever I get back from annots.

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

That type for tuple is completely unrelated to this PR. I will add work on It in a different PR.

@nicolasstucki nicolasstucki force-pushed the dotty-staging:add-seq-to-expr-tuple branch from 8979768 to 0a12ad3 Aug 15, 2019

@milessabin

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

That type for tuple is completely unrelated to this PR. I will add work on It in a different PR.

Awesome ... thanks. And let me know if there's anything I can do to help here.

@nicolasstucki nicolasstucki marked this pull request as ready for review Aug 15, 2019

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

That type for tuple is completely unrelated to this PR. I will add work on It in a different PR.

Awesome ... thanks. And let me know if there's anything I can do to help here.

Could you make minimal full example/test where the only thing missing is the type of the tuple?

@nicolasstucki nicolasstucki requested a review from milessabin Aug 15, 2019

@anatoliykmetyuk

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Possibly related: #7047 though it lifts tuples, not sequences, and is currently blocked.

Add test
Test does not compile with type annotations uncommented.
@milessabin

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@nicolasstucki I'm not sure if that's exactly what you meant by "Could you make minimal full example/test where the only thing missing is the type of the tuple?", but the test I've just added doesn't compile if you uncomment the type annotations, failing with,

-- Error: tests/run/quote-toExprOfTuple/Test_2.scala:2:19 ----------------------
2 |  val t2 = Macro.t2(23, "foo")
  |           ^^^^^^^^^^^^^^^^^^^
  |Exception occurred while executing macro expansion.
  |scala.tasty.reflect.ExprCastError: Expr: {
  |  Tuple2.apply[Any, Any](
  |    {
  |      {
  |        23
  |      }
  |    }
  |  , 
  |    {
  |      {
  |        "foo"
  |      }
  |    }
  |  )
  |}
  |did not conform to type: (Int, String)
  |
  |     at dotty.tools.dotc.tastyreflect.ReflectionCompilerInterface.QuotedExpr_cast(ReflectionCompilerInterface.scala:1767)
  |     at dotty.tools.dotc.tastyreflect.ReflectionCompilerInterface.QuotedExpr_cast(ReflectionCompilerInterface.scala:1759)
  |     at scala.tasty.reflect.QuotedOps$QuotedExprAPI.cast(QuotedOps.scala:13)
  |     at Macro$.impl2(Macro_1.scala:12)
  |
  | This location is in code that was inlined at Test_2.scala:2
one error found
[error] Nonzero exit code returned from runner: 1
[error] (dotty-compiler-bootstrapped / Compile / runMain) Nonzero exit code returned from runner: 1
[error] Total time: 3 s, completed 15-Aug-2019 16:05:29
sbt:dotty>
@@ -0,0 +1,14 @@
import scala.quoted._

object Macro {

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Aug 15, 2019

Author Contributor

This test should be in tests/run-macros

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Aug 15, 2019

Author Contributor

That fixed it. It was probably a bad interaction with the non bootstrapped compiler.

@nicolasstucki nicolasstucki requested a review from liufengyun Aug 17, 2019

@liufengyun
Copy link
Contributor

left a comment

LGTM

@nicolasstucki nicolasstucki merged commit cea5a62 into lampepfl:master Aug 19, 2019

1 check passed

continuous-integration/drone/pr Build is passing
Details

@nicolasstucki nicolasstucki deleted the dotty-staging:add-seq-to-expr-tuple branch Aug 19, 2019

@milessabin

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

I'm a bit confused ... the test I added didn't compile with the type annotation uncommented, which looked wrong to me.

Was that only the case because the test was being compiled with the wrong flags? If that's so then the test should have the type annotations restored, surely?

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

I will have a second look at it

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

Followed by #7076

@anatoliykmetyuk anatoliykmetyuk added this to the 0.18 Tech Preview milestone Aug 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.