scala3 icon indicating copy to clipboard operation
scala3 copied to clipboard

Add migration helpers for named tuples

Open odersky opened this issue 1 year ago • 3 comments

Compiler version

3.6

Minimized example

From the SIP 58 proposal, which was recently accepted:

There are some source incompatibilities involving named tuples of length one. First, what was previously classified as an assignment could now be interpreted as a named tuple. Example:

var age: Int
(age = 1)

This was an assignment in parentheses before, and is a named tuple of arity one now. It is however not idiomatic Scala code, since assignments are not usually enclosed in parentheses.

Second, what was a named argument to an infix operator can now be interpreted as a named tuple.

class C:
  infix def f(age: Int)
val c: C

then

c f (age = 1)

will now construct a tuple as second operand instead of passing a named parameter.

Expectation

These problems can be detected and diagnosed fairly straightforwardly: When faced with a unary named tuple, try to interpret it as an assignment, and if that succeeds, issue a migration error and suggest a workaround of these kinds:

  {age = 1}     // ok
  c.f(age = 1)  // ok

odersky avatar Oct 01 '24 10:10 odersky

Maybe suitable for the next Spree?

odersky avatar Oct 01 '24 10:10 odersky

Discussion of infix named args https://github.com/scala/scala3/discussions/19072

and https://github.com/scala/scala3/pull/21565 was a quickie deprecation from last month.

som-snytt avatar Oct 01 '24 18:10 som-snytt

This issue was picked for the Scala Issue Spree of tomorrow, Monday, October 21st. @nicolasstucki, @bracevac and I 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 avatar Oct 20 '24 10:10 mbovel

Seems to me that the simplest would be to add a check in Desugar:

diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala
index e1a6b97fc7..e32b5ac1ab 100644
--- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala
+++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala
@@ -1605,9 +1605,10 @@ object desugar {
 
   /** Translate tuple expressions
    *
-   *     ()             ==>   ()
-   *     (t)            ==>   t
-   *     (t1, ..., tN)  ==>   TupleN(t1, ..., tN)
+   *     ()                       ==>   ()
+   *     (t)                      ==>   t
+   *     (t1, ..., tN)            ==>   TupleN(t1, ..., tN)
+   *     (n1 = t1, ..., nN = tN)  ==>   NamedTuple.build[(n1, ..., nN)]()(TupleN(t1, ..., tN))
    */
   def tuple(tree: Tuple, pt: Type)(using Context): Tree =
     var elems = checkWellFormedTupleElems(tree.trees)
@@ -1638,6 +1639,7 @@ object desugar {
       if ctx.mode.is(Mode.Type) then
         AppliedTypeTree(ref(defn.NamedTupleTypeRef), namesTuple :: tup :: Nil)
       else
+        checkNamedTuple(names, elemValues)
         Apply(
           Apply(
             TypeApply(
@@ -1646,6 +1648,10 @@ object desugar {
             Nil), // ++ ()
           tup :: Nil) // .++ ((values...))
 
+  def checkNamedTuple(names: List[Name], values: List[Tree])(using Context): Unit =
+    println(i"checkNamedTuple($names, $values)")

Would that make sense? Could we try alternative interpretations of the named tuple expression there?

mbovel avatar Oct 21 '24 14:10 mbovel

The approach I suggested above should work okay for the assignment case ((age = 1)). There we could probably just check if age is in scope and if it is a var:

  def checkNamedTuple(names: List[Name], values: List[Tree])(using Context): Unit =
    println(i"checkNamedTuple($names, $values)")
    if names.length == 1 && ctx.scope.lookup(names.head).is(Flags.Mutable) then
      report.migrationWarning(
        em"""`(${names.head} = ${values.head})` is interpreted as a named tuple containing a single element,
            |not as an assignment. If you meant to write an assignment, use curly braces
            |instead of parentheses: `{${names.head} = ${values.head}}`.""",
        values.head.srcPos
      )

For the named argument case (c f (age = 1)), we'd need more context however… That should probably be checked at the level of the Apply.

mbovel avatar Oct 21 '24 14:10 mbovel

does this mean (age=1) and {age=1} mean different things now? i always assumed (...) and {...} to be interchangeable as long as they are on a single line.

ritschwumm avatar Oct 22 '24 00:10 ritschwumm

@ritschwumm people.map(_.copy(age = 42)) was already sensitive to "bracket syntax". I think your assumption is misguided (wrong). The useful syntax is f { stuff } where an invocation takes a single arg in braces. But there is no general rule that parens and braces are the same.

som-snytt avatar Oct 22 '24 03:10 som-snytt

Second, what was a named argument to an infix operator can now be interpreted as a named tuple

For the record: I have experienced this issue when testing my project against 3.6.1 RC. There is a small part of the project using a DSL for constructing trees in unit tests. It took me a while to realize that this code is now interpreted as named tuples and that I need to use dot notation to avoid the issue:

None of the overloaded alternatives of method joint in class What with types ... match arguments ((mirror : Boolean))

OndrejSpanel avatar Nov 01 '24 15:11 OndrejSpanel