Scala 3 support #11

mrdziuban · 2024-05-23T21:48:29Z

Closes #2

Macros

Just like @KuceraMartin in #3, I ran into scala/scala3#19493 and scala/scala3#19436 when trying to resolve a TypeMapper by importing from a DialectTypeMappers. As a workaround, I introduced additional implicit defs in the TableMapper companion object that instead rely on an implicit instance of DialectTypeMappers, i.e. in a macro:

// bad, causes a compiler crash
// TableMacro.scala
(dialect: DialectTypeMappers) => {
  import dialect.*
  summonInline[TypeMapper[t]]
}

// good
// TypeMapper.scala
implicit def stringFromDialectTypeMappers(implicit d: DialectTypeMappers): TypeMapper[String] = d.StringType

// TableMacro.scala
(dialect: DialectTypeMappers) => {
  given d: DialectTypeMappers = dialect
  summonInline[TypeMapper[t]]
}

Supporting changes

In addition to building out the macros in Scala 3, the following changes were necessary:

Update the generated code to ensure defs aren't too far to the left -- this is to silence Scala 3 warnings
Convert CharSequences to Strings explicitly -- see the error the Scala 3 compiler reported here
Remove try block without a corresponding catch -- see the warning the Scala 3 compiler reported here
Add types to implicit definitions
Mark renderSql as private[scalasql] instead of protected -- see the error the Scala 3 compiler reported here
Use Scala 3.4 -- this is a little unfortunate since it's not the LTS but it's necessary for the Scala 3 macros to match on higher kinded types like this. This type of match doesn't work in Scala 3.3
Replace _ wildcards with ? -- this is to silence Scala 3 warnings
Replace Foo with Bar in types with Foo & Bar -- this is to silence Scala 3 warnings
Add the -Xsource:3 compiler option for Scala 2 -- this is necessary to use the language features mentioned in points 7 and 8
Add a number of type annotations to method overrides -- this is to silence warnings reported by the Scala 2 compiler as a result of enabling -Xsource:3. All of the warnings relate to the inferred type of the method changing between Scala 2 and 3

The Scala 3 compiler reports an error: ```scala -- [E008] Not Found Error: /Users/matt/scalasql/scalasql/core/src/SqlStr.scala:53:33 53 | if (castParams) part + s"CAST(? AS $jdbcTypeString)" else part + "?" | ^^^^^^ |value + is not a member of CharSequence, but could be made available as an extension method. | |One of the following imports might make progress towards fixing the problem: | | import scala.math.Fractional.Implicits.infixFractionalOps | import scala.math.Integral.Implicits.infixIntegralOps | import scala.math.Numeric.Implicits.infixNumericOps | import sourcecode.Text.generate ```

The Scala 3 compiler reports a warning: ```scala -- [E002] Syntax Warning: /Users/matt/scalasql/scalasql/core/src/DbApi.scala:205:8 205 | try { | ^ | A try without catch or finally is equivalent to putting | its body in a block; no exceptions are handled. 206 | val res = stream(query, fetchSize, queryTimeoutSeconds)( 207 | qr.asInstanceOf[Queryable[Q, Seq[_]]], 208 | fileName, 209 | lineNum 210 | ) 211 | if (qr.isSingleRow(query)) { 212 | val results = res.take(2).toVector 213 | assert( 214 | results.size == 1, 215 | s"Single row query must return 1 result, not ${results.size}" 216 | ) 217 | results.head.asInstanceOf[R] 218 | } else { 219 | res.toVector.asInstanceOf[R] 220 | } 221 | } ```

The Scala 3 compiler reported an error: ```scala -- [E173] Reference Error: /Users/matt/scalasql/scalasql/query/src/Query.scala:74:50 74 | def renderSql(q: Q, ctx: Context): SqlStr = q.renderSql(ctx) | ^^^^^^^^^^^ |method renderSql in trait Renderable cannot be accessed as a member of (q : Q) from class QueryQueryable. | Access to protected method renderSql not permitted because enclosing class QueryQueryable in object Query | is not a subclass of trait Renderable in object SqlStr where target is defined | |where: Q is a type in class QueryQueryable with bounds <: scalasql.query.Query[R] ```

