Fix some varargs issues in scala 2 and scala 3 #433

jxnu-liguobin · 2024-02-29T06:47:55Z

Regarding #354

After wrapping varargs, the user code fails to compile.

In Scala 2, there were no inline parameters, and the subtype of args was obtained during compilation. However, this approach may not always be accurate.

Regarding #191 , #436

These two are about correctly obtaining inline parameters in scala3

To fix this, we recursively obtain the actual value of inline parameters.

For #191, we need to continue using inlining in the parameters of the wrapper function. for example. For instance:

class LogWrapper(val underlying: Logger) {
   // must inline
    inline def info(message: String, inline args: AnyRef*): Unit = underlying.info(message, args: _*)
}

This ensures compatibility and accurate handling of inline parameters in both Scala 2 and Scala 3.

For #436, now we can support this:

// work well
inline def argss = Seq("1")
logger.info("""Hello {}""", argss*)

// also work well
logger.info("""Hello {}""", Seq(arg5ref)*)
logger.info("""Hello {}""", forceVarargs(arg5ref):_*)

val argss = Seq("1")
logger.info("""Hello {}""", argss*)

Note that this is equivalent to using hard-coded match syntax trees. Another option is to add specific APIs that do not not use formatArgs

After wrapping varargs, the user code fails to compile. In Scala 2, there were no inline parameters, and the subtype of args was obtained during compilation. However, this approach may not always be accurate. Regarding #191 There is an issue with obtaining inline parameters in Scala 3. To address this, we recursively obtain the actual value of inline parameters. This necessitates the ongoing use of inlining in the parameters of the wrapper function. For instance: ```scala class LogWrapper(val underlying: Logger) { inline def info(message: String, inline args: AnyRef*): Unit = underlying.info(message, args: _*) } ``` This ensures compatibility and accurate handling of inline parameters in both Scala 2 and Scala 3.

lightbend-cla-validator · 2024-02-29T06:47:57Z

Hi @jxnu-liguobin,

Thank you for your contribution! We really value the time you've taken to put this together.

We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it.
Please review the new CLA and sign it before we proceed with reviewing this pull request:

http://www.lightbend.com/contribute/cla

SethTisue · 2024-02-29T15:41:43Z

I don't know Scala 3 macros or the code in this library, so I'm not able to review this.

SakulK · 2024-03-01T08:28:56Z

Unfortunately this change doesn't solve the Scala 3 issue I wrote about in #354 (comment)
here's a test I tried in Scala3LoggerSpec:

    "work when passing a Seq as repeated arguments" in {
      val f = fixture(_.isInfoEnabled, isEnabled = true)
      import f._
      val args = Seq(arg5ref)
      logger.info("""Hello {}""", args*)
      verify(underlying).info("""Hello {}""", arg5ref)
    }

Output:

info] - should work when passing a Seq as repeated arguments *** FAILED ***
[info]   org.mockito.exceptions.verification.ArgumentsAreDifferent: Argument(s) are different! Wanted:
[info] logger.info("Hello {}", true);
[info] -> at com.typesafe.scalalogging.Scala3LoggerSpec.f$proxy2$1(Scala3LoggerSpec.scala:29)
[info] Actual invocations have different arguments:
[info] logger.isInfoEnabled();
[info] -> at com.typesafe.scalalogging.Scala3LoggerSpec.f$proxy2$1(Scala3LoggerSpec.scala:28)
[info] logger.info("Hello {}");
[info] -> at com.typesafe.scalalogging.Scala3LoggerSpec.f$proxy2$1(Scala3LoggerSpec.scala:28)
[info]   at com.typesafe.scalalogging.Scala3LoggerSpec.f$proxy2$1(Scala3LoggerSpec.scala:29)
[info]   at com.typesafe.scalalogging.Scala3LoggerSpec.$init$$$anonfun$1$$anonfun$2(Scala3LoggerSpec.scala:24)

jxnu-liguobin · 2024-03-01T09:17:26Z

