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

Change to implicit method invocation should define a migration rewrites #22792

Open
WojciechMazur opened this issue Mar 13, 2025 · 3 comments · May be fixed by #22835
Open

Change to implicit method invocation should define a migration rewrites #22792

WojciechMazur opened this issue Mar 13, 2025 · 3 comments · May be fixed by #22835
Assignees
Labels
area:implicits related to implicits area:rewriting tool itype:bug regression This worked in a previous version but doesn't anymore

Comments

@WojciechMazur
Copy link
Contributor

Based on OpenCB build failure in dacr/sotohp - build logs

The new behaviour seems to be correct, but it might lead to issues when upgrading to Scala 3.7. If possible we should provide an automatic migration under -source:3.7-migration -rewrite

Compiler version

3.7.0-RC1
Last good release: 3.7.0-RC1-bin-20250309-2f639e2-NIGHTLY
First bad release: 3.7.0-RC1-bin-20250312-3fe9304-NIGHTLY

Bisect points to c37dc8b

Minimized code

trait Permit
class Foo:
  def run(implicit ev: Permit): Unit = ???
  
given Permit = ???
@main def Test = new Foo().run()

Output

[error] ./test.scala:6:18
[error] missing argument for parameter ev of method run in class Foo: (implicit ev: Permit): Unit
[error] @main def Test = new Foo().run()
[error]                  ^^^^^^^^^^^

Expectation

Rewrite callsite to new Foo().run

@WojciechMazur WojciechMazur added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 13, 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 13, 2025
@Gedochao
Copy link
Contributor

@som-snytt I'll assign you for now, let me know if you'll have the time for this

@som-snytt
Copy link
Contributor

som-snytt commented Mar 13, 2025

I'll take a look. It's a little strange to rewrite a bug.

Edit: I see it's analogous to the existing new warning, but for the zero-arg case.

Implicit parameters should be provided with a `using` clause.

@som-snytt
Copy link
Contributor

som-snytt commented Mar 13, 2025

Just noticed that you don't get rewrites on errors (I knew that). Since this is "just a bug" and not a syntax migration "per se", maybe it should just supply an extra-helpful message.

I forgot to check that the bug was introduced at some point (maybe in the change I reverted):

➜  dotty git:(issue/22792-rewrite-bad-parens) ✗ scala-cli compile --server=false -S 3.3.3 tests/rewrites/i22792.scala
Downloading Scala 3.3.3 compiler
-- [E171] Type Error: /home/amarki/projects/dotty/tests/rewrites/i22792.scala:8:30
8 |@main def Test = new Foo().run()
  |                 ^^^^^^^^^^^^^^^
  |missing argument for parameter ev of method run in class Foo: (implicit ev: Permit): Unit
1 error found
Compilation failed
➜  dotty git:(issue/22792-rewrite-bad-parens) ✗ scala-cli compile --server=false -S 3.3.4 tests/rewrites/i22792.scala
Downloading Scala 3.3.4 compiler
➜  dotty git:(issue/22792-rewrite-bad-parens) ✗

Probably backported to LTS in 3.3.4? (Edit: from 3.4.2. 2024 was quite a busy year. I remember I restored code from early in the year; 3.4.2 was May; 3.3.4 was Sept, I'm not sure it inherited all the changes.)

For the record, I see how to emit the patch when failing the application -- there is no position to use when the error is emitted, as there are no args...

Edit: that is, at the one-liner that caused the regression last year, there is no position to emit a warning with a patch; failing allows checking the diagnostics up the stack, mapping it to a warning, and restarting the adapt to no args (i.e. effectively dropping the spurious parens). (Keeping the 3.6 behavior as "bug-compatible" seems dubious to me, but I'll think about it more.)

@som-snytt som-snytt linked a pull request Mar 19, 2025 that will close this issue
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
Development

Successfully merging a pull request may close this issue.

3 participants