Some test cases show inliner warnings in the PR builds but I cannot reproduce them locally.

It looks like the @inline annotation on Growable.+= is pointless now and should be removed:

t9020.scala:15: warning: scala/collection/mutable/AbstractBuffer::$plus$eq(Ljava/lang/Object;)Lscala/collection/mutable/Growable; is annotated @inline but could not be inlined:
The method is not final and may be overridden.
  def add(s: String): Unit = b += s
                               ^

It's not clear to me why the tests only fail if you compile the standard library with optimizations. This should be depend on whether or not the tests themselves are compiled with optimizations, regardless of the standard library.

It looks like the @inline annotation on Growable.+= is pointless now and should be removed

It's not entirely pointless. The method can still be inlined if the receiver is final

➜  scala-asm git:(95474d0c) scala -opt:l:inline '-opt-inline-from:**' -Yopt-log-inline _ -opt-warnings:_
Welcome to Scala 2.12.6 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_172).
Type in expressions for evaluation. Or try :help.

scala> trait T { @inline def += = addOne; def addOne: Unit }
defined trait T

scala> final class C extends T { def addOne = () }
defined class C

scala> object O { def f(c: C) = c.+= }
Inline into $line5/$read$$iw$$iw$O$.f: inlined $line4/$read$$iw$$iw$C.$plus$eq. Before: 7 ins, inlined: 5 ins.
  inlined $line3/$read$$iw$$iw$T.$plus$eq$. Before: 18 ins, inlined: 5 ins.
    inlined $line3/$read$$iw$$iw$T.$plus$eq. Before: 24 ins, inlined: 5 ins.
Inline into $line5/$read$$iw$$iw$O$.f: inlined $line4/$read$$iw$$iw$C.$plus$eq. Before: 7 ins, inlined: 5 ins.
  inlined $line3/$read$$iw$$iw$T.$plus$eq$. Before: 18 ins, inlined: 5 ins.
    inlined $line3/$read$$iw$$iw$T.$plus$eq. Before: 24 ins, inlined: 5 ins.
defined object O

But for example ListBuffer is not final, so a warning is issued.

It's not clear to me why the tests only fail if you compile the standard library with optimizations. This should be depend on whether or not the tests themselves are compiled with optimizations, regardless of the standard library.

I agree this should not make a difference. Are you sure that's what you were observing / that you had the flags on locally?

This override of += for the purpose of inlining seems fishy to me (I mean, I think I recommended it at some point, but I had not thought it through). The reasonable thing to do would be:

trait Growable[-A] extends Clearable {
  def addOne(elem: A): this.type

  @`inline` final def += (elem: A): this.type = addOne(elem)
}

final class LongMap[V] extends ... {
  @inline
  override def addOne(kv: (Long, V)): this.type = { update(kv._1, kv._2); this }
}

This should be enough for the optimizer to be able to optimize

val longMap: LongMap[T] = ...
longMap += 1L -> t

because first longMap.+=(tuple) is @inline final, so it can inlined as

val tuple = (box(1L), t)
longMap.addOne(tuple)

Now addOne is called on longMap which has the static type LongMap, and it is also @inline and LongMap is final, so it can also be inlined:

val tuple = (box(1L), t)
longMap.update(unbox(tuple._1), tuple._2)
longMap

tuple can then be scala-replaced to become:

val tuple_1 = box(1L)
val tuple_2 = t
longMap.update(unbox(tuple_1), tuple_2)
longMap

and finally the box can be removed:

val tuple_1 = 1L
val tuple_2 = t
longMap.update(tuple_1, tuple_2)
longMap

All of this is straightforward. Why on earth does the optimizer need the override of += in LongMap to be able to do its job?

The inliner does only one round currently, so sees longMap.+= and inlines it, but it doesn't touch the resulting code (for now, still hoping to get to work on this eventually..)

Ouch. That's ... disappointing.

It should be fixable, though. So I don't think we should expose a bad public API to work around it.

Yes, agree, on both points :)

Improving the optimizer in this way should also solve @odd's problem of not being able to specialize the symbolic aliases in specialized collection subclasses.

Can we merge this PR?

I'm working on an improvement to the optimizer so it will handle this case, so we can remove the override.

Marking as WIP. This should be simplified once @lrytz's changes have been merged.

Similarly to my comment on #6584, I'd like to suggest to not merge changes like that unless they are approved by the SIP committee.

Deprecations may seem harmless, but for folks who use -Xfatal-warnings, deprecations become compilation errors. Since Scalac doesn't currently provide a way to selectively disable warnings, deprecations of language features become breaking changes of the language.

Tupling should be truly zero-overhead before we consider this. Then m += (k, v) is as clear as ever and perfectly fast.

