-Wunused reporting should wait until macro expansion for analysis

[markehammons]

Compiler version

3.3.0-RC2

Minimized code

This is hard to minimize because it's macro related. I'll add an example macro in a bit, but lets do a thought experiment. Imagine a macro whose job is to generate a method body for a method. We'll call this macro adder, and it will add together all the Int parameters of the method. It should be implementable via:

  def findMethodSymbol(using q: Quotes)(s: q.reflect.Symbol): Symbol =
    import quotes.reflect.*
    if s.isDefDef then s
    else findMethodSymbol(s.owner)
  end findMethodSymbol


  inline def adder: Int = ${
    adderImpl
  }
  def adderImpl(using Quotes): Expr[Int] =
    import quotes.reflect.*

    val inputs = findMethodSymbol(Symbol.spliceOwner).tree match
      case DefDef(_, params, _, _) =>
        params.last match
          case TermParamClause(valDefs) =>
            valDefs.map(vd => Ref(vd.symbol).asExprOf[Int])
    inputs.reduce((exp1, exp2) => '{$exp1 + $exp2})

Its usage will turn

def myMethod(a: Int, b: Int, c: Int) = adder

into

def myMethod(a: Int, b: Int, c: Int) = a + b + c

Right now, the -Wunused scanning happens too early, and it just sees adder, and the a, b, and c parameters unused. It does not see the transformed result of adder using a, b, or c.

Expectation

I would like, if possible, that the unused analysis is pushed back to after macro expansion, so that macro usage like above doesn't trigger unused explicit parameter warnings.

[mrdziuban]

I think I'm running into this as well with -Wunused:implicits and this simple code using kittens:

https://scastie.scala-lang.org/TpJADGRTQ7u6bhnud2xiug

import cats.Eq

case class Foo[A](a: List[A])
object Foo {
  given eq[A: Eq]: Eq[Foo[A]] = cats.derived.semiauto.eq[Foo[A]]
         // ^
         // unused implicit parameter
}

Removing the context bound causes a compile error on the RHS of the instance

[mrdziuban]

Do I understand correctly that the -Ywarn-macros:after option controlled this behavior in scala 2?

[Kordyjan]

We'll call this macro adder, and it will add together all the Int parameters of the method. Its usage will turn

def myMethod(a: Int, b: Int, c: Int) = adder

into

def myMethod(a: Int, b: Int, c: Int) = a + b + c

The funny thing is that this macro is really hard to write. It is not impossible, but it probably would be very fragile and cannot be moved around without risk of breaking it. Macros API seems to be created in such a way as to make the implementation of adder hard. And this macro should be considered a super-not-idiomatic hack. The idiomatic way of doing that would be either passing the parameters directly to the adder (that should accept inlined varargs) or writing it as

def myMethod = adder("a", "b", "c")

that is expanded into

def myMethod = (a: Int, b: Int, c: Int) => a + b + c

It can be even done using macro annotations, which are an experimental feature in 3.3:

@adder
def myMethod(a: Int, b: Int, c: Int) = ???

I don't think we should in any way prioritize supporting code written in such a way, and definitely, it shouldn't block the 3.3.0 release.

[Kordyjan]

@mrdziuban I'm not sure that I understand this example correctly, but the semiauto derivation seems to be existing only for the sake of compatibility with Scala 2 as it can be easily achieved in Scala 3 using derivation.

I think we can check if we can do something to eliminate false positives in cases such as yours with some risk of having false negatives.

[markehammons]

The funny thing is that this macro is really hard to write. It is not impossible, but it probably would be very fragile and cannot be moved around without risk of breaking it. Macros API seems to be created in such a way as to make the implementation of adder hard. And this macro should be considered a super-not-idiomatic hack. The idiomatic way of doing that would be either passing the parameters directly to the adder (that should accept inlined varargs) or writing it as

It's not particularly hard. It requires analysis of the half-constructed DefDef that's myMethod, but it's doable with code like such: ```scala def findMethodSymbol(using q: Quotes)(s: q.reflect.Symbol): Symbol = import quotes.reflect.* if s.isDefDef then s else findMethodSymbol(s.owner) end findMethodSymbol def adderImpl(using Quotes): Expr[Int] = import quotes.reflect.* val inputs = findMethodSymbol(Symbol.spliceOwner).tree match case DefDef(_, params, _, _) => params.last match case TermParamClause(valDefs) => valDefs.map(vd => Ref(vd.symbol).asExprOf[Int]) inputs.reduce((exp1, exp2) => '{$exp1 + $exp2}) ``` Can this implementation be error prone? Yes, multi-clause methods won't work properly, nor methods that have non-int params. Guards against such things can easily be added though. The problem with the lambda example is that it's not as performant as code that defines the body of a method. Macro annotations probably have an easier time finding the symbol they're working on, and would be a welcome addition, but you'd still have this warning in the case of a macro annotation that updates the body of myMethod with `a + b + c`.

[Kordyjan]

Still, I think

def myMethod(a: Int, b: Int, c: Int) = adder(a, b, c)

is the best way to go here, from the perspective of readability, performance, and maintainability, and should be considered the best way of doing this.

Of course, I think we can revisit why the checking of unused symbols was put where it was. Your problem will be fixed one way or the other in the future. However, I still believe that use cases affected by this problem are so rare that we don't need to delay the 3.3 because of that, and it can be fixed later (this is why I removed the issue from the milestone, and not closed it).

[markehammons]

Your version is not wrong, but may be undesirable in some cases. adder here is just a reduced sample of something I'm doing with real code:

  def abs(i: Int): Int = binding

expanding into something like

def abs(i: Int): Int =  TransitionModule.methodReturn(IntDescriptor, absHandle.invokeExact(TransitionModule.methodArgument(IntDescriptor, i): Int))

Having the user needing to manually specify inputs to the macro doesn't really help because in this case, the real input to the macro is the method definition it's attached to.

Yes, macro annotations will likely end up being a better tool for this, but they aren't here yet, and for the work I'm doing, I'm frequently having to find non-experimental alternatives to things that would be better done with features that are still marked experimental in Scala 3.

I understand that this isn't high priority because the "proper" support in Scala 3 for this pattern is still marked experimental, but it's still kind of disappointing that this pattern will produce false-positives for unused params till perhaps much later. Especially since Scala 3.3 is supposed to be an LTS version and I'd like to have a version of my library working nicely for it.

[Kordyjan]

Especially since Scala 3.3 is supposed to be an LTS version and I'd like to have a version of my library working nicely for it.

I cannot see anything preventing the fix to be released for the LTS version. It won't be in 3.3.0, but it is probable that we will squeeze it into 3.3.2 which is only 3 or 4 months away, and still part of the LTS line.

[mrdziuban]

@mrdziuban I'm not sure that I understand this example correctly, but the semiauto derivation seems to be existing only for the sake of compatibility with Scala 2 as it can be easily achieved in Scala 3 using derivation.

@Kordyjan sure, this could easily be done using derivation but it causes the same issue:

https://scastie.scala-lang.org/mrdziuban/ZGLpLKeTQIW9BD3qDIJRaQ

import cats.Eq
import cats.derived._

case class Foo[A](a: List[A]) derives Eq

[Kordyjan]

The derivation case should have been fixed by #17095, which will be available in 3.3.0-RC4. If it still doesn't work after that, we need to investigate.

[prolativ]

Beside the problems with an implementation of a method like adder or binding (in which one has to be very careful to assure the method is invoked in a very specific way, e.g. as the sole element of the body of another method) one additional argument against writing code (or even designing an API) like

def foo(x: Int) = binding
def bar(x: Int, y: Int) = binding

is that an invocation of binding macro isn't visually distinguishable from an invocation of an ordinary method. In the latter case this indeed might look (from a programmer's perspective) like the method parameters were unused. One might also expect both invocations of binding to return the same value (assuming there is no hidden mutable runtime state here).

In case of semi-automatic derivation like

trait Show[A]
object Show:
  def derived[A]: Show[A] = ???
  
case class Foo(x: Int)
case class Bar(x: Int, y: Int)
given showFoo: Show[Foo] = Show.derived
given showBar: Show[Bar] = Show.derived

one at least indicates the difference in expected results by explicit type annotations, but this mechanism is not specific to macros.

Writing

def foo(x: Int) = binding(x)
def bar(x: Int, y: Int) = binding(x, y)

might seem suboptimal but I'm quite convinced the benefits of this approach overweigh the problems created by writing just binding

[markehammons]

Regarding the need to make sure binding is called in a certain specific place, I had managed to do that by enforcing with compile-time errors that the binding macro was only used as a method body in a place where Library auto derivation occured:

object Cstd derives Library:
  def abs(i: Int): Int = binding

It's quite easy to enforce this, by checking that the first owner class/object encountered has Library[L] context from the Library derivation. Then it's just a matter of making sure that the method that binding is defining a body for is one of the top level method symbols of L. If a method symbol isn't found between the splice location and the symbol for L, that's a compiler error too.

Yes, this is stuff that requires some effort to write, and yes, this isn't my preferred approach (I'd rather create an instance of a trait L with its methods implemented by the macro from method signature, but that too is still experimental), but it works within the confines of what's available in Scala 3 right now.

[SethTisue]

Do I understand correctly that the -Ywarn-macros:after option controlled this behavior in scala 2?

Yes. But the canonical form is -Wmacros:after. (I can't recall if we formally deprecated the older -Y equivalent.)

The choices are after, before, both, and none.

The original PR where it landed was @som-snytt's scala/scala#5876

[szymon-rd]

This is an issue I created for the example with Cats:
#17156
I have a fix ready for it, I will be merging it soon.

[szymon-rd]

The fix: #17157

[ornicar]

I'm also getting a lot of false positive unused warnings when using JSON/BSON mapping macros, or when doing DI with macwire:

[Kordyjan]

@markehammons: We debated the original problem and decided that moving the entire phase after the inlining would cause more problems than it would fix. We can delay reporting unused symbols after the inlining and, before reporting, do the second check to see if macros haven't added references to some previously unused symbols. This will sort out the problems you mentioned and the macwire case brought up by @ornicar.

If we encounter further problems, we will release 3.3.0-RC4 with -Wunused disabled for all compilation units containing macros expanded after the typer and sort out the issue in future releases.