Fix regression #17245: Overloaded methods with ClassTags #18286

jchyb · 2023-07-25T11:06:50Z

The problem lied with slightly adjusted unapply of FunctionOf in a previous PR, which caused different behavior in resolveOverloaded, where due to a pattern match into a FunctionOf (here), resolveOverloaded1 would return no candidates, causing more issues later on.

To keep the new behavior of FunctionOf unapply (which as a side-effect ended up fixing few issues represented with tests), with the previous behavior of overloaded functions, we allow the method candidate filtering to fallback from the FunctionOf candidate filtering, into the previous behavior, in case no candidates are kept. This also fixes and additional case, which is not part of a #17245 regression, but produces an incorrect error in similar manner.

Fixes #17245

nicolasstucki · 2023-07-25T11:33:06Z

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

@@ -1118,7 +1118,7 @@ class Definitions {
        case ErasedFunctionOf(mt) =>
          Some(mt.paramInfos, mt.resType, mt.isContextualMethod)
        case _ =>
-          val tsym = ft.dealias.typeSymbol
+          val tsym = ft.typeSymbol


What was the type of the ft that caused the issue and what was the dealiased type?

It was OnChannel, which when dealiased became Channel => Any, which I suppose made sense to match as a FunctionOf. I will admit after a certain point I mostly focused on obtaining previous behavior, without much consideration on what is the most correct (I saw the dealias change in the PR introducing the regression). Ideally I imagine we would keep the dealiased unapply and try to fix the issue directly inside resolveOverloaded methods?

Maybe we should figure out what this has in common with #18276. I'm currently suspecting a change in Typer in https://github.com/lampepfl/dotty/pull/16507/files#diff-8c9ece1772bd78160fc1c31e988664586c9df566a1d22ff99ef99dd6d5627a90 could have broken this.

I think they are mostly unrelated unfortunately, this was the line that broke #17245, but It does not seem to have any effect on #18276 in my tests.

Indeed, they are unrelated. It fixed the other one (waiting for CI confirmation) in #18288.

Now I am wondering if you need #18288 to fix the new issue you are having.

Just tested now and it does not seem to affect it, but keeping the dealiasing in FunctionOf unapply and instead changing the resolveOverloaded1 method here seems to work for all of the now-failing tests, even if changing only one line very naively, so we can skip that case which previously would not be accessed, like this:

- case defn.FunctionOf(args, resultType, _) => + case defn.FunctionOf(args, resultType, _) if pt.dealias.typeSymbol == pt.typeSymbol =>

But I want to read up on this method some more today in case there is a cleaner solution, and then I will update this PR by tomorrow morning

jchyb · 2023-07-25T12:29:14Z

Hmm, some tests are failing

nicolasstucki · 2023-07-25T12:45:58Z

The failing tests might be related to #18276.

jchyb · 2023-07-25T12:49:39Z

I think this means that the added dealias in FunctionOf unapply was what ended up fixing #12663

jchyb · 2023-07-25T13:00:45Z

Sorry, this wasn't a reply! I was just thinking out loud about the failing tests. I tested just now the minimization from #18276 on the changes introduced here and it does not fix anything there

jchyb · 2023-07-26T10:12:49Z

In my last commit here I decided to just remove the separate case for FunctionOf, as I could not find a use case for it, all the tests still passed, and that fixed an additional similar issue for overloaded methods returning non-aliased Function types. This made the tests pass, but just now I found that for:

def f(s: Int): Unit = println("a")
def f: Int => Unit = _ => println("b")

@main def Test =
  val test: String => Unit = f
  test(0)

Before my last last commit "a" would be printed out, and after it "b" (ideally I imagine here an exception should be thrown, but that is a topic for a separate issue). So this is still work in progress despite the tests clearing, but I have a few more ideas left.

Before the regression, FunctionOf unapply would not try dealiasing, meaning that an aliased function type would be handled by a general case. To fix that, instead of handling Function types separately when filtering overloaded methods in `resolveOverloaded1`, we allow to fallback to the general case if the previous one returns nothing. Along with fixing the regression, this also improves other cases, one of which was added to the test. Readd a separate FunctionOf case, but with a fallback

jchyb · 2023-07-26T13:57:21Z

Passes all the tests now. I reopened the PR and updated the original comment to better reflect the changes introduced

…18329) Backports #18286

