Fix Issue 15371 - __traits(getMember) should bypass the protection #9585
Conversation
|
Thanks for your pull request and interest in making D better, @RazvanN7! 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#9585" |
|
interesting this would make things easier to use |
| Scope* scx = sc.push(); | ||
| scx.flags |= SCOPE.ignoresymbolvisibility; | ||
| scx.flags |= SCOPE.ignoresymbolvisibility | SCOPE.noaccesscheck; |
There was a problem hiding this comment.
Doesn’t this apply to more traits than those mentioned above?
There was a problem hiding this comment.
Yes, but it does not affect them; more specifically: from a total of 5 affected traits we have hasMember, that was already working and SCOPE.noaccesscheck does not affect it and getVirtualMethod/Functions for which it does not apply, because private members are never virtual. The other 2 are specified.
There was a problem hiding this comment.
What about protected, did that already work.
|
Ok, this causes phobos to break; silent change of code behavior. Ideas? I don't see other way then to deprecate this trait and implement a new one :-S |
|
On Wed, Apr 10, 2019 at 01:15:42PM +0000, Razvan Nitu wrote:
Ok, this causes phobos to break; silent change of code behavior. Ideas? I don't see other way then to deprecate this trait and implement a new one :-S
That Phobos test is broken and should be removed - it specifically is
testing if getMember failed on a private member!
|
|
@adamdruppe yeah, I think that commenting it and then fixing it after this is merged is the way to go |
| $(LI getOverloads) | ||
| ) | ||
|
|
||
| This fixes a long-standing issue in D where the allMembers trait would correctly return non-public members but those non-public members would be inaccessible to other traits. |
There was a problem hiding this comment.
The problem I and other encountered is also tied to the fact that a library trait located in one module could not work at a at the use site because it was instantiated elsewhere. The same library trait, copied (or mixed in) at the use site worked just fine.
There was a problem hiding this comment.
I forgot to say but somewhere it should be documented that from now it's a good practice to check the protection and to apply it unless "you know what you do".
|
This GTG? |
test/compilable/test15371.d
Outdated
| void main() | ||
| { | ||
| A a; | ||
| static assert(__traits(compiles, __traits(hasMember, A, "a"))); |
There was a problem hiding this comment.
Why is this wrapped in a __traits(compiles? We can drop that.
There was a problem hiding this comment.
I simply copy pasted the test. Yes we can, done.
There was a problem hiding this comment.
Thanks! __traits(compiles is a bit of an anti-pattern in compilation as you don't get the real error message if you change/break DMD and the test fails.
As far as I am concerned, yes. |
|
well, this is exciting!
|
|
I should mention that we should remove the old access mechanism altogether. |
No description provided.