Merge pull request #6731 from WalterBright/fix17349

fix Issue 17349 - Covariant overrides should be allowed
merged-on-behalf-of: Andrei Alexandrescu <andralex@users.noreply.github.com>

src/ddmd/declaration.h

@@ -582,7 +582,7 @@ class FuncDeclaration : public Declaration
     bool equals(RootObject *o);
 
     int overrides(FuncDeclaration *fd);
-    int findVtblIndex(Dsymbols *vtbl, int dim);
+    int findVtblIndex(Dsymbols *vtbl, int dim, bool fix17349 = true);
     BaseClass *overrideInterface();
     bool overloadInsert(Dsymbol *s);
     FuncDeclaration *overloadExactMatch(Type *t);

src/ddmd/dtemplate.d

@@ -2555,6 +2555,21 @@ void functionResolve(Match* m, Dsymbol dstart, Loc loc, Scope* sc, Objects* tiar
                 if (c1 < c2) goto LlastIsBetter;
             }
 
+            /* The 'overrides' check above does covariant checking only
+             * for virtual member functions. It should do it for all functions,
+             * but in order to not risk breaking code we put it after
+             * the 'leastAsSpecialized' check.
+             * In the future try moving it before.
+             * I.e. a not-the-same-but-covariant match is preferred,
+             * as it is more restrictive.
+             */
+            if (!m.lastf.type.equals(fd.type))
+            {
+                //printf("cov: %d %d\n", m.lastf.type.covariant(fd.type), fd.type.covariant(m.lastf.type));
+                if (m.lastf.type.covariant(fd.type) == 1) goto LlastIsBetter;
+                if (fd.type.covariant(m.lastf.type) == 1) goto LfIsBetter;
+            }
+
             /* If the two functions are the same function, like:
              *    int foo(int);
              *    int foo(int x) { ... }

src/ddmd/func.d

@@ -779,9 +779,17 @@ extern (C++) class FuncDeclaration : Declaration
                     if (fdv.isFinalFunc())
                         error("cannot override final function %s", fdv.toPrettyChars());
 
-                    doesoverride = true;
                     if (!isOverride())
-                        .error(loc, "cannot implicitly override base class method %s with %s; add 'override' attribute", fdv.toPrettyChars(), toPrettyChars());
+                    {
+                            int vi2 = findVtblIndex(&cd.baseClass.vtbl, cast(int)cd.baseClass.vtbl.dim, false);
+                            if (vi2 < 0)
+                                // https://issues.dlang.org/show_bug.cgi?id=17349
+                                .deprecation(loc, "cannot implicitly override base class method `%s` with `%s`; add `override` attribute", fdv.toPrettyChars(), toPrettyChars());
+                            else
+                                .error(loc, "cannot implicitly override base class method %s with %s; add 'override' attribute", fdv.toPrettyChars(), toPrettyChars());
+                    }
+
+                    doesoverride = true;
                     if (fdc.toParent() == parent)
                     {
                         // If both are mixins, or both are not, then error.
@@ -2289,12 +2297,15 @@ extern (C++) class FuncDeclaration : Declaration
      * Find index of function in vtbl[0..dim] that
      * this function overrides.
      * Prefer an exact match to a covariant one.
+     * Params:
+     *      fix17349 = enable fix https://issues.dlang.org/show_bug.cgi?id=17349
      * Returns:
      *      -1      didn't find one
      *      -2      can't determine because of forward references
      */
-    final int findVtblIndex(Dsymbols* vtbl, int dim)
+    final int findVtblIndex(Dsymbols* vtbl, int dim, bool fix17349 = true)
     {
+        //printf("findVtblIndex() %s\n", toChars());
         FuncDeclaration mismatch = null;
         StorageClass mismatchstc = 0;
         int mismatchvi = -1;
@@ -2321,7 +2332,7 @@ extern (C++) class FuncDeclaration : Declaration
                 }
 
                 StorageClass stc = 0;
