Handle TypeProxy of Named Tuples in unapply #22325

aherlihy · 2025-01-08T20:58:00Z

Fixes #22150.
Previously, there were several ways to check if something was a Named Tuple (derivesFromNamedTuple, isNamedTupleType and NamedTuple.unapply), this PR moves everything into NamedTuple.unapply. namedTupleElementTypes now takes an argument derived that when false will skip unapply (to avoid infinite recursion, used in desugaring and RefinedPrinter where trees can have invalid cycles).

odersky · 2025-01-09T12:45:28Z

compiler/src/dotty/tools/dotc/core/TypeUtils.scala

      (if normalize then self.normalized else self).dealias match
        case defn.NamedTuple(nmes, vals) =>
          val names = nmes.tupleElementTypesUpTo(bound, normalize).getOrElse(Nil).map(_.dealias).map:
            case ConstantType(Constant(str: String)) => str.toTermName
            case t => throw TypeError(em"Malformed NamedTuple: names must be string types, but $t was found.")
          val values = vals.tupleElementTypesUpTo(bound, normalize).getOrElse(Nil)
          names.zip(values)
+        case tp: TypeProxy =>


Not sure about this. In principle it looks OK, but TypeProxy#superType goes through normalize AFAIK, so it is as if the normalize argument has no effect anymore. Does that break things somewhere? What do the tests say?

All compilation tests are passing, haven't finished the full suite yet. From what I could tell the normalize argument is just used for pretty-printing?

The only unrelated tests that failed with this change were i12456 and i9328 which were fixed with the bounds check. I'm really not sure this is the right use of bounds, but I noticed it wasn't used anywhere in this method but was used in the regular tuple method to limit recursion. Given the docstring, this is a bit of a misuse, but for the moment I couldn’t find a nice way to check for cycles / don’t know if that’s done anywhere else I can reuse?

OK, should we predicate TypeProxy and following cases on normalize = false then?

Will the printer need to print trees with invalid cycles? It's used in error messages so I assume it's possible. We can either keep using the original namedTupleElementTypes in the printer, which does not call TypeProxy.supertype (so no need to update it to respect the normalization flag). If the printer does have to deal with cyclic structures but also if it uses the derived namedTupleElementTypes, then we can update TypeProxy to respect the normalization flag but then we'd need some way to check there aren't cycles.

This was resolved by avoiding recurring on TypeProxy when called from the printer.

odersky · 2025-01-09T13:20:27Z

compiler/src/dotty/tools/dotc/core/TypeUtils.scala

@@ -128,6 +128,7 @@ class TypeUtils:
        case None => throw new AssertionError("not a tuple")

    def namedTupleElementTypesUpTo(bound: Int, normalize: Boolean = true)(using Context): List[(TermName, Type)] =
+      if bound < 0 then Nil else


I think that's a bit too coarse. You recurse 2^31 times in the normal case. It's a bit tricky to limit recursion in the face of illegal recursive aliases, because we do the namedTupleElementTypesUpTo before Typer in desugaring. There we have not yet checked and ruled out such illegal cycles.

One idea would be to leave namedTupleElementTypesUpTo as it is for desugaring, but have another version that follows superTypes of TypeProxies in Typer and Completions. Or have another Boolean flag that says whether we should follow proxies and &/I.

One idea would be to leave namedTupleElementTypesUpTo as it is for desugaring, but have another version that follows superTypes of TypeProxies in Typer and Completions.

It's not clear to me why desugaring should be treated specially, in

scala3/compiler/src/dotty/tools/dotc/ast/Desugar.scala

Lines 1747 to 1748 in 0677702

var selNames = pt.namedTupleElementTypes.map(_(0))

if selNames.isEmpty && pt.classSymbol.is(CaseClass) then

immediately after calling pt.namedTupleElementTypes we call pt.classSymbol and classSymbol also recurses on TypeProxy:

scala3/compiler/src/dotty/tools/dotc/core/Types.scala

Lines 580 to 581 in 0677702

case tp: TypeProxy =>

tp.superType.classSymbol

I'm not 100% sure, but it's true having desugar recur doesn't cause any of the existing invalid cycle tests to fail. If we do want desugar to recur then we can collapse derived and normalize into one flag.

tests/run/i22150.scala

…down This reverts commit 2f03f86.

odersky

The logic looks correct. One can probably streamline this a bit more. But this can be dine later.

odersky · 2025-01-13T18:20:47Z

compiler/src/dotty/tools/dotc/ast/Desugar.scala

