Skip to content
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

Jump to bottom

Drop special treatment of function types in overloading resolution #19654

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Drop special treatment of function types in overloading resolution #19654

merged 1 commit into from
Feb 12, 2024

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Feb 9, 2024 •
edited
Loading

Fixes #19641

How we got here:

Originally, overloading resolution for types that were not applied was handled like this:

      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.

@nicolasstucki nicolasstucki linked an issue Feb 9, 2024 that may be closed by this pull request
Infinite recursion when matching a function in an enum case with the same name as a method of that enum #19641
Closed
@odersky odersky marked this pull request as ready for review February 9, 2024 16:26
@odersky odersky requested a review from jchyb February 9, 2024 16:43
@odersky
Copy link
Contributor Author

odersky commented Feb 12, 2024

@jchyb ping for review. It would be good if we got this into 3.4.1, but that means it has to be merged today. /cc @Kordyjan

jchyb
jchyb reviewed Feb 12, 2024
View reviewed changes
Copy link
Contributor

@jchyb jchyb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Apologies for the very late review (and for not replying quickly in the related threads).

When working on #18286 I initially tried to just remove the FunctionOf case, but I discovered that doing only that led to an undesirable change in runtime behavior (which I should have documented somewhere, I apologize for that), so I added some (removed in this PR) workarounds to remedy them. The change can be observed in the snippet below (with additional discussion here):

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

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

I tested this PR with the above snippet, and sure enough, previously it printed "a", after this PR it prints "b". Apparently it was also "b" in scala 2, and "b" makes slightly more sense, so personally I am fine with this change, but I don't think I should have a say in approving runtime changing behavior. @Kordyjan what do you think? Perhaps we should delay 3.4.1? Or is this fine? If this change in behavior is not a problem, I can of course approve this PR.

@odersky
Copy link
Contributor Author

odersky commented Feb 12, 2024 •
edited
Loading

"b" is correct and as specified, and since it is also Scala 2's behavior, we should definitely classify "a" as a bug. Maybe backport to LTS at some point, so that we don't get differing behavior on LTS on the one hand and Scala 2, Scala 3.4 on the other hand. But the backport is not so urgent, since I don't think many people expected "a" anyway. On the other hand, #19641 is a pretty bad bug that needs to be fixed ASAP.

I think we should merge today then it makes it into 3.4.1 tomorrow.

@odersky odersky added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Feb 12, 2024
@odersky odersky merged commit c529a48 into scala:main Feb 12, 2024
19 checks passed
@odersky odersky deleted the fix-19641 branch February 12, 2024 16:45
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
@WojciechMazur WojciechMazur mentioned this pull request Jul 1, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jul 2, 2024
@WojciechMazur
Backport "Drop special treatment of function types in overloading res…
7c12968 
…olution" to LTS (#20914)

Backports #19654 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@som-snytt som-snytt mentioned this pull request Oct 27, 2024
Closed
@WojciechMazur WojciechMazur removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 8, 2024
@som-snytt som-snytt mentioned this pull request Nov 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchyb jchyb jchyb approved these changes

Assignees

@jchyb jchyb

Labels
None yet
Projects
None yet
Milestone
3.4.1
Development

Successfully merging this pull request may close these issues.

Infinite recursion when matching a function in an enum case with the same name as a method of that enum
4 participants
@odersky @jchyb @Kordyjan @WojciechMazur