godenji · 2023-08-29T01:17:41Z

This fix appears to have broken ScalaTest's assertThrows.

Here's the signature with a dummy implementation to illustrate the issue:

inline def assertThrows[T <: AnyRef](f: => Any)(implicit classTag: scala.reflect.ClassTag[T]) = f

assertThrows(1)

No ClassTag available for T
where: T is a type variable with constraint <: AnyRef

jchyb · 2023-08-29T10:54:23Z

Hmm, the above minimization fails regardless of the Scala 3 version used (which seems like the correct behavior to me, as the type inference does not get any information about what T could even resolve to). Moreover I don't think assertThrows is being overloaded anywhere in ScalaTest, so the above changes should not affect have any effect on that method, I think.

godenji · 2023-08-29T14:48:53Z

Ok, this was the only change I could find that would explain why upgrading to RC6 (from RC1) prevents tests from running (i.e. due to aforementioned compilation failure).

Only sbt (1.9.0 to 1.9.4) and scala were updated.

Interestingly vscode doesn't mark assertThrows test line with an error, whereas sbt fails compilation -- I'll dig around some more.

EDIT
here's a scastie, compiles on RC1:
https://scastie.scala-lang.org/0dU3VZ1JTZqWDdJnSCur3Q

@jchyb ☝️

p.s. sorry if this has nothing to do with the merged PR here

…19654) Fixes #19641 How we got here: Originally, overloading resolution for types that were not applied was handled like this: ```scala case defn.FunctionOf(args, resultType, _) => narrowByTypes(alts, args, resultType) case pt => val compat = alts.filterConserve(normalizedCompatible(_, pt, keepConstraint = false)) if (compat.isEmpty) /* * the case should not be moved to the enclosing match * since SAM type must be considered only if there are no candidates * For example, the second f should be chosen for the following code: * def f(x: String): Unit = ??? * def f: java.io.OutputStream = ??? * new java.io.ObjectOutputStream(f) */ pt match { case SAMType(mtp, _) => narrowByTypes(alts, mtp.paramInfos, mtp.resultType) case _ => // pick any alternatives that are not methods since these might be convertible // to the expected type, or be used as extension method arguments. val convertible = alts.filterNot(alt => normalize(alt, IgnoredProto(pt)).widenSingleton.isInstanceOf[MethodType]) if convertible.length == 1 then convertible else compat } else compat ``` Note the warning comment that the case for SAM types should not be moved out, yet we do exactly the same thing for plain function types. I believe this was simply wrong, but it was not discovered in a test. Then in #16507 we changed the `defn.FunctionOf` extractor so that aliases of function types were matched by it. This triggered test failures since we now hit the wrong case with aliases of function types. In #18286, we moved the extractor test around, but that was not enough, as #19641 shows. Instead the test for `FunctionOf` should be aligned with the test for SAM case. But it turns out that's not even necessary since the preceding `val compat = ...` handles function prototypes correctly by simulating an eta expansion. So in the end we could simply delete the problematic case.

jchyb added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 25, 2023

jchyb force-pushed the fix-i17245 branch from 463017d to 94e4df9 Compare July 25, 2023 11:31

nicolasstucki reviewed Jul 25, 2023

View reviewed changes

jchyb marked this pull request as draft July 25, 2023 12:29

jchyb force-pushed the fix-i17245 branch from 5cb14fe to 101ce3a Compare July 26, 2023 12:06

jchyb mentioned this pull request Jul 26, 2023

Ambiguously overloaded methods fail to error when assigned to a val #18294

Closed

jchyb marked this pull request as ready for review July 26, 2023 13:56

