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

Add explanation to checkCaseClassInheritanceInvariant error msg #20141

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

EugeneFlesselle
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle commented Apr 9, 2024

Closes #18552 which was actually not an error, see:

/** Check that inheriting a case class does not constitute a variant refinement
* of a base type of the case class. It is because of this restriction that we
* can assume invariant refinement for case classes in `constrainPatternType`.
*/
def checkCaseClassInheritanceInvariant() =

Co-authored-by: Anna Herlihy [email protected]
Co-authored-by: Natsu Kagami [email protected]

@EugeneFlesselle EugeneFlesselle requested a review from mbovel April 9, 2024 20:28
Copy link
Member

@mbovel mbovel left a comment

Choose a reason for hiding this comment

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

Looks okay to me!

Two minor questions:

  • Is the word "conflicting" really appropriate here? If it is easy to change, maybe I'd write something like "class MB cannot inherit from two instances of base trait M".
  • Is it clear that "enabling better GADT constraints" explains why it is implemented that way? Maybe split in a second sentence "This is an implementation limitation that enables better GADT constraints in case class patterns".

@mbovel
Copy link
Member

mbovel commented Apr 10, 2024

@SimonGuilloud what do you think? Would this message have helped?

@EugeneFlesselle
Copy link
Contributor Author

If it is easy to change, maybe I'd write something like "class MB cannot inherit from two instances of base trait M".

Technically speaking, it can inherit from two instances

trait M[+T]
case class MAny(id:Int) extends M[Int]
class MInt(id:Int)  extends MAny(id) with M[Any] 
// Ok: they do not conflict since M[Int] <: M[Any]

trait M[+T]
case class Mcc(id:Int)
class MAny(id:Int) extends Mcc(id) with M[Any]
class MInt(id:Int)  extends MAny(id) with M[Int] 
// Ok: the "conflicting" instances are only below the case class

trait M[+T]
case class MAny(id:Int) extends M[Any]
class MInt(id:Int)  extends MAny(id) with M[Int] 
// Error because M[Int] refines M[Any] which is a base type of an intermediate case class

Is it clear that "enabling better GADT constraints" explains why it is implemented that way? Maybe split in a second sentence "This is an implementation limitation that enables better GADT constraints in case class patterns".

I agree that's better 👍

@mbovel mbovel self-requested a review April 10, 2024 11:00
@EugeneFlesselle EugeneFlesselle enabled auto-merge (squash) April 10, 2024 11:03
@SimonGuilloud
Copy link

I think it is certainly better ("This error is blatantly wrong" => "I don't understand the message but alright").
It's the second time I encounter a "bug" which is actually a limitation due to GADT reasoning, but I'm not even sure what it actually does. So for me the solution was to make every class non-case and reimplement hashCode and equals every time. Not great and bug prone, but that's the best I found.

@EugeneFlesselle EugeneFlesselle merged commit ed9fecc into scala:main Apr 10, 2024
18 checks passed
@EugeneFlesselle EugeneFlesselle deleted the issues/i18552 branch April 10, 2024 12:11
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
WojciechMazur added a commit that referenced this pull request Jul 5, 2024
WojciechMazur added a commit that referenced this pull request Jul 5, 2024
… msg" to LTS (#21055)

Backports #20141 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
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.

conflicting instances with inheritance from case class
4 participants