scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

`AnyRefMap` and `LongMap`'s `+=` method's signature is incorrect, should be a tuple #12012

Closed SethTisue closed 4 years ago

SethTisue commented 4 years ago

e.g. AnyRefMap has:

  def +=(key: K, value: V): this.type = { update(key, value); this }
  @inline final def addOne(key: K, value: V): this.type = { update(key, value); this }
  @inline override final def addOne(kv: (K, V)): this.type = { update(kv._1, kv._2); this }

But the += signature is inconsistent with other maps, where the method of the same name takes a tuple. It's also inconsistent with Scala's gradual move away from allowing multi-arg infix calls.

This turned up in https://github.com/scala/scala/pull/8998, which increases the level of linting we do in src/library, resulting in:

[error] /Users/tisue/scala.213/src/library/scala/collection/mutable/AnyRefMap.scala:297:7: multiarg infix syntax looks like a tuple and will be deprecated
[error]   def +=(key: K, value: V): this.type = { update(key, value); this }
[error]       ^
Ichoran commented 4 years ago

This is an overloaded method. Since AnyRefMap and LongMap have no reason to exist save for performance, it's good to be able to use them in a high-performance fashion. This allows the familiar m += (key, value) notation at full speed.

You can also (with any mutable map, and with these ones) do m(key) = value to avoid tuples.

It's true that it's inconsistent with the move away from allowing multi-arg infix calls. I view this as an argument against that move, but if you take the necessity of the move as a given, then I agree that this has to go.

som-snytt commented 4 years ago

I was going to say, Ichoran had commented long ago on this particular issue, but I'd forgotten what his recommendation was for the idiom. I haven't searched for that previous judgment on some old issue.

I see there is specialized addOne(k, v) to get this.type back.

som-snytt commented 4 years ago

@SethTisue I don't mind submitting a quick deprecation in favor of addOne if you like. IIUC that silences the deprecation? Or maybe not if it's a syntax warning.

som-snytt commented 4 years ago

For some reason, the multi-arg infix issue has yielded no "adderAll" puns.

som-snytt commented 4 years ago

@som-snytt Good idea! I guess we'll let Jenkins tell us if the enclosing-deprecation rule applies. I think you had a PR once that deferred checks until after typer?

SethTisue commented 4 years ago

@som-snytt sure, yes please (and LongMap too). maybe it should suggest both addOne and updated?

som-snytt commented 4 years ago

@SethTisue OK that sounds like a plan! Let's do it!