Fix #2529: explicit protection package #3651

mihails-strasuns · 2014-06-08T15:34:20Z

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

This does not implement original proposal from linked bugzilla entry but addresses same goal.

Currently there is no way to use package protection attribute with deeply nested package hierarchy, forcing to either use flat one or public protection. This is one of blocking issues for further usage of package.d in Phobos and one of reasons why namespace hacks are so popular.

For example, if helpers in std.internal will be marked as package, only std.internal will be able to access those, but not rest of std. This PR fixes it by allowing package(<pkgname>) syntax to explicitly define owning package.

This new syntax will work:

module std.internal.mod1;
package(std) void foo() {}

module std.mod2;
import std.internal.mod2;
void bar() { foo(); }

Exact semantics can are described by added "protection" tests to test/compilable (last commit in this PR).

Plain package behavior is unchanged and thus no breaking changes introduced.

jacob-carlborg · 2014-06-08T19:01:42Z

Perhaps you could add a couple of failing tests. Like accessing a symbol which you don't have access to. Also pass some invalid stuff to the package keyword.

jacob-carlborg · 2014-06-08T19:02:45Z

Could you also create a pull request which updates the documentation.

mihails-strasuns · 2014-06-08T23:20:01Z

Perhaps you could add a couple of failing tests.

Added tests have failing one too, via !is(typeof(foo())). I wanted to put all relevant tests in one place and thus didn't separate those into "failing" directory. Don't know what is better - to keep locality or actually check the error message.

Also pass some invalid stuff to the package keyword.

Any valid identifier is valid there right now and it is kind of intended. You may define protection for a package not yet encountered. I can't see any reason why it should be prohibited but I may have missed something.

mihails-strasuns · 2014-06-08T23:24:51Z

Could you also create a pull request which updates the documentation.

Updated linked grammar PR to include doc changes too.

jacob-carlborg · 2014-06-09T06:05:57Z

Added tests have failing one too, via !is(typeof(foo())). I wanted to put all relevant tests in one place and thus didn't separate those into "failing" directory. Don't know what is better - to keep locality or actually check the error message.

I thought it would be nice to check the error message as well. I don't know what others think.

Any valid identifier is valid there right now and it is kind of intended. You may define protection for a package not yet encountered. I can't see any reason why it should be prohibited but I may have missed something.

Then add a test that doesn't use a valid identifier 😉, or something that is not an identifier at all. I don't know if I'm too cautious, but I don't think it hurts add.

w0rp · 2014-06-09T07:07:31Z

src/module.c

+ * Returns:
+ *  see description
+ */
+bool Package::isSubpackage(Package* pkg)


This reads funny as a binary relation.

x.isSubpackage(y);

I would read that as "is x a subpackage of y?" It took me a while to figure it out, and I only understood it by reading the document comment in the end. I would rename the function to something like isAncestorPacakgeOf or similar.

That's literally the only thing wrong I found with this diff.

mihails-strasuns · 2014-06-10T11:45:00Z

Will update according to all comments as soon as I will figure out what this DDOC test failure is about :)

mihails-strasuns · 2014-06-11T19:07:55Z

Also link to NG discussion : http://forum.dlang.org/post/cwjqwlkakjuwhfewvlhf@forum.dlang.org
So far no objections against feature itself.

jacob-carlborg · 2014-06-11T19:11:32Z

Why does the error message says: "attribute __anonymous"?

mihails-strasuns · 2014-06-11T19:28:40Z

Because this is how defailt formatting for attribute / dsymbol is defined. And I have no idea how it supposed to be changed (other than overriding all relevant methods) :( Need someone more experienced with dmd to chime in here.

mihails-strasuns · 2014-06-14T12:18:26Z

Ok, kind of of figured out that error message thing, does not look pretty but it works.

All previous review comments should be addressed by now.

mihails-strasuns · 2014-06-14T16:13:54Z

Test failed: Invalid UTF-8 sequence (at index 1)
can't reproduce this locally >_<

andralex · 2014-06-14T21:53:39Z

status?

mihails-strasuns · 2014-06-14T22:06:29Z

trying to guess why auto-tester has considered added samples invalid UTF-8
waiting if @9rnsr has any other objections / comments

mihails-strasuns · 2014-06-15T01:29:50Z

This failure drives me crazy. @DmitryOlshansky maybe you have any ideas? Exception refers to Invalid UTF sequence: e9 but I can't even find such byte in hexdump of source files. And other added protattr cases work just fine X_X

mihails-strasuns · 2014-06-15T01:52:43Z

Nailed it. Exception was referring not to source file but compiler output. Stupid invalid memory access mistake :)

