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

Minimal support for dependent case classes #21698

Merged
merged 4 commits into from
Feb 19, 2025
Merged

Conversation

smarter
Copy link
Member

@smarter smarter commented Oct 3, 2024

This lets us write:

    trait A:
      type B

    case class CC(a: A, b: a.B)

Pattern matching works but isn't dependent yet:

    x match
      case CC(a, b) =>
        val a1: A = a
        // Dependent pattern matching is not currently supported
        // val b1: a1.B = b
        val b1 = b // Type is CC#a.B

(for my usecase this isn't a problem, I'm working on a type constraint API which lets me write things like case class CC(a: Int, b: Int GreaterThan[a.type]))

Because case class pattern matching relies on the product selectors _N, making it dependent is a bit tricky, currently we generate:

    case class CC(a: A, b: a.B):
      def _1: A = a
      def _2: a.B = b

So the type of _2 is not obviously related to the type of _1, we probably need to change what we generate into:

    case class CC(a: A, b: a.B):
      @uncheckedStable def _1: a.type = a
      def _2: _1.B = b

But this can be done in a separate PR.

Fixes #8073.

@noti0na1
Copy link
Member

noti0na1 commented Oct 3, 2024

I am doing some experiment with dependent pattern match in CC, and this PR is useful for me as well!

I modified the desuger so _1, _2, etc will have better types: always use singleton types when the field is not var. I haven't finished because I also need to modify the typer so the selections in the cases are binded to something.

@smarter smarter force-pushed the i8073 branch 2 times, most recently from 83c5e98 to 8d1c7fb Compare October 3, 2024 20:06
@smarter
Copy link
Member Author

smarter commented Oct 3, 2024

@noti0na1 Awesome! Let me know how it goes.

@smarter smarter self-assigned this Oct 3, 2024
@smarter smarter force-pushed the i8073 branch 4 times, most recently from 3f3ce18 to e5327ac Compare October 5, 2024 15:52
@smarter smarter requested a review from odersky October 5, 2024 15:53
@smarter smarter assigned odersky and unassigned smarter Oct 5, 2024
@odersky
Copy link
Contributor

odersky commented Oct 7, 2024

I think it's probably better to wait after the cutoff with this?

@smarter
Copy link
Member Author

smarter commented Oct 7, 2024

So 3.6.1 rather than 3.6.0?

@odersky
Copy link
Contributor

odersky commented Oct 7, 2024

Yes, 3.6.1 should work OK.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

AFAICS this only touches the fromProduct method. So it would be good to clarify some more what the limitations are. You mention dependent pattern matching. Ae there other things that don't work as one would expect?

But anyway this change in isolation LGTM

@odersky odersky assigned smarter and odersky and unassigned odersky Nov 4, 2024
@sjrd
Copy link
Member

sjrd commented Nov 4, 2024

Will we be able to ever fix the remaining things without breaking binary/TASTy compat in the future?

Currently we're safe because the definition does not compile. But if we make it compile today with incorrect code for some methods, we might be stuck with those incorrect definitions forever.

@noti0na1
Copy link
Member

noti0na1 commented Dec 4, 2024

Hi @smarter , is there any plan to merge this?

@smarter
Copy link
Member Author

smarter commented Dec 5, 2024

@noti0na1 I wanted to look into combining this with #21828 to future-proof it, but I haven't had the time to do that yet.

@noti0na1
Copy link
Member

noti0na1 commented Dec 5, 2024

I wanted to look into combining this with #21828 to future-proof it

I don't have more progress on #21828 right now😢

There are still many things broken, and I'm also worry about binary compatibility.

@sjrd
Copy link
Member

sjrd commented Feb 12, 2025

Core Meeting says let's go ahead with this as is.

@smarter Could you rebase on the latest main to get a recent CI run?

This used to fail with:
    trait <refinement> in value x extends enum EC, but extending enums is prohibited.
This lets us write:

    trait A:
      type B

    case class CC(a: A, b: a.B)

