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

Merged
mbovel merged 1 commit into from
Dec 16, 2025
+22 −2
Merged

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 numberDiff line numberDiff 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

@mbovelmbovelDec 14, 2025

Choose a reason for hiding this comment

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

Suggested change
valtpNorm= tp.tryNormalize
if tpNorm.existsthen apply(n, tpNorm)
valtpNorm= tp.normalized.normalizedTupleType
if tpNorm ne tpthen apply(n, tpNorm)

Copy link
ContributorAuthor

@Lluc24Lluc24Dec 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 */
deftoNestedPairs(usingContext):Type=
tupleElementTypes match
caseSome(types) =>TypeOps.nestedPairs(types)
caseNone=>thrownewAssertionError("not a tuple")

Also note that tp.normalized on TupleN returns NoType.

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

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

Copy link
Member

@mbovelmbovelDec 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{casetp: AppliedType=>valtpNorm= 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
ContributorAuthor

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

Copy link
Member

@mbovelmbovelDec 16, 2025

Choose a reason for hiding this comment

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

Yes, but from my understanding, the requirement is simply that both TupleN and nested *: pairs be normalized to the same representation. Does it matter whether TupleN is normalized to nested pairs, or vice versa? If we normalize to TupleN, then both types in the unit tests would have size 1, wouldn't they?

Copy link
Member

@mbovelmbovelDec 16, 2025

Choose a reason for hiding this comment

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

Well, I guess for your current implementation of match types termination checks, it's better if (1, 2) and (1, 2, 3) have sizes 2 and 3.

Expand Down
18 changes: 18 additions & 0 deletions compiler/test/dotty/tools/dotc/core/TypesTest.scala
View file
Open in desktop
Original file line numberDiff line numberDiff 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 numberDiff line numberDiff 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