Skip to content

Add missing version of ValDef.let which also accepts flags #23388

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add missing version of ValDef.let which also accepts flags #23388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,12 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler
def unapply(vdef: ValDef): (String, TypeTree, Option[Term]) =
(vdef.name.toString, vdef.tpt, optional(vdef.rhs))

def let(owner: Symbol, name: String, rhs: Term, flags: Flags)(body: Ref => Term): Term =
Symbol.checkValidFlags(flags.toTermFlags, Flags.validValFlags)
val vdef = tpd.SyntheticValDef(name.toTermName, rhs, flags)(using ctx.withOwner(owner))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

val ref = tpd.ref(vdef.symbol).asInstanceOf[Ref]
Block(List(vdef), body(ref))

def let(owner: Symbol, name: String, rhs: Term)(body: Ref => Term): Term =
val vdef = tpd.SyntheticValDef(name.toTermName, rhs)(using ctx.withOwner(owner))
val ref = tpd.ref(vdef.symbol).asInstanceOf[Ref]
Expand Down Expand Up @@ -2863,7 +2869,7 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler

def noSymbol: Symbol = dotc.core.Symbols.NoSymbol

private inline def checkValidFlags(inline flags: Flags, inline valid: Flags): Unit =
private[QuotesImpl] inline def checkValidFlags(inline flags: Flags, inline valid: Flags): Unit =
xCheckMacroAssert(
flags <= valid,
s"Received invalid flags. Expected flags ${flags.show} to only contain a subset of ${valid.show}."
Expand Down
16 changes: 16 additions & 0 deletions library/src/scala/quoted/Quotes.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,22 @@ trait Quotes { self: runtime.QuoteUnpickler & runtime.QuoteMatching =>
def copy(original: Tree)(name: String, tpt: TypeTree, rhs: Option[Term]): ValDef
def unapply(vdef: ValDef): (String, TypeTree, Option[Term])

/** Creates a block `{ val <name> = <rhs: Term>; <body(x): Term> }`
*
* Usage:
* ```
* ValDef.let(owner, "x", rhs1, Flags.Lazy) { x =>
* ValDef.let(x.symbol.owner, "y", rhs2, Flags.Mutable) { y =>
* // use `x` and `y`
* }
* }
* ```
*
* @param flags extra flags to with which the symbol should be constructed. Can be `Private | Protected | Override | Deferred | Final | Param | Implicit | Lazy | Mutable | Local | ParamAccessor | Module | Package | Case | CaseAccessor | Given | Enum | JavaStatic | Synthetic | Artifact`
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

@Kalin-Rudnicki Kalin-Rudnicki Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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... 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

*/
// Keep: `flags` doc aligned with QuotesImpl's `validValFlags`
def let(owner: Symbol, name: String, rhs: Term, flags: Flags)(body: Ref => Term): Term

/** Creates a block `{ val <name> = <rhs: Term>; <body(x): Term> }`
*
* Usage:
Expand Down
3 changes: 3 additions & 0 deletions project/MiMaFilters.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ object MiMaFilters {

ProblemFilters.exclude[DirectMissingMethodProblem]("scala.Conversion.underlying"),
ProblemFilters.exclude[MissingClassProblem]("scala.Conversion$"),

ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.quoted.Quotes#reflectModule#ValDefModule.let"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.quoted.Quotes#reflectModule#ValDefModule.let"),
Comment on lines +22 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

),

// Additions since last LTS
Expand Down
Loading