8000 Allow overriding `protected[Base]` methods by som-snytt · Pull Request #9814 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

Allow overriding protected[Base] methods #9814

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 24, 2022
Merged

Allow overriding protected[Base] methods #9814

merged 2 commits into from
Nov 24, 2022

Conversation

som-snytt
Copy link
Contributor
@som-snytt som-snytt commented Nov 24, 2021 •
edited
Loading

Fixes scala/bug#12494

This is for the static module case, not for local companions.

@scala-jenkins scala-jenkins added this to the 2.13.8 milestone Nov 24, 2021
@som-snytt
Copy link
Contributor Author

TFW Travis passes but validate-main fails.

I guess spurious due to longer-lived test

ok  8 - presentation/doc                        [duration 5.59s]

The actual diff does not look benign:

!!  1 - presentation/doc                          [output differs][duration 13.69s]
% diff /home/jenkins/workspace/scala-2.13.x-validate-main@2/test/files/presentation/doc.check /home/jenkins/workspace/scala-2.13.x-validate-main@2/test/files/presentation/doc-presentation.log
@@ -1,2 +1,3 @@
 warning: 1 deprecation (since 2.13.0); re-run with -deprecation for details
 reload: Base.scala, Class.scala, Derived.scala
+Unexpected foo method comment:None

Wait, doesn't my test include a Base class? Just coincidence.

@som-snytt
Copy link
Contributor Author

/rebuild

@SethTisue SethTisue added prio:blocker release blocker (used only by core team, only near release time) and removed prio:blocker release blocker (used only by core team, only near release time) labels Nov 25, 2021
som-snytt
som-snytt commented Nov 26, 2021
View reviewed changes
@som-snytt
Copy link
Contributor Author

oops, I need a version of partest --update-check that also does git add and commit.

@lrytz
Copy link
Member
lrytz commented Nov 30, 2021

LGTM, but I'd like Scala 3 to agree. A PR is the best way to get their attention, would you mind submitting one?

https://github.com/lampepfl/dotty/blob/51828cc215ac2b181b6a978144e33e971ba01589/compiler/src/dotty/tools/dotc/typer/RefChecks.scala#L414-L423

@som-snytt
Copy link
Contributor Author

I will follow up in this PR with spec language and also PR to version 3.

@som-snytt som-snytt mentioned this pull request Dec 13, 2021
Merged
@som-snytt
Copy link
Contributor Author
som-snytt commented Dec 13, 2021 •
edited
Loading

While adding spec words, I noticed we didn't mention private[C] which has the same issue.

Scala 3 is prepared to allow it, except that the access modifier is currently wrong. "it should be public" (It compiles in the linked PR.)

Scala 2 doesn't see the override. "Missing implementation"

It seems I tried to understand qualified private a few times since the antepandemic year. I asked about Scala 2's behavior when I noticed that override private[C] works in Dotty.

@dwijnand dwijnand modified the milestones: 2.13.8, 2.13.9 Dec 15, 2021
@SethTisue SethTisue marked this pull request as draft December 15, 2021 17:26
@SethTisue SethTisue modified the milestones: 2.13.8, 2.13.9 Dec 15, 2021
@som-snytt
Copy link
Contributor Author

Since it got bumped from the next milestone, I pushed not name mangling qualified privates.

@som-snytt
Copy link
Contributor Author

The Scala 3 PR was approved, with the limitation on local classes.

The broken commit here must be reworked to work without mangling mangling.

@SethTisue
Copy link
Member

@som-snytt interested in returning to this, now that the Scala 3 PR has been merged?

@som-snytt
Copy link
Contributor Author
som-snytt commented Apr 20, 2022 •
edited
Loading

"disinterested", perhaps. The error is serialization! OMG.

The last commit IIRC had something to do with detecting matching members in the face of mangling. That is, matching was correct without mangling.

It's dispiriting when ordinary fixes are derailed by MIMA-style compatibility or similar problems with encodings.

Edit: the dispiriting part (now I recall) is that the failure was due to Enumeration with its unfortunate encoding.

java.lang.AssertionError: assertion failed: instance = X : class scala.Enumeration$Val  serialization unstable:

@SethTisue SethTisue modified the milestones: 2.13.9, 2.13.10 Apr 25, 2022
@som-snytt som-snytt force-pushed the issue/12494 branch 2 times, most recently from 3fedd33 to 3520472 Compare July 17, 2022 08:05
@lrytz lrytz modified the milestones: 2.13.10, 2.13.11 Sep 26, 2022
@SethTisue
Copy link
Member

this is passing CI, now. does that mean it's ready for review?

@som-snytt
Copy link
Contributor Author

Is there a label for 1 Year Anniversary :tada:?

@som-snytt
Allow companion access boundary
8dfd7f0 
When overriding `protected[C]`, `C` may refer to
the enclosing companion of `C`, which provides the same
access guarantees.

Update spec to allow override protected[C] companion.
@som-snytt
Partest --branch
e6c8da2
@som-snytt
Copy link
Contributor Author

This commit does not add checkfile autocommit per #9814 (comment) but does add partest --branch to run partests touched since upstream (approximately).

Minor syntax tweaks in first commit since previous "looking good!" comment, and rebase.

@som-snytt som-snytt marked this pull request as ready for review November 23, 2022 22:38
@lrytz
Copy link
Member
lrytz commented Nov 24, 2022

Happy Birthday, PR!

@lrytz lrytz merged commit f1be61f into scala:2.13.x Nov 24, 2022
@som-snytt
Copy link
Contributor Author

🍰

I learned "just enough" git and also reflect.io.Path to get something to work for partest --branch but it is not battle-tested.

@som-snytt som-snytt deleted the issue/12494 branch November 24, 2022 16:47
@SethTisue SethTisue changed the title Allow companion access boundary Allow overriding protected[Base] methods Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joroKr21 joroKr21 joroKr21 left review comments

@lrytz lrytz lrytz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.13.11
Development

Successfully merging this pull request may close these issues.

Cannot override protected[Base] methods
6 participants
@som-snytt @lrytz @SethTisue @joroKr21 @dwijnand @scala-jenkins
0