@@ -1744,7 +1744,7 @@ object desugar {
  def adaptPatternArgs(elems: List[Tree], pt: Type)(using Context): List[Tree] =

    def reorderedNamedArgs(wildcardSpan: Span): List[untpd.Tree] =
-      var selNames = pt.namedTupleElementTypes.map(_(0))
+      var selNames = pt.namedTupleElementTypes(false).map(_(0))


Suggested change

var selNames = pt.namedTupleElementTypes(false).map(_(0))

var selNames = pt.namedTupleElementTypes(derived = false).map(_(0))

We have a rule to always pass a booleans as named arguments.

odersky · 2025-01-13T18:28:02Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

@@ -110,7 +110,7 @@ object Applications {
  }

  def namedTupleOrProductTypes(tp: Type)(using Context): List[Type] =
-    if tp.isNamedTupleType then tp.namedTupleElementTypes.map(_(1))
+    if tp.isNamedTupleType then tp.namedTupleElementTypes(true).map(_(1))


Suggested change

if tp.isNamedTupleType then tp.namedTupleElementTypes(true).map(_(1))

if tp.isNamedTupleType then tp.namedTupleElementTypes(derived = true).map(_(1))

And the same for the other boolean arguments.

odersky · 2025-01-13T20:54:20Z

compiler/src/dotty/tools/dotc/core/TypeUtils.scala

      (if normalize then self.normalized else self).dealias match
+        // for desugaring and printer, ignore derived types to avoid infinite recursion in NamedTuple.unapply


Can we avoid the duplication with the extractor? Maybe have two extractors NamedTuple.unapply and NamedTupleDirect.unapply that share the same logic?

aherlihy marked this pull request as draft January 8, 2025 23:37

odersky reviewed Jan 9, 2025

View reviewed changes

aherlihy changed the title ~~Handle TypeProxy of Named Tuples, minimal fix without refactoring~~ Handle TypeProxy of Named Tuples in unapply Jan 10, 2025

aherlihy force-pushed the i22150 branch 3 times, most recently from c1997ce to 1a5a8b3 Compare January 10, 2025 21:56

aherlihy commented Jan 10, 2025

View reviewed changes

tests/run/i22150.scala Show resolved Hide resolved

aherlihy marked this pull request as ready for review January 10, 2025 22:12

aherlihy requested a review from odersky January 10, 2025 22:12

aherlihy force-pushed the i22150 branch from d3da968 to de77ca0 Compare January 10, 2025 22:13

aherlihy added 5 commits January 13, 2025 08:52

Handle TypeProxy of Named Tuples, minimal fix without refactoring

3d11e5e

Refactor check into NamedTuple.unapply for consistency

5b31eda

remove normalize and rely on stackoverflow

be88246

Revert "remove normalize and rely on stackoverflow" because test slow…

f54f5d3

…down This reverts commit 2f03f86.

Re-add test that will pass when scala#22340 is merged

d42347f

aherlihy force-pushed the i22150 branch from de77ca0 to 83ae00d Compare January 13, 2025 13:59

Update rebased

83ae00d

aherlihy assigned aherlihy and odersky and unassigned aherlihy Jan 13, 2025

odersky approved these changes Jan 13, 2025

View reviewed changes

odersky merged commit c10def4 into scala:main Jan 13, 2025
29 checks passed

aherlihy added a commit to aherlihy/scala3 that referenced this pull request Jan 16, 2025

Address codereview comments from scala#22325

ded5d9c

aherlihy mentioned this pull request Jan 16, 2025

Reduce repeated code in NamedTuple.unapply and namedTupleElementTypesUpTo #22384

Open

WojciechMazur added this to the 3.6.4 milestone Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle TypeProxy of Named Tuples in unapply #22325

Handle TypeProxy of Named Tuples in unapply #22325

aherlihy commented Jan 8, 2025 •

edited

Loading

odersky Jan 9, 2025

aherlihy Jan 9, 2025

odersky Jan 9, 2025

aherlihy Jan 9, 2025 •

edited

Loading

aherlihy Jan 10, 2025

odersky Jan 9, 2025

aherlihy Jan 9, 2025

smarter Jan 9, 2025

aherlihy Jan 13, 2025 •

edited

Loading

odersky left a comment

odersky Jan 13, 2025

odersky Jan 13, 2025

odersky Jan 13, 2025

odersky Jan 13, 2025

	var selNames = pt.namedTupleElementTypes.map(_(0))
	if selNames.isEmpty && pt.classSymbol.is(CaseClass) then

	if tp.isNamedTupleType then tp.namedTupleElementTypes(true).map(_(1))
	if tp.isNamedTupleType then tp.namedTupleElementTypes(derived = true).map(_(1))

		(if normalize then self.normalized else self).dealias match
		// for desugaring and printer, ignore derived types to avoid infinite recursion in NamedTuple.unapply

Handle TypeProxy of Named Tuples in unapply #22325

Handle TypeProxy of Named Tuples in unapply #22325

Conversation

aherlihy commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aherlihy Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aherlihy Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

odersky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aherlihy commented Jan 8, 2025 •

edited

Loading

aherlihy Jan 9, 2025 •

edited

Loading

aherlihy Jan 13, 2025 •

edited

Loading