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

Disallow overloading from breaking stable patterns #18327

Merged
merged 2 commits into from
Aug 14, 2023

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Aug 2, 2023

No description provided.

In particular, must continue to use the type's prefix, rather than the
denotation's prefix.  That way, e.g. in tests/neg/i3812b.scala,
referencing value class'\''s value, will fail on the non-stable value
class prefix, rather than the denotation'\''s prefix, which is NoSymbol.
@dwijnand dwijnand marked this pull request as ready for review August 3, 2023 16:40
@dwijnand dwijnand requested a review from SethTisue August 3, 2023 17:42
@SethTisue
Copy link
Member

SethTisue commented Aug 4, 2023

I had commented on the ticket:

It isn't obvious to me that we can just carve out an exception for this in the compiler unless we also change the spec?

But I noticed that it works fine in Scala 2, so that's precedent for considering this simply a regression and not a design change.

And Dale points out that the workaround suggested by original reporter, val x = Op.foo, compiles, so the compiler isn't bothered by the ambiguity when it's just a term, only in a pattern.

So all of that together convinces me that we don't need to question this as a design change, it's okay to just make it work.

@SethTisue
Copy link
Member

Implementationwise, perhaps it would be more elegant to put the new smarts inside isStable? Of course, isStable is called elsewhere as well. Would anything else progress — or regress — if it were implemented that way?

Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

Dale has tried putting it in isStable and all the tests still pass. Offhand, we can't think of any additional tests we could add that might show a difference.

So I'm going to hit "approve", but it isn't obvious to me which implementation is better. Perhaps someone else would like to weigh in on that? If nobody else has an opinion, Dale could make the call.

@SethTisue SethTisue removed their assignment Aug 8, 2023
@dwijnand dwijnand merged commit ad6da48 into scala:main Aug 14, 2023
@dwijnand dwijnand deleted the stable-alt branch August 14, 2023 20:36
Kordyjan added a commit that referenced this pull request Dec 8, 2023
…19163)

Backports #18327 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@Kordyjan Kordyjan added this to the 3.3.2 milestone Dec 14, 2023
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.

Backquoted == operator not treat as stable identifier during pattern match.
3 participants