Skip to content

Wrong rewrite of implicit arguments passed as { } #22731

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

Closed
OndrejSpanel opened this issue Mar 6, 2025 · 13 comments · Fixed by #22937
Closed

Wrong rewrite of implicit arguments passed as { } #22731

OndrejSpanel opened this issue Mar 6, 2025 · 13 comments · Fixed by #22937
Assignees
Labels
area:implicits related to implicits area:rewriting tool itype:bug regression This worked in a previous version but doesn't anymore

Comments

@OndrejSpanel
Copy link
Member

Compiler version

3.7.0-RC1-bin-20250303-91985db-NIGHTLY

Minimized code

class Input(x: String)

trait Decoder[T]

object Decoder {
  inline def apply[T](implicit f: () => Unit): Decoder[T] = ???
}

object Input {
  given Decoder[Input] = Decoder { () =>
    Input("")
  }
}

Compile this with "-rewrite", "-source", "3.7-migration".

Output

object Input {
  given Decoder[Input] = Decoder using { () =>
    Input("")
  }
}

The code cannot be compiled, the using word is placed at a location where it makes no sense.

Note:

  • this style is used quite a lot do define custom codecs when using Borer library.
  • the code is rewritten correctly when using parentheses, like `given Decoder[Input] = Decoder(() => Input(""))

Expectation

The rewrite should not produce invalid code.

@OndrejSpanel OndrejSpanel added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 6, 2025
@Gedochao Gedochao added area:rewriting tool area:implicits related to implicits regression This worked in a previous version but doesn't anymore and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 7, 2025
@mbovel
Copy link
Member

mbovel commented Apr 4, 2025

This issue was picked for the Scala Issue Spree of Monday, April 7th. @mbovel, @mrdziuban and @ajafri2001 will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

Reproduced with scala -S 3.nightly -rewrite -source:3.7-migration 22731.scala, which rewrites:

class Input(x: String)

trait Decoder[T]

object Decoder {
  inline def apply[T](implicit f: () => Unit): Decoder[T] = ???
}

object Input {
  given Decoder[Input] = Decoder { () =>
    Input("")
  }
}

to (using inserted before { () =>):

class Input(x: String)

trait Decoder[T]

object Decoder {
  inline def apply[T](implicit f: () => Unit): Decoder[T] = ???
}

object Input {
  given Decoder[Input] = Decoder using { () =>
    Input("")
  }
}

while we'd probably want the following (curly braces changed to parentheses, and using added within the parentheses):

class Input(x: String)

trait Decoder[T]

object Decoder {
  inline def apply[T](implicit f: () => Unit): Decoder[T] = ???
}

object Input {
  given Decoder[Input] = Decoder(using () =>
    Input("")
  )
}

@OndrejSpanel
Copy link
Member Author

while we'd probably want the following (curly braces changed to parentheses, and using added within the parentheses):

This may be valid sometimes, but often you may need to keep curly braces, and just add parentheses around the block, as the function body may contain constructs not available without braces, like variable declarations.

@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

Ah right, so should we aim to rewrite to the following?

object Input {
  given Decoder[Input] = Decoder (using {() => 
    Input("")
  })
}

@OndrejSpanel
Copy link
Member Author

As a primary form, yes, this. Removing the inner braces may be sometimes possible, but I find this secondary - I do not expect rewrites to create a code which is always nice, but it should be compilable.

@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

[...] the function body may contain constructs not available without braces, like variable declarations.

Here is such an example:

trait Decoder[T]

object Decoder {
  inline def apply[T](implicit f: () => Unit): Decoder[T] = ???
}

object Input {
  given Decoder[Int] = Decoder(using {() => val x = 42; println("blip")})
}

@main def main = ()

We should add it as a test case.

@OndrejSpanel
Copy link
Member Author

Does this compile? I would expect Decoder[Int] to return Int, this returns Unit.

@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

Does this compile? I would expect Decoder[Int] to return Int, this returns Unit.

Yes, the type parameter of Decoder is not used, and Decoder.apply requires a function () => Unit.

This can actually further be minimized to:

trait Decoder

def Decoder(implicit f: () => Unit): Decoder = ???

@main def main =
  Decoder { () => val x = 42; println("blip") }

or even

def foo(implicit bar: () => Unit): Unit = ???

@main def main =
  foo { () => val x = 42; println("blip") }

@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

The error message and patch are generated here:

def implicitParams(tree: Tree, tp: MethodOrPoly, pt: FunProto)(using Context): Unit =
val mversion = mv.ImplicitParamsWithoutUsing
if tp.companion == ImplicitMethodType && pt.applyKind != ApplyKind.Using && pt.args.nonEmpty then
val rewriteMsg = Message.rewriteNotice("This code", mversion.patchFrom)
report.errorOrMigrationWarning(
em"""Implicit parameters should be provided with a `using` clause.$rewriteMsg
|To disable the warning, please use the following option:
| "-Wconf:msg=Implicit parameters should be provided with a `using` clause:s"
|""",
pt.args.head.srcPos, mversion)
if mversion.needsPatch then
patch(Span(pt.args.head.span.start), "using ")
end implicitParams

@mbovel
Copy link
Member

mbovel commented Apr 7, 2025

So, from what we observed, the parentheses should be added if and only if the function was applied with the trailing lambda syntax (curly braces):

def foo(implicit bar: () => Unit): Unit = ???

@main def main =
  foo { () => val x = 42; println("blip") } // should add parentheses
  foo({ () => val x = 42; println("blip") }) // should not add parentheses

However we are missing this information. Could we add it to the Apply node?

@som-snytt
Copy link
Contributor

Probably I'm the only one who was hoping for "infix using" syntax.

@odersky
Copy link
Contributor

odersky commented Apr 8, 2025

I think adding an applyStyle to every apply node is overkill by a large margin. Why can't we simply not suggest a rewrite when the argument is enclosed in braces. The user has to do it manually then, but that's OK I think. Not worth spending a lot of effort on a corner case.

@mbovel
Copy link
Member

mbovel commented Apr 8, 2025

Okay, I tried an alternative implementation based on @som-snytt's suggestion: #22937. The changeset is indeed much smaller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:implicits related to implicits area:rewriting tool itype:bug regression This worked in a previous version but doesn't anymore
Projects
None yet
5 participants