Fix Issue 18951 - package static method masked by public static method in class #8338
Conversation
|
Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#8338" |
| @@ -543,7 +543,7 @@ extern (C++) bool checkSymbolAccess(Scope *sc, Dsymbol s) | |||
| * but doesn't recurse nor resolve aliases because protection/visibility is an | |||
| * attribute of the alias not the aliasee. | |||
| */ | |||
| public Dsymbol mostVisibleOverload(Dsymbol s) | |||
| public Dsymbol mostVisibleOverload(Dsymbol s, Module mod = null) | |||
There was a problem hiding this comment.
You can get rid of the optional parameter and obtain the module form s by calling s.getModule [1]
[1] https://github.com/dlang/dmd/blob/master/src/dmd/dsymbol.d#L327
There was a problem hiding this comment.
No? The module is the module the symbol is supposed to be visible from.
JinShil
changed the title
src/dmd/access.d
Outdated
| * except package() is "private" if the module is outside the package; | ||
| * otherwise, "public". | ||
| */ | ||
| Prot effectiveProtAt(Dsymbol d, Module mod) |
There was a problem hiding this comment.
Please spell this out as "effectiveProtection". I had to read the comment to understand what "Prot" meant.
Ok, I'll take that comment back. It appears prot is the current convention...unfortunately. But it would probably benefit from "From" rather than "At". Even the comment uses "from".
There was a problem hiding this comment.
"Protection from Module" sounds like a D&D spell. 😄
src/dmd/access.d
Outdated
| * except package() is "private" if the module is outside the package; | ||
| * otherwise, "public". | ||
| */ | ||
| Prot effectiveProtAt(Dsymbol d, Module mod) |
There was a problem hiding this comment.
I also recommend making this a nested function in the mostVisibleOverload function, or at a minimum, make it private.
There was a problem hiding this comment.
Done, and renamed to protectionInModule
FeepingCreature
force-pushed
the
fix/Issue_18951
branch
3 times, most recently
from
652c322
to
9afabdb
Compare
src/dmd/access.d
Outdated
| * except package() is "private" if the module is outside the package; | ||
| * otherwise, "public". | ||
| */ | ||
| Prot protectionInModule(Dsymbol d, Module mod = null) |
There was a problem hiding this comment.
Looks like this function can be static.
There was a problem hiding this comment.
I still prefer "From" over "In".
When selecting the "most-visible" overload method to check access, treat package methods as public if they're visible; otherwise, private. This avoids package methods that are perfectly visible being passed over in favor of public (numerically more visible) overloads appearing later in the class.
FeepingCreature
force-pushed
the
fix/Issue_18951
branch
from
9afabdb
to
3c87880
Compare
|
ping? Fail is almost certainly unrelated. |
|
I deprecated that test. It should retest in about 30 minutes or so. |
|
yup, works. do we need another reviewer? |
|
Woo! |
When selecting the "most-visible" overload method to check access,
treat package methods as public if they're visible; otherwise, private.
This avoids package methods that are perfectly visible being passed over
in favor of public (numerically more visible) overloads appearing later
in the class.