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

Remove Two Warts: Auto-Tupling and Multi-Parameter Infix Operations #4311

Open
wants to merge 7 commits into
base: master
from

Conversation

@odersky
Copy link
Contributor

commented Apr 13, 2018

Prompted by the Scala contributors thread, this was initially an attempt to drop auto-tupling altogether. But it became quickly annoying. The main problem is that it's unnatural to add another pair of parentheses to infix operations. Compare:

(x, y) == z
z == ((x, y))    // !yuck

Same thing for cons:

((1, x)) :: ((2, y)) :: xs   // double yuck!

So at the very least we have to allow auto-tupling for arguments of infix operators. I went a little bit
further and also allowed auto-tupling if the expected type is already a (some kind of) product or tuple of the right arity. The result is this commit.

The language import noAutoTupling has been removed. The previous auto-tupling is still reported under Scala 2 mode, and there is -rewrite support to add the missing (...) automatically.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

Comparing with the status quo: Auto-tupling of method arguments is supported if

  1. There are at least two arguments
  2. The method has a single parameter (and in the case of overloaded methods, all alternatives
    have a single parameter.)

Both of these conditions are missing in scalacs auto-tupling. Arguably, all reported confusing situations are already eliminated by these two conditions. So it is not clear why we would want to go further in restricting auto-tupling. It would complicate the spec and implementation, and I don't see really a gain.

So my current position would be to simply drop the noAutoTupling language import and keep the status quo.

@sjrd

This comment has been minimized.

Copy link
Member

commented Apr 13, 2018

Note that the two yucks are because we allow infix method application with multiple parameters ... which is never what I want. Is there any known good use case for that?

If infix application only admitted one parameter, then z == (x, y) would parse the same way as z == ((x, y)) which we need now. Same for the :: example.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

Is there any known good use case for that?

 buf += ("hello", "world")

Good or not, it's pretty common. If we started from scratch, I would be happy not to support this feature. Maybe we should work on eliminating this first? I don't yet see a clear way for migration, though.

@sjrd

This comment has been minimized.

Copy link
Member

commented Apr 13, 2018

buf += ("hello", "world")

Well, see, I don't even know whether that's supposed to add 2 elements to buf ("hello" and "world") or 1 element (("hello", "world")) ...

An easy migration path, if a bit long:

  1. Deprecation warning when multiple parameters are used in an infix method call (without changing any semantics). This forces people to use dot notation if they want it.
  2. Compilation error in the same situation. Still does not change any existing semantics.
  3. Apply the new interpretation, where the argument is necessarily unique, hence a tuple.

-Xsource:n+1 can be used to get one step ahead in an opt-in way, as usual.

