Deprecate adding functions with different access specifiers in the same overload set

[RazvanN7]

Backlog:

While trying to fix: https://issues.dlang.org/show_bug.cgi?id=3254 I have stumbled upon the fact that it is hard/impossible to fix it as long as public aliases to private symbols are allowed. I presented this issue to @WalterBright and @acehreli and they both suggested that having private symbols in the same overload set with public symbols does not make any sense. So here is the PR.

Currently this PR is pretty blunt: if the overloads have different access specifiers, the deprecation is issued. Also, it does not currently test for template declarations. That can be added in a future PR if we decide to go on with this.

Let's see the amount of breakage that this causes.

[dlang-bot]

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14547"

[Geod24]

I think the build log will tell you why this is a bad idea

[ghost]

I have stumbled upon the fact that it is hard/impossible to fix it

Have you try to add a supplemental access check on the fd used by a CallExp (and if the CallExp has no other errors) ?

[RazvanN7]

Have you try to add a supplemental access check on the fd used by a CallExp (and if the CallExp has no other errors) ?

Yes, the problem is that you may have public aliases to private symbols. If you use the alias to call the function, by the time it gets to CallExp semantic, the alias has already been resolved and it appears like you are illegally trying to call a private function. For more info check: https://github.com/dlang/dmd/pull/13257#discussion_r741850340

[Geod24]

@RazvanN7 : The solution is not to destructively resolve aliases. This will solve loads of issues.

[RazvanN7]

@Geod24 I agree, but last time I checked that requires a major re-engineering of the entire alias implementation.

[Geod24]

Correct. But deprecating a used and useful feature because fixing the bug is hard is not the way to go.

[RazvanN7]

Well, having private and public functions in the same overload set is arguably something that should be avoided. The fix is quite trivial: just keep your private functions in a different overload set.

[RazvanN7]

@Geod24 Keep in mind that this solution was not my idea. It does make sense to not have public and private functions in the same overload set, but I have no strong feelings one way or the other.

[Geod24]

Well, having private and public functions in the same overload set is arguably something that should be avoided. The fix is quite trivial: just keep your private functions in a different overload set.

You can't always do that. The obvious example is constructors. Other examples where you can't split out the overload set are operator overloads (opEquals / opCmp, etc...).

Keep in mind that this solution was not my idea. It does make sense to not have public and private functions in the same overload set, but I have no strong feelings one way or the other.

If you thought it wasn't a good idea, why are you implementing it ? The CI is telling you there is a widespread use case for it. Trying one approach is never a bad idea, but more often than not, you will see that things are used in a way you didn't foresee. We can either take this information in, and re-iterate on our expectations, or yell to them "You are wrong" and alienate a part of our userbase.

[ghost]

Yes, the problem is that you may have public aliases to private symbols

This is a problem of specification I believe then. The only aliases allowed to change the visibility should be aliases to template instanciations (i.e the rhs does an instanciation) because those copy the declaration.

Otherwise public aliases should not promote the visibility of declarations that are already fully instantiated because there's no AST copies, it's still the same symbol, it cannot be both private and public.

[ghost]

The solution is not to destructively resolve aliases. This will solve loads of issues.

This will lead to the need to use .toAlias() everywhere, the initial refactoring will be ok I think but this will lead to bugs in the future, e.g on new feature addition, because at some point someone will forget to use the function or the PR reviewers will miss the problem.