Skip to content

Compare types as capture sets in mergeRefinedOrApplied #23045

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
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Compare types as capture sets in mergeRefinedOrApplied #23045

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: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/TypeOps.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ object TypeOps:
}
case AndType(tp11, tp12) =>
mergeRefinedOrApplied(tp11, tp2) & mergeRefinedOrApplied(tp12, tp2)
case tp1: TypeParamRef if tp1 == tp2 => tp1
Copy link
Member

Choose a reason for hiding this comment

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

=:= uses TypeComparer to check two types are equivalent (subtypes of each other), which is expensive and we need to be careful when using it.

The reason we have one TypeParamRef and one TypeVar is because we choose to widen tp1 in approximateOr.

I would like to try the following:

case tp1: TypeParamRef =>
  tp2.stripTypeVar match
    case tp2: TypeParamRef if tp1 == tp2 => tp1
    case _ => fail
case tp1: TypeVar =>
  tp2 match
    case tp2: TypeVar if tp1 == tp2 => tp1
    case tp2: TypeParamRef if tp1.stripTypeVar == tp2 => tp2
    case _ => fail

Copy link
Author

Choose a reason for hiding this comment

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

Your suggested change works. I also decided to experiment with other solutions and found out that either adding the following case:

case tp1 if tp1.stripTypeVar == tp2.stripTypeVar => tp1

or applying the following patch

-       case tp1: TypeParamRef if tp1 == tp2 => tp1
+       case tp1: TypeParamRef if tp1 == tp2.stripTypeVar => tp1

also fix the issue. So my questions are:

  1. Is it possible to hit case tp1: TypeVar branch?
  2. is adding .stripTypeVar also a valid solution?

Copy link
Member

Choose a reason for hiding this comment

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

  1. case tp1: TypeVar can be hit if tp1 is TypeVar and tp2 is TypeParamRef, although it may not happen here because the left branch of a union type is widened first in approximateOr if both sides widening succeed (see 2.6). Since the ordering is arbitary, I think it's better to cover all possibilities in case the algorithm is changed.
  2. I would match a specific type (TypeParamRef and TypeVar) before stripTypeVar, to avoid covering unintended types.

case tp1: TypeParamRef if tp1 =:= tp2 => tp1
case _ => fail
}
}
Expand Down
2 changes: 2 additions & 0 deletions tests/pos/i23032.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def f[F[_], T]: F[Unit] | F[T] = ???
def x[F[_]] = f.toString
Loading