Safety0ff · 2014-06-15T02:12:28Z

Hmm, 0xe9 in a file with 0xe9 bytes.

Looks like its passing on the build hosts it was failing on, glad I could be of help!

mihails-strasuns · 2014-06-15T02:44:47Z

Yay, green.

9rnsr · 2014-06-15T07:22:18Z

src/dsymbol.h

@@ -283,14 +303,14 @@ class ScopeDsymbol : public Dsymbol

 private:
    Dsymbols *imports;          // imported Dsymbol's
-    PROT *prots;                // array of PROT, one for each import
+    PROTKIND *prots;            // array of PROTTYPE, one for each import


s/PROTTYPE/PROTKIND/

9rnsr · 2014-06-15T08:04:03Z

The overall direction looks fine to me.

mihails-strasuns · 2014-06-15T12:02:44Z

That should be all, waiting for an auto-tester.

WalterBright · 2014-08-23T06:29:56Z

src/parse.c

+                        // protection to
+                        if ((pAttrs->protection.kind == PROTpackage) && (token.value == TOKlparen))
+                        {
+                                pkg_prot_idents = parseQualifiedIdentifier("protection package");


whitespace problem

WalterBright · 2014-08-23T06:36:09Z

Meta comment - renaming PROT to Prot made for a lot of extra diffs, making it harder to see what was actually changing. It also likely makes for frequent rebasing of this and other PR's.

mihails-strasuns · 2014-08-23T19:23:38Z

Thanks for comments!

Meta comment - renaming PROT to Prot made for a lot of extra diffs, making it harder to see what was actually changing. It also likely makes for frequent rebasing of this and other PR's.

Keeping old name will only hide the issue, most of conflicts come from the fact that Prot is not a plain enum anymore and this pretty much any new code that works with it results in a failure. If same name is kept it may not result in conflict but will still stop compiling.

Will update to latest master this weekend.

Makes it possible to easily add additional protection information and differentiates between protected imports and actual attribute.

This commit adds capability of parsing and resolving explicit package defined for package protection attribute. Adjusting acess checks to use it will be done in separate commit.

Fix relevant issue by allowing to explicitly define package to bind protection attribute to. Defaults remain unchanged.

Using non-existent or non-parent package as an argument to 'package' protection attribute will result in compile-time error. Test cases with error message output added.

Now includes actual protection attribute kind and package name (if any) into error message. Also stores location information from the parser.

mihails-strasuns · 2014-08-24T16:20:45Z

Rebased, addressed all but one @WalterBright comment (asked clarification for that one)

mihails-strasuns · 2014-08-24T20:56:14Z

Green

Fix #2529: explicit protection package

mihails-strasuns · 2014-08-26T16:44:22Z

Thanks! A most pleasant surprise :)

[Refactoring] around protection code fixed in the PR #3651

MartinNowak · 2015-03-06T20:59:49Z

Needs to go into the changelog.

mihails-strasuns · 2015-03-06T21:24:24Z

Assign me please

MartinNowak · 2015-03-07T18:42:30Z

Assign me please

Github wouldn't let, sorry @Dicebot, but I can ping you for a reminder.

Implemented in dlang/dmd#3651

mihails-strasuns mentioned this pull request Jun 8, 2014

Add package(ident) syntax to grammar dlang/dlang.org#591

Merged

w0rp reviewed Jun 9, 2014
View reviewed changes

9rnsr reviewed Jun 15, 2014
View reviewed changes

WalterBright reviewed Aug 23, 2014
View reviewed changes

Dicebot added 10 commits August 24, 2014 16:45

[refactoring] turn PROT into struct and rename it

d6c770a

Makes it possible to easily add additional protection information and differentiates between protected imports and actual attribute.

Support 'package(ident)' syntax (parser/AST)

c595ba4

This commit adds capability of parsing and resolving explicit package defined for package protection attribute. Adjusting acess checks to use it will be done in separate commit.

Fix #2529: package protection + sub-package

5c4018e

Fix relevant issue by allowing to explicitly define package to bind protection attribute to. Defaults remain unchanged.

Reject non-parent protection packages

8c8f939

Using non-existent or non-parent package as an argument to 'package' protection attribute will result in compile-time error. Test cases with error message output added.

Improve ProtDeclaration error formatting.

2b25d9e

Now includes actual protection attribute kind and package name (if any) into error message. Also stores location information from the parser.

Do not generate code for protection tests

