Skip to content

Conversation

mberndt123
Copy link
Contributor

@mberndt123 mberndt123 commented Aug 24, 2025

Hi,

this is my attempt at implementing the SIP-67 specification. Thanks to @noti0na1 and @dwijnand for the hints you gave in #22732. Sorry it took so long, please let me know what you think.

@mberndt123 mberndt123 force-pushed the sip-67-strict-equality-improvements branch from 3bf5df8 to e3d22ab Compare August 26, 2025 21:14
@mberndt123 mberndt123 force-pushed the sip-67-strict-equality-improvements branch from e3d22ab to 0a565d0 Compare August 26, 2025 21:14
@Gedochao Gedochao requested review from noti0na1 and odersky August 27, 2025 11:19
untpd.Select(untpd.TypedSplice(tree), nme.EQ),
untpd.TypedSplice(dummyTreeOfType(pt)))
typedExpr(cmp, defn.BooleanType)
if ! (tree.tpe <:< pt && (tree.symbol.flags.isAllOf(Flags.EnumValue) || (tree.symbol.flags.isAllOf(Flags.Module | Flags.Case)))) then
Copy link
Contributor

@odersky odersky Aug 30, 2025

Choose a reason for hiding this comment

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

Some remarks:

  1. The motivation for the SIP was that it makes strictEquality easier to use, but this also changes the pattern matching behavior of patterns without that import. And it breaks the law that pattern matching values is done with an == check. I think this goes too far, even though this is what's specified in the SIP.

  2. We cannot implement a SIP change without going through an experimenal language import.

  3. I think it would be better to add code to assumedCanEqual instead. Under strictEquality and the experimental import, if the left operand (or either one) is an enum constant, and the other operand is a supertype, assume the two CanEqual.

Copy link
Contributor Author

@mberndt123 mberndt123 Aug 31, 2025

Choose a reason for hiding this comment

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

Hi @odersky,

Thank you for your review.

Regarding 1: 🤔 I'm not sure I understand. The SIP says “The semantics of pattern matching against a constant are otherwise unchanged”, meaning that it will still be equivalent to ==, except it doesn't require CanEqual. If that's not what it does, then it's a bug in my implementation. But I think that's solved by moving it to assumeCanEqual as you suggested in point 3, right?

Regarding 2: ✅ I've added an experimental language import, strictEqualityPatternMatching, to enable this behaviour. I also tried adding a Mode for this so it can be enabled with a command line flag similar to -language:strictEquality, but unless I'm misreading the code, the modes are stored in an Int bitmask and all the bits are already taken 😒

Regarding 3: ✅ I've moved the implementation to assumedCanEqual, which was a bit tricky because in that function the Tree isn't available, but I need it to know if it's a case object or enum case. It's also called from Synthesizer where there is no Tree, so I'm passing it as an Option[Tree] now.

@odersky odersky assigned mberndt123 and unassigned odersky Aug 30, 2025
@mberndt123 mberndt123 requested a review from a team as a code owner August 31, 2025 20:54
@mberndt123 mberndt123 force-pushed the sip-67-strict-equality-improvements branch 3 times, most recently from 2bb5197 to e9fb60b Compare August 31, 2025 21:17
@mberndt123 mberndt123 force-pushed the sip-67-strict-equality-improvements branch from e9fb60b to 4f7a9a3 Compare August 31, 2025 21:46
@mberndt123 mberndt123 requested a review from odersky August 31, 2025 22:06
Copy link
Member

Choose a reason for hiding this comment

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

Please add this object in scala.language.experimental too (Not just in the stdLibPatches).

Copy link
Contributor Author

@mberndt123 mberndt123 Sep 15, 2025

Choose a reason for hiding this comment

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

Thanks for pointing this out Hamza, I've now added it.

@mberndt123 mberndt123 force-pushed the sip-67-strict-equality-improvements branch from a82ddbd to 79fc91d Compare September 15, 2025 21:28
@mberndt123
Copy link
Contributor Author

Hey @Gedochao ,

It's been a while since you assigned this to Martin, and I addressed his feedback as well as @hamzaremmal's. But I have the impression that it wasn't noticed yet, perhaps you could assign it back to @odersky?

@soronpo
Copy link
Contributor

soronpo commented Sep 23, 2025

I will mention the implementation is ready in the upcoming SIP meeting this Friday. It's already on the agenda.

@mberndt123
Copy link
Contributor Author

Thanks @soronpo!

@SethTisue SethTisue added this to the 3.8.0 milestone Sep 24, 2025
@SethTisue
Copy link
Member

tentatively milestoning for 3.8.0 since it should least be evaluated for inclusion

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.

Otherwise LGTM in the sense that it implements SIP 67 as specced and I believe after the suggested changes are implemented it will fit well with the rest of the compiler.

* unless strict equality is set.
*/
def assumedCanEqual(ltp: Type, rtp: Type)(using Context) = {
def assumedCanEqual(leftTreeOption: Option[Tree], ltp: Type, rtp: Type)(using Context): Boolean = {
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use EmptyTree for missing trees instead of an option type. Also, leftTree should be the last argument. Suggestion:

def assumedCanEqual(ltp: Type, rtp: Type, leftTree: Tree = EmptyTree)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks 👍🏻

/** Check that equality tests between types `ltp` and `rtp` make sense */
def checkCanEqual(ltp: Type, rtp: Type, span: Span)(using Context): Unit =
if (!ctx.isAfterTyper && !assumedCanEqual(ltp, rtp)) {
def checkCanEqual(left: Tree, rtp: Type, span: Span)(using Context): Unit =
Copy link
Contributor

Choose a reason for hiding this comment

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

ltp is not part of the API anymore, so the doc comment needs to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've updated the comment

|| locally:
if strictEquality then
strictEqualityPatternMatching && leftTreeOption.exists: leftTree =>
ltp <:< lift(rtp) && (leftTree.symbol.flags.isAllOf(Flags.EnumValue) || leftTree.symbol.flags.isAllOf(Flags.Module | Flags.Case))
Copy link
Contributor

Choose a reason for hiding this comment

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

.flags is redundant, should be omitted. Suggestion:

          if strictEqualityPatternMatching 
            && (leftTree.symbol.isAllOf(Flags.EnumValue) || leftTree.symbol.isAllOf(Flags.Module | Flags.Case))
          then ltp <:< lift(rtp)
          else false

More pedestrian but much clearer and more efficient.

Copy link
Contributor Author

@mberndt123 mberndt123 Sep 25, 2025

Choose a reason for hiding this comment

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

Thanks, I've removed the .flags. Re replacing && with if: that would lead to nested if expressions, and I think a simple && is easier to read than that. I'd also like to understand why if would be more efficient given that && implements short-circuiting.

I'd prefer leaving it like this, but will change it if you insist.

@mberndt123 mberndt123 requested a review from odersky September 26, 2025 11:14
@tgodzik
Copy link
Contributor

tgodzik commented Sep 29, 2025

@odersky is it ok to merge now?

@odersky
Copy link
Contributor

odersky commented Oct 2, 2025

Yes, let's merge.

@odersky
Copy link
Contributor

odersky commented Oct 2, 2025

@hamzaremmal You also need to approve before we can merge.

@odersky odersky merged commit 172687a into scala:main Oct 2, 2025
49 checks passed
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.

7 participants