DIP22 - private symbol (in)visibility #5472

MartinNowak · 2016-02-23T02:19:10Z

The main goal of DIP22 and this implementation is to resolve conflicts between private and public symbols. Adding a private symbol to a module or a class should not interfere w/ any code in another module.
This is achieved by ignoring private/package/protected symbols during lookup, thus turning protection from an access into a visibility check.

This PR solves the main hurdle to fix 314, b/c enabling protection for selective/renamed import declarations creates many public/private symbol conflicts (e.g. between the public writeln symbol in std.stdio and the private alias import std.stdio : writeln in another module).
The visibility of overloaded symbols with mixed protection is determined by the most visible symbol, but an additional access check is performed after overload resolution.
Because dmd did previously miss some access checks, it was possible to use private symbols.
In order to not break code by making those symbols invisible an additional lookup with a deprecation warning is performed if the previous scope searches didn't found any symbol.
In order to also fix Issue 3254 it was necessary to disable the eager access error during lookup for overloadable symbols. This will for now turn a possibly incorrect error into a deprecation (see test/compilable/protection.d).
This PR fixes package and package(std) protection, but also deprecates invisible symbol for now.

dlang-bot · 2016-02-23T02:19:15Z

Fix	Bugzilla	Description
✓	143	'package' does not work at all
✓	1238	Private identifiers in imported modules create conflicts with public ones
✓	1754	module member visibility does not affect conflicts
✓	2991	[module] Import within aggregate causes conflicts with private identifiers
✓	3254	[module] Module member visibility depends on declaration order

HK47196 · 2016-02-23T03:04:56Z

Slightly off-topic, but does this DIP address internal linkage at all?

IIRC the spec makes statements about private symbols having the same linkage as static in C, but this is not true.

MartinNowak · 2016-02-23T03:12:17Z

Linkage is completely off-topic, and no dmd always emits fully visible symbols, which isn't a problem due to D's mangling. We want to switch to visibility=hidden for non-exported symbols which allows further LTOs.

- to clarify the function of this flag

WalterBright · 2016-03-01T07:58:33Z

This seems to share a lot of the commits for #5485 which makes it even more worthwhile to have each commit be a separate PR.

MartinNowak · 2016-03-01T11:59:31Z

This seems to share a lot of the commits for #5485 which makes it even more worthwhile to have each commit be a separate PR.

#5485 (comment) is based on this PR, so github also shows the commits of this PR in the other PR.

MartinNowak · 2016-03-01T12:07:18Z

For example, the first commit is a simple refactor, there's no need to lump it in with the rest.

It's already hard enough to get anything reviewed. Why should I split off a trivial constant renaming to a separate PR, paying all the transaction costs again, and creating additional dependency issues for downstream maintainers or releases.
It's not like this is a big unrelated refactoring, you can click on the commit MartinNowak@fd44e00, review it on it's own, and even put an OK comment on it if you can't review the complete PR in one go.

- replace access checks by symbol invisibility - no longer find private/package protected symbols - resolves private vs. public symbol collisions in imported modules - resolve mixed private/public overloads

- what symbols are visible depends on each module in an import chain so that anyone importing a module w/ a public import cannot see more than the module itself

MartinNowak · 2016-03-01T12:10:04Z

Github isn't the best tool for reviews (e.g. it show commits in the wrong order).
Those are the 2 core commits of this PR.
MartinNowak@4bfbd43
MartinNowak@1a25b72

WalterBright · 2016-03-02T00:03:29Z

Why should I split off a trivial constant renaming to a separate PR, paying all the transaction costs again, and creating additional dependency issues for downstream maintainers or releases.

As I explained to Kenji:

trivial PRs are going to be reviewed a lot quicker, because they are much, much, much easier to review
encapsulated PRs make just as much sense as encapsulated code does. Having tangled code is as hard as tangled PRs
We have lots and lots of PRs that result in regressions. If the regression is caused by a smaller PR, it is a LOT easier to figure out what went wrong than a large PR
Having lots of stuff thrown into one PR means it's all or nothing. With a sequence of smaller PRs, one can reject one without rejecting all the rest of otherwise good stuff.
I know that some PRs will depend on previous PRs. I don't see that this is a big deal, just throw them up, and rebase as necessary.
Bug fixes should be focused, one per PR, and not have refactorings and other kitchen appliances thrown in.
Once a PR spans more than one screen, it gets a lot harder to review. Have you ever noticed that the more lines a PR has, the more the reviews consist of complaints about formatting and other trivia, instead of meat? I sure have, and I'm guilty of it as well.

