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.

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

Reimplement support for type aliases in SAM types #18317

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

smarter
Copy link
Member

@smarter smarter commented Jul 31, 2023

This was dropped in #18201 which restricted SAM types to valid parent types, but it turns out that there is code in the wild that relies on refinements being allowed here.

To support this properly, we had to enhance ExpandSAMs to move refinements into type members to pass Ycheck (previous Scala 3 releases would accept the code in tests/run/i18315.scala but fail Ycheck).

Fixes #18315.

This should be backported to any branch where #18201 is backported.

@smarter smarter added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 31, 2023
@smarter smarter requested a review from prolativ July 31, 2023 22:20
@dwijnand
Copy link
Member

Missing the test case?

This was dropped in scala#18201 which restricted SAM types to valid parent types,
but it turns out that there is code in the wild that relies on refinements
being allowed here.

To support this properly, we had to enhance ExpandSAMs to move refinements into
type members to pass Ycheck (previous Scala 3 releases would accept the code in
tests/run/i18315.scala but fail Ycheck).

Fixes scala#18315.
@smarter
Copy link
Member Author

smarter commented Jul 31, 2023

oops yes


object Test:
def main(args: Array[String]): Unit =
val s1: Sam1 { type T = String } = x => x.trim
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I'm asking for too much but I thought it would be nice if the type member of a SAM type could be inferred based on an explicitly provided type in a lambda, which led me to suggesting 2 additional test cases:

    def foo[S <: Sam1](s: S): S = s
    foo { (x: String) => x.trim }

    def bar[A](s: Sam1 { type T = A }): s.type = s
    bar { (x: String) => x.trim }

bar works as expected but the application of foo doesn't compile:

-- [E007] Type Mismatch Error: -------------------------------------------------
1 |foo { (x: String) => x.trim }
  |                     ^^^^^^
  |                     Found:    String
  |                     Required: Sam1#T
  |-----------------------------------------------------------------------------
  | Explanation (enabled by `-explain`)
  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
  |
  | Tree: x.trim()
  | I tried to show that
  |   String
  | conforms to
  |   Sam1#T
  | but the comparison trace ended with `false`:
  |
  |   ==> String  <:  Sam1#T
  |   <== String  <:  Sam1#T = false
  |
  | The tests were made under a constraint with:
  |  uninstantiated variables:
  |  constrained types: [S <: Sam1](s: S): S
  |  bounds:
  |      S <: Sam1
  |  ordering:
  |  co-deps:
  |  contra-deps:
   -----------------------------------------------------------------------------

Would it be a big deal to make foo work?
Otherwise LGTM

Copy link
Member

Choose a reason for hiding this comment

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

We should definitely not make more things work in this space. What this PR "fixes" is already off-spec, because it is not legal to put a refinement type in an extends clause. It happened to work in 3.3.0 by accident, and so we're preserving the status quo in the name of backward compatibility within a patch series. But we should in fact make it an error in 3.4.x.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wouldn't want to add more than this and definitely not in a PR that fixes a regression

@smarter smarter merged commit 10503b0 into scala:main Aug 1, 2023
@smarter smarter deleted the sam-refinements branch August 1, 2023 10:06
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Oct 10, 2023
WojciechMazur added a commit that referenced this pull request Sep 6, 2024
…#21553)

Backports #18317 to 3.3.4-RC2 LTS. The PR fixes a regression introduced
in 3.3.2
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.

Output type mismatch in SAM
5 participants