Remove visiblity and lookup deprecation by MartinNowak · Pull Request #7241 · dlang/dmd

MartinNowak · 2017-10-24T16:42:18Z

No description provided.

dlang-bot · 2017-10-24T16:42:19Z

Thanks for your pull request, @MartinNowak!

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.

mathias-lang-sociomantic · 2017-10-24T17:21:24Z

If memory serves me well, that should also automatically fix some of the issues we've seen in the past, e.g. public alias to private functions ? If so I have a test case for you. Unless you want to leave this for a later P.R. (but I fear it might get forgotten) ?

MartinNowak · 2017-10-24T17:44:19Z

If memory serves me well, that should also automatically fix some of the issues we've seen in the past, e.g. public alias to private functions ? If so I have a test case for you. Unless you want to leave this for a later P.R. (but I fear it might get forgotten) ?

That at least needs to wait until we also remove the access check which is run after symbol resolution.

RazvanN7 · 2017-10-25T10:52:47Z

You might want to check this out [1]

[1] https://issues.dlang.org/show_bug.cgi?id=17824

andralex

cc @WalterBright

ibuclaw

Fine by me

andralex · 2017-11-07T15:27:01Z

@MartinNowak status?

WalterBright · 2017-11-09T06:19:08Z

src/ddmd/dscope.d

            return cast(void*)s;
        }

+        Dsymbol scopesym = null;


Test case for this added code?

WalterBright · 2017-11-09T06:19:35Z

src/ddmd/dsymbol.d

            return null; // don't do it for speculative compiles; too time consuming
+        // search for exact name ignoring visibility first
+        if (auto s = search(Loc(), ident, IgnoreErrors | IgnoreSymbolVisibility))
+            return s;


test case for added code?

ibuclaw · 2018-01-05T20:09:57Z

This needs moving forward. @wilzbach I'd put this as something for the next release schedule.

ibuclaw · 2018-01-05T20:10:37Z

@MartinNowak - please rebase and add coverage tests.

wilzbach · 2018-01-08T14:53:40Z

(rebased)

wilzbach · 2018-01-23T00:51:11Z

src/dmd/mtype.d

-
-        s = searchSym();
+        int flags = sc.flags & SCOPE.ignoresymbolvisibility ? IgnoreSymbolVisibility : 0;
+        s = sym.search(e.loc, ident, flags | SearchLocalsOnly | IgnorePrivateImports);


I wasn't sure about this line as it was very recently changed (IgnorePrivateImports was added in #7668)

andralex · 2018-01-23T17:37:08Z

@MartinNowak status of this?

wilzbach · 2018-01-25T00:29:08Z

@MartinNowak status of this?

This is still needed:

The following packages remain in the "needs to be updated list"

Fix imports CyberShadow/ae#33 (there's a weird error with the deprecation only showing up with -de)
Fix import leak vibe-d/vibe.d#2044
Bump dependencies ddox#194

PR needs to be adapted a bit after @RazvanN7's import PRs

andralex · 2018-02-01T19:09:28Z

@jacob-carlborg @JinShil:

Is this already implemented? If not, shouldn't that be implemented first before merging this?

FYI: There's some history about that in #6078 and #5289 I'm getting confused about what the general philosophy is with regard to reflection and protection.

Sorry for the late answer. The overall philosophy (obviously not quite cast in stone yet) is that introspection should be able to bypass protection for object layout. Possibly essential tasks such as construction and destruction.

On the other hand, private module-level functions are entirely the business of the respective module and it would be unreasonable to export them just to make them accessible for introspection from another module. Same goes about private member functions.

All in all, a topic worth a closer look.

jacob-carlborg · 2018-02-01T20:30:34Z

@andralex Where are we with __traits(allMembers), is that still a problem with private functions or are those not part of the result anymore?

andralex · 2018-02-01T23:06:55Z

@jacob-carlborg I think @MartinNowak worked on this recently.

MartinNowak · 2018-02-17T21:16:30Z

@andralex Where are we with __traits(allMembers), is that still a problem with private functions or are those not part of the result anymore?

At the same place as before, allMembers and getMember can see private symbols, and we exempted getMember from the soon to be removed access checks. In any case that's not part of this PR.

MartinNowak · 2018-02-17T21:26:13Z

On the other hand, private module-level functions are entirely the business of the respective module and it would be unreasonable to export them just to make them accessible for introspection from another module. Same goes about private member functions.

Not sure about your terminology, to clarify this.
We figured that allowing to access private symbols (of other modules, but in the same DSO) has negligible performance implications for compilers, so we disabled visibility checks for those traits (

dmd/src/dmd/traits.d

Line 875 in 59b705c

    
           // ignore symbol visibility for these traits, should disable access checks as well

).
There are still the old access checks that have been here since ever. Those will be removed once the deprecation phase for visibility ended (this PR).

It's weird that a thread with a consensus (https://forum.dlang.org/post/nq512d$9po$1@digitalmars.com) still leaves so much confusion and uncertainty.

MartinNowak · 2018-02-17T21:53:52Z

FYI: There's some history about that in #6078 and #5289 I'm getting confused about what the general philosophy is with regard to reflection and protection.

There is an old mechanism (access checks) and a new one (visibility checks). Once visibility is fully deprecated (with this PR), we'll remove the access checks.
Indeed after some discussion we decided to ignore visibility for allMembers and getMember. The old access check has (and still does) error on access private fields via getMember though. As a limited workaround, one can use .tupleof. Reflection on private fields should fully work as reflection does not access a symbol (it only looks at it).