Those are the 2 core commits of this PR.

Please put the non-core ones as separate PRs.

MartinNowak · 2016-03-02T08:43:08Z

Let's please stop wasting time and get to the matter. This PR isn't trivial and it can't be, but it's already simpler than your lookup change and doesn't contain any unrelated changes.
The renaming of IgnorePrivateMember to IgnorePrivateImports (MartinNowak@fd44e00) is part of this, b/c this PR changes search to ignore private symbols which collides with the name of the flag.
The other commits just add test cases of relevant bugs.

MartinNowak · 2016-03-02T09:06:52Z

Once a PR spans more than one screen, it gets a lot harder to review. Have you ever noticed that the more lines a PR has, the more the reviews consist of complaints about formatting and other trivia, instead of meat? I sure have, and I'm guilty of it as well.

Well, that's partly b/c github notifications are mostly gone once you click on them, so confronted with complex PRs and a lack of time, people use small comments as a TODO reminder. And if someone can't grasp a PR right away she should get into contact with the author, e.g. by leaving questions or by scheduling a Skype or IRC meeting if lots of discussion is necessary.
All of these are workflow issues and in any case it's a bad idea to disguise a complex change like this one behind many artificial and seemingly trivial changes.
This is also why Trello is a useful tool for us, b/c I can easily see what Kenji works on and can plan my time for review. https://trello.com/c/ihKhGeiu/115-reg2-031-forward-reference-to-inferred-return-type-of-function-call-when-using-auto-return-type

MartinNowak · 2016-03-02T09:10:49Z

This PR changes +118/-23 lines of non-test code. All it does is adding a new symbolIsVisible(Module, Symbol) function, uses that to ignore symbols during lookup, and performs another deprecated lookup when nothing was found.

andralex · 2016-03-03T03:11:07Z

The way I see it is, the project lead is asking for some reasonable changes. Sure, packaging PRs is not an exact science, everybody has their quirks etc, so each and every thing asked can be debated. But at the top level: the project lead is asking for some reasonable changes. @MartinNowak, do you think there are ways to implement them so he can review and pull this PR?

MartinNowak · 2016-03-03T11:43:52Z

The way I see it is, the project lead is asking for some reasonable changes.

If you're referring to #5472 (comment), this was a misunderstanding, #5485 is completely based on the PR, so of course both share the same commits. There isn't anything in this PR that isn't part of the DIP22 change.
I said in #5472 (comment) why I had to rename a search flag before adding another flag and changing the search semantics.

WalterBright · 2016-03-03T12:05:45Z

src/dscope.d

+         * checked by the compiler remain usable.  Once the deprecation is over,
+         * this should be moved to search_correct instead.
+         */
+        s = searchScopes(flags | SearchLocalsOnly | IgnoreSymbolVisibility);


Put check for IgnorErrors and useDeprecated==1 here to avoid unnecessary extra symbol lookups.

This code may also need to be in expression.searchUFCS().

Put check for IgnorErrors and useDeprecated==1 here to avoid unnecessary extra symbol lookups.

I'd still have to return the correct symbol, even if not reporting the deprecation warning.
I can try to gather some stats how often symbol lookup falls through to this old behavior.

This code may also need to be in expression.searchUFCS().

Seems so, will add that and a test case this evening.

MartinNowak · 2016-03-03T13:04:18Z

I'll run some benchmarks, but I don't think this affects anything b/c failed lookups are usually an error and in any case rare.
Comparing 2 test-runs already suggests that the timing didn't change.
https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=1976810&isPull=true
https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=1976778&isPull=true

- for better error messages and to still allow any usage that might slip access check after overload resolution - the better error message should be produced by search_correct once the deprecation is over

