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

Fix #21402: Always allow type member extraction for stable scrutinees in match types. #21700

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Oct 4, 2024

Previously, through the various code paths, we basically allowed type member extraction for stable scrutinees if the type member was an alias or a class member. In the alias case, we took the alias, whereas in the class case, we recreated a selection on the stable scrutinee. We did not allow that on abstract type members.

We now uniformly do it for all kinds of type members. If the scrutinee is a (non-skolem) stable type, we do not even look at the info of the type member. We directly create a selection to it, which corresponds to what we did before for class members.

We only try to dealias type members if the scrutinee type is not a stable type.


Note that this goes against the current spec and should in theory require (yet another) SIP amendment. The core meeting decided a few ago to go for it anyway and "ask for forgiveness" later so that we can have it in 3.6.0 🤷‍♂️.

@sjrd sjrd requested a review from dwijnand October 4, 2024 08:09
@sjrd sjrd linked an issue Oct 4, 2024 that may be closed by this pull request
@sjrd sjrd added this to the 3.6.0 milestone Oct 4, 2024
Comment on lines 3686 to 3692
/* If it is a skolem type, we cannot have class selections nor
* abstract type selections. If it is an alias, we try to remove
* any reference to the skolem from the right-hand-side. If that
* succeeds, we take the result, otherwise we fail as not-specific.
*/
def adaptToTriggerNotSpecific(info: Type): Type = info match
Copy link
Member

@smarter smarter Oct 4, 2024

Choose a reason for hiding this comment

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

It looks like the doc comment refers to the code below starting with denot.info match, not to the behavior of the def itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's about the whole body of the case SkolemType. I added a blank line, which hopefully makes it not misleading anymore?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, usually we write those comments with //.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought /* was just the multi-line version of //, whereas /** is the doc-of-the-def thing.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah using /* is fine, just uncommon in this codebase I think

Copy link
Member

Choose a reason for hiding this comment

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

I always doc'd local defs with /* seeing as the docs don't appear anywhere in the scaladocs anyways, to not be misleading as to that. It's funny how that thought process leads to unintended confusion in this case 😄

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

It would be nice to add a minimal testcase to illustrate the feature.

…inees in match types.

Previously, through the various code paths, we basically allowed
type member extraction for stable scrutinees if the type member
was an alias or a class member. In the alias case, we took the
alias, whereas in the class case, we recreated a selection on the
stable scrutinee. We did not allow that on abstract type members.

We now uniformly do it for all kinds of type members. If the
scrutinee is a (non-skolem) stable type, we do not even look at
the info of the type member. We directly create a selection to it,
which corresponds to what we did before for class members.

We only try to dealias type members if the scrutinee type is not
a stable type.
@sjrd sjrd enabled auto-merge October 8, 2024 08:07
@sjrd sjrd added the needs-minor-release This PR cannot be merged until the next minor release label Oct 8, 2024
@sjrd sjrd merged commit 2023c5d into scala:main Oct 8, 2024
26 checks passed
@sjrd sjrd deleted the fix-i21402 branch October 8, 2024 09:33

def adaptToTriggerNotSpecific(info: Type): Type = info match
case info: TypeBounds => info
case _ => RealTypeBounds(info, info)
Copy link
Member

Choose a reason for hiding this comment

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

This looks similar to Type#bounds. That might or might not be appropriate here, but wanted to bring it to your attention.

@dwijnand
Copy link
Member

dwijnand commented Oct 9, 2024

I'm surprised that the spec language in https://docs.scala-lang.org/sips/match-types-spec.html isn't part of the spec in this repo.

@sjrd
Copy link
Member Author

sjrd commented Oct 9, 2024

I'm surprised that the spec language in https://docs.scala-lang.org/sips/match-types-spec.html isn't part of the spec in this repo.

I did not have time to integrate it yet. :(

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.

No workaround to dependent-type match types after SIP-56
3 participants