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

Add migration helpers for named tuples #21681

Closed
odersky opened this issue Oct 1, 2024 · 8 comments · Fixed by #21823 or #21949
Closed

Add migration helpers for named tuples #21681

odersky opened this issue Oct 1, 2024 · 8 comments · Fixed by #21823 or #21949
Labels
area:named-tuples Issues tied to the named tuples feature. itype:enhancement Spree Suitable for a future Spree

Comments

@odersky
Copy link
Contributor

odersky commented Oct 1, 2024

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 odersky added stat:needs triage Every issue needs to have an "area" and "itype" label itype:enhancement and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 1, 2024
@odersky
Copy link
Contributor Author

odersky commented Oct 1, 2024

Maybe suitable for the next Spree?

@odersky odersky added the Spree Suitable for a future Spree label Oct 1, 2024
@Gedochao Gedochao added the area:named-tuples Issues tied to the named tuples feature. label Oct 1, 2024
@som-snytt
Copy link
Contributor

Discussion of infix named args #19072

and #21565 was a quickie deprecation from last month.

@mbovel
Copy link
Member

mbovel commented Oct 20, 2024

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
Copy link
Member

mbovel commented Oct 21, 2024

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
Copy link
Member

mbovel commented Oct 21, 2024

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.

@ritschwumm
Copy link

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.

@som-snytt
Copy link
Contributor

@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.

mbovel added a commit that referenced this issue Oct 22, 2024
This PR adds a warning for named tuples that look like assignment, such
as `(x = 1)`.

This is the first half to implement #21681. The second will be to add
warnings for named arguments to infix method calls (as a separate PR?).

Started during the Spree of October 21st.

Closes #21770.
@mbovel mbovel reopened this Oct 22, 2024
@OndrejSpanel
Copy link
Member

OndrejSpanel commented Nov 1, 2024

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))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:named-tuples Issues tied to the named tuples feature. itype:enhancement Spree Suitable for a future Spree
Projects
None yet
6 participants