Skip to content
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.

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

Add TransformerClass to avoid multiple anonymous classes creation during macro expansion. #637

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

Conversation

pawelsadlo
Copy link

@pawelsadlo pawelsadlo commented Nov 20, 2024

Scala 3.3.4 added a new warning for inline methods in case an anonymous class is created.
This introduces concrete class and instantiates it instead of creating anonymous class.

@pawelsadlo pawelsadlo marked this pull request as ready for review November 20, 2024 11:41
@MateuszKubuszok
Copy link
Member

MateuszKubuszok commented Nov 20, 2024

Hello, thank you for the contribution!

From my point of view this solution is problematic:

  1. first of all, while it shuts the compiler complaints, it still creates an anonymous class - every lambda on JVM is an anonymous class, so it replaced anonymous Transformer with a boxed anonymous Function1 (new Transformer$1 {} -> new TransformerClass(new Function1$anon$1 {}, maybe even with some runtime overhead). Other libraries might have used this approach as they were creating anonymous instanced for something that composed 2 other concrete instances but Chimney will always create a new expression, whether we box it with as a lambda in some class or inline as Single Abstract Method
  2. compiletime package is no place for runtime constructs as it isn't checked with MiMa, this code creates dependency from users' runtime on macros' internals which are a moving target (which isn't an issue since runtime has no business calling them, so bytecode mismatch never happens)
  3. since it doesn't actually prevent anonymous class creation, only masks it, it should be possible to be implemented as just a new default to @nowarn annotation (
    val nowarnCfg = XMacroSettings.foldLeft(Option.empty[Option[String]]) {
    ), something like @nowarn("msg=anonymous") or similar Scala-3 only workaround, however I cannot verify it since... this errors doesn't show either in our CI tests (on 3.3.4) not in Scastie when I tried to reproduce it

EDIT: I just remembered that I already made some tests a few weeks ago - this warnings only appears when you do:

inline def notAMacro = {
  // some inline ifs, summonInline etd
}

but when you do

inline def aMacro = ${ impl }

it never appears - it makes sense since:

  • inline def without a macro can only call building blocks and combine expressions, it cannot combine arbitrary parts of a tree, so the compiler knows upfront the generated type (its shape), and use cases force all anonymous instances to be something that could always be replaces with a named instance with specialized constructor
  • macro can build an arbitrary tree, and its shape would be only known at the callsite, so compiler could have to make this check at the callsite rather than during inline def compilation... while also severely constraining what a macro can do

so this check was never run against what Chimney's macros are producing (see CI) and probably never will (we have 3.3.4, enabled all compiler linters that I can think of, -Xfatal-warnings, no -WConf suppressing anonymous instances... and everything works perfectly). I see no reason fixing what cannot be proven to be broken (if you have some examples of Chimney macros that produce this warning, please share it, as I was never able to obtain them).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants