Skip to content

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

Merged
WalterBright merged 1 commit intodlang:masterfrom
MartinNowak:fix314
Mar 10, 2016
Merged

fix Issue 314 - static, renamed, and selective imports are always public#5485
WalterBright merged 1 commit intodlang:masterfrom
MartinNowak:fix314

Conversation

@MartinNowak
Copy link
Member

  • enable protection for imports

Based on DIP22 (#5472) in order to resolve plenty of private/public symbol collisions as in this example.

module b;
import core.stdc.stdio : print; // private printf alias
import core.stdc.stdio; // public printf definition
import b; // private printf alias from selective import

void test() { printf(""); }

Depends On: #5509

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
314 [module] Static, renamed, and selective imports are always public

@WalterBright
Copy link
Member

Can this be done as smaller PRs? These giant things are very hard to review. For example, the first commit is a simple refactor, there's no need to lump it in with the rest. Queue all the arguments I made to Kenji about this.

@MartinNowak
Copy link
Member Author

Can this be done as smaller PRs? These giant things are very hard to review. For example, the first commit is a simple refactor, there's no need to lump it in with the rest. Queue all the arguments I made to Kenji about this.

This PR is based on DIP22 (#5472) as mentioned above, so github shows commits from both.
It only consists of a single commit MartinNowak@a8b96b4.

@WalterBright
Copy link
Member

If the previous commits are already pulled, why do they show here? I don't understand. Maybe rebase?

@Geod24
Copy link
Member

Geod24 commented Mar 1, 2016

@WalterBright : They aren't pulled yet, #5472 is still open.
When it is merged, they won't show up here anymore (unless some of the commits are changed, in which case it will need a rebase yes). Note that merging this P.R. would also automatically close #5472.

@MartinNowak
Copy link
Member Author

Rebased now and also added a changelog entry @WalterBright.

AliasDeclaration ad = void;
// accessing private selective and renamed imports is
// deprecated by restricting the symbol visibility
if (s.isImport() || (ad = s.isAliasDeclaration()) !is null && ad._import !is null)
Copy link
Member Author

Choose a reason for hiding this comment

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

The eager access checks during lookup are disabled for renamed and selective imports. This is instead handled by the new DIP22 symbolIsVisible check above.
This means that people will only get a deprecation warning for now (b/c of the additional IgnoreSymbolVisibility lookup), and that -transition=import/-transition=checkimports work as for any other private symbol w/ the DIP22 changes.

@MartinNowak
Copy link
Member Author

Fixed the Win64 failure, see #5509.

- 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
@MartinNowak
Copy link
Member Author

Rebased on top of the merged BitArray bugfix. The Win32_64 is a bit overwhelmed, can we get this auto-merged once it's reviewed?

@andralex
Copy link
Member

andralex commented Mar 9, 2016

Guess it's about time. As I mentioned in email comm, it would be great to integrate all lookup changes under the same scalable -dip22 flag. @MartinNowak @WalterBright thoughts?

@WalterBright
Copy link
Member

Auto-merge toggled on

WalterBright added a commit that referenced this pull request Mar 10, 2016
fix Issue 314 - static, renamed, and selective imports are always public
@WalterBright WalterBright merged commit 8be10ec into dlang:master Mar 10, 2016
@MartinNowak MartinNowak deleted the fix314 branch March 10, 2016 00:58
@MartinNowak
Copy link
Member Author

As I mentioned in email comm, it would be great to integrate all lookup changes under the same scalable -dip22 flag.

The can all be turned off/mitigated using -transition=imports/-transition=checkimports. DIP22, 313, and 314 are all implemented as deprecations and shouldn't change the semantic of any currently compiling code.

@JackStouffer
Copy link
Contributor

Does this mean we can finally get rid of the complete imports of modules at the top of every Phobos file?

@MartinNowak
Copy link
Member Author

Does this mean we can finally get rid of the complete imports of modules at the top of every Phobos file?

It means you can safely add selective imports add module level, w/o leaking symbols. Don't jump onto that too fast until the deprecation is over and we remove the -transition=imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants