Skip to content

Test inline method returning match type #23276

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.

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented May 27, 2025

OP test (that fails to compile) is tweaked with transparent inline (so that it compiles).

OP test that is neg completely demonstrates the status quo, but perhaps is not desirable behavior.

Addressing #17210

@som-snytt som-snytt force-pushed the issue/17210-eta-v-ctx-apply branch from 2e4c2ae to a995b26 Compare May 31, 2025 17:37
@som-snytt som-snytt force-pushed the issue/17210-eta-v-ctx-apply branch from a995b26 to 6df641e Compare May 31, 2025 17:50
@som-snytt som-snytt marked this pull request as ready for review June 2, 2025 17:42
@Gedochao Gedochao requested a review from jchyb June 4, 2025 08:24
@jchyb
Copy link
Contributor

jchyb commented Jun 16, 2025

Hi @som-snytt,
I tried to dig around to understand what's going on, and I'm not really comfortable accepting and merging this PR. I don't think it's just an issue with an expected order of operations, since replacing the match type with another, simpler one, works:

object BugReport:
  trait Executor
  trait Updatable[+A]

  def tupled: ((Int, Int)) => Unit = ???

  type UpdatableMap[T <: Tuple] = T match // does not work
    case EmptyTuple => EmptyTuple
    case h *: t => Updatable[h] *: UpdatableMap[t]

  type Passthrough[T <: Tuple] = T match // works
    case _ => (Updatable[Int], Updatable[Int])

  def liftAsTupledInThreads[A <: Tuple](f: A => Unit)(using e: Executor): Passthrough[A] => Unit = _ => ()

  run:
    val lifted: ((Updatable[Int], Updatable[Int])) => Unit = liftAsTupledInThreads(tupled)

I feel like having that additional neg test (the behavior of which, to me, also seems undesirable), would be more confusing than helpful in the long run.

I tried to find a fix in the compiler, and the difference between the behaviors of the two match-types seems to be caused by the constrainResult method (where for a method returning Passthrough it returns true, and for the other case it returns false, in turn causing the eta expansion later). I'll try to dig around some more, but I cannot promise anything unfortunately.

@som-snytt
Copy link
Contributor Author

@jchyb one could craft a pending test, but that is just a purgatory for tests.

Maybe it needs a status quo test group for undesired behavior. With no "regression" test at all, there's no way to tell when behavior accidentally changes. (And sometimes "bug compatibility" is a factor in whether to change behavior intentionally.)

I think a confusing case like this one (where it isn't clear how to fix it, or whether to prioritize working on it, or maybe even what the desired behavior is) needs a test (of some kind).

I understand your concern, as I just removed a Scala 2 pos test that actually tested the "bad" behavior because in ancient times it was moved in bulk from pending. Which PR was that? oh the one merged yesterday:

scala/bug#4649 (comment)

That comment was just a year ago but also feels like ancient history.

@som-snytt som-snytt marked this pull request as draft June 16, 2025 18:05
@som-snytt
Copy link
Contributor Author

Drafting to reformulate "status quo" or "pending".

@som-snytt
Copy link
Contributor Author

Sample "document status quo" that may progress soon.

#22621

Where there is a reference (doc) change not linked to

#23378

I guess every PR risks doc going stale.

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.

2 participants