- also do a deprecated search for private symbols in the new part of checkimports - keep old behavior (ignoring symbol visibility) w/ -transition=import and the old part of -transition=checkimports

MartinNowak · 2016-03-04T18:08:18Z

I added 3 commits as you can see above.

Add deprecated search for invisible symbols to searchUFCS. This is mostly improve error messages b/c most UFCS symbols should have access checks.
Still it's a lot more helpful to get Deprecation: imports.dip22a.bar is not visible from module dip22a instead of Error: bar is not a property of .... Once the visibility deprecation is over we should add the fallback search for private symbols to the spell checker (search_correct).
Integrate DIP22 with -transition=import and -transition=checkimports
Describe all the changes in the changelog

MartinNowak · 2016-03-04T18:23:22Z

Slowdown is very small 0.1-0.2% for phobos measured without object creation (-o-).

WalterBright · 2016-03-05T06:06:01Z

Slowdown is very small 0.1-0.2% for phobos measured without object creation

without object creation

Not sure what that means.

Slow compile times are an accumulation of those "very small" .1-.2 slowdowns, like barnacles on a ship.

WalterBright · 2016-03-05T06:11:25Z

Due to these slowdowns, I propose a shorter deprecation cycle for this.

MartinNowak · 2016-03-05T11:48:14Z

Due to these slowdowns, I propose a shorter deprecation cycle for this.

A few releases (2-3 / 4-6 month) should be fine, it's quite a mess to maintain these extra flags and lookups.

Not sure what that means.

It mean I skipped the codegen pass by using -o-.

Slow compile times are an accumulation of those "very small" .1-.2 slowdowns, like barnacles on a ship.

Well, 0.1% change in semantic speed shouldn't be an argument against such a big change.
We have quite a few bigger performance issues lingering around, e.g. the 10-15% slist_reset for static libraries (partly fixed now), ~4% in addDeferredSemantic3, or the pervasive use of lists instead of arrays.

Anything left to do?

WalterBright · 2016-03-05T12:25:22Z

I know we have other time suckers, like the excessive template instantiations. We need to work on all of them, and try not to make things worse.

WalterBright · 2016-03-05T12:25:45Z

Auto-merge toggled on

MartinNowak · 2016-03-05T13:00:31Z

We need to work on all of them

Yes, we need to work on all of them, but they are all fixable problems.
Just figured that the addDeferred is very low hanging fruit, so I fixed it #5506.

DIP22 - private symbol (in)visibility

CyberShadow · 2016-03-10T23:19:55Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=15785

MartinNowak force-pushed the dip22 branch from 8130ea2 to cbd157c Compare February 26, 2016 21:03

MartinNowak mentioned this pull request Feb 28, 2016

fix Issue 314 - static, renamed, and selective imports are always public #5485

Merged

rename IgnorePrivateMembers -> IgnorePrivateImports

fd44e00

- to clarify the function of this flag

MartinNowak force-pushed the dip22 branch from cbd157c to f55e109 Compare February 29, 2016 15:03

MartinNowak mentioned this pull request Feb 29, 2016

add -transition=checkimports switch #5476

Merged

MartinNowak and others added 9 commits March 1, 2016 13:09

implement DIP22 - private symbol invisibility

4bfbd43

- replace access checks by symbol invisibility - no longer find private/package protected symbols - resolves private vs. public symbol collisions in imported modules - resolve mixed private/public overloads

visibility checks for dot identifier lookups

1a25b72

fix Issue 143

431dd20

fix issue 1238

8839a6c

fix issue 1754

054db4f

fix issue 2991

2c371ce

add test case for invisible private symbols in base class

7aafd3a

test that public imports filter invisible symbols

b341254

- what symbols are visible depends on each module in an import chain so that anyone importing a module w/ a public import cannot see more than the module itself

fix Issue 3254 - Module member visibility depends on declaration order

e527108

MartinNowak force-pushed the dip22 branch from f55e109 to e527108 Compare March 1, 2016 12:14

WalterBright reviewed Mar 3, 2016
View reviewed changes

MartinNowak added 3 commits March 4, 2016 18:41