jchyb requested a review from nicolasstucki July 26, 2023 13:56

nicolasstucki self-assigned this Jul 26, 2023

nicolasstucki approved these changes Jul 27, 2023

View reviewed changes

nicolasstucki assigned jchyb and unassigned nicolasstucki Jul 27, 2023

jchyb merged commit 989c55c into scala:main Jul 27, 2023

jchyb deleted the fix-i17245 branch July 27, 2023 08:55

Kordyjan added this to the 3.4.0 milestone Aug 1, 2023

Kordyjan added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Aug 2, 2023

Kordyjan mentioned this pull request Aug 2, 2023

Backport "Fix regression #17245: Overloaded methods with ClassTags" #18329

Merged

Kordyjan modified the milestones: 3.4.0, 3.3.1 Aug 2, 2023

This was referenced Aug 9, 2023

Record failures to adapt application arguments #18269

Merged

3.3.1-RC5 Release procedure #18373

Closed

Kordyjan added a commit that referenced this pull request Aug 9, 2023

Backport "Fix regression #17245: Overloaded methods with ClassTags" (#…

35f07b2

…18329) Backports #18286

godenji mentioned this pull request Aug 29, 2023

3.3.1-RC4 compiles but 3.3.1-RC5 fails ScalaTest assertThrows without a type parameter #18478

Closed

Kordyjan added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Nov 29, 2023

WojciechMazur mentioned this pull request Dec 4, 2023

Regression in thoughtworksinc / binding.scala #19191

Closed

This was referenced Feb 8, 2024

Infinite recursion when matching a function in an enum case with the same name as a method of that enum #19641

Closed

Drop special treatment of function types in overloading resolution #19654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression #17245: Overloaded methods with ClassTags #18286

Fix regression #17245: Overloaded methods with ClassTags #18286

jchyb commented Jul 25, 2023 •

edited

Loading

nicolasstucki Jul 25, 2023 •

edited

Loading

jchyb Jul 25, 2023 •

edited

Loading

nicolasstucki Jul 25, 2023

jchyb Jul 25, 2023

nicolasstucki Jul 25, 2023

nicolasstucki Jul 25, 2023

jchyb Jul 25, 2023

jchyb commented Jul 25, 2023

nicolasstucki commented Jul 25, 2023

jchyb commented Jul 25, 2023

jchyb commented Jul 25, 2023

jchyb commented Jul 26, 2023 •

edited

Loading

jchyb commented Jul 26, 2023

godenji commented Aug 29, 2023

jchyb commented Aug 29, 2023

godenji commented Aug 29, 2023 •

edited

Loading

Fix regression #17245: Overloaded methods with ClassTags #18286

Fix regression #17245: Overloaded methods with ClassTags #18286

Conversation

jchyb commented Jul 25, 2023 • edited Loading

nicolasstucki Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

jchyb Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

nicolasstucki Jul 25, 2023

Choose a reason for hiding this comment

jchyb Jul 25, 2023

Choose a reason for hiding this comment

nicolasstucki Jul 25, 2023

Choose a reason for hiding this comment

nicolasstucki Jul 25, 2023

Choose a reason for hiding this comment

jchyb Jul 25, 2023

Choose a reason for hiding this comment

jchyb commented Jul 25, 2023

nicolasstucki commented Jul 25, 2023

jchyb commented Jul 25, 2023

jchyb commented Jul 25, 2023

jchyb commented Jul 26, 2023 • edited Loading

jchyb commented Jul 26, 2023

godenji commented Aug 29, 2023

jchyb commented Aug 29, 2023

godenji commented Aug 29, 2023 • edited Loading

jchyb commented Jul 25, 2023 •

edited

Loading

nicolasstucki Jul 25, 2023 •

edited

Loading

jchyb Jul 25, 2023 •

edited

Loading

jchyb commented Jul 26, 2023 •

edited

Loading

godenji commented Aug 29, 2023 •

edited

Loading