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

Multiarg infix is not supported in op= rewrite #14418

Open
som-snytt opened this issue Feb 5, 2022 · 8 comments
Open

Multiarg infix is not supported in op= rewrite #14418

som-snytt opened this issue Feb 5, 2022 · 8 comments

Comments

@som-snytt
Copy link
Contributor

som-snytt commented Feb 5, 2022

Compiler version

3.1.0

Minimized code

case class C(c: Int) {
  def +(i: Int, j: Int): C = new C(c + i*j)
}

object Test extends App {
  println {
    var x = new C(42)
    x = x + (3, 9)
    x
  }
  println {
    var x = new C(42)
    x.+=(3, 9)
    x
  }
  println {
    var x = new C(42)
    x += (3, 9)
    x
  }
}

Output

-- [E008] Not Found Error: multi.scala:19:6 -----------------------------------------------------------------------------------------------------
19 |    x += (3, 9)
   |    ^^^^
   |    value += is not a member of C - did you mean C.!=?
1 error found

Expectation

If the compiler accepts multiarg infix (and maybe it should not), it should accept multiarg assignment syntax.

Scala 2 -Xlint says

multi.scala:3: warning: multiarg infix syntax looks like a tuple and will be deprecated
  def +(i: Int, j: Int): C = new C(c + i*j)
      ^
multi.scala:9: warning: multiarg infix syntax looks like a tuple and will be deprecated
    x = x + (3, 9)
          ^

and fails to warn for assignment syntax.

Reported on discord:

just started learning Scala (3)... i'm a bit confused why this works [snip] but this does not

@som-snytt som-snytt added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 5, 2022
@smarter
Copy link
Member

smarter commented Feb 5, 2022

If the compiler accepts multiarg infix (and maybe it should not)

#4311 was put on hold and it's not clear to me what the way forward is. The Scala 2 warning about multiarg infix was annoying enough that it had to be put under -Xlint, so it seems we can't just adopt it, much less promote it to an error. And since x += (3, 9) works in Scala 2 I guess we have no choice but to support it too.

@smarter smarter added area:parser compat:scala2 Spree Suitable for a future Spree and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 5, 2022
@som-snytt
Copy link
Contributor Author

som-snytt commented Feb 5, 2022

"it's just syntax".

Worth adding that -Xlint is intended as a baseline of warnings; arguably, there should be no warnings from scalac except that are lints (either built-in or scalafix), though -Xlint has been used as a quarantine for annoying warnings (and arguably all warnings are annoying, because that is how they do their job).

Conversely, Scala 3 is opinionated. It could error under -source future.

@scala-center-bot
Copy link

This issue was picked for the Issue Spree No. 24 of 29 November 2022 which takes place in a week from now. @mbovel 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 Nov 29, 2022

Worked on this today during the issue spree with @Billal-B and @odd.

This issue seems to be linked to #9159. We should check if untupling for infix types was working before.

From today's explorations, it seems like the ApplyKind.InfixTuple attachement set at:

https://github.com/lampepfl/dotty/blob/7b9c1a85f1ca227e7a0b9971bf3b74f575a2bf8b/compiler/src/dotty/tools/dotc/ast/Desugar.scala#L1322-L1323

Is not preserved until it is read at:

https://github.com/lampepfl/dotty/blob/7b9c1a85f1ca227e7a0b9971bf3b74f575a2bf8b/compiler/src/dotty/tools/dotc/typer/Applications.scala#L1208-L1213

Could it be due to an Apply node being copied without copying the attachements?

@scala-center-bot
Copy link

This issue was picked for the Issue Spree No. 25 of 24 January 2023 which takes place in a week from now. @Billal-B, @odd 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 Jan 17, 2023

If I understood correctly, multi-parameter infix calls will be deprecated. Therefore, is it relevant to fix this issue?

@mbovel
Copy link
Member

mbovel commented Jan 23, 2023

The compiler team confirmed that support for multiple-parameter infix calls will be removed in the near future.

It might still be interesting to fix the bug for the time being. It is up to you @Billal-B and @odd.

@som-snytt
Copy link
Contributor Author

That is amazing information, in the sense that my previous understanding was that deprecation was premature and that my previous understanding before that was mistaken.

I agree that it's not worth investing effort into such fixes unless they are an important pain point. Smooth migrations are nice in the sense of "would be nice to have."

There are many issues in the backlog. Although it's useful to work on a ticket just for educational value, it's more useful to work on a ticket with a useful outcome.

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

No branches or pull requests

5 participants