Fix 15371: Disable access checks for __traits(getMember) and relatives. by LightBender · Pull Request #9325 · dlang/dmd

LightBender · 2019-02-04T12:31:24Z

Removes access checks for __traits(getMember) and relatives. This should have been done a long time ago. As it stands right now .offsetof is useless because it cannot be used on non-public members and therefore cannot be used to accurately map the type. Indeed there is NO way to accurately map any type that contains private data.

Bug: https://issues.dlang.org/show_bug.cgi?id=15371

dlang-bot · 2019-02-04T12:31:25Z

Thanks for your pull request and interest in making D better, @LightBender! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If 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#9325"

test/compilable/test15371_types.d

jacob-carlborg · 2019-02-04T13:06:19Z

Please use a better commit message and PR title. Something else than jus the link, like the title of the issue.

ghost

Sorry but this bug fix is formally not ok. Also This is is a serious change that requires a DIP, which is all about the new super omnisciencynessment that the change bring.

LightBender · 2019-02-04T22:43:58Z

Umm, what? How does fixing a genuine bug, which is acknowledge as something that should be fixed in the comment sitting above the code require an entire DIP? Your assertion is going to require a very detailed explanation to convince me of it's correctness.

ghost · 2019-02-04T23:06:23Z

My thoughts on trying to fix this issue:

This is a major step forward in term of metaprogramming. You know everything by using __trait(allMembers), __trait(getMember), i.e the "god omniscience", but you decide on your own to apply __trait(getProtection). That's a major change in the language semantics.

ghost · 2019-02-04T23:07:37Z

TBH, this is how i think this issue should be fixed.

LightBender · 2019-02-05T03:37:11Z

How is it a "major change to language semantics"? __traits(allMembers) is at best confusing, it should really be labelled __traits(publicMembers) because it sure doesn't actually mean "All Members". At best this is fixing a bug in the language by making the language do what it says it's going to do. I can guarantee that nobody who uses __traits(allMembers) without carefully reading the documentation and archaic forum posts on the matter is going to just assume that it really means "Public Members Only". They will justifiably assume it means "All Members".

Futhermore, you did not explain WHY you consider this a major change to the language semantics, only repeated your assertion that it is. At this time I cannot accept your argument.

test/compilable/test15371.d

ghost

Sorry for the confusing review comments yesterday @LightBender, if the issue can be solved as simply as that then it's nice.

jacob-carlborg

The change affects more than just getMember, the tests should include a test for each trait it affects
Even though this is fixing a bug, I think a changelog entry should be included. This adds new functionality and therefore should have a changelog entry explaining the new behavior. Have a look in the changelog directory

jacob-carlborg · 2019-02-07T14:53:52Z

test/runnable/test15371.d

+    }
+    auto field = __traits(getMember, test, "privateField");
+    printf("Existing private field value: %d\r\n", field);
+    field = 1;


Doesn't this just update a local variable?

jacob-carlborg · 2019-02-07T14:54:20Z

test/runnable/test15371.d

+TEST_OUTPUT:
+---
+
+---


Shouldn't there be any output?

That was the weird part, I could get the output when I placed an failing assert in the code, but without it, I got nothing.

I think TEST_OUTPUT is usually used for asserting error messages for failing tests. Is there any difference if you print to stderr?

You need to use pragma(msg,...) for TEST_OUTPUT as it only listens to output during compilation.

jacob-carlborg · 2019-02-07T14:55:32Z

test/runnable/test15371.d