wilzbach · 2018-07-29T17:02:22Z

 CyberShadow/ae#33 (there's a weird error with the deprecation only showing up with -de)

FYI: this has been reduced by Mike to https://issues.dlang.org/show_bug.cgi?id=19107 and been proposed as #8519

wilzbach · 2019-04-25T10:23:38Z

Superseded by #9636

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Oct 24, 2017

MartinNowak force-pushed the rm_import_transition branch from 7c674ae to a9906de Compare October 24, 2017 17:07

MartinNowak force-pushed the rm_import_transition branch from a9906de to 56368b8 Compare October 24, 2017 17:25

andralex approved these changes Oct 26, 2017

View reviewed changes

MartinNowak mentioned this pull request Oct 27, 2017

Eliminate static shared this from std/parallelism.d dlang/phobos#5470

Merged

MartinNowak force-pushed the rm_import_transition branch from 56368b8 to ba7f15c Compare October 28, 2017 16:13

ibuclaw approved these changes Nov 5, 2017

View reviewed changes

WalterBright reviewed Nov 9, 2017

View reviewed changes

src/ddmd/dscope.d

return cast(void*)s;

}

Dsymbol scopesym = null;

Copy link

Member

WalterBright Nov 9, 2017

Choose a reason for hiding this comment

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

Test case for this added code?

WalterBright reviewed Nov 9, 2017

View reviewed changes

dlang-bot added Review:Needs Rebase Review:Needs Work labels Jan 1, 2018

ibuclaw self-assigned this Jan 5, 2018

wilzbach added this to the 2.079.0 milestone Jan 5, 2018

wilzbach force-pushed the rm_import_transition branch from ba7f15c to cdc0f4f Compare January 8, 2018 14:53

dlang-bot removed Review:Needs Work Review:Needs Rebase labels Jan 8, 2018

dlang-bot added Review:Needs Work Review:Needs Rebase labels Jan 8, 2018

wilzbach force-pushed the rm_import_transition branch from cdc0f4f to 9f6aa3c Compare January 18, 2018 00:30

MartinNowak requested review from dnadlinger, mathias-lang-sociomantic and rainers as code owners January 18, 2018 00:30

wilzbach force-pushed the rm_import_transition branch from 3afe88b to a99e767 Compare January 23, 2018 00:10

wilzbach reviewed Jan 23, 2018

View reviewed changes

wilzbach force-pushed the rm_import_transition branch from a99e767 to 9aedb20 Compare January 25, 2018 00:24

This was referenced Jan 25, 2018

Fix import leak vibe-d/vibe.d#2044

Merged

Bump dependencies dlang/ddox#194

Merged

MartinNowak and others added 5 commits January 25, 2018 11:15

remove visiblity and lookup deprecation

e865e05

suggest non-visible symbols in search_correct

ac66837

Mark -transition=import as deprecated and trigger deprecation options

2feca68

Update tests

4ec085c

Remove -transition=imports flag from its test

245460b

wilzbach force-pushed the rm_import_transition branch from 9aedb20 to 245460b Compare January 25, 2018 10:15

dlang-bot added Review:Needs Rebase Review:Needs Work labels Feb 1, 2018

wilzbach removed this from the 2.079.0 milestone Apr 6, 2018

RazvanN7 mentioned this pull request Jul 3, 2018

Turn deprecation into error for issue 313 #8443

Merged

wilzbach mentioned this pull request Jul 29, 2018

Fix Issue 19107 - -de produces compilation error, -dw does not #8519

Merged

This was referenced Dec 10, 2018

Fix Issue 19471 - Duplicate error messages when trying to use an inaccessible package symbol #9056

Closed

Remove visiblity and lookup deprecation - Take 2 #9058

Closed

JinShil mentioned this pull request Apr 17, 2019

Remove visiblity and lookup deprecation (Take 2) #9636

Merged

wilzbach closed this Apr 25, 2019

Uh oh!

Comments

Conversation

MartinNowak commented Oct 24, 2017

Uh oh!

dlang-bot commented Oct 24, 2017

Bugzilla references

Uh oh!

mathias-lang-sociomantic commented Oct 24, 2017

Uh oh!

MartinNowak commented Oct 24, 2017

Uh oh!

RazvanN7 commented Oct 25, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

ibuclaw left a comment

Choose a reason for hiding this comment

Uh oh!

andralex commented Nov 7, 2017

Uh oh!

WalterBright Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

WalterBright Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Jan 5, 2018

Uh oh!

ibuclaw commented Jan 5, 2018

Uh oh!

wilzbach commented Jan 8, 2018

Uh oh!

wilzbach Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

andralex commented Jan 23, 2018

Uh oh!

wilzbach commented Jan 25, 2018 • edited by thewilsonator Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Feb 1, 2018

Uh oh!

jacob-carlborg commented Feb 1, 2018

Uh oh!

andralex commented Feb 1, 2018

Uh oh!

MartinNowak commented Feb 17, 2018

Uh oh!

MartinNowak commented Feb 17, 2018

Uh oh!

MartinNowak commented Feb 17, 2018

Uh oh!

wilzbach commented Jul 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Apr 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

wilzbach commented Jan 25, 2018 •

edited by thewilsonator

Loading

wilzbach commented Jul 29, 2018 •

edited

Loading