Skip to content

Question about method '+=' on ArrayBuilder[Unit] #9132

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

Closed
FabioPinheiro opened this issue Jun 8, 2020 · 7 comments
Closed

Question about method '+=' on ArrayBuilder[Unit] #9132

FabioPinheiro opened this issue Jun 8, 2020 · 7 comments

Comments

@FabioPinheiro
Copy link
Contributor

Minimized example

scala.collection.mutable.ArrayBuilder.make[Unit] += ()

Output

Compiles on Scala 2.13.2 but not on Dotty

scala> scala.collection.mutable.ArrayBuilder.make[Unit] += ()                                                         
1 |scala.collection.mutable.ArrayBuilder.make[Unit] += ()
  |^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |None of the overloaded alternatives of method += in trait Growable with types
  | (elem1: Unit, elem2: Unit, elems: Unit*): 
  |  scala.collection.mutable.ArrayBuilder[Unit]
  | (elem: Unit): scala.collection.mutable.ArrayBuilder[Unit]
  |match arguments ()

Expectation

Created this issue so we can officialize if this is the intended behavior or not. In order to see what to do on scala-js/scala-js#4071

The discussion starts Gitter where @smarter and @sjrd participated
https://gitter.im/lampepfl/dotty?at=5ede17109da05a060a5b9b14

@smarter
Copy link
Member

smarter commented Jun 8, 2020

Scala 2 parses a += () as a.+=(()) whereas dotty parses it as a.+=(), is this intended @odersky ?

@som-snytt
Copy link
Contributor

This ticket may be used to forward-port the improvement at scala/scala#7684 which is in 2.13.0.

@FabioPinheiro
Copy link
Contributor Author

The code a += () is creating an ast Tree like InfixOp(Ident(a),Ident(+=),Tuple(List()))

After filling the Parsers.scala file with prints I added the line case Nil => unitLiteral before https://github.com/lampepfl/dotty/blob/3ea2841db764bef11871a168e8d669bdd68d5ee7/compiler/src/dotty/tools/dotc/ast/untpd.scala#L469

After this change the previous tree is replaced with InfixOp(Ident(b),Ident(+=),Parens(Literal(Constant(()))))

I probably broke a lot of stuff. But for this particular case, I got the result that I was expecting after the typer phase.
Now dotc -Xprint:type MyTest.scala prints:

package <empty> {
  final lazy module val MyTest: MyTest$ = new MyTest$()
  final module class MyTest$() extends Object(), _root_.scala.Serializable { 
    this: MyTest.type =>
    def main(): Unit = 
      {
        val a: scala.collection.mutable.ArrayBuilder[Unit] = 
          scala.collection.mutable.ArrayBuilder.make[Unit](
            scala.reflect.ClassTag.Unit
          )
        {
          a.+=(())
          ()
        }
      }
  }
}

instead

package <empty> {
  final lazy module val MyTest: MyTest$ = new MyTest$()
  final module class MyTest$() extends Object(), _root_.scala.Serializable { 
    this: MyTest.type =>
    def main(): Unit = 
      {
        val a: scala.collection.mutable.ArrayBuilder[Unit] = 
          scala.collection.mutable.ArrayBuilder.make[Unit](
            scala.reflect.ClassTag.Unit
          )
        a.+=()
      }
  }
}

@FabioPinheiro
Copy link
Contributor Author

FabioPinheiro commented Jun 9, 2020

Without much surprise List(() => 1) does not compile now XD

@odersky
Copy link
Contributor

odersky commented Jun 9, 2020

Scala 2 parses a += () as a.+=(()) whereas dotty parses it as a.+=(), is this intended @odersky ?

I am surprised at Scala 2's behavior here. Here is another example:

  Console.println ()
  Console println ()

parses as

    Console.println();
    Console.println(())

and prints one (), not two or none. So it seems that infix applications are no longer equivalent to dotted applications. Maybe this was part of the changes to eta expansion in 2.12? EDIT: I see now that it was #7684 so a lot more recent than that.

But with the changes intended for auto-tupling in mind we should parse it the same way as Scala 2.

@som-snytt
Copy link
Contributor

The Scala 2 PR refers to an earlier PR scala/scala#6974 that says "Remove adaptation of 0-arg methods under -Xsource:3.0", so that was the context. Two years ago, we weren't yet comparing Dotty behavior as carefully as now. In fact,

scala> () == ()
1 |() == ()
  |^^^^^
  |not enough arguments for method ==: (x$0: Any): Boolean

@odersky
Copy link
Contributor

odersky commented Jun 9, 2020

@som-snytt Thanks for giving the context. Things goes in the right direction.

In fact it turns out that Dotty does parse the argument as an empty tuple, but then the pretty printer fooled us in dropping the extra pair of parentheses. The Tuple gets replaced with an empty argument list in desugaring.

liufengyun added a commit that referenced this issue Jun 10, 2020
Fix #9132: Align with Scala 2's handling of () infix arguments
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

4 participants