65ff492

Printing of ProtDeclaration is handled by visitor

9432019

Move isAncestorPackageOf docs to definition

50bf169

Simplify isAncestorPackageOf implementations

8ea09ac

Trivial whitespace fix

ab2d968

Dicebot added 3 commits August 24, 2014 20:18

Move package resolution to semantic phase

c72470d

Fix debug printfs in access.c

cc1079a

Consume remaining line tokens on error

9873073

WalterBright added a commit that referenced this pull request Aug 26, 2014

Merge pull request #3651 from Dicebot/package-protection

a41dccc

Fix #2529: explicit protection package

WalterBright merged commit a41dccc into dlang:master Aug 26, 2014

9rnsr added a commit that referenced this pull request Aug 28, 2014

Merge pull request #3906 from 9rnsr/refactor_protection

1f9abd5

[Refactoring] around protection code fixed in the PR #3651

MartinNowak added this to the 2.067 milestone Mar 6, 2015

mihails-strasuns mentioned this pull request Mar 7, 2015

Mention package protection attribute in changelog dlang/dlang.org#922

Merged

John-Colvin pushed a commit to John-Colvin/dlang.org that referenced this pull request Mar 17, 2015

Mention package protection attribute in changelog

3b2818d

Implemented in dlang/dmd#3651

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2529: explicit protection package #3651

Fix #2529: explicit protection package #3651

mihails-strasuns commented Jun 8, 2014

jacob-carlborg commented Jun 8, 2014

jacob-carlborg commented Jun 8, 2014

mihails-strasuns commented Jun 8, 2014

mihails-strasuns commented Jun 8, 2014

jacob-carlborg commented Jun 9, 2014

w0rp Jun 9, 2014

mihails-strasuns commented Jun 10, 2014

mihails-strasuns commented Jun 11, 2014

jacob-carlborg commented Jun 11, 2014

mihails-strasuns commented Jun 11, 2014

mihails-strasuns commented Jun 14, 2014

mihails-strasuns commented Jun 14, 2014

andralex commented Jun 14, 2014

mihails-strasuns commented Jun 14, 2014

mihails-strasuns commented Jun 15, 2014

mihails-strasuns commented Jun 15, 2014

Safety0ff commented Jun 15, 2014

mihails-strasuns commented Jun 15, 2014

9rnsr Jun 15, 2014

9rnsr commented Jun 15, 2014

mihails-strasuns commented Jun 15, 2014

WalterBright Aug 23, 2014

WalterBright commented Aug 23, 2014

mihails-strasuns commented Aug 23, 2014

mihails-strasuns commented Aug 24, 2014

mihails-strasuns commented Aug 24, 2014

mihails-strasuns commented Aug 26, 2014

MartinNowak commented Mar 6, 2015

mihails-strasuns commented Mar 6, 2015

MartinNowak commented Mar 7, 2015

Fix #2529: explicit protection package #3651

Fix #2529: explicit protection package #3651

Conversation

mihails-strasuns commented Jun 8, 2014

jacob-carlborg commented Jun 8, 2014

jacob-carlborg commented Jun 8, 2014

mihails-strasuns commented Jun 8, 2014

mihails-strasuns commented Jun 8, 2014

jacob-carlborg commented Jun 9, 2014

w0rp Jun 9, 2014

Choose a reason for hiding this comment

mihails-strasuns commented Jun 10, 2014

mihails-strasuns commented Jun 11, 2014

jacob-carlborg commented Jun 11, 2014

mihails-strasuns commented Jun 11, 2014

mihails-strasuns commented Jun 14, 2014

mihails-strasuns commented Jun 14, 2014

andralex commented Jun 14, 2014

mihails-strasuns commented Jun 14, 2014

mihails-strasuns commented Jun 15, 2014

mihails-strasuns commented Jun 15, 2014

Safety0ff commented Jun 15, 2014

mihails-strasuns commented Jun 15, 2014

9rnsr Jun 15, 2014

Choose a reason for hiding this comment

9rnsr commented Jun 15, 2014

mihails-strasuns commented Jun 15, 2014

WalterBright Aug 23, 2014

Choose a reason for hiding this comment

WalterBright commented Aug 23, 2014

mihails-strasuns commented Aug 23, 2014

mihails-strasuns commented Aug 24, 2014

mihails-strasuns commented Aug 24, 2014

mihails-strasuns commented Aug 26, 2014

MartinNowak commented Mar 6, 2015

mihails-strasuns commented Mar 6, 2015

MartinNowak commented Mar 7, 2015