Skip to content

Conversation

@Lluc24
Copy link
Contributor

@Lluc24 Lluc24 commented Nov 20, 2025

Closes #24120: remove the TODO comment.

The TODO comment left by @odersky regarding MatchReducer was:

/** A type comparer for reducing match types.
* TODO: Not sure this needs to be a type comparer. Can we make it a
* separate class?
*/
class MatchReducer(initctx: Context) extends TypeComparer(initctx) {

Keep MatchReducer as a TypeComparer

In the end, we do need MatchReducer to be a TypeComparer. The refactoring to composition over inheritance was not possible because MatchReducer requires modifying the mutable protected field caseLambda of its superclass, ConstraintHandling (which is a superclass of TypeComparer), before invoking inherited methods that depends it.

Mutable protected field caseLambda:

/** Potentially a type lambda that is still instantiatable, even though the constraint
* is generally frozen.
*/
protected var caseLambda: Type = NoType

MatchReducer modifies caseLambda variable of the superclass, and this modification is later needed when invoking methods of TypeComparer:

def matchLegacyPatMat(spec: MatchTypeCaseSpec.LegacyPatMat): MatchResult =
val caseLambda = constrained(spec.origMatchCase, ast.tpd.EmptyTree)._1.asInstanceOf[HKTypeLambda]
this.caseLambda = caseLambda

Maybe we could revisit this if we get rid of matchLegacyPatMat one day.

Keep MatchReducer in the same file

Moving MatchReducer to its own file creates significant issues with Git history tracking:

  • .git-blame-ignore-revs Limitation: This mechanism only works for in-place refactorings. When a file is moved, Git looks at the file state immediately before the commit. Since MatchReducer.scala didn't exist before, Git cannot trace the history back to TypeComparer.scala.

  • GitHub UI Limitation: GitHub's web interface does not support git blame -C (copy detection), so the GitHub UI would incorrectly show the person who moved the file as the author of all lines, losing attribution to the original authors.

  • Loss of Context: Keeping MatchReducer in the same file as TypeComparer makes it easier to understand their tight coupling and why inheritance is necessary.

  • Local Workaround Required: Users would need to run git blame -C locally to see the correct history, which is not ideal for most contributors:

    git blame -C compiler/src/dotty/tools/dotc/core/MatchReducer.scala

@mbovel
Copy link
Member

mbovel commented Nov 24, 2025

In its current state, it seems the .git-blame-ignore-revs doesn't work as expected:

image

Maybe it's due to the merge commit? Or maybe this is just the GitHub UI.

I'd try to rebase to avoid the merge commit, and have three commits: 1. move the match reducer, 2. add the move commit to .git-blame-ignore-revs and 3. remove the the todo (with an explanation in the commit description).

@Lluc24 Lluc24 force-pushed the i24120-move-MatchReducer branch from 9be03a5 to 1e0c4e4 Compare November 26, 2025 15:36
@Lluc24 Lluc24 closed this Dec 4, 2025
@Lluc24 Lluc24 force-pushed the i24120-move-MatchReducer branch from 1e0c4e4 to c9309e7 Compare December 4, 2025 08:51
The suggested refactoring to composition over inheritance was blocked because MatchReducer requires modifying the mutable protected state (caseLambda) of its superclass, TypeComparer, before invoking dependent inherited methods.
@Lluc24 Lluc24 reopened this Dec 4, 2025
@mbovel mbovel enabled auto-merge December 4, 2025 10:40
@mbovel mbovel self-requested a review December 4, 2025 13:36
@mbovel mbovel changed the title i24120 Separate MatchReducer Remove TODO in MatchReducer Dec 4, 2025
@mbovel mbovel disabled auto-merge December 4, 2025 13:37
@mbovel mbovel merged commit 95f9aff into scala:main Dec 4, 2025
46 checks passed
lidaisy pushed a commit to lidaisy/scala3 that referenced this pull request Dec 5, 2025
Closes scala#24120: remove the TODO comment.

The TODO comment left by @odersky regarding `MatchReducer` was:

https://github.com/scala/scala3/blob/c9309e730b6a261ea9ccc0ba86ca7de59ef3bd59/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L3597-L3601

###  Keep `MatchReducer` as a `TypeComparer`

In the end, we _do_ need `MatchReducer` to be a `TypeComparer`. The
refactoring to composition over inheritance was not possible because
`MatchReducer` requires modifying the mutable protected field
`caseLambda` of its superclass, `ConstraintHandling` (which is a
superclass of `TypeComparer`), before invoking inherited methods that
depends it.

Mutable protected field `caseLambda`:


https://github.com/scala/scala3/blob/c9309e730b6a261ea9ccc0ba86ca7de59ef3bd59/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala#L44-L47

`MatchReducer` modifies `caseLambda` variable of the superclass, and
this modification is later needed when invoking methods of TypeComparer:


https://github.com/scala/scala3/blob/c9309e730b6a261ea9ccc0ba86ca7de59ef3bd59/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L3879-L3881

Maybe we could revisit this if we get rid of `matchLegacyPatMat` one
day.

### Keep `MatchReducer` in the same file

Moving `MatchReducer` to its own file creates significant issues with
Git history tracking:

-
[.git-blame-ignore-revs](https://github.com/scala/scala3/blob/main/.git-blame-ignore-revs)
Limitation: This mechanism only works for in-place refactorings. When a
file is moved, Git looks at the file state immediately before the
commit. Since `MatchReducer.scala` didn't exist before, Git cannot trace
the history back to `TypeComparer.scala`.
- GitHub UI Limitation: GitHub's web interface does not support `git
blame -C` (copy detection), so the GitHub UI would incorrectly show the
person who moved the file as the author of all lines, losing attribution
to the original authors.
- Loss of Context: Keeping `MatchReducer` in the same file as
`TypeComparer` makes it easier to understand their tight coupling and
why inheritance is necessary.
- Local Workaround Required: Users would need to run `git blame -C`
locally to see the correct history, which is not ideal for most
contributors:

    ```bash
    git blame -C compiler/src/dotty/tools/dotc/core/MatchReducer.scala
    ```

---------

Co-authored-by: Matt Bovel <matthieu@bovel.net>
@Lluc24 Lluc24 deleted the i24120-move-MatchReducer branch December 6, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Separate MatchReducer from TypeComparer?

3 participants