@publicInBinary override spec change
#19296
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicolasstucki
force-pushed
the
binaryAPI-override-spec-change
branch
from
34f3dee to
5e80305
Compare
sjrd
reviewed
Dec 19, 2023
| @@ -544,6 +544,12 @@ object Checking { | |||
| else if !sym.owner.isClass && !(sym.is(Param) && sym.owner.isConstructor) then fail(em"@publicInBinary cannot be used on local definitions") | |||
| else if sym.is(ParamAccessor) && sym.is(Private) then fail(em"@publicInBinary cannot be non `val` constructor parameters") | |||
| else if sym.is(Private) && !sym.privateWithin.exists && !sym.isConstructor then fail(em"@publicInBinary cannot be used on private definitions\n\nConsider using `private[${sym.owner.name}]` or `protected` instead") | |||
| else | |||
| sym.allOverriddenSymbols.find(sym => sym.hasAnnotation(defn.PublicInBinaryAnnot)) match | |||
There was a problem hiding this comment.
As mentioned by @smarter somewhere else, there's a good chance that this code would behave much better in RefChecks. One of the main purposes of RefChecks is to check properties across the inheritance chain of methods, so it would make sense there.
There was a problem hiding this comment.
I made the change. I will test the community build with the CI.
nicolasstucki
force-pushed
the
binaryAPI-override-spec-change
branch
from
5e80305 to
dcf42cf
Compare
sjrd
requested changes
Dec 19, 2023
Comment on lines
583
to
584
| else if !member.hasAnnotation(defn.PublicInBinaryAnnot) && other.hasAnnotation(defn.PublicInBinaryAnnot) then | ||
| overrideError("also needs to be declared with @publicInBinary") |
There was a problem hiding this comment.
This must come before the overrideDeprecation test, otherwise a deprecation warning can shadow an actual error.
There was a problem hiding this comment.
Moved and updated docs.
nicolasstucki
force-pushed
the
binaryAPI-override-spec-change
branch
from
dcf42cf to
7205496
Compare
sjrd
approved these changes
Dec 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup of #18402 (see #18402 (comment)).
This was split from the main PR due to the blocker described in #18402 (comment). During the Dotty meeting, we decided to merge #18402 without the second commit (Require @publicInBinary on overrides). If we manage to fix this issue before 3.4.0-RC1 we will include it there, otherwise it will be part of 3.4.0-RC2.