Skip to content

Fix inconsistent typeSize calculation for TupleN vs recursive pair encodings #24743

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.

Sign up for GitHub

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

Open
Lluc24 wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −2
Open

Fix inconsistent typeSize calculation for TupleN vs recursive pair encodings #24743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Types.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7125,6 +7125,8 @@ object Types extends TypeUtils {
var seen = util.HashSet[Type](initialCapacity = 8)
def apply(n: Int, tp: Type): Int =
tp match {
case tp: AppliedType if defn.isTupleNType(tp) =>
foldOver(n + 1, tp.toNestedPairs)
case tp: AppliedType =>
val tpNorm = tp.tryNormalize
if tpNorm.exists then apply(n, tpNorm)
Comment on lines 7131 to 7132
Copy link
Member

@mbovel mbovel Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
val tpNorm = tp.tryNormalize
if tpNorm.exists then apply(n, tpNorm)
val tpNorm = tp.normalized.normalizedTupleType
if tpNorm ne tp then apply(n, tpNorm)

Copy link
Contributor Author

@Lluc24 Lluc24 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normalizedTupleType does the opposite of what we want. We need to convert TupleN to *: equivalent. I found this function in TypeUtils.scala that does exactly this:

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

Lines 122 to 126 in c82b623

/** The `*:` equivalent of an instance of a Tuple class */
def toNestedPairs(using Context): Type =
tupleElementTypes match
case Some(types) => TypeOps.nestedPairs(types)
case None => throw new AssertionError("not a tuple")

Also note that tp.normalized on TupleN returns NoType.

Using this function, the code is cleaner and more readable:

def apply(n: Int, tp: Type): Int =
  tp match {
    case tp: AppliedType if defn.isTupleNType(tp) =>
      foldOver(n + 1, tp.toNestedPairs)
    // From here the following code is the same as the original
    case tp: AppliedType =>
      val tpNorm = tp.tryNormalize
      if tpNorm.exists then apply(n, tpNorm)
      else foldOver(n + 1, tp)
    ...

Copy link
Member

@mbovel mbovel Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to convert TupleN to *: equivalent.

Why not do the opposite? TupleN needs less object allocations.

Also note that tp.normalized on TupleN returns NoType.

Maybe the following would work?

  tp match {
    case tp: AppliedType =>
      val tpNorm = tp.tryNormalize
      if tpNorm.exists then apply(n, tpNorm.normalizedTupleType)
      else foldOver(n + 1, tp.normalizedTupleType)

The above has the advantage that it always normalizes the type, even when it's a tuple.

Copy link
Contributor Author

@Lluc24 Lluc24 Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation of this issue is that the concatenation type of two tuples is made by a Match Type. When a TupleN and another tuple were to be concatenated, the result type uses the *: equivalent. The resulting typeSize is larger and created a lot of false positives in the algorithm of PR #24661

Expand Down
18 changes: 18 additions & 0 deletions compiler/test/dotty/tools/dotc/core/TypesTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package dotty.tools.dotc.core

import dotty.tools.DottyTest
import dotty.tools.dotc.core.Symbols.defn
import dotty.tools.dotc.core.TypeOps

import org.junit.Test
import org.junit.Assert.assertEquals

class TypesTest extends DottyTest:

@Test def tuple3TypeSize =
val tpe = defn.TupleType(3).nn.appliedTo(List(defn.IntType, defn.BooleanType, defn.DoubleType))
assertEquals(3, tpe.typeSize)

@Test def tuple3ConsTypeSize =
val tpe = TypeOps.nestedPairs(List(defn.IntType, defn.BooleanType, defn.DoubleType))
assertEquals(3, tpe.typeSize)
4 changes: 2 additions & 2 deletions compiler/test/dotty/tools/dotc/typer/DivergenceChecker.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ class DivergenceCheckerTests extends DottyTest {
1,
1,
1,
3,
5
4,
6
)

tpes.lazyZip(expectedSizes).lazyZip(expectedCoveringSets).foreach {
Expand Down
Loading