Pattern matching works but isn't dependent yet:

    x match
      case CC(a, b) =>
        val a1: A = a
        // Dependent pattern matching is not currently supported
        // val b1: a1.B = b
        val b1 = b // Type is CC#a.B

(for my usecase this isn't a problem, I'm working on a type constraint API
which lets me write things like `case class CC(a: Int, b: Int
GreaterThan[a.type])`)

Because case class pattern matching relies on the product selectors `_N`, making
it dependent is a bit tricky, currently we generate:

    case class CC(a: A, b: a.B):
      def _1: A = a
      def _2: a.B = b

So the type of `_2` is not obviously related to the type of `_1`, we probably
need to change what we generate into:

    case class CC(a: A, b: a.B):
      @uncheckedStable def _1: a.type = a
      def _2: _1.B = b

But this can be done in a separate PR.

Fixes scala#8073.
@odersky
Copy link
Contributor

odersky commented Feb 16, 2025

Looks like there is an bad interaction with @unroll which needs to be investigated.

@odersky odersky removed their assignment Feb 16, 2025
@tgodzik tgodzik added the needs-minor-release This PR cannot be merged until the next minor release label Feb 17, 2025
@smarter
Copy link
Member Author

smarter commented Feb 17, 2025

Looks like the unroll phase assumes a specific shape for the fromProduct method:

val Apply(select, args) = defdef.rhs: @unchecked

It already crashes if I define fromProduct manually with braces:

case class Foo(x: String, @unroll y: Int = 1)
object Foo:
  def fromProduct(x: Product): Foo = {
    new Foo(
      x.productElement(0).asInstanceOf[String],
      x.productElement(1).asInstanceOf[Int]
    )
  }

@bishabosha instead of transforming the body of fromProduct, couldn't we directly define the unroll-friendly version of fromProduct in SyntheticMembers ?

@bishabosha
Copy link
Member

bishabosha commented Feb 18, 2025

instead of transforming the body of fromProduct, couldn't we directly define the unroll-friendly version of fromProduct in SyntheticMembers ?

It should work but probably worth checking that these are still generated after we detect unroll annotations on parameters (which updates the compilation unit flag)

But it might be simpler to detect user defined method if the goal is to 'not crash'?
Edit: i see now that this is about compiler generated code crashing

so yeah architecture is in PostTyper we scan parameters for unroll, which updates a field in CompilationUnit, you can probably check for that in SyntheticMembers - then remove the special handling in Unroll phase which would simplify that a bunch.

@smarter smarter force-pushed the i8073 branch 2 times, most recently from 0b1c9e3 to f3bd4e2 Compare February 18, 2025 20:03
* x$0.productElement(2)
* else
* <default getter for the third parameter of C>
* ).asInstanceOf[a$1.Elem]
Copy link
Member

Choose a reason for hiding this comment

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

.asInstanceOf[Seq[String]]

)
).setDefTree
}

private enum Gen:
case Substitute(origin: Symbol, newDef: DefDef)
Copy link
Member

Choose a reason for hiding this comment

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

this should be dead code now i think

UnrollDefinitions assumed that the body of `fromProduct` had a specific shape
which is no longer the case with the dependent case class support introduced in
the previous commit. This caused compiler crashes for
tests/run/unroll-multiple.scala and tests/run/unroll-caseclass-integration

This commit fixes this by directly generating the correct fromProduct in
SyntheticMembers. This should also prevent crashes in situations where code is
injected into existing trees like the code coverage support or external compiler
plugins.
This accounts for singletons wrapping an ErasedValueType.
@smarter smarter merged commit 345b2da into scala:main Feb 19, 2025
27 checks passed
@smarter smarter deleted the i8073 branch February 19, 2025 20:56
@smarter
Copy link
Member Author

smarter commented Feb 19, 2025

oops, I forgot the auto-merge was on, can you double-check the changes related to unroll @bishabosha ?

@bishabosha
Copy link
Member

bishabosha commented Feb 20, 2025

@smarter seems good - the test case for illegal unroll on fromProduct is here https://github.com/scala/scala3/blob/main/tests/neg/unroll-duped.scala, so it seems no behavior change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement parameter dependent case classes
6 participants