also do a deprecated search for private symbols in searchUFCS

b2ec7f9

- for better error messages and to still allow any usage that might slip access check after overload resolution - the better error message should be produced by search_correct once the deprecation is over

handle -transition=import/-transition=checkimports

5608837

- also do a deprecated search for private symbols in the new part of checkimports - keep old behavior (ignoring symbol visibility) w/ -transition=import and the old part of -transition=checkimports

changelog entry for DIP22

93aebd1

MartinNowak force-pushed the dip22 branch from 7b7c998 to 93aebd1 Compare March 4, 2016 18:09

WalterBright added a commit that referenced this pull request Mar 5, 2016

Merge pull request #5472 from MartinNowak/dip22

618acc4

DIP22 - private symbol (in)visibility

WalterBright merged commit 618acc4 into dlang:master Mar 5, 2016

MartinNowak deleted the dip22 branch March 6, 2016 14:48

MartinNowak mentioned this pull request Mar 15, 2016

fix Issue 15785 - spurious warning when calling protected super #5530

Merged

Geod24 mentioned this pull request May 7, 2016

Revert "Merge pull request #5727 from MartinNowak/fix15897" #5736

Closed

MartinNowak mentioned this pull request Aug 8, 2016

fix Issue 16085 - wrong visibility warning for overloaded alias symbol #5847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DIP22 - private symbol (in)visibility #5472

DIP22 - private symbol (in)visibility #5472

MartinNowak commented Feb 23, 2016

dlang-bot commented Feb 23, 2016

HK47196 commented Feb 23, 2016

MartinNowak commented Feb 23, 2016

WalterBright commented Mar 1, 2016

MartinNowak commented Mar 1, 2016

MartinNowak commented Mar 1, 2016

MartinNowak commented Mar 1, 2016

WalterBright commented Mar 2, 2016

MartinNowak commented Mar 2, 2016

MartinNowak commented Mar 2, 2016

MartinNowak commented Mar 2, 2016

andralex commented Mar 3, 2016

MartinNowak commented Mar 3, 2016

WalterBright Mar 3, 2016

WalterBright Mar 3, 2016

MartinNowak Mar 3, 2016

MartinNowak Mar 3, 2016

MartinNowak commented Mar 3, 2016

MartinNowak commented Mar 4, 2016

MartinNowak commented Mar 4, 2016

WalterBright commented Mar 5, 2016

WalterBright commented Mar 5, 2016

MartinNowak commented Mar 5, 2016

WalterBright commented Mar 5, 2016

WalterBright commented Mar 5, 2016

MartinNowak commented Mar 5, 2016

CyberShadow commented Mar 10, 2016

DIP22 - private symbol (in)visibility #5472

DIP22 - private symbol (in)visibility #5472

Conversation

MartinNowak commented Feb 23, 2016

dlang-bot commented Feb 23, 2016

HK47196 commented Feb 23, 2016

MartinNowak commented Feb 23, 2016

WalterBright commented Mar 1, 2016

MartinNowak commented Mar 1, 2016

MartinNowak commented Mar 1, 2016

MartinNowak commented Mar 1, 2016

WalterBright commented Mar 2, 2016

MartinNowak commented Mar 2, 2016

MartinNowak commented Mar 2, 2016

MartinNowak commented Mar 2, 2016

andralex commented Mar 3, 2016

MartinNowak commented Mar 3, 2016

WalterBright Mar 3, 2016

Choose a reason for hiding this comment

WalterBright Mar 3, 2016

Choose a reason for hiding this comment

MartinNowak Mar 3, 2016

Choose a reason for hiding this comment

MartinNowak Mar 3, 2016

Choose a reason for hiding this comment

MartinNowak commented Mar 3, 2016

MartinNowak commented Mar 4, 2016

MartinNowak commented Mar 4, 2016

WalterBright commented Mar 5, 2016

WalterBright commented Mar 5, 2016

MartinNowak commented Mar 5, 2016

WalterBright commented Mar 5, 2016

WalterBright commented Mar 5, 2016

MartinNowak commented Mar 5, 2016

CyberShadow commented Mar 10, 2016