Also, the rule that symbolic methods are just methods is an exceedingly useful one. That they're "just methods" except this rule and that exception and whatever this other thing is isn't something I look forward to explaining to people, or remembering myself. So I'm against the change in general.

But without even a sensible refactoring to preserve the semantics of high-performance code (without doing Java-style mangling of operations) seems like it's creating considerable hardship for very little benefit.

Similarly to my comment on #6584, I'd like to suggest to not merge changes like that unless they are approved by the SIP committee.

There are no language changes here, we're improving the encoding of += which also makes it compatible with a future Scala version (i.e. 2.14) that may not support operators with multiple arguments anymore. The language changes will have be part of a SIP for 2.14.

But after looking at these changes again, deprecating the two-argument version of += is actually wrong. It needs to be removed entirely because it doesn't exist in 2.12 (and adding it would make += for Map types considerably more confusing). We added it for performance reasons but with the inlining here and the upcoming inliner changes that's no longer an issue.

I still think we should deprecate the varargs version to avoid the confusion when calling it with 2 arguments on a Map.

Deprecations may seem harmless, but for folks who use -Xfatal-warnings, deprecations become compilation errors. Since Scalac doesn't currently provide a way to selectively disable warnings, deprecations of language features become breaking changes of the language.

Don't use -Xfatal-warnings then. When cross-compiling you can selectively enable it for 2.12 but leave it off for 2.13. I agree that we need a feature to enable or disable warnings in a more fine-grained way but it's hardly reasonable to stop deprecating things entirely just because -Xfatal-warnings exists as an optional feature.

Tupling should be truly zero-overhead before we consider this

The inlinable version in this PR has zero overhead. The optimizer can eliminate the tupling. The current version increases the cost of other uses of += though (because it is no longer final so they cannot be inlined). That's what @lrytz is working on. With the improved inliner we can make the overridden addOne inlinable instead of overriding +=.

Also, the rule that symbolic methods are just methods is an exceedingly useful one. That they're "just methods" except this rule and that exception and whatever this other thing is isn't something I look forward to explaining to people, or remembering myself.

Note that only left-associative infix operators are currently allowed to have multiple parameters. All other operators (prefix, postfix, right-associative infix) already have similar restrictions on the number of parameters.

The two-arg version exists on AnyRefMap.

Given concerns about inlining breaking separate compilation, should we count on optimizations that require inlining? Anyway, a more adept optimizer is certainly welcome!

I hadn't realized that the restriction was suggested for all infix operators, not just symbolic ones. If universally applied like that, my consistency argument is invalid. On the other hand, it strengthens the "this is a major hassle" argument. If there is decent evidence that multi-arg infix is confusing people in a way that doesn't have anything to do with tuples, then I agree it should go. Otherwise I don't understand the downside of allowing it. Maybe add a lint rule, but why make people change a bunch of their code if it's not something that induces mistakes? (The people who wrote it probably wouldn't have written it that way if it didn't seem clearer to them, so a priori one should assume that at least the original coder will find the new code harder to understand.)

There are no language changes here, we're improving the encoding of += which also makes it compatible with a future Scala version (i.e. 2.14) that may not support operators with multiple arguments anymore.

My bad - I overlooked this. Somehow, I thought that there was a pull request that deprecates all multiple-argument operators, but this pull request is definitely not that.

it's hardly reasonable to stop deprecating things entirely just because -Xfatal-warnings exists as an optional feature.

It looks like this boils down to a difference in perception. Indeed, if we consider all -X flags to be features that don't come with any compatibility guarantees, then compatibility concerns don't apply to them.

@jvican I think that it would be useful to discuss what compatibility guarantees we're planning to uphold during the transition period from Scala 2 to Scala 3. What would be an appropriate way to bring this up at a SIP meeting?

I think that it would be useful to discuss what compatibility guarantees we're planning to uphold during the transition period from Scala 2 to Scala 3. What would be an appropriate way to bring this up at a SIP meeting?

The best way would be to open a thread in our SIP meeting group in Scala Contributors so that other people can chime in before the meeting, and then we can quickly follow up on that discussion in the SIP meeting and schedule a proper day to discuss that.

@Ichoran Ah, right, the two-argument version being deprecated here is only in LongMap and it was also defined on AnyRefMap but is currently missing there. I'll undeprecate it in LongMap and add it back to AnyRefMap.

Updated:

• Bring back the two-argument LongMap.+=
• Do not deprecate two-argument += methods
• Deprecate the varargs version
• Keep Growable.+= final to allow inlining.
• Override tupled addOne methods with inlinable final versions that can be optimized away.

@szeiger Is this ready to be merged (and make M5)? Can it lose its WIP label?