-                int cov = type.covariant(fdv.type, &stc);
+                int cov = type.covariant(fdv.type, &stc, fix17349);
                 //printf("\tbaseclass cov = %d\n", cov);
                 switch (cov)
                 {

src/ddmd/mtype.d

@@ -647,6 +647,10 @@ extern (C++) abstract class Type : RootObject
     /*******************************
      * Covariant means that 'this' can substitute for 't',
      * i.e. a pure function is a match for an impure type.
+     * Params:
+     *      t = type 'this' is covariant with
+     *      pstc = if not null, store STCxxxx which would make it covariant
+     *      fix17349 = enable fix https://issues.dlang.org/show_bug.cgi?id=17349
      * Returns:
      *      0       types are distinct
      *      1       this is covariant with t
@@ -655,7 +659,7 @@ extern (C++) abstract class Type : RootObject
      *      3       cannot determine covariance because of forward references
      *      *pstc   STCxxxx which would make it covariant
      */
-    final int covariant(Type t, StorageClass* pstc = null)
+    final int covariant(Type t, StorageClass* pstc = null, bool fix17349 = true)
     {
         version (none)
         {
@@ -668,7 +672,7 @@ extern (C++) abstract class Type : RootObject
             *pstc = 0;
         StorageClass stc = 0;
 
-        int inoutmismatch = 0;
+        bool notcovariant = false;
 
         TypeFunction t1;
         TypeFunction t2;
@@ -698,9 +702,42 @@ extern (C++) abstract class Type : RootObject
 
                 if (!fparam1.type.equals(fparam2.type))
                 {
+                    if (!fix17349)
+                        goto Ldistinct;
+                    Type tp1 = fparam1.type;
+                    Type tp2 = fparam2.type;
+                    if (tp1.ty == tp2.ty)
+                    {
+                        if (tp1.ty == Tclass)
+                        {
+                            if ((cast(TypeClass)tp1).sym == (cast(TypeClass)tp2).sym && MODimplicitConv(tp2.mod, tp1.mod))
+                                goto Lcov;
+                        }
+                        else if (tp1.ty == Tstruct)
+                        {
+                            if ((cast(TypeStruct)tp1).sym == (cast(TypeStruct)tp2).sym && MODimplicitConv(tp2.mod, tp1.mod))
+                                goto Lcov;
+                        }
+                        else if (tp1.ty == Tpointer)
+                        {
+                            if (tp2.implicitConvTo(tp1))
+                                goto Lcov;
+                        }
+                        else if (tp1.ty == Tarray)
+                        {
+                            if (tp2.implicitConvTo(tp1))
+                                goto Lcov;
+                        }
+                        else if (tp1.ty == Tdelegate)
+                        {
+                            if (tp1.implicitConvTo(tp2))
+                                goto Lcov;
+                        }
+                    }
                     goto Ldistinct;
                 }
-                inoutmismatch = !fparam1.isCovariant(t1.isref, fparam2);
+            Lcov:
+                notcovariant |= !fparam1.isCovariant(t1.isref, fparam2);
             }
         }
         else if (t1.parameters != t2.parameters)
@@ -712,7 +749,7 @@ extern (C++) abstract class Type : RootObject
         }
 
         // The argument lists match
-        if (inoutmismatch)
+        if (notcovariant)
             goto Lnotcovariant;
         if (t1.linkage != t2.linkage)
             goto Lnotcovariant;

src/ddmd/mtype.h

@@ -237,7 +237,7 @@ class Type : public RootObject
     bool equivalent(Type *t);
     // kludge for template.isType()
     int dyncast() const { return DYNCAST_TYPE; }
-    int covariant(Type *t, StorageClass *pstc = NULL);
+    int covariant(Type *t, StorageClass *pstc = NULL, bool fix17349 = true);
     const char *toChars();
     char *toPrettyChars(bool QualifyTypes = false);
     static void _init();

test/compilable/fix17349.d

@@ -0,0 +1,40 @@
+/* REQUIRED_ARGS: -dw
+ * PERMUTE_ARGS:
+ * TEST_OUTPUT:
+---
+compilable/fix17349.d(37): Deprecation: cannot implicitly override base class method `fix17349.E.foo` with `fix17349.F.foo`; add `override` attribute
+---
+ */
+
+// https://issues.dlang.org/show_bug.cgi?id=17349
+
+struct S { }
+
+class C {
+    void bar();
+    void foo(void* p);
+    void abc(Object);
+    void def(S);
+}
+
+class D : C {
+    override void bar() const;
+    override void foo(const void*);
+    override void abc(const Object);
+    override void def(const S);
+}
+
+alias fp_t = void function(int*);
+@safe void abc(const int*);
+fp_t fp = &abc;
+
+
+class E {
+    void foo(void*);
+}
+
+class F : E {
+    void foo(const void*);
+}
+
+

test/compilable/test16303.d

@@ -0,0 +1,13 @@
+// https://issues.dlang.org/show_bug.cgi?id=16303
+
+void yayf(void function(int*) fp);
+void yayd(void delegate(int*) dg);
+
+void bar()
+{
+    void function(const(int)* p) fp;
+    yayf(fp);                   // should be good but produces error
+
+    void delegate(const(int)* p) dg;
+    yayd(dg);                   // should be good but produces error
+}

test/compilable/test17349.d