logger.info("""Hello {}""", args*)

Unfortunately this change doesn't solve the Scala 3 issue I wrote about in #354 (comment) here's a test I tried in Scala3LoggerSpec:

    "work when passing a Seq as repeated arguments" in {
      val f = fixture(_.isInfoEnabled, isEnabled = true)
      import f._
      val args = Seq(arg5ref)
      logger.info("""Hello {}""", args*)
      verify(underlying).info("""Hello {}""", arg5ref)
    }

Output:

info] - should work when passing a Seq as repeated arguments *** FAILED ***
[info]   org.mockito.exceptions.verification.ArgumentsAreDifferent: Argument(s) are different! Wanted:
[info] logger.info("Hello {}", true);
[info] -> at com.typesafe.scalalogging.Scala3LoggerSpec.f$proxy2$1(Scala3LoggerSpec.scala:29)
[info] Actual invocations have different arguments:
[info] logger.isInfoEnabled();
[info] -> at com.typesafe.scalalogging.Scala3LoggerSpec.f$proxy2$1(Scala3LoggerSpec.scala:28)
[info] logger.info("Hello {}");
[info] -> at com.typesafe.scalalogging.Scala3LoggerSpec.f$proxy2$1(Scala3LoggerSpec.scala:28)
[info]   at com.typesafe.scalalogging.Scala3LoggerSpec.f$proxy2$1(Scala3LoggerSpec.scala:29)
[info]   at com.typesafe.scalalogging.Scala3LoggerSpec.$init$$$anonfun$1$$anonfun$2(Scala3LoggerSpec.scala:24)

Indeed, I just realized that it only solves the problem in #354 for scala3 and scala 2.

so logger.info("""Hello {} {}""", args:_*) is still unavailable

jxnu-liguobin · 2024-03-01T10:36:47Z

Hi@SakulK ,Since val can't be inlined, no workaround has been found, but functions are possible.

inline def argss = Seq("1")
  logger.info("""Hello {}""", argss*)

// also work well
logger.info("""Hello {}""", Seq(arg5ref)*)
logger.info("""Hello {}""", forceVarargs(arg5ref):_*)

SakulK · 2024-03-01T10:56:59Z

@jxnu-liguobin unfortunately the current version still doesn't help with my actual use case - calling a method that returns a Seq[net.logstash.logback.argument.StructuredArgument] which I want to pass to the logger with *. I feel like LoggerImpl methods that take inline args: Any* ( for example https://github.com/lightbend-labs/scala-logging/blob/main/src/main/scala-3/com/typesafe/scalalogging/LoggerImpl.scala#L32 ) could use a simpler macro without calling formatArgs, like

  def infoMessageArgsSimple(underlying: Expr[Underlying], message: Expr[String], args: Expr[Seq[Any]]) (using Quotes) = {
    '{ if ($underlying.isInfoEnabled) $underlying.info($message, ${args}*) }
  }

But maybe I'm missing some downside of this approach

jxnu-liguobin · 2024-03-01T11:28:37Z

@jxnu-liguobin unfortunately the current version still doesn't help with my actual use case - calling a method that returns a Seq[net.logstash.logback.argument.StructuredArgument] which I want to pass to the logger with *. I feel like LoggerImpl methods that take inline args: Any* ( for example https://github.com/lightbend-labs/scala-logging/blob/main/src/main/scala-3/com/typesafe/scalalogging/LoggerImpl.scala#L32 ) could use a simpler macro without calling formatArgs, like
  def infoMessageArgsSimple(underlying: Expr[Underlying], message: Expr[String], args: Expr[Seq[Any]]) (using Quotes) = {

    '{ if ($underlying.isInfoEnabled) $underlying.info($message, ${args}*) }

  }
But maybe I'm missing some downside of this approach

I think it's infeasible as SLF4j has three overloads, and we must know the size in order to choose whether to use Expr.ofSeq. But I'm not sure, I need to wait for others to see.

