Bug 314 – [module] Static, renamed, and selective imports are always public

In file a.d: -- // explicit privates unnecessary but added for clarity version(stat) private static import std.stdio; version(rena) private import io = std.stdio; version(sele) private import std.stdio : writefln; -- In file b.d: -- import a; void main() { version(stat) std.stdio.writefln("This should not work."); version(rena) io.writefln("This should not work."); version(sele) writefln("This should not work."); } -- Compiled with version=stat, version=rena, or version=sele, the program outputs "This should not work.", when it shouldn't even compile. Note that the version(stat) case is dependant on Issue 313.

errr, d 1.0 still has this bug.. but bug 313's example doesn't work any more.

*** Bug 604 has been marked as a duplicate of this bug. ***

Created attachment 246 Fix by checking protection attribute for all nonlocal symbols The patch has been tested rudimentarily on llvmdc. I have commented the two crucial changes. Even if there is a good reason for not checking the protection attribute of all symbols, it should still be simple to special case this fix for import declarations and the alias declarations generated by them. Note that module c; int var; -- module b; import c; -- module a; import b; void main { var = 1; // var: undefined identifier (no change) c.var = 1; // c: undefined identifier (instead of ok) b.c.var = 1; // still ok }

(In reply to comment #3) > Created an attachment > Fix by checking protection attribute for all nonlocal symbols I aimed a little high; this patch does not add checks for the protection attribute for all nonlocal symbols. It adds checks only for the ones contained in ScopeDsymbol and only in a coarse (public or private) way. But doing that consistently is a different bug/enhancement anyway. Using a variation of this patch to fix the import issue bugs 313 and 314 describe should still be fine.

*** Bug 2330 has been marked as a duplicate of this bug. ***

Created attachment 364 patch Ignores nonlocal private symbols when looking up and identifier. Sets the protection attribute of imports and generated aliases to the protection level specified for the import.

Access protection is defined to happen after lookup and overload resolution. Making private symbols invisible defeats this, and also doesn't work as the overload list is a linked list, and making the head of it private would hide the rest of the possibly public overloads.

Thanks for the thorough reply. I disagree that overload resolution happens strictly before access checks. Consider: a.d -- void foo(int) { printf("foo-i"); } b.d -- import a : foo; void foo(float) { printf("foo-f"); } c.d -- import b; void main() { int i; foo(i); } Like this, with dmd 1.043, you get "foo-i". If you remove the selective import of foo, you get "foo-f". Private imports are invisible at lookup-time, but renamed or selective private imports aren't. Therefore it seems to me that making these invisible is the right thing to do. The patch does have a problem with overloads though and that ought to be fixed. Would you accept a patch that works along the same lines but handles overloads correctly?

I've updated the patch. Treating overloads correctly complicated the issue quite a bit. What I've done is to store the import protection in the AliasDeclarations and FuncAliasDeclarations generated by selective and renamed imports. These are then ignored when traversing the overload tree if they are in a different module than the one initiating the traversal. That means, however, that overload resolution needs to know which module is triggering it and has led to a lot of module passing. :/ I've also made the hiding of private symbols in ScopeDsymbol::search specific to AliasDeclarations generated by ImportStatement. Making the protection attributes apply consistently to more than Func- and VarDecls is a separate issue. The LDC changesets are: http://www.dsource.org/projects/ldc/changeset/1358 http://www.dsource.org/projects/ldc/changeset/1362 I can make a patch against DMD if requested.

(In reply to comment #7) > Access protection is defined to happen after lookup and overload resolution. Defined where in the spec? > Making private symbols invisible defeats this, Would it be reasonable to change it as I described in the final paragraph of issue 3254 comment 3? > and also doesn't work as the > overload list is a linked list, and making the head of it private would hide > the rest of the possibly public overloads. That's obviously an implementation issue. Possible ways to deal with this: (a) have multiple linked lists, one for each protection attribute (b) build the linked list in such a way that the head element will always be one of the ones of the most public access level that exists among the overloads

*** Issue 3275 has been marked as a duplicate of this issue. ***

Whenever I compile some code in ldc, that has been developed with dmd, I get compilation errors related to this bug. That's because ldc (at least partially) fixed it. As far as I can tell, ldc never rejected actually valid code related to this bug. It's a bit ridiculous. What keeps back the patches going into dmd?

stubbornness ?

when will this be fixed?

In D4, maybe =P Don't be impatient, is just number 1 in votes!

Well that's the point. - 32 votes! - present since v0.165! - patch is available that seems to work for ldc

I was being sarcastic =)

I have tried this patch on the latest D2. I've found two problems with it: (1) object needs special treatment, it must not default to private. (2) It completely fails for selective imports. The first issue is trivial to fix; the main patch in import.c becomes: void Import::importAll(Scope *sc) { if (!mod) { load(sc); mod->importAll(0); + /* Default to private importing, except for object. + */ + if (id != Id::object) { + protection = sc->protection; + if (!sc->explicitProtection) + protection = PROTprivate; + } OTOH applying the patch has shown up several bugs in druntime and in the compiler test suite.

Don, which version of the patch did you apply - the one attached here or the one I applied to LDC? Selective imports work correctly in LDC, so maybe there's some extra work needed for D2. I also expect the patch to require some work with regard to overload resolution, it works differently in D2.

(In reply to comment #19) > Don, which version of the patch did you apply - the one attached here or the > one I applied to LDC? Selective imports work correctly in LDC, so maybe there's > some extra work needed for D2. I also expect the patch to require some work > with regard to overload resolution, it works differently in D2. The one attached here. Although I've found some problems with selective imports, I no longer think they are the fault of this patch. For example, the existing release of D1 doesn't like this example: --- import std.stdio : writefln; void main() { std.stdio.writefln("xyz"); } test0.d(338): Error: undefined identifier std Error: no property 'writefln' for type 'TOK149' test0.d(338): Error: function expected before (), not __error of type TOK149 --- After applying the patch and my change to Id::object, and fixing a bug in each of druntime, Phobos, and the test suite, all Phobos unittests pass, and the DMD test suite passes all tests. Looks great to me.

In my understanding, "import std.stdio : writefln;" only imports the name "writefln", not "std" and "writefln". If the user wants "std", he has to write "static import std.stdio;". I would assume your example is invalid and is expected to fail. Why would the user do "import std.stdio : writefln;" if he doesn't use writefln directly? Is "std.stdio.writefln" the only name he's supposed to be able to use here? If yes, what the hell is the use of that?

(In reply to comment #21) > In my understanding, "import std.stdio : writefln;" only imports the name > "writefln", not "std" and "writefln". If the user wants "std", he has to write > "static import std.stdio;". I would assume your example is invalid and is > expected to fail. > > Why would the user do "import std.stdio : writefln;" if he doesn't use writefln > directly? Is "std.stdio.writefln" the only name he's supposed to be able to use > here? If yes, what the hell is the use of that? It's invalid code. But you should never see TOKxxx in an error message. It indicates something is fouled up.

(In reply to comment #22) > (In reply to comment #21) > > In my understanding, "import std.stdio : writefln;" only imports the name > > "writefln", not "std" and "writefln". If the user wants "std", he has to write > > "static import std.stdio;". I would assume your example is invalid and is > > expected to fail. > > > > Why would the user do "import std.stdio : writefln;" if he doesn't use writefln > > directly? Is "std.stdio.writefln" the only name he's supposed to be able to use > > here? If yes, what the hell is the use of that? > > It's invalid code. But you should never see TOKxxx in an error message. It > indicates something is fouled up. One more for bug 4329 =)

(In reply to comment #20) > (In reply to comment #19) > > Don, which version of the patch did you apply - the one attached here or the > > one I applied to LDC? > > The one attached here. Well, ass Walter pointed out the attached patch has problems with overload resolution. The corrected patch doesn't though. If you're interested in looking at it, I could make it work against the D2 frontend and post it here. I don't want the effort to be in vain though, so could you check with Walter whether he'd accept a patch that works as described in comment #9?

Gah, can you edit comments somehow? That typo is extremely embarrassing.

Comment on attachment 364 patch Marking this patch as obsolete, since it is not correct.

(In reply to comment #24) > (In reply to comment #20) > > (In reply to comment #19) > > > Don, which version of the patch did you apply - the one attached here or the > > > one I applied to LDC? > > > > The one attached here. > > Well, as Walter pointed out the attached patch has problems with overload > resolution. The corrected patch doesn't though. If you're interested in looking > at it, I could make it work against the D2 frontend and post it here. > > I don't want the effort to be in vain though, so could you check with Walter > whether he'd accept a patch that works as described in comment #9? From discussion with Walter -- It's too difficult to evaluate the patch in its present form. It's in two parts, both diffed against the LDC codebase rather than DMD, and the context is really unclear -- it's not clear which functions are being patched. I don't think a complete patch is required for evaluation -- in fact, a complete patch would be more difficult to quickly understand. But if you can write the essence of the code here, which I think is really only a couple of functions, that should be enough. And with a explanation of what it's doing. Leave out the myriad of changes which are just passing the module handle around.

> But if you can write the > essence of the code here, which I think is really only a couple of functions, > that should be enough. AliasDeclaration and FuncAliasDeclaration get a new 'importprot' member which is set for aliases generated by the import declaration and stores the import's protection. In ScopeDSymbol::search, we discard aliases which shouldn't be accessible - unless it's a FuncAliasDeclaration, to avoid making a chain invisible because the first member is privately imported: + // hide the aliases generated by selective or renamed private imports + if (s && flags & 1) + if (AliasDeclaration* ad = s->isAliasDeclaration()) + // may be a private alias to a function that is overloaded. these + // are sorted out during overload resolution, accept them here + if (ad->importprot == PROTprivate && !ad->aliassym->isFuncAliasDeclaration()) + s = NULL; And for overload resolution, skip over functions that should be invisible: -int overloadApply(FuncDeclaration *fstart, +int overloadApply(Module* from, FuncDeclaration *fstart, int (*fp)(void *, FuncDeclaration *), void *param) ... if (fa) { - if (overloadApply(fa->funcalias, fp, param)) - return 1; + if (fa->getModule() == from || fa->importprot != PROTprivate) + if (overloadApply(from, fa->funcalias, fp, param)) + return 1; next = fa->overnext;

Frelling bug got me again :( Any progress?

How embarrassing this might be, this needs a warning on the website.

How embarrassing this might be, this needs a warning on the website.

I was just bitten by this one testing out a library I'm developing. I use private selective imports in it quite heavily. It was confused when I started getting conflicts. It only took a minute or two to figure out what was going on, but now I have to go through and eliminate all of the selective imports. This bug renders them useless. I'm amazed this has been open for so long. It seems like a pretty major issue to me.

(In reply to comment #33) > https://github.com/D-Programming-Language/dmd/pull/190 So it looks like the patch is good and ready, when are you planning to merge it?

https://github.com/D-Programming-Language/dmd/commit/ff9fa25f3f7f3091787f7459e1950add6cff50b2 https://github.com/D-Programming-Language/dmd/commit/93a643aba6f62db1b7658c2bfb51f9d0b576c337

And the most voted bug from all times (41 votes, the next most voted have 28 votes) finally get fixed. Thanks for the persistence, ckamm!!! :)

*** Issue 5161 has been marked as a duplicate of this issue. ***

(In reply to comment #33) > https://github.com/D-Programming-Language/dmd/pull/190 The pull breaks Issue 7373. Christian, can you have a look, please?

Commit pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/c022036c8f9fea1672bb2c56edd610715f6335c8 Revert "Merge pull request #591 from ckamm/bug314" This reverts commit ff9fa25f3f7f3091787f7459e1950add6cff50b2, reversing changes made to 227769c47847fd577d423469c656e7a72246553c.

Reopened because the original test case now fails again due to the revert. Please, folks, when doing pull requests that fix problems, add the cases into the test suite. The current test suite passes even with the reversion, meaning that no useful test cases were added with the patch.

*** Issue 9516 has been marked as a duplicate of this issue. ***

(In reply to comment #21) > In my understanding, "import std.stdio : writefln;" only imports the name > "writefln", not "std" and "writefln". If the user wants "std", he has to write > "static import std.stdio;". I would assume your example is invalid and is > expected to fail. > > Why would the user do "import std.stdio : writefln;" if he doesn't use writefln > directly? Is "std.stdio.writefln" the only name he's supposed to be able to use > here? If yes, what the hell is the use of that? The use case is this: import std.stdio : File; import std.stream : File; ... new std.stream.File(...); There is no reason for that to fail.

Commit pushed to master at https://github.com/D-Programming-Language/druntime https://github.com/D-Programming-Language/druntime/commit/efe7e94030d75780b16a4eaf45c8dfb789c899aa Merge pull request #532 from 9rnsr/fix_imports Issue 313 & 314 - Add package access to rt.lifetime.BlkInfo

Commits pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/03a32d6fa4635543eed17b874fbf7a0330572ac3 Fix issue 313 & 314 https://github.com/D-Programming-Language/phobos/commit/b7216eae2e9ec73e186a713c59839fd91c9347b7 Merge pull request #1368 from 9rnsr/fix_imports Fix issue 313 & 314 - Add necessary imports and fix FQN accesses

(In reply to comment #45) > https://github.com/D-Programming-Language/dmd/pull/2256 Got hit by this and was going to file a bug. Searched to see if it was already filed. Found this. Noticed it was issue number 314 => Looked at date: Filed 7 years ago :/ Look inside: Fix was issued *4* days ago (!!!) Thank you Kenji for your hard work :)

Commits pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/3e791f6bc6046e4ca7e9e650a573ffaa9f0c3403 Add import declarations for issue 313 & 314 https://github.com/D-Programming-Language/phobos/commit/e2e986c7b6533c9042dbbdc23960e287e206ea12 Merge pull request #1811 from 9rnsr/fix_imports Add import declarations for issue 313 & 314

Commit pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/21a2d202098900148878a0e358052b3ec15d8ac4 Address problems with issue 314

So, can this bug finally be marked as fixed now that the PRs for it have been merged?

(In reply to Orvid King from comment #49) > So, can this bug finally be marked as fixed now that the PRs for it have > been merged? No. Some behavior changed whcch introduced by the merged PR (#2256) could not get agreement, and it was finally reverted. So this issue is not yet fixed in git-head. Instead of that, I'm opening more conservative fix for the issues 313 & 314. https://github.com/D-Programming-Language/dmd/pull/3407

Commits pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/03ec7f38bd29d1c6eb851e4516b74370345e9e49 Don't use top-level selective import in std.math because of DMD issue 314. https://github.com/D-Programming-Language/phobos/commit/33dbfa819fafd4d78378b9ca0e5086ffe824d485 Merge pull request #3961 from tsbockman/math_import_fix Don't use selective import in std.math due to DMD issue 314.

Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/7cf24a4b5f07f920c0d772ac84a3c5547db2c538 fix Issue 314 - static, renamed, and selective imports are always public - enable protection for imports (unless -transition=import or -transition=checkimports is used) - relies on DIP22 in order to resolve public/private symbol conflicts hence cannot be enabled when DIP22 is turned off by the transition switches https://github.com/D-Programming-Language/dmd/commit/8be10ece1b6f81a81f8c794b258ca2c49a9e7652 Merge pull request #5485 from MartinNowak/fix314 fix Issue 314 - static, renamed, and selective imports are always public

Is this time for real? I think I'm gonna cry.

(In reply to Leandro Lucarella from comment #53) > Is this time for real? I think I'm gonna cry. It is, but the new import semantics that came with fixing various import bugs for the next release are surprising to a number of us. The number of resulting deprecation messages in Phobos was quite large, and it's quite likely that there's a lot of code out there that's going to at minimum get a lot of deprecation messages and probably end up with broken code due to imports that don't work as expected anymore. We may be better off in the long run (and certainly, finally getting bugs like this fixed is fantastic), but I can't say that I'm particularly enthused with the resulting semantics, and it's going to take some getting used to.

(In reply to Jonathan M Davis from comment #54) > (In reply to Leandro Lucarella from comment #53) > > Is this time for real? I think I'm gonna cry. > > It is, but the new import semantics that came with fixing various import > bugs for the next release are surprising to a number of us. So it would be good to post a link to its description here.

Commits pushed to stable at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/03ec7f38bd29d1c6eb851e4516b74370345e9e49 Don't use top-level selective import in std.math because of DMD issue 314. https://github.com/D-Programming-Language/phobos/commit/33dbfa819fafd4d78378b9ca0e5086ffe824d485 Merge pull request #3961 from tsbockman/math_import_fix

Commit pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/3d37aee77d12e13b970a84d3e06101e5fd04a00c Byte Order Mark (BOM) handling functions rewrite * move to std.encoding * less overengineering https://github.com/D-Programming-Language/phobos/pull/3870 rework Don't use top-level selective import in std.math because of DMD issue 314. some quickfur comments whitespace remove used import steven suggestion utfBom andrei nitpicks andrei null

*** Issue 11794 has been marked as a duplicate of this issue. ***

dlang/dmd pull request #11704 "Improve error message for 'already defined error'" was merged into master: - ee023891d1b0a33b128087d2849b18543bc34aa8 by Geod24: Improve error message for 'already defined error' Those errors message could get very confusing if the package name was common. In this case, core is used, but working in 'dub' is another situation I encountered. The 'fail314' test didn't have anything to do with issue 314 so was merged with the others. I couldn't find a way to trigger the error message on inserting the template parameter, so it was left as-is. https://github.com/dlang/dmd/pull/11704