Skip to content

Fix Issue 18243 - selective import + overload = private visibility#7766

Merged
andralex merged 1 commit intodlang:masterfrom
RazvanN7:Issue_18243
Jan 25, 2018
Merged

Fix Issue 18243 - selective import + overload = private visibility#7766
andralex merged 1 commit intodlang:masterfrom
RazvanN7:Issue_18243

Conversation

@RazvanN7
Copy link
Contributor

I fixed the issue by modifying the overloadApply method. The problem was that when resolving overloads the visibility attribute is not checked at all, so I had to add the scope as an optional parameter so that when an alias declaration is encountered (from imports or plain alias declarations) a check is made to make sure that the called function is indeed visible.

@RazvanN7 RazvanN7 requested a review from MartinNowak as a code owner January 23, 2018 16:24
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
18243 selective import + overload = private visibility

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

@@ -0,0 +1,5 @@
module a18243;

import std.math : isNaN;
Copy link
Contributor

Choose a reason for hiding this comment

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

Selective imports creatingalias in the current scope is really an annoyance, and violates the principle of least surprise. I wish we could change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a small project on its own which will propagate cascade changes throughout a large number of components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I agree that the current state is hackish and messy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Last time I checked with Walter, it was by design. Maybe that changed since. If you have any way to find out, it would be appreciated.

@wilzbach
Copy link
Contributor

@RazvanN7 It looks like you need to fix a couple of projects. However, they are all in dlang-comunity or dlang-tour, so I have merge rights & thus I will be able to help you ;-)

@wilzbach
Copy link
Contributor

@RazvanN7 making the PRs isn't too hard:

Fork and clone the respective repo

git checkout fix-imports-for-dmd-7766
dub test --compiler=/home/seb/dlang/dmd/generated/linux/release/64/dmd 
# fix the error & run until it passes
git add -u
git commit "Fix imports for dmd 7766"
pr_send

pr_send

@RazvanN7
Copy link
Contributor Author

The jenkins failure is unrelated to this PR. Can we move forward with this? ping @andralex

@andralex andralex merged commit ccbaaa1 into dlang:master Jan 25, 2018
@stefan-koch-sociomantic

Is this in the changelog ?

@jacob-carlborg
Copy link
Contributor

Is this in the changelog ?

Not in this PR.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants