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

Allow override protected[C] in companion #14105

Merged
merged 2 commits into from
Feb 28, 2022

Conversation

som-snytt
Copy link
Contributor

When extending C in its companion, allow
override protected[C] where C denotes the
enclosing companion module of C.

Forward ports scala/scala#9814

@som-snytt som-snytt marked this pull request as draft December 13, 2021 21:36
@som-snytt
Copy link
Contributor Author

Getting the local companion fails under -from-tasty; therefore, that half of the test is commented out. Since that is consistent with the implementation restriction in Scala 2, consider the local companion check unsupported.

@som-snytt som-snytt marked this pull request as ready for review December 26, 2021 22:39
/*
object X {
// restriction in Scala 2 for local companions
// restriction in Scala 3 under -from-tasty
Copy link
Member

Choose a reason for hiding this comment

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

What's going on with -from-tasty exactly here? If something can be compiled it should also work from tasty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

asking for companionModule fails for local class. Perhaps you mean, conversely, if it fails from tasty, it must also fail always. Probably there is a convenient is(Local) flag check to add to the condition.

Copy link
Member

@smarter smarter Jan 25, 2022

Choose a reason for hiding this comment

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

asking for companionModule fails for local class

Hmm that might be a bug since I could see that leading to different behavior in other situations. Also I tried uncommenting this code and running it with testCompilation but the pickling tests didn't fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try again, against the moving target of HEAD.

@smarter smarter assigned som-snytt and unassigned smarter Jan 25, 2022
@som-snytt
Copy link
Contributor Author

Maybe something has progressed.

Your branch and 'upstream/master' have diverged,
and have 1 and 234 different commits each, respectively.

I had to consult the page on how to test your changes:

sbt:scala3> scala3-compiler-bootstrapped/testCompilation --from-tasty t12494
[info] Test run started
[info] Test dotty.tools.dotc.FromTastyTests.runTestFromTasty started
No files matched "t12494" in test
No files matched "t12494" in test
[info] Test dotty.tools.dotc.FromTastyTests.posTestFromTasty started
[=======================================] completed (1/1, 0 failed, 1s)
-- [E164] Declaration Error: tests/pos/t12494.scala:23:25 --------------------------------------------------------------
23 |        protected[C] def f: Int = 42  // ok
   |                         ^
   |                         error overriding method f in trait C of type => Int;
   |                           method f of type => Int has weaker access privileges; it should be at least protected
Compilation failed for: 'tests/pos/t12494.scala'
[=======================================] completed (1/1, 1 failed, 0s)

Same after rebase.

When extending C in its companion, allow
override protected[C] where C denotes the
enclosing companion module of C.

Lookup of local companion is broken under
`-from-tasty`, so this accommodation is
disallowed for local companions.
@som-snytt
Copy link
Contributor Author

Did not research the problem with getting local companion under -from-tasty, but enabled the neg test and enforce nonlocal class only, so that will fail under any compilation regime.

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.

Otherwise LGTM.

compiler/src/dotty/tools/dotc/typer/RefChecks.scala Outdated Show resolved Hide resolved
@TheElectronWill TheElectronWill enabled auto-merge (squash) February 28, 2022 17:59
@TheElectronWill TheElectronWill merged commit fa08d00 into scala:main Feb 28, 2022
@som-snytt som-snytt deleted the issue/12494-port branch February 28, 2022 19:57
@Kordyjan Kordyjan added this to the 3.1.3 milestone Aug 1, 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.

4 participants