fix Issue 16085 - wrong visibility warning for overloaded alias symbol

- mark OverDeclaration as overloadable
- mark AliasDeclaration as overloadable (depends on aliasee)
- replace overloadApply with a custom iteration b/c aliases
  must not be resolved for visibility checks (i.e. public aliases to
  private symbols are public)
- deal with the messy overloading of aliasees (see comments)

src/access.d

@@ -477,16 +477,7 @@ extern (C++) bool checkAccess(Loc loc, Scope* sc, Package p)
 extern (C++) bool symbolIsVisible(Module mod, Dsymbol s)
 {
     // should sort overloads by ascending protection instead of iterating here
-    if (s.isOverloadable())
-    {
-        // Use the least protected overload to determine visibility
-        // and perform an access check after overload resolution.
-        overloadApply(s, (s2) {
-          if (s.prot().isMoreRestrictiveThan(s2.prot()))
-            s = s2;
-          return 0;
-        });
-    }
+    s = mostVisibleOverload(s);
     final switch (s.prot().kind)
     {
     case PROTundefined: return true;
@@ -517,17 +508,7 @@ extern (C++) bool symbolIsVisible(Dsymbol origin, Dsymbol s)
  */
 extern (C++) bool symbolIsVisible(Scope *sc, Dsymbol s)
 {
-    // should sort overloads by ascending protection instead of iterating here
-    if (s.isOverloadable())
-    {
-        // Use the least protected overload to determine visibility
-        // and perform an access check after overload resolution.
-        overloadApply(s, (s2) {
-          if (s.prot().isMoreRestrictiveThan(s2.prot()))
-            s = s2;
-          return 0;
-        });
-    }
+    s = mostVisibleOverload(s);
     final switch (s.prot().kind)
     {
     case PROTundefined: return true;
@@ -538,3 +519,78 @@ extern (C++) bool symbolIsVisible(Scope *sc, Dsymbol s)
     case PROTpublic, PROTexport: return true;
     }
 }
+
+/**
+ * Use the most visible overload to check visibility. Later perform an access
+ * check on the resolved overload.  This function is similar to overloadApply,
+ * but doesn't recurse nor resolve aliases because protection/visibility is an
+ * attribute of the alias not the aliasee.
+ */
+private Dsymbol mostVisibleOverload(Dsymbol s)
+{
+    if (!s.isOverloadable())
+        return s;
+
+    Dsymbol next, fstart = s, mostVisible = s;
+    for (; s; s = next)
+    {
+        // void func() {}
+        // private void func(int) {}
+        if (auto fd = s.isFuncDeclaration())
+            next = fd.overnext;
+        // template temp(T) {}
+        // private template temp(T:int) {}
+        else if (auto td = s.isTemplateDeclaration())
+            next = td.overnext;
+        // alias common = mod1.func1;
+        // alias common = mod2.func2;
+        else if (auto fa = s.isFuncAliasDeclaration())
+            next = fa.overnext;
+        // alias common = mod1.templ1;
+        // alias common = mod2.templ2;
+        else if (auto od = s.isOverDeclaration())
+            next = od.overnext;
+        // alias name = sym;
+        // private void name(int) {}
+        else if (auto ad = s.isAliasDeclaration())
+        {
+            /* This is a bit messy due to the complicated implementation of
+             * alias.  Aliases aren't overloadable themselves, but if their
+             * Aliasee is overloadable they can be converted to an overloadable
+             * alias.
+             *
+             * This is done by replacing the Aliasee w/ FuncAliasDeclaration
+             * (for functions) or OverDeclaration (for templates) which are
+             * simply overloadable aliases w/ weird names.
+             *
+             * Usually aliases should not be resolved for visibility checking
+             * b/c public aliases to private symbols are public. But for the
+             * overloadable alias situation, the Alias (_ad_) has been moved
+             * into it's own Aliasee, leaving a shell that we peel away here.
+             */
+            assert(ad.isOverloadable, "Non overloadable Aliasee in overload list");
+            auto aliasee = ad.toAlias();
+            if (aliasee.isFuncAliasDeclaration || aliasee.isOverDeclaration)
+                next = aliasee;
+            else
+            {
+                /* A simple alias can be at the end of a function or template overload chain.
+                 * It can't have further overloads b/c it would have been
+                 * converted to an overloadable alias.
+                 */
+                assert(ad.overnext is null, "Unresolved overload of alias");
+                break;
+            }
+
+            // handled by ddmd.func.overloadApply for unknown reason
+            assert(next !is ad); // should not alias itself
+            assert(next !is fstart); // should not alias the overload list itself
+        }
+        else
+            break;
+
+        if (next && mostVisible.prot().isMoreRestrictiveThan(next.prot()))
+            mostVisible = next;
+    }
+    return mostVisible;
+}

src/declaration.d

@@ -679,13 +679,29 @@ public:
     override bool overloadInsert(Dsymbol s)
     {
         //printf("[%s] AliasDeclaration::overloadInsert('%s') s = %s %s @ [%s]\n",
-        //    loc.toChars(), toChars(), s->kind(), s->toChars(), s->loc.toChars());
-
-        /* semantic analysis is already finished, and the aliased entity
-         * is not overloadable.
+        //       loc.toChars(), toChars(), s.kind(), s.toChars(), s.loc.toChars());
+
+        /** Aliases aren't overloadable themselves, but if their Aliasee is
+         *  overloadable they are converted to an overloadable Alias (either
+         *  FuncAliasDeclaration or OverDeclaration).
+         *
+         *  This is done by moving the Aliasee into such an overloadable alias
+         *  which is then used to replace the existing Aliasee. The original
+         *  Alias (_this_) remains a useless shell.
+         *
+         *  This is a horrible mess. It was probably done to avoid replacing
+         *  existing AST nodes and references, but it needs a major
+         *  simplification b/c it's too complex to maintain.
+         *
+         *  A simpler approach might be to merge any colliding symbols into a
+         *  simple Overload class (an array) and then later have that resolve
+         *  all collisions.
          */
         if (semanticRun >= PASSsemanticdone)
         {
+            /* Semantic analysis is already finished, and the aliased entity
+             * is not overloadable.
+             */
             if (type)
                 return false;
 
@@ -695,21 +711,26 @@ public:
             auto sa = aliassym.toAlias();
             if (auto fd = sa.isFuncDeclaration())
             {
-                aliassym = new FuncAliasDeclaration(ident, fd);
-                aliassym.parent = parent;
+                auto fa = new FuncAliasDeclaration(ident, fd);
+                fa.protection = protection;
+                fa.parent = parent;
+                aliassym = fa;
                 return aliassym.overloadInsert(s);
             }
             if (auto td = sa.isTemplateDeclaration())
             {
-                aliassym = new OverDeclaration(ident, td);
-                aliassym.parent = parent;
+                auto od = new OverDeclaration(ident, td);
+                od.protection = protection;
+                od.parent = parent;
+                aliassym = od;
                 return aliassym.overloadInsert(s);
             }
             if (auto od = sa.isOverDeclaration())
             {
                 if (sa.ident != ident || sa.parent != parent)
                 {
                     od = new OverDeclaration(ident, od);
+                    od.protection = protection;
                     od.parent = parent;
                     aliassym = od;
                 }
@@ -720,6 +741,22 @@ public:
                 if (sa.ident != ident || sa.parent != parent)
                 {
                     os = new OverloadSet(ident, os);
+                    // TODO: protection is lost here b/c OverloadSets have no protection attribute
+                    // Might no be a practical issue, b/c the code below fails to resolve the overload anyhow.
+                    // ----
+                    // module os1;
+                    // import a, b;
+                    // private alias merged = foo; // private alias to overload set of a.foo and b.foo
+                    // ----
+                    // module os2;
+                    // import a, b;
+                    // public alias merged = bar; // public alias to overload set of a.bar and b.bar
+                    // ----
+                    // module bug;
+                    // import os1, os2;
+                    // void test() { merged(123); } // should only look at os2.merged
+                    //
+                    // os.protection = protection;
                     os.parent = parent;
                     aliassym = os;
                 }
@@ -833,6 +870,11 @@ public:
         return s;
     }
 
+    override bool isOverloadable()
+    {
+        return aliassym && aliassym.isOverloadable();
+    }
+
     override inout(AliasDeclaration) isAliasDeclaration() inout
     {
         return this;
@@ -918,6 +960,11 @@ public:
         return true;
     }
 
+    override bool isOverloadable()
+    {
+        return true;
+    }
+
     Dsymbol isUnique()
     {
         if (!hasOverloads)

src/dsymbol.d

@@ -901,19 +901,12 @@ public:
                 }
                 if (*ps)
                 {
-                    static bool isOverloadableAlias(Dsymbol s)
-                    {
-                        auto ad = s.isAliasDeclaration();
-                        return ad && ad.aliassym && ad.aliassym.isOverloadable();
-                    }
-
                     assert(ident);
                     if (!(*ps).ident || !(*ps).ident.equals(ident))
                         continue;
                     if (!s)
                         s = *ps;
-                    else if ((   s .isOverloadable() || isOverloadableAlias(  s)) &&
-                             ((*ps).isOverloadable() || isOverloadableAlias(*ps)))
+                    else if (s.isOverloadable() && (*ps).isOverloadable())
                     {
                         // keep head of overload set
                         FuncDeclaration f1 = s.isFuncDeclaration();

test/compilable/imports/imp16085.d

@@ -0,0 +1,29 @@
+struct Pass
+{
+}
+
+struct S
+{
+    import imports.imp16085b : functionAndFunction, staticFunctionAndFunction,
+        functionAndTemplate, templateAndTemplate; //<- private
+    // public
+    Pass functionAndFunction()
+    {
+        return Pass();
+    }
+
+    static Pass staticFunctionAndFunction()
+    {
+        return Pass();
+    }
+
+    Pass functionAndTemplate()
+    {
+        return Pass();
+    }
+
+    Pass templateAndTemplate()()
+    {
+        return Pass();
+    }
+}

test/compilable/imports/imp16085b.d

@@ -0,0 +1,25 @@
+import imp16085 : S;
+
+struct Fail
+{
+}
+
+Fail functionAndFunction(ref S)
+{
+    return Fail();
+}
+
+Fail staticFunctionAndFunction(int)
+{
+    return Fail();
+}
+
+Fail functionAndTemplate(T)(T)
+{
+    return Fail();
+}
+
+Fail templateAndTemplate(T)(T)
+{
+    return Fail();
+}

test/compilable/test16085.d

@@ -0,0 +1,13 @@
+// REQUIRED_ARGS: -de
+// PERMUTE_ARGS:
+import imports.imp16085;
+
+void test()
+{
+    S s;
+    assert(s.functionAndFunction() == Pass());
+    assert(s.staticFunctionAndFunction() == Pass());
+    // assert(S.staticFunctionAndFunction() == Pass()); // erroneous not accessible error
+    assert(s.functionAndTemplate() == Pass());
+    assert(s.templateAndTemplate() == Pass());
+}

test/compilable/test6013.d

@@ -1,3 +1,4 @@
+// REQUIRED_ARGS: -de
 import imports.test6013;
 
 static assert(__traits(compiles, public_alias_value));