Checking flags for new symbols using Quotes API is too restrictive

[pweisenburger]

Compiler version

3.3.2-RC1-bin-20230602-57c2b66-NIGHTLY

Minimized code

#16565 introduced checks for the flags when creating new symbols using the Quotes API. However, this makes it difficult to copy flags from existing symbols under expansion to new symbols. Below is a (not very useful but minimized) example that empties the body of DefDef nodes and changes the return type to Unit. It works under Scala 3.3.0 but fails in Scala 3.3.1 due to the check and the fact that DefDef the macro encounters has more flags than the check allows; in the example these are Flags.Artifact and Flags.Synthetic.

Overall, I would advocate for allowing the flags with the comment "Flags that could be allowed", plus Flags.Artifact. I think there is no reason to be overly restrictive.

Also, the code worked in 3.3.0; it would be nice to make 3.3.1 not break it.

Example

import scala.annotation.experimental
import scala.quoted.*

@experimental
inline def makeUnit(inline expr: Unit): Unit = ${ makeUnitImpl('expr) }

@experimental
def makeUnitImpl(expr: Expr[Unit])(using Quotes): Expr[Unit] =
  import quotes.reflect.*

  object transformer extends TreeMap:
    override def transformTerm(term: Term)(owner: Symbol) = term match
      case Lambda(_, _) =>
        val Block(List(lambda: DefDef), closure: Closure) = term: @unchecked
        val transformedLambda = transformSubTrees(List(lambda))(owner).head
        Block(List(transformedLambda), transformTerm(Closure(Ref(transformedLambda.symbol), closure.tpeOpt))(owner))
      case _ =>
        super.transformTerm(term)(owner)

    override def transformStatement(stat: Statement)(owner: Symbol) = stat match
      case DefDef(name, List(TermParamClause(_)), _, rhs) =>
        val MethodType(paramNames, paramTypes, resultType) = stat.symbol.info: @unchecked
        val info = MethodType(paramNames)(_ => paramTypes, _ => TypeRepr.of[Unit])
        val privateWithin = if stat.symbol.flags is Flags.Protected then stat.symbol.protectedWithin else stat.symbol.privateWithin

        //
        // this is the part where we want to use the same flags as the existing symbol `stat.symbol.flags`
        //
        val symbol = Symbol.newMethod(owner, name, info, stat.symbol.flags, privateWithin.fold(Symbol.noSymbol) { _.typeSymbol })
        DefDef(symbol, paramss => rhs map { _ => Literal(UnitConstant()) })
      case _ =>
        super.transformStatement(stat)(owner)

  transformer.transformTerm(expr.asTerm)(Symbol.spliceOwner).asExprOf[Unit]

import scala.annotation.experimental
import scala.quoted.*

@experimental
def test = makeUnit((x: Int) => x)

[jchyb]

Hi @pweisenburger. The code will still work if the -Xcheck-macros is removed. That option's main purpose is to find incoherencies in the created macro functions. It is tough to argue that e.g. Flags.Artifact, which by the definition, shows symbols solely generated by the compiler should be able to be used in a macro. I don't actually know where and for what purpose the flag is used in the compiler itself, but it is easy for me to imagine another macro using it to filter out some internally generated methods. An easy fix would be something like this:

val allowedFlags = Flags.Deferred | Flags.Final | Flags.Given | Flags.Implicit | Flags.JavaStatic | Flags.Local | 
  Flags.Method | Flags.Override | Flags.Private | Flags.PrivateLocal | Flags.Protected
val newFlags = stat.symbol.flags & allowedFlags
val symbol = Symbol.newMethod(owner, name, info, newFlags, privateWithin.fold(Symbol.noSymbol) { _.typeSymbol })

I understand the concern with the code suddenly breaking, but I am pretty sure the previous behavior was more of a buggy one. @nicolasstucki what do you think, should those two flags be reallowed?

[pweisenburger]

The code will still work if the -Xcheck-macros is removed. That option's main purpose is to find incoherencies in the created macro functions.

You're right, that's true. I still would prefer the compilation not to depend on the presence/absence of certain compiler flags; especially as the flag are set on the project using the macro (not the one implementing it).

An easy fix would be something like this

Thanks, that would indeed fix the issue. It still begs the questions

1. why the Symbol.new* methods don't do this automatically as this is the only way to pass the check, and
2. if it is even safe not to set the Artifact Flag on the DefDef in lambda expressions (as the compiler does) or if this could break compiler invariants?

[nicolasstucki]

We should allow Flags.Artifact and Flags.Synthetic.

[pweisenburger]

I added a pull request that allows Flags.Artifact and Flags.Synthetic and the other "flags that could be allowed" as they are valid flags and I think there is no reason to forbid them. Forbidding them only complicates the development macros.

In case you want to merge this, is there a chance to merge for 3.3.1? As this code worked in the last release 3.3.0, this would avoid a release that breaks the code.

[nicolasstucki]

This will probably be fixed in 3.3.2. The checks are only run with -Xcheck-macros and they will not break any production code in 3.3.1. They will only break macro tests in 3.3.1, the flag can be disabled in that version.

[pweisenburger]

They will only break macro tests in 3.3.1

Yes, this is what I was hoping to avoid; but I suppose I can work around it ...