def test(tp: Type): Boolean = tp match {
case tp: TypeRef =>
val sym = tp.symbol
defn.isTupleClass(sym) && defn.arity(sym, str.Tuple) == args.length ||

This comment has been minimized.

Copy link
@adriaanm

adriaanm Apr 13, 2018

Contributor

How about directly comparing to the class with name s"Tuple${args.length}"?

@adriaanm

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

Very happy to see this! Looks like the best compromise we can get for now. Will work towards this through deprecation in 2.13 (and make it available under -Xsource:2.14)

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

I dropped the case where we auto-tuple if the expected type is a tuple. Only two lines in the whole codebase broke. So this is clearly a feature that does not pull its own weight. This means the only
issue is operators, and indeed, we should get rid of the variable number of arguments for an operator instead.

One additional measure to take: Warn if a symbolic method takes multiple parameters (since then there is no longer a way to use the method infix.

It would be good to start with this already for Scala 2.13. This also means that the variadic += operators in the collections should be deprecated or dropped. /cc @julienrf @szeiger @lrytz

@szeiger

This comment has been minimized.

Copy link

commented Apr 13, 2018

I agree with always tupling the RHS of an operator. It never made much sense to me that a single arg could be passed without parens but if you added parens it was an argument list, not a tuple.

We'd have to deprecate the variadic version of Growable.+= now.

Another instance is LongMap.+=:

  def +=(key: Long, value: V): this.type = { update(key, value); this }

Semantics and call syntax are the same as for the inherited += method (which takes a Tuple2 in a Map) but this overload takes two primitive Long values instead of boxing them and wrapping them in a tuple. No longer being able to call it as an operator would cause a performance regression.

@sjrd

This comment has been minimized.

Copy link
Member

commented Apr 13, 2018

For LongMap:

@inline override final def +=(t: (Long, V)): this.type = {
  update(t._1, t._2)
  this
}

and let the optimizer get rid of it. If not scalac's, then the JVM's.

Additional benefit: also works for m += x -> v in addition to m += (x, v).

@szeiger

This comment has been minimized.

Copy link

commented Apr 13, 2018

Nice. I didn't think of using inlining to let the optimizer remove the boxing overhead.

@odersky odersky changed the title Restrict auto-tupling more Remove Two Warts: Auto-Tupling and Multi-Parameter Infix Operations Apr 15, 2018
@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

This is the second part of the cleanup, which turned out to be quite a bit harder than the first. In particular:

  • An infix operation x op (y, z) now unconditionally interprets
    (y, z) as a tuple, unless in Scala-2 mode.
  • In Scala-2 mode, the pattern will give a migration warning with
    option to rewrite in most cases. Excluded from rewrite are
    operator assignments and right-associative infix operations.
  • Auto-tupling is only enabled in Scala-2 mode, and will give
    a migration warning with option to rewrite in that mode.

This change broke a lot more code than the auto-tupling change. As I feared, the pattern of infix operations that take multiple arguments on their right hand side is quite common. But I also think that in most cases the idiom is confusing and benefits from a rewrite to method syntax.

The other problem is that these patterns now mean something different (i.e. tuple on the rhs), which will likely lead to type errors later on, but it's also conceivable that they change behavior. The best remedy is to compile under -language:Scala2 first. This will issue migration warnings for all
problematic cases and rewrite most of them automatically.

@SethTisue

This comment has been minimized.

Copy link
Member

commented Apr 15, 2018

buf += ("hello", "world")
Good or not, it's pretty common.

I am skeptical it is common. I have barely ever seen this used. I'd be happy to lose this.

As I feared, the pattern of infix operations that take multiple arguments on their right hand side is quite common

In what codebase? In the compiler codebase? If so, perhaps it's an outlier?

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

In what codebase? In the compiler codebase? If so, perhaps it's an outlier?

Compiler, collections, testing framework, and tests themselves. The most common usage is something like

 xs and (x, y, z)

because the author decided that looks better than

xs.and(x, y, z)
@ritschwumm

This comment has been minimized.

Copy link

commented Apr 15, 2018

ouch,this will probably need a lot of my code to be changed. especially in longer method chains like a foo b bar c quux (c,d) which would have to become sth like a.foo(b).bar(c).quux(c,d) or (a foo b bar c).quux(c,d) then - both of which are imho more difficult to read.

@odersky odersky force-pushed the dotty-staging:drop-autotupling branch from ec7c8ac to 66e4a83 Apr 15, 2018
@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

@ritschwumm I would argue that a.foo(b).bar(c).quux(c,d) is easier to read than the other two. But I am also concerned that we require changes in a lot of places here.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

languageserver has build issues now. @allanrenucci, @nicolasstucki could you take a look what needs to change?

@nafg

This comment has been minimized.

Copy link

commented Apr 15, 2018

@ritschwumm the change doesn't have to be so invasive. You can write (a foo b bar c).quux(c,d)

In IntelliJ you can just Alt-Enter on quux and say "Convert from infix expression" and it will put the necessary parentheses in the right place automatically.

@odersky is there any sense in distinguishing between symbolic infix methods and alphanumeric infix methods? For instance a less breaking change would be to only not interpret parentheses as an argument list for symbolic infix methods. Of course it would be more elegant not to distinguish.

Also there would be the equivalent of a deprecation cycle, as was done for procedure syntax. -language:Scala2 counts, and @adriaanm already said there would be on the scalac side as well.

I just wonder if it makes any sense at all to deprecate parentheses as an argument list for symbolic infix methods earlier (or stricter in some way) than for alphanumeric infix methods.

Another point here is whether you can call it "auto-untupling" if the primary meaning of the syntax is a single tuple argument, but the method is defined to take separate parameters instead.

@Duhemm

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

The CI failure is caused by sbt-buildinfo which generates the configuration for the language server in tests. It generates code which looks like

override val toString: String = "..." format (v1, v2, v3)

Which compiles but will throw an exception at runtime. It would be great to have this kind of issues caught by the compiler though; this is going to be painful to migrate.

See sbt/sbt-buildinfo#124

odersky added 5 commits Apr 13, 2018
This was initially an attempt to drop auto-tupling altogether. But it became quickly annoying. The
main problem is that it's unnatural to add another pair of parentheses to infix operations. Compare:

    (x, y) == z
    z == ((x, y))    // !yuck

Same thing for cons:

    ((1, x)) :: ((2, y)) :: xs   // double yuck!

So at the very least we have to allow auto-tupling for arguments of infix operators. I went a little bit
further and also allowed auto-tupling if the expected type is already a (some kind of) product or tuple of the right
arity. The result is this commit.

The language import `noAutoTupling` has been removed. The previous auto-tupling is still reported
under Scala 2 mode, and there is -rewrite support to add the missing (...) automatically.
idiom

    x op (y, z)

will in the future be interpreted as taking a tuple `(y, z)`. So all
infix operations with more than one parameter have to be rewritten to
method calls. This was for the most part done using an automatic rewrite
(introduced in one of the next commits). Most of the tests were not re-formatted
afterwards so one sees the traces of the rewrite. E.g. the reqrite would yield

    x .op (y, z)

instead of the more idiomatic

    x.op(y. z)
Under Scala-2 mode, issue a migration warning for a symbolic method that is
not unary.

Also: Two more tests updated to new infix operator regime.
 - An infix operation `x op (y, z)` now unconditionally interprets
   `(y, z)` as a tuple, unless in Scala-2 mode.
 - In Scala-2 mode, the pattern will give a migration warning with
   option to rewrite in most cases. Excluded from rewrite are
   operator assignments and right-associative infix operations.
 - Auto-tupling is only enabled in `Scala-2` mode, and will give
   a migration warning with option to rewrite in that mode.
@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

The consensus with Adriaan is that we first want to deprecate multi-parameter infix operations in scalac. Suggested rewrite for a symbolic operator with more than one parameter is to take a tuple instead. Once that is done, the migration to Scala 3 should be easy enough that we can do it.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

SIP meeting proposal:

  • Deprecate definition of symbolic multi-parameter operators in 2.13
  • Deprecate use of multi-parameter infix operators in 2.14 and 3.0
  • Deprecate auto-tupling except for multi-parameter infix operators in 3.0
  • Drop multi-parameter infix operators and auto-tupling in 3.1
@Ichoran

This comment has been minimized.

Copy link

commented Dec 6, 2018

This clobbers mathematical vector classes unless tupling is always perfectly removed by the optimizer (without having to explicitly turn it on, because people might not want it for other reasons). Having things like

case class Vec2D(x: Double, y: Double) {
  def +(v: Vec2D): Vec2D = new Vec2D(x + v.x, y + v.y)
  def +(x: Double, y: Double): Vec2D = new Vec2D(this.x + x, this.y + y)
  /* Not this:
  def +(xy: (Double, Double)): Vec2D = new Vec2D(x + xy._1, y + xy._2)
  */
}

has always made a big difference to performance thus far.

Also, tupling isn't a workaround for varargs. What's the rewrite story there?

@Rich2

This comment has been minimized.

Copy link

commented Dec 6, 2018

@Ichoran I think what we really need is Structs for compound deep value classes. On Native they could be just passed in and out of methods as their atomic component parameters on the stack, on the Jvm and Js runtime, they could still be passed as atomic component parameters, they would just have to be returned as a heap object. I don't think that would be a big problem as far more values are passed as parameters than are returned, particularly in recursive code.

Deep value Structs don't cover all cases, sometimes you want a reference field, but I'm sure they cover the bulk of the performance loss from unnecessary heap object creation and they're great because they don't stress the garbage collector. I don't think we should wait for Valhalla.

@Ichoran

This comment has been minimized.

Copy link

commented Dec 6, 2018

@Richtype - That's one mechanism to try to reach zero-overhead, but my point is that right now we have exactly one way to have zero overhead, namely avoid the constructor and take the components in the method, and I want the zero-overhead problem solved before removing the feature that currently is the only solution (without turning math into not-math).

@Ichoran

This comment has been minimized.

Copy link

commented Dec 6, 2018

Here is a complete benchmark runnable with Thyme that illustrates the problem.

val th = new ichi.bench.Thyme
val a = Array.fill(2000)(scala.util.Random.nextInt(1000))
case class V(x: Int, y: Int) { 
  def *(p: (Int, Int)): Int = p._1*x + p._2*x
  def *(x: Int, y: Int) = x*this.x + y*this.y
}
th.pbenchOff(){ 
  val v = new V(2, 7)
  var s, i = 0
  while (i < a.length) { s += v * (a(i), a(i+1)); i += 2 }
  s 
}{ 
  val v = new V(2, 7)
  var s, i = 0
  while (i < a.length) { s += v * ((a(i), a(i+1))); i += 2 }
  s
}

Running on my machine:

Welcome to Scala 2.12.7 (OpenJDK 64-Bit Server VM, Java 1.8.0_191).
Type in expressions for evaluation. Or try :help.
...

Benchmark comparison (in 659.1 ms)
Significantly different (p ~= 0)
  Time ratio:    2.47109   95% CI 2.44881 - 2.49337   (n=20)
    First     533.5 ns   95% CI 531.0 ns - 536.0 ns
    Second    1.318 us   95% CI 1.308 us - 1.328 us
res43: Int = 2027128

There's no workaround that preserves performance.

So the tupling solution isn't much of a solution presently. You'd be better off just rewriting everything in v.*(x, y) format and deal with the (obvious) ugliness than (non-obviously) introduce a bunch of potential performance hits.

@smarter

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

@Ichoran Can you point to a library that uses this pattern ?

@nafg

This comment has been minimized.

Copy link

commented Dec 6, 2018

@nafg

This comment has been minimized.

Copy link

commented Dec 6, 2018

@Ichoran

This comment has been minimized.

Copy link

commented Dec 6, 2018

@smarter - mutable.AnyRefMap and mutable.LongMap use it so you can m += (k, v) at full speed (put there by me, because I knew about the difference in speed and benchmarked it to be significant). I use it in my own code in a bunch of places, for instance https://github.com/Ichoran/kse/blob/master/src/main/scala/maths/Vc.scala

I don't know if it's used in other Scala libraries. Java uses the pattern, albeit without symbolic operators because it doesn't have any. E.g. in awt.Point: https://docs.oracle.com/javase/8/docs/api/index.html?java/awt/Point.html

@smarter

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

For mutable maps at least you can do m(k) = v instead, right ?

@Ichoran

This comment has been minimized.

Copy link

commented Dec 7, 2018

@smarter - This is not general syntax, but yes, m(k) = v is equally fast.

@ritschwumm

This comment has been minimized.

Copy link

commented Feb 21, 2019

i'm still not happy with this change, mostly because one of the reasons i fell in love with scala was the "no dots, no parens" style of writing code which to me is a huge win in readability. i'm afraid with this change, it would become impossible to use in any halfway consistent way.

can we get tuples as proposed here, and still somehow save this style? ideally in a way that improves on what was possible before?

having to add parens for more than one parameter always struck me as inconsistent, same as having to break out of a nice and clean "subject verb object verb object" chain whenever one of the methods did not have a parameter list. for the latter, i'd have wished for "subject verb .. verb object" syntax (.. meaning "no parameter list here"). maybe something similar or better would work for mutiple parameters, too?

eed3si9n added a commit to eed3si9n/scala that referenced this pull request Feb 24, 2019
Ref scala/scala-dev#496
Ref lampepfl/dotty#4311 (comment)

Using the `compileTimeError` annotation, this removes the multi-parameter, vararg variants of `+` and `-` operators from the collections, and instructs the users to migrate to `++` and `--` instead.

This is part of an effort to remove the ambiguity between tuple literal and parameter list when in an infixed operator notation.
@eed3si9n eed3si9n referenced this pull request Feb 24, 2019
eed3si9n added a commit to eed3si9n/scala that referenced this pull request Feb 24, 2019
Ref scala/scala-dev#496
Ref lampepfl/dotty#4311 (comment)

Using the `compileTimeError` annotation, this removes the multi-parameter, vararg variants of `+` and `-` operators from the collections, and instructs the users to migrate to `++` and `--` instead.

This is part of an effort to remove the ambiguity between tuple literal and parameter list when in an infixed operator notation.
eed3si9n added a commit to eed3si9n/scala that referenced this pull request Feb 24, 2019
Ref scala/scala-dev#496
Ref lampepfl/dotty#4311 (comment)

Using the `compileTimeError` annotation, this removes the multi-parameter, vararg variants of `+` and `-` operators from the collections, and instructs the users to migrate to `++` and `--` instead.

This is part of an effort to remove the ambiguity between tuple literal and parameter list when in an infixed operator notation.
@SethTisue

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

meanwhile over in scala/scala, scala/scala#7795

@Ichoran

This comment has been minimized.

Copy link

commented Mar 12, 2019

Was anyone going to comment on the performance concerns?

@eed3si9n

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2019

@Ichoran This looks kind of interesting - #5200

  inline def sum(inline tup: Tuple2[Int, Int]): Int = ~Macros.tup2(tup)

It would be some more work on the library authors, but I think the overall change to limit infix to one argument is good tradeoff since the parameter list and tuple syntax looks the same.

@Ichoran

This comment has been minimized.

Copy link

commented Mar 18, 2019

@eed3si9n - If the deprecation is going in now, the performance concerns should also be addressed now. How bad are they, how widely used is the high-performance alternative, and will there be a replacement before the deprecated behavior is removed (so you can at least keep the old behavior if you're willing to tolerate the deprecation messages)?

@Ichoran

This comment has been minimized.

Copy link

commented Mar 18, 2019

I guess it is highly relevant for this PR, but not to the linked one from which it was suggested to discuss the issue here. I think that is actually the wrong decision; this PR shouldn't be cluttered up with decisions about 2.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.