Also, there is no support for log.info("a {}", 11) if no format. So I created a new issue #436 which is a different issue from the other two.

jxnu-liguobin · 2024-03-02T15:06:40Z

@SakulK I have preliminarily solved the problem with val, please take a look. More matches may need to be added to handle the dotty ast.

SakulK · 2024-03-04T07:04:31Z

Looks like it works only for statically created Seq in the val, changing it to def or making the val get the value from another def breaks it again. I think it's not great that the macro defaults to omitting the args Expr when it couldn't find a match, simple refactoring can break the code (even though it still compiles).

jxnu-liguobin · 2024-03-04T07:39:17Z

Looks like it works only for statically created Seq in the val, changing it to def or making the val get the value from another def breaks it again. I think it's not great that the macro defaults to omitting the args Expr when it couldn't find a match, simple refactoring can break the code (even though it still compiles).

Indeed, I agree that once we if we need to match the entire syntax tree doesn't make much sense.

we can add a dedicated method without formatArgs and wait for others to vote.

This was linked to issues Feb 29, 2024

From v3.6.0 logging message cannot be compiled properly #191

Open

Varargs compilation error as of v3.9.5 #354

Open

jxnu-liguobin requested review from SethTisue and mox692 February 29, 2024 06:50

Adding case to capture Blocks

1cbde29

jxnu-liguobin added 3 commits March 1, 2024 17:42

Fix for varargs

ac2ed10

Fix for varargs

14db4cb

Fix for varargs

a6e2c8b

jxnu-liguobin mentioned this pull request Mar 1, 2024

In Scala3, using a non-inline variable-length parameter doesn't print correctly #436

Open

jxnu-liguobin added 3 commits March 2, 2024 23:01

fixes #436

59603af

fix fmt

eba83bc

fix fmt

1de9edc

fix test

84698ea

jxnu-liguobin linked an issue Mar 4, 2024 that may be closed by this pull request

In Scala3, using a non-inline variable-length parameter doesn't print correctly #436

Open

jxnu-liguobin marked this pull request as draft March 4, 2024 08:38

SethTisue removed their request for review March 12, 2024 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some varargs issues in scala 2 and scala 3 #433

Fix some varargs issues in scala 2 and scala 3 #433

jxnu-liguobin commented Feb 29, 2024 •

edited

Loading

lightbend-cla-validator commented Feb 29, 2024

SethTisue commented Feb 29, 2024

SakulK commented Mar 1, 2024 •

edited

Loading

jxnu-liguobin commented Mar 1, 2024

jxnu-liguobin commented Mar 1, 2024

SakulK commented Mar 1, 2024

jxnu-liguobin commented Mar 1, 2024 •

edited

Loading

jxnu-liguobin commented Mar 2, 2024

SakulK commented Mar 4, 2024

jxnu-liguobin commented Mar 4, 2024 •

edited

Loading

Fix some varargs issues in scala 2 and scala 3 #433

Are you sure you want to change the base?

Fix some varargs issues in scala 2 and scala 3 #433

Conversation

jxnu-liguobin commented Feb 29, 2024 • edited Loading

lightbend-cla-validator commented Feb 29, 2024

SethTisue commented Feb 29, 2024

SakulK commented Mar 1, 2024 • edited Loading

jxnu-liguobin commented Mar 1, 2024

jxnu-liguobin commented Mar 1, 2024

SakulK commented Mar 1, 2024

jxnu-liguobin commented Mar 1, 2024 • edited Loading

jxnu-liguobin commented Mar 2, 2024

SakulK commented Mar 4, 2024

jxnu-liguobin commented Mar 4, 2024 • edited Loading

jxnu-liguobin commented Feb 29, 2024 •

edited

Loading

SakulK commented Mar 1, 2024 •

edited

Loading

jxnu-liguobin commented Mar 1, 2024 •

edited

Loading

jxnu-liguobin commented Mar 4, 2024 •

edited

Loading