@@ -0,0 +1,30 @@
+
+/* REQUIRED_ARGS:
+   PERMUTE_ARGS:
+ */
+
+// https://issues.dlang.org/show_bug.cgi?id=16538
+
+const(int) retConst1();
+int retConst2();
+auto retConst = [&retConst1, &retConst2];
+
+const(int*) retConstPtr1();
+const(int)* retConstPtr2();
+auto retConstPtr = [&retConstPtr1, &retConstPtr2];
+
+void constArray1(const(int)[1]);
+void constArray2(const(int[1]));
+auto constArray = [&constArray1, &constArray2];
+
+const(int)[] retConstSlice1();
+const(int[]) retConstSlice2();
+auto retConstSlice = [&retConstSlice1, &retConstSlice2];
+
+void constSlice1(const(int)[]);
+void constSlice2(const(int[]));
+auto constSlice = [&constSlice1, &constSlice2];
+
+void ptrToConst1(const(int)*);
+void ptrToConst2(const(int*));
+auto ptrToConst = [&ptrToConst1, &ptrToConst2];

test/fail_compilation/fail16600.d

@@ -1,21 +1,26 @@
-/*
-TEST_OUTPUT:
+/* TEST_OUTPUT:
 ---
-fail_compilation/fail16600.d(19): Error: fail16600.S.__ctor called with argument types (string) const matches both:
-fail_compilation/fail16600.d(13):     fail16600.S.this(string)
+fail_compilation/fail16600.d(22): Error: fail16600.S.__ctor called with argument types (string) const matches both:
+fail_compilation/fail16600.d(16):     fail16600.S.this(string _param_0)
 and:
-fail_compilation/fail16600.d(14):     fail16600.S.this(string) immutable
+fail_compilation/fail16600.d(17):     fail16600.S.this(string _param_0) immutable
 ---
 */
 
+// https://issues.dlang.org/show_bug.cgi?id=16600
+
 struct S
 {
-    this(string);
-    this(string) immutable;
+    int i;
+
+    this(string) { i = 1; }
+    this(string) immutable { i = 2; }
 }
 
 void main()
 {
     auto a = const(S)("abc");
+    assert(a.i == 2);
 }
 
+

test/fail_compilation/fail17354.d

@@ -0,0 +1,19 @@
+/* REQUIRED_ARGS: -de
+ * TEST_OUTPUT:
+---
+fail_compilation/fail17354.d(13): Deprecation: cannot implicitly override base class method `object.Object.opEquals` with `fail17354.Foo.opEquals`; add `override` attribute
+fail_compilation/fail17354.d(18): Deprecation: cannot implicitly override base class method `object.Object.opEquals` with `fail17354.Bar.opEquals`; add `override` attribute
+---
+ */
+
+// https://issues.dlang.org/show_bug.cgi?id=17354
+
+final class Foo
+{
+    bool opEquals(const Object) const {return true;}
+}
+
+class Bar
+{
+    bool opEquals(const Object) const {return true;}
+}

test/fail_compilation/fail_scope.d

@@ -5,10 +5,6 @@ TEST_OUTPUT:
 ---
 fail_compilation/fail_scope.d(45): Error: returning `cast(char[])string` escapes a reference to local variable `string`
 fail_compilation/fail_scope.d(63): Error: returning `s.bar()` escapes a reference to local variable `s`
-fail_compilation/fail_scope.d(74): Error: fail_scope.foo8 called with argument types (int) matches both:
-fail_compilation/fail_scope.d(68):     fail_scope.foo8(ref int x)
-and:
-fail_compilation/fail_scope.d(69):     fail_scope.foo8(return ref int x)
 fail_compilation/fail_scope.d(82): Error: returning `& string` escapes a reference to local variable `string`
 fail_compilation/fail_scope.d(92): Error: returning `cast(int[])a` escapes a reference to local variable `a`
 fail_compilation/fail_scope.d(100): Error: returning `cast(int[])a` escapes a reference to local variable `a`
@@ -25,6 +21,10 @@ fail_compilation/fail_scope.d(137): Error: returning `foo16226(i)` escapes a ref
 //fail_compilation/fail_scope.d(40): Error: scope variable p may not be returned
 */
 
+
+
+
+
 alias int delegate() dg_t;
 
 int[]  checkEscapeScope1(scope int[]  da) { return da; }

test/runnable/xtest46.d

@@ -7921,6 +7921,27 @@ void test16408()
     );
 }
 
+/***************************************************/
+// https://issues.dlang.org/show_bug.cgi?id=17349
+
+void test17349()
+{
+    static struct S
+    {
+        int bar(void delegate(ref int*)) { return 1; }
+        int bar(void delegate(ref const int*)) const { return 2; }
+    }
+
+    void dg1(ref int*) { }
+    void dg2(ref const int*) { }
+    S s;
+    int i;
+    i = s.bar(&dg1);
+    assert(i == 1);
+    i = s.bar(&dg2);
+    assert(i == 2);
+}
+
 /***************************************************/
 
 int main()
@@ -8240,6 +8261,7 @@ int main()
     test16233();
     test16466();
     test16408();
+    test17349();
 
     printf("Success\n");
     return 0;