+    auto test = new Test15371();
+    const auto hasMember = __traits(hasMember, test, "privateField");
+    if (hasMember) {
+        printf("_traits(hasMember) found privateField\r\n");


I've never seen anyone use \r\n when printing to the console.

Without it is just kept printing on the same line. And only with the failing assert.

I mean, only \n is usually used.

This works correctly on Windows too.

You almost never need \r on Windows, too.

It's an ingrained habit from the bad old days. I'll remove it.

jacob-carlborg · 2019-02-07T14:55:45Z

test/runnable/test15371.d

+
+void main() {
+    auto test = new Test15371();
+    const auto hasMember = __traits(hasMember, test, "privateField");


auto is not required.

Habit from my C# days where any variable decl would be var if it had the type somewhere in the initializer.

jacob-carlborg · 2019-02-07T14:57:15Z

test/runnable/test15371.d

+    auto virtualMethods = __traits(getVirtualMethods, test, "overload").length;
+    printf("Found %d virtual methods for private method 'overload'\r\n", virtualMethods);
+    auto virtualFunctions = __traits(getVirtualFunctions, test, "overload").length;
+    printf("Found %d virtual functions for private method 'overload'\r\n", virtualFunctions);


This looks quite dense, perhaps add some blank lines.

jacob-carlborg · 2019-02-07T14:59:09Z

changelog/private_member_traits.dd

+
+The following traits can now access non-public members:
+$(UL
+    $(LI hasMember)


I think this worked before this PR. Since this is not try to access the symbol. I'm guessing that what's ignoresymbolvisibility is for.

jacob-carlborg · 2019-02-07T14:59:52Z

changelog/private_member_traits.dd

+    $(LI getVirtualFunctions)
+)
+
+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.


I suggest adding backticks (to indicate code) around allMembers.

wilzbach · 2019-02-08T10:12:12Z

test/runnable/test15371.d

@@ -0,0 +1,31 @@
+// EXTRA_SOURCES: imports/test15371.d
+


Runnable tests are very expensive. Please move this to compilable and use things like static assert or pragma(msg).

RazvanN7 · 2019-03-26T12:58:56Z

@LightBender Will you take this to the finish line or should I adopt it?

jacob-carlborg · 2019-03-26T15:41:34Z

@RazvanN7 I recommend you take it so it’s completed as soon as possible.

RazvanN7 · 2019-04-10T12:48:32Z

PR submitted: #9585 . Closing this as @LightBender seems to have abandoned it. We can continue the discussion on the new PR.

Fix 15371: https://issues.dlang.org/show_bug.cgi?id=15371

eb0c4df

jacob-carlborg suggested changes Feb 4, 2019

View reviewed changes

test/compilable/test15371_types.d Outdated Show resolved Hide resolved

ghost suggested changes Feb 4, 2019

View reviewed changes

LightBender changed the title ~~Fix 15371: https://issues.dlang.org/show_bug.cgi?id=15371~~ Fix 15371: Disable access checks for __traits(getMember) and relatives. Feb 4, 2019

thewilsonator reviewed Feb 5, 2019

View reviewed changes

test/compilable/test15371.d Outdated Show resolved Hide resolved

Address PR feedback.

cc9a930

thewilsonator reviewed Feb 5, 2019

View reviewed changes

test/compilable/test15371.d Outdated Show resolved Hide resolved

Address PR feedback.

d7bf143

ghost reviewed Feb 5, 2019

View reviewed changes

test/compilable/test15371.d Outdated Show resolved Hide resolved

ghost approved these changes Feb 5, 2019

View reviewed changes

jacob-carlborg suggested changes Feb 5, 2019

View reviewed changes

LightBender added 2 commits February 6, 2019 23:11

Added test for all affected traits. Added changelog entry.

0b910a3

Moved tests to runnable. Added output.

5626165

jacob-carlborg reviewed Feb 7, 2019

View reviewed changes

wilzbach suggested changes Feb 8, 2019

View reviewed changes

jacob-carlborg mentioned this pull request Feb 27, 2019

Turn deprecation into error for privately imported symbols #9393

Merged

RazvanN7 mentioned this pull request Apr 10, 2019

issue 15371 - some traits should bypass the protection #5289

Closed

RazvanN7 closed this Apr 10, 2019

LightBender deleted the fix_15371 branch August 31, 2023 11:03

Uh oh!

Comments

Conversation

LightBender commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Feb 4, 2019

Bugzilla references

Testing this PR locally

Uh oh!

Uh oh!

jacob-carlborg commented Feb 4, 2019

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

LightBender commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 4, 2019

Uh oh!

ghost commented Feb 4, 2019

Uh oh!

LightBender commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Mar 26, 2019

Uh oh!

jacob-carlborg commented Mar 26, 2019

Uh oh!

RazvanN7 commented Apr 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

LightBender commented Feb 4, 2019 •

edited

Loading

LightBender commented Feb 4, 2019 •

edited

Loading

LightBender commented Feb 5, 2019 •

edited

Loading