mrdziuban · 2024-05-24T00:15:26Z

Sorry I forgot to update the docs, just pushed that

lihaoyi · 2024-05-24T03:27:43Z

scalasql/core/src/SqlStr.scala

@@ -211,7 +211,7 @@ object SqlStr {
    new SqlStr(Array(s), emptyInterpArray, false, referencedExprs)

  trait Renderable {
-    protected def renderSql(ctx: Context): SqlStr
+    private[scalasql] def renderSql(ctx: Context): SqlStr


Is there a reason you changed this from protected to private?

Yeah, the Scala 3 compiler reported a reference error in Query.scala. It makes sense to me, I'm not sure why the Scala 2 compiler didn't catch it

-- [E173] Reference Error: /Users/matt/scalasql/scalasql/query/src/Query.scala:74:50 74 | def renderSql(q: Q, ctx: Context): SqlStr = q.renderSql(ctx) | ^^^^^^^^^^^ |method renderSql in trait Renderable cannot be accessed as a member of (q : Q) from class QueryQueryable. | Access to protected method renderSql not permitted because enclosing class QueryQueryable in object Query | is not a subclass of trait Renderable in object SqlStr where target is defined | |where: Q is a type in class QueryQueryable with bounds <: scalasql.query.Query[R]

lihaoyi · 2024-05-24T04:39:55Z

Left one small comment.

One slightly bigger ask: to demonstrate the Scala 3 stuff working, could you take the test/src/examples folder and add a test/src-3/examples/ folder containing a Scala3Example.scala exercising the Scala 3 syntax and show it working?

I pinged the #metaprogramming discord to see if I can find anyone to help review the Scala 3 macro stuff. It looks fine to me, but I'm not an expert. If nobody steps up to help review, I'd be happy to merge as-is given tests are passing

KuceraMartin · 2024-05-24T06:28:01Z

Hi @mrdziuban, amazing that you found this workaround! I had a brief look at the code and it looks good to me – pretty similar to what I was trying to do in my PR otherwise.

One question: do you really need to enforce V[_[_]] <: Product? I didn't notice it being used anywhere, and the Scala 2 macro doesn't do it so it's a small incompatibility.

mrdziuban · 2024-05-24T12:24:47Z

One slightly bigger ask: to demonstrate the Scala 3 stuff working, could you take the test/src/examples folder and add a test/src-3/examples/ folder containing a Scala3Example.scala exercising the Scala 3 syntax and show it working?

Yeah I can take a look at this. By Scala 3 syntax do you mean using things like fewer braces?

One question: do you really need to enforce V[_[_]] <: Product?

Yeah, it's necessary because TableQueryable has a <: Product constraint. Without the constraint in the macro, there's a compile error in the construct0 param here

-- [E007] Type Mismatch Error: /Users/matt/scalasql/scalasql/query/src-3/TableMacro.scala:85:14
85 |              Apply(
   |              ^
   |Found:    scala.quoted.Expr[V[scalasql.core.Sc]]
   |Required: scala.quoted.Expr[Product]
   |
   |where:    V is a type in method applyImpl with bounds <: [_[_$2]] =>> Any
86 |                TypeApply(
87 |                  Select.unique(New(TypeIdent(TypeRepr.of[V].typeSymbol)), "<init>"),
88 |                  List(TypeTree.of[Sc])
89 |                ),
90 |                constructorValueParams.zipWithIndex.map { case (param, i) =>
91 |                  val paramTpe = paramType(param)
92 |                  val tpe = subParam(paramTpe, TypeRepr.of[Sc]).asType
93 |                  val tpe2 = subParam(paramTpe, TypeRepr.of[SqlExpr]).asType
94 |                  (tpe, tpe2) match {
95 |                    case ('[t], '[t2]) => '{ queryable[t2, t](${ Expr(i) }).construct(args) }.asTerm
96 |                  }
97 |                }
98 |              ).asExprOf[V[Sc]]
   |
   | longer explanation available when compiling with `-explain`
-- [E007] Type Mismatch Error: /Users/matt/scalasql/scalasql/query/src-3/TableMacro.scala:169:57
169 |    '{ new Table.Metadata[V]($queryables, $walkLabels0, $queryable, $vExpr0) }
    |                                                         ^^^^^^^^^
    |Found:    (queryable :
    |  scala.quoted.Expr[(() => Seq[String], scalasql.core.DialectTypeMappers,
    |    scalasql.query.Table.Metadata.QueryableProxy) =>
    |    scalasql.query.Table.Internal.TableQueryable[V[scalasql.core.Expr²], Product
    |       & V[scalasql.core.Sc]]
    |  ]
    |)
    |Required: scala.quoted.Expr[(() => Seq[String], scalasql.core.DialectTypeMappers,
    |  scalasql.query.Table.Metadata.QueryableProxy) =>
    |  scalasql.core.Queryable[V[scalasql.core.Expr²], V[scalasql.core.Sc]]]
    |
    |where:    Expr  is a class in package scala.quoted
    |          Expr² is a trait in package scalasql.core
    |          V     is a type in method applyImpl with bounds <: [_[_$2]] =>> Any
    |
    | longer explanation available when compiling with `-explain`

lihaoyi · 2024-05-24T12:35:20Z

Yeah just an example without the braces should be enough to sanity check that things work on Scala 3 (and that we didn't accidentally mix up the scala versions in Mill or something)

mrdziuban · 2024-05-24T12:39:14Z

👍 done, I ported the H2Example

lihaoyi · 2024-05-25T00:01:40Z

scalasql/query/src-3/TableMacro.scala

+              Apply(
+                TypeApply(
+                  Select.unique(New(TypeIdent(TypeRepr.of[V].typeSymbol)), "<init>"),
+                  List(TypeTree.of[SqlExpr])
+                ),


Could this AST-constructions of the new Foo call be moved into a named helper method? To help avoid duplication (it appears three times in this PR) and to give it a name so it's obvious what it is without having to read through the AST structure

Addressed this along with the deduplication of the paramTpe, tpe, and tpe2 logic

lihaoyi · 2024-05-25T00:03:34Z

scalasql/query/src-3/TableMacro.scala

+                val paramTpe = paramType(param)
+                val tpe = subParam(paramTpe, TypeRepr.of[Sc])
+                val tpe2 = subParam(paramTpe, TypeRepr.of[SqlExpr])
+                (tpe.asType, tpe2.asType) match {
+                  case ('[t], '[t2]) =>


Could this chain of logic be moved into a helper method? I understand there's some duplication in the Scala 2 equivalent logic, but here it's even more verbose, so I think it's worth DRYing up

I had tried this initially and ran into a compile error about leaking Quotes or something like that. I must have been doing something wrong though because I just tried again and it worked 🎉

lihaoyi · 2024-05-25T00:05:48Z

scalasql/query/src-3/TableMacro.scala

+    val queryables = '{ (dialect: DialectTypeMappers, n: Int) =>
+      {
+        @annotation.nowarn("msg=unused")
+        given d: DialectTypeMappers = dialect


Do we actually need the name d here, or can we do given DialectTypeMappers = dialect?

Also, why do we need the nowarn annotation here, since I assume the DialectTypeMappers instance should be used by the following generated summonInline call?

Nope, no need for the name, good call.

The compiler reports it as unused, I guess because in the context of the macro definition it doesn't know that it's used yet, it only knows once the result of the macro is expanded and typechecked...?

That said, I think this could trigger warnings in downstream users code if either of the following are true:

The class has no fields, or only has fields with TypeMapper instances defined outside of DialectTypeMappers, so this instance isn't necessary -- I confirmed that this is then reported as unused after macro expansion

The class has fields with TypeMapper instances defined within DialectTypeMappers, so this instance is necessary -- in this case, if the user has the -Wunused:nowarn option enabled, then the compiler may report this @annotation.nowarn as one that doesn't suppress any warnings. I haven't been able to confirm this, but it's theoretically possible

To work around both of these, I updated this to ensure the given is always used, both in the macro definition and after macro expansion:

given DialectTypeMappers = dialect lazy val _ = summon[DialectTypeMappers]

lihaoyi · 2024-05-25T00:09:01Z

scalasql/query/src-3/TableMacro.scala

+          if (isTypeParamType(param))
+            paramType(param) match {
+              case AppliedType(tpeCtor, _) =>
+                tpeCtor.asType match {
+                  case '[
+                      type t[_[_]]; t] =>
+                    '{ summonInline[Table.ImplicitMetadata[t]].value.walkLabels0() }
+                }
+            }


This whole block appears twice; could it be moved into a named helper method, with the final .walkLabels0() or .vExpr(tableRef, dialect) passed as a lambda or something to configure it?

Yeah good idea, updated to add a helper method for this

lihaoyi

Gave it as detailed a review as I could. Looks good to me overall, just some minor tidying and I think it's good to go.

…icitMetadata` method.

lihaoyi · 2024-05-25T14:39:30Z

Thanks @mrdziuban !

mrdziuban added 11 commits May 22, 2024 10:56

Enable Scala 3 in build.

88bd111

Prevent indentation warnings on Scala 3.

2356551

Add types to implicits.

afed97f

Upgrade to Scala 2.13.14 and 3.4.2, use new Scala 3 syntax.

c082117

Add Scala 3 TableMacros.

44a04a2

Get scalafix working again.

f73c048

Remove need for Internal.

24eba9d

Update docs.

95607da

lihaoyi reviewed May 24, 2024

View reviewed changes

Port H2Example to Scala 3 with fewer braces.

405ab33

mrdziuban force-pushed the scala-3-support branch from e12ca39 to 405ab33 Compare May 24, 2024 13:33

lihaoyi reviewed May 25, 2024

View reviewed changes

mrdziuban added 3 commits May 25, 2024 10:09

Dedupe class construction with constructV method.

34cd660

Remove name from given DialectTypeMappers, ensure it's always used.

5fe569a

Dedupe retrieval of data from Table.ImplicitMetadata with `fromImpl…

0f142b3

…icitMetadata` method.

lihaoyi merged commit 7cbc5cc into com-lihaoyi:main May 25, 2024
6 checks passed

lihaoyi mentioned this pull request May 27, 2024

[WIP] Scala 3 #3

Closed

5 tasks

mrdziuban deleted the scala-3-support branch May 31, 2024 03:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scala 3 support #11

Scala 3 support #11

mrdziuban commented May 23, 2024

mrdziuban commented May 24, 2024

lihaoyi May 24, 2024

mrdziuban May 24, 2024

lihaoyi commented May 24, 2024

KuceraMartin commented May 24, 2024

mrdziuban commented May 24, 2024 •

edited

Loading

lihaoyi commented May 24, 2024

mrdziuban commented May 24, 2024

lihaoyi May 25, 2024 •

edited

Loading

mrdziuban May 25, 2024

lihaoyi May 25, 2024

mrdziuban May 25, 2024

lihaoyi May 25, 2024

mrdziuban May 25, 2024

lihaoyi May 25, 2024

mrdziuban May 25, 2024

lihaoyi left a comment

lihaoyi commented May 25, 2024

Scala 3 support #11

Scala 3 support #11

Conversation

mrdziuban commented May 23, 2024

Macros

Supporting changes

mrdziuban commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi commented May 24, 2024

KuceraMartin commented May 24, 2024

mrdziuban commented May 24, 2024 • edited Loading

lihaoyi commented May 24, 2024

mrdziuban commented May 24, 2024

lihaoyi May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi left a comment

Choose a reason for hiding this comment

lihaoyi commented May 25, 2024

mrdziuban commented May 24, 2024 •

edited

Loading

lihaoyi May 25, 2024 •

edited

Loading