Add missing version of ValDef.let which also accepts flags

[Kalin-Rudnicki]

Seems like a pretty straight forward one.

• Theres a version of this in Symbol.newVal which accepts flags
• The function downstream of this accepts flags, and all other ValDef.let are just passing EmptyFlags by way of default arg

[Kalin-Rudnicki]

Not sure what the correct way to add this is so that it doesnt fail that bincompat test. Could use some advisement on that

[jchyb]

Hi @Kalin-Rudnicki. Thank you for submitting the PR. I have some comments about the supported flags (otherwise the PR looks good).
To resolve the mima issues, you have to add a filter in MimaFilters.scala (the exact line to add should be suggested in the CI logs somewhere). This is not a problem, because we can only merge changes like this when we release new minor versions.

[jchyb]

Let's also check if the flags are valid, as suggested in the doc:

Suggested change

	val vdef = tpd.SyntheticValDef(name.toTermName, rhs, flags)(using ctx.withOwner(owner))
	checkValidFlags(flags.toTermFlags, Flags.validValFlags)
	val vdef = tpd.SyntheticValDef(name.toTermName, rhs, flags)(using ctx.withOwner(owner))

Of course, as with my other comment, the flags should ideally be different here than in Flags.validValFlags

[Kalin-Rudnicki]

Accepted suggestion as-is. Very open to limiting allowed flags, if I knew what that allowed set was.

[jchyb]

Since we know that this new val is going to be defined in a Block (as opposed to class, object, etc.), we know that less flags/modifiers are going to be allowed there, so ideally we would limit this list here (e.g. access modifiers like Private, or Protected do not make sense to have here).

[Kalin-Rudnicki]

Is there a defined list where the allowed subset is defined? The only ones that I can think of, or at least that I was trying to use, were Lazy and Mutable.

[Kalin-Rudnicki]

To your point, if only a valid subset is allowed, it would probably make sense to check for what that subset is.

Also, just putting the idea out there, it would be really nice if quoted code let you splice in a Definition.

Feels kinda sad to have this:

{
  lazy val instance_a: oxygen.core.typeclass.Show[scala.Int] = oxygen.core.typeclass.Show.int
  lazy val instance_b: oxygen.core.typeclass.Show[scala.Option[java.lang.String]] = oxygen.core.typeclass.Show.option[java.lang.String](oxygen.core.typeclass.Show.string)
  final class $anon() extends oxygen.core.typeclass.Show.PreferBuffer[oxygen.meta.NewDeriveShowSpec.CaseClass1] {
    override def writeTo(builder: scala.collection.mutable.StringBuilder, value: oxygen.meta.NewDeriveShowSpec.CaseClass1): scala.Unit = {
      builder.append("CaseClass1(a = ")
      instance_a.writeTo(builder, value.a)
      builder.append(", b = ")
      instance_b.writeTo(builder, value.b)
      builder.append(")")
      ()
    }
  }

  (new $anon(): oxygen.core.typeclass.Show.PreferBuffer[oxygen.meta.NewDeriveShowSpec.CaseClass1])
}

This would be nicer if it wasnt so difficult:

{
  final class $anon() extends oxygen.core.typeclass.Show.PreferBuffer[oxygen.meta.NewDeriveShowSpec.CaseClass1] {
  
    private lazy val instance_a: oxygen.core.typeclass.Show[scala.Int] = oxygen.core.typeclass.Show.int
    private lazy val instance_b: oxygen.core.typeclass.Show[scala.Option[java.lang.String]] = oxygen.core.typeclass.Show.option[java.lang.String](oxygen.core.typeclass.Show.string)
  
    override def writeTo(builder: scala.collection.mutable.StringBuilder, value: oxygen.meta.NewDeriveShowSpec.CaseClass1): scala.Unit = {
      builder.append("CaseClass1(a = ")
      instance_a.writeTo(builder, value.a)
      builder.append(", b = ")
      instance_b.writeTo(builder, value.b)
      builder.append(")")
      ()
    }
  
  }

  (new $anon(): oxygen.core.typeclass.Show.PreferBuffer[oxygen.meta.NewDeriveShowSpec.CaseClass1])
}

Realizing there is probably some weirdness there to get things to line up, though, would probably need a new syntax...

        final case class Definitions(exprs: Seq[Ref])
        private def definitions: Definitions = 
          '%{ 
            private val myVal1: String = "value-1" 
            private def myVal2(input: String): String = s"value-2:$input" 
          }

        override def derive: Expr[Show[A]] =
          '{
            new PreferBuffer[A] {
              
              $%{ definitions as blah }

              override def writeTo(builder: mutable.StringBuilder, value: A): Unit = {
                builder.append(${ blah.exprs(0).asExprOf[String] })
                builder.append('\n')
                builder.append(${ blah.exprs(1).appliedToArg(Expr("hi").toTerm).asExprOf[String] })
              }
            }
          }

This is not the right place to be suggesting this... 😅

[jchyb]

Sorry for adding more work, let me know if you would want me to take this over. Also, previously I somehow didn't notice that there aren't any tests here (sorry about that...). Ideally we would have one. Something simple should be enough, like:
tests/run-macros/ValDef-let-flags/Macro_1.scala

import scala.quoted._

object Macro:
  inline def makeBlock(): Unit = ${makeBlockImpl}
  def makeBlockImpl(using Quotes): Expr[Unit] = {
    import quotes.reflect._
    ValDef.let(Symbol.spliceOwner, "x", '{0}.asTerm, Flags.Lazy) { x =>
      ValDef.let(Symbol.spliceOwner, "y", '{"string"}.asTerm, Flags.Mutable) { y =>
        '{
          ${Assign(x, '{1}.asTerm).asExpr}
          println("x: " + ${x.asExprOf[Int]} + " " + ${y.asExprOf[String]})
        }.asTerm
      }
    }.asExprOf[Unit]
  }

tests/run-macros/ValDef-let-flags/Test_2.scala

@main def Test = Macro.makeBlock()

tests/run-macros/ValDef-let-flags.check

x: 1, y: string

[jchyb]

Those won't do much here. Instead, put ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.quoted.Quotes#reflectModule#ValDefModule.let") in BackwardsBreakingChanges, somewhere under the // ReversedMissingMethodProblems are acceptable. See comment in Breaking changes since last LTS ` comment. Sorry for not being clear before

[Kalin-Rudnicki]

I can take another stab at it, and if I cant get it, Im happy with you getting it over the finish line.

Im not sure why other tests seem to be failing though... Are there some sort of bootstrap steps that I have not executed?

[jchyb]

Ah, it seems we recently added a test that depends on line positions in QuotesImpl.scala... Sorry about that. You can run sbt scala3-bootstrapped/testCompilation i23008 and then mv tests/neg-macros/i23008.check.out tests/neg-macros/i23008.check and commit the newly generated .check file. This should fix the issue.

[jchyb]

In my opinion, after checking what can work in a Block and what makes sense there, the flags consist of Final | Implicit | Lazy | Mutable | Given | Synthetic. To make things easier, something like private[QuotesImpl] def validValInLetFlags with the flags above should be defined in FlagsModule in QuotesImpl.scala, like it was before with validValFlags (then we would use it instead of validValFlags in the comment below and in the QuotesImpl.scala implementation).