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

Extend pattern type constraining to sealed hierarchies #16924

Closed
wants to merge 1 commit into from

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Feb 15, 2023

Fixes #4790
Adds sealed to case, where #14820 looked to handle all classes.

Require sealed traits to be extended invariantly.

@dwijnand dwijnand changed the title Extend pattern type constraining to closed hierarchies Extend pattern type constraining to sealed hierarchies Feb 20, 2023
@dwijnand dwijnand self-assigned this Feb 20, 2023
@dwijnand dwijnand force-pushed the sealed-hierarchy-invariant branch from 8a09d70 to 9e430a4 Compare February 21, 2023 11:06
@dwijnand dwijnand marked this pull request as ready for review February 21, 2023 12:30
@dwijnand dwijnand requested a review from Linyxus February 21, 2023 12:32
@dwijnand dwijnand assigned Linyxus and unassigned dwijnand Feb 21, 2023
@dwijnand dwijnand assigned abgruszecki and unassigned Linyxus Mar 2, 2023
@dwijnand dwijnand requested review from abgruszecki and removed request for Linyxus March 2, 2023 08:57
@abgruszecki
Copy link
Contributor

abgruszecki commented Mar 6, 2023

OK, so if I understand this correctly, after this PR sealed trait-s will also require themselves to be extended invariantly. At first glance, everything looks like it works, though I'll need to spend a bit more time thinking about it, before the end of this week.

I think we should have more tests too, for instance ones that show Guillaume's examples from here #16885 (comment) do not work anymore. Also, it's not clear to me if this example should compile, FB should be rejected since I don't think it's possible to create an instance of it :

scala> object O {
     |   sealed trait Foo[+A] {
     |     def foo: A
     |   }
     | 
     |   sealed trait Bar[+A] extends Foo[A] {
     |     def bar: A
     |   }
     | 
     |   sealed trait FB extends Bar[Any] with Foo[Int] {
     |     def bar: Any = ""
     |     def foo: Int = 1
     |   }
     | }
// defined object O

Last thing: the impact of changing how sealed trait-s work looks reasonable to me, but it will have to be discussed with Martin.

@dwijnand
Copy link
Member Author

dwijnand commented Mar 6, 2023

I think we should have more tests too, for instance ones that show Guillaume's examples from here #16885 (comment) do not work anymore.

Sure, no problem.

Also, it's not clear to me if this example should compile, FB should be rejected since I don't think it's possible to create an instance of it

It isn't necessary to forbid that, is it? I assume there are other cases like that - but perhaps not.

@abgruszecki
Copy link
Contributor

Also, it's not clear to me if this example should compile, FB should be rejected since I don't think it's possible to create an instance of it

It isn't necessary to forbid that, is it? I assume there are other cases like that - but perhaps not.

It's not necessary, but FB already inherits from Foo in two different conflicting ways. If the trait can't be instantiated we should emit an error when it is defined, not when we try to extend it.

@abgruszecki
Copy link
Contributor

Ok, so it's really not looking like I will have any time to experiment with this branch and see what the code is actually doing. As far as I can tell, there are two approaches here:

1) Require sealed traits to be extended invariantly

This is effectively what the changes to RefChecks implement, as far as I can tell. Previously only case classes had this restriction, and it was implemented IIUC because it was seen as very strange to extend a case class. Extending the restriction to sealed traits does feel reasonable to me, but you will need to come up with a justification as to why it won't break too much code.

Also, if this is what you intend to do, then refinementIsInvariantCls is too complicated. It'd be enough to check if cls is sealed.

2) Check sealed traits for being effectively extended invariantly

This seems to be what you're trying to do in refinementIsInvariantCls. In this approach, we have special reasoning for sealed traits that are only extended by case and final classes. This is as though we had "invariant refinement" that Guillaume mentioned here, but it was only inferred in particular positions: for type arguments of case classes and type arguments of some sealed traits. Note that you need to be very careful with a check like this, because as Guillaume's example shows, it's enough to extend sealed trait Foo with sealed trait Bar so that it's possible to define a case class that extends Foo variantly. (In Guillaume's exmaple class FB isn't case, but it could be.)

If this is what you intend to do, then there should be no change to RefChecks; it'd be polite if you'd also come up with an explanation as to why the check is actually sound, and include the explanation in the PR.

Also, this is a very subtle change. I would expect to see multiple tests that exercise the change and show what does and what doesn't work, ideally closer to 5 tests rather than 1.

@dwijnand dwijnand force-pushed the sealed-hierarchy-invariant branch from 9e430a4 to 7e3765b Compare March 23, 2023 12:32
@dwijnand dwijnand force-pushed the sealed-hierarchy-invariant branch from 7e3765b to 849598f Compare March 25, 2023 18:38
@dwijnand dwijnand force-pushed the sealed-hierarchy-invariant branch from 849598f to 1d88777 Compare May 8, 2024 10:04
@dwijnand dwijnand closed this Sep 25, 2024
@dwijnand dwijnand deleted the sealed-hierarchy-invariant branch September 25, 2024 22:29
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.

Typing varargs in pattern position
4 participants