Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start deprecation period of identical functions in a single module #8429

Merged
merged 6 commits into from
Sep 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions changelog/duplicate-implementations-deprecation.dd
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
Diagnostics for conflicting function definitions within a module

Previously, multiple definitions of identical functions within a module were not
recognized, although they have the same mangling. This was problematic because a
binary cannot contain multiple definitions of a symbol, which caused undefined
behavior depending on the compiler backend.

DMD will now raise an error message if there are conflicting implementations
within a single module:

---
void foo() {}
void foo() {} // error
---

Multiple declarations are still allowed as long as there is at most one definition:

---
void bar(int);
void bar(int) { }
void bar(int);
---

DMD will issue a deprecation for mangling schemes that don't support overloading
(`extern(C|Windows|System)`):

---
extern(C):
void foo(int) { }
void foo(double) { } // deprecation
---

This deprecation will become an error in 2.105.
1 change: 1 addition & 0 deletions src/dmd/frontend.h
Original file line number Diff line number Diff line change
Expand Up @@ -5388,6 +5388,7 @@ class TypeFunction : public TypeNext
bool isInOutQual() const;
void isInOutQual(bool v);
bool iswild() const;
bool attributesEqual(const TypeFunction* const other) const;
void accept(Visitor* v);
};

Expand Down
39 changes: 39 additions & 0 deletions src/dmd/mtype.d
Original file line number Diff line number Diff line change
Expand Up @@ -5198,6 +5198,18 @@ extern (C++) final class TypeFunction : TypeNext
return (funcFlags & (FunctionFlag.inoutParam | FunctionFlag.inoutQual)) != 0;
}

/// Returns: whether `this` function type has the same attributes (`@safe`,...) as `other`
bool attributesEqual(const scope TypeFunction other) const pure nothrow @safe @nogc
{
enum attributes = FunctionFlag.isnothrow
| FunctionFlag.isnogc
| FunctionFlag.islive;

return this.trust == other.trust &&
this.purity == other.purity &&
(this.funcFlags & attributes) == (other.funcFlags & attributes);
}

override void accept(Visitor v)
{
v.visit(this);
Expand Down Expand Up @@ -6674,6 +6686,33 @@ extern (C++) struct ParameterList
{
return ParameterList(Parameter.arraySyntaxCopy(parameters), varargs);
}

/// Compares this to another ParameterList (and expands tuples if necessary)
extern (D) bool opEquals(scope ref ParameterList other) const
{
if (stc != other.stc || varargs != other.varargs || (!parameters != !other.parameters))
return false;

if (this.parameters is other.parameters)
return true;

size_t idx;
bool diff;

// Pairwise compare each parameter
// Can this avoid the O(n) indexing for the second list?
foreach (_, p1; cast() this)
{
auto p2 = other[idx++];
if (!p2 || p1 != p2) {
diff = true;
break;
}
}

// Ensure no remaining parameters in `other`
return !diff && other[idx] is null;
}
}


Expand Down
54 changes: 23 additions & 31 deletions src/dmd/semantic2.d
Original file line number Diff line number Diff line change
Expand Up @@ -359,17 +359,19 @@ private extern(C++) final class Semantic2Visitor : Visitor

//printf("FuncDeclaration::semantic2 [%s] fd0 = %s %s\n", loc.toChars(), toChars(), type.toChars());

// https://issues.dlang.org/show_bug.cgi?id=18385
// Disable for 2.079, s.t. a deprecation cycle can be started with 2.080
if (0)
if (fd.overnext && !fd.errors)
// Only check valid functions which have a body to avoid errors
// for multiple declarations, e.g.
// void foo();
// void foo();
if (fd.fbody && fd.overnext && !fd.errors)
{
OutBuffer buf1;
OutBuffer buf2;

// Always starts the lookup from 'this', because the conflicts with
// previous overloads are already reported.
auto f1 = fd;
alias f1 = fd;
auto tf1 = cast(TypeFunction) f1.type;
mangleToFuncSignature(buf1, f1);

overloadApply(f1, (Dsymbol s)
Expand All @@ -379,7 +381,7 @@ private extern(C++) final class Semantic2Visitor : Visitor
return 0;

// Don't have to check conflict between declaration and definition.
if ((f1.fbody !is null) != (f2.fbody !is null))
if (f2.fbody is null)
return 0;

/* Check for overload merging with base class member functions.
Expand All @@ -393,35 +395,26 @@ private extern(C++) final class Semantic2Visitor : Visitor
if (f1.overrides(f2))
return 0;

auto tf2 = cast(TypeFunction) f2.type;

// extern (C) functions always conflict each other.
if (f1.ident == f2.ident &&
f1.toParent2() == f2.toParent2() &&
(f1.linkage != LINK.d && f1.linkage != LINK.cpp) &&
(f2.linkage != LINK.d && f2.linkage != LINK.cpp))
(f2.linkage != LINK.d && f2.linkage != LINK.cpp) &&

// But allow the hack to declare overloads with different parameters/STC's
(!tf1.attributesEqual(tf2) || tf1.parameterList != tf2.parameterList))
{
/* Allow the hack that is actually used in druntime,
* to ignore function attributes for extern (C) functions.
* TODO: Must be reconsidered in the future.
* BUG: https://issues.dlang.org/show_bug.cgi?id=18206
*
* extern(C):
* alias sigfn_t = void function(int);
* alias sigfn_t2 = void function(int) nothrow @nogc;
* sigfn_t bsd_signal(int sig, sigfn_t func);
* sigfn_t2 bsd_signal(int sig, sigfn_t2 func) nothrow @nogc; // no error
*/
if (f1.fbody is null || f2.fbody is null)
return 0;

auto tf2 = cast(TypeFunction)f2.type;
error(f2.loc, "%s `%s%s` cannot be overloaded with %s`extern(%s)` function at %s",
f2.kind(),
f2.toPrettyChars(),
parametersTypeToChars(tf2.parameterList),
(f1.linkage == f2.linkage ? "another " : "").ptr,
linkageToChars(f1.linkage), f1.loc.toChars());
f2.type = Type.terror;
f2.errors = true;
// @@@DEPRECATED_2.094@@@
// Deprecated in 2020-08, make this an error in 2.104
f2.deprecation("cannot overload `extern(%s)` function at %s",
linkageToChars(f1.linkage),
f1.loc.toChars());

// Enable this when turning the deprecation into an error
// f2.type = Type.terror;
// f2.errors = true;
return 0;
}

Expand All @@ -434,7 +427,6 @@ private extern(C++) final class Semantic2Visitor : Visitor
//printf("+%s\n\ts1 = %s\n\ts2 = %s @ [%s]\n", toChars(), s1, s2, f2.loc.toChars());
if (strcmp(s1, s2) == 0)
{
auto tf2 = cast(TypeFunction)f2.type;
error(f2.loc, "%s `%s%s` conflicts with previous declaration at %s",
f2.kind(),
f2.toPrettyChars(),
Expand Down
42 changes: 42 additions & 0 deletions test/compilable/test18385.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
Reduced from the assertion failure in the glue layer when compiling DWT.
A `compilable` test because it needs codegen.

Remove this test once the deprecation for conflicting deprecations ends,
see visit(FuncDeclaration) in semantic2.d for details.

TEST_OUTPUT:
---
compilable/test18385.d(23): Deprecation: function `test18385.is_paragraph_start` cannot overload `extern(C)` function at compilable/test18385.d(22)
compilable/test18385.d(26): Deprecation: function `test18385.foo` cannot overload `extern(C)` function at compilable/test18385.d(25)
compilable/test18385.d(29): Deprecation: function `test18385.trust` cannot overload `extern(C)` function at compilable/test18385.d(28)
compilable/test18385.d(32): Deprecation: function `test18385.purity` cannot overload `extern(C)` function at compilable/test18385.d(31)
compilable/test18385.d(35): Deprecation: function `test18385.nogc` cannot overload `extern(C)` function at compilable/test18385.d(34)
compilable/test18385.d(38): Deprecation: function `test18385.nothrow_` cannot overload `extern(C)` function at compilable/test18385.d(37)
compilable/test18385.d(41): Deprecation: function `test18385.live` cannot overload `extern(C)` function at compilable/test18385.d(40)
---
*/

extern(C)
{
uint is_paragraph_start(){ return 0; }
uint is_paragraph_start(int){ return 0; }

void foo(char, bool) {}
void foo(byte, char) {}

void trust() {}
void trust() @safe {}

void purity() {}
void purity() pure {}

void nogc() {}
void nogc() @safe {}

void nothrow_() {}
void nothrow_() nothrow {}

void live() {}
void live() @live {}
}
8 changes: 3 additions & 5 deletions test/fail_compilation/fail17492.d
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
/*
https://issues.dlang.org/show_bug.cgi?id=18385
Disabled for 2.079, s.t. a deprecation cycle can be started with 2.080
DISABLED: win32 win64 osx linux freebsd dragonflybsd
REQUIRED_ARGS: -de
TEST_OUTPUT:
---
fail_compilation/fail17492.d(20): Error: class `fail17492.C.testE.I` already exists at fail17492.d(13). Perhaps in another function with the same name?
fail_compilation/fail17492.d(37): Error: struct `fail17492.S.testE.I` already exists at fail17492.d(30). Perhaps in another function with the same name?
fail_compilation/fail17492.d(20): Error: function `fail17492.C.testE()` conflicts with previous declaration at fail_compilation/fail17492.d(13)
fail_compilation/fail17492.d(37): Error: function `fail17492.S.testE()` conflicts with previous declaration at fail_compilation/fail17492.d(30)
---
https://issues.dlang.org/show_bug.cgi?id=17492
*/
Expand Down
13 changes: 7 additions & 6 deletions test/fail_compilation/fail2789.d
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
/*
DISABLED: win32 win64 osx linux freebsd dragonflybsd
https://issues.dlang.org/show_bug.cgi?id=18385
Disabled for 2.079, s.t. a deprecation cycle can be started with 2.080

TEST_OUTPUT:
---
fail_compilation/fail2789.d(15): Error: function `fail2789.A2789.m()` conflicts with previous declaration at fail_compilation/fail2789.d(10)
fail_compilation/fail2789.d(25): Error: function `fail2789.A2789.m()` conflicts with previous declaration at fail_compilation/fail2789.d(10)
---
*/
#line 7

class A2789
{
int m()
Expand All @@ -33,12 +34,12 @@ class A2789
/*
TEST_OUTPUT:
---
fail_compilation/fail2789.d(46): Error: function `fail2789.f3()` conflicts with previous declaration at fail_compilation/fail2789.d(45)
fail_compilation/fail2789.d(49): Error: function `fail2789.f4()` conflicts with previous declaration at fail_compilation/fail2789.d(48)
fail_compilation/fail2789.d(52): Error: function `fail2789.f5()` conflicts with previous declaration at fail_compilation/fail2789.d(51)
fail_compilation/fail2789.d(55): Error: function `fail2789.f6()` conflicts with previous declaration at fail_compilation/fail2789.d(54)
---
*/

void f1();
void f1() {} // ok

Expand All @@ -60,9 +61,9 @@ auto f6() { return ""; } // string(), conflict
/*
TEST_OUTPUT:
---
fail_compilation/fail2789.d(67): Error: function `fail2789.f_ExternC1()` cannot be overloaded with another `extern(C)` function at fail_compilation/fail2789.d(66)
fail_compilation/fail2789.d(70): Error: function `fail2789.f_ExternC2(int)` cannot be overloaded with another `extern(C)` function at fail_compilation/fail2789.d(69)
fail_compilation/fail2789.d(73): Error: function `fail2789.f_ExternC3()` cannot be overloaded with another `extern(C)` function at fail_compilation/fail2789.d(72)
fail_compilation/fail2789.d(67): Error: function `fail2789.f_ExternC1()` conflicts with previous declaration at fail_compilation/fail2789.d(66)
fail_compilation/fail2789.d(70): Deprecation: function `fail2789.f_ExternC2` cannot overload `extern(C)` function at fail_compilation/fail2789.d(69)
fail_compilation/fail2789.d(73): Deprecation: function `fail2789.f_ExternC3` cannot overload `extern(C)` function at fail_compilation/fail2789.d(72)
fail_compilation/fail2789.d(76): Error: function `fail2789.f_MixExtern1()` conflicts with previous declaration at fail_compilation/fail2789.d(75)
---
*/
Expand Down
7 changes: 1 addition & 6 deletions test/fail_compilation/fail47.d
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
/*
TEST_OUTPUT:
---
fail_compilation/fail47.d(13): Error: variable `fail47._foo` is aliased to a function
fail_compilation/fail47.d(13): Error: variable `fail47._foo` is aliased to a function
fail_compilation/fail47.d(13): Error: variable `fail47._foo` is aliased to a function
fail_compilation/fail47.d(18): Error: none of the overloads of `foo` are callable using argument types `(int)`, candidates are:
fail_compilation/fail47.d(12): `fail47.foo()`
fail_compilation/fail47.d(13): Error: variable `fail47._foo` is aliased to a function
fail_compilation/fail47.d(8): Error: variable `fail47._foo` is aliased to a function
---
*/
void foo() {}
Expand Down
4 changes: 2 additions & 2 deletions test/fail_compilation/fail5634.d
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/*
TEST_OUTPUT:
----
fail_compilation/fail5634.d(9): Error: only one `main`$?:windows=, `WinMain`, or `DllMain`$ allowed. Previously found `main` at fail_compilation/fail5634.d(8)
----
fail_compilation/fail5634.d(9): Error: function `D main()` conflicts with previous declaration at fail_compilation/fail5634.d(8)
---
*/

void main() { }
Expand Down
2 changes: 1 addition & 1 deletion test/fail_compilation/fail7443.d
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ class Foo
{
public static { this() {}}
public shared static { this() {}}
public {this() {}}
public {this(int) {}}
}
2 changes: 1 addition & 1 deletion test/fail_compilation/test18282.d
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ S2000 bar2()
}


void bar2()
void bar3()
{
int i;
char c;
Expand Down
31 changes: 31 additions & 0 deletions test/fail_compilation/test18385.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
REQUIRED_ARGS: -de
TEST_OUTPUT:
---
fail_compilation/test18385.d(13): Deprecation: function `test18385.foo` cannot overload `extern(C)` function at fail_compilation/test18385.d(12)
fail_compilation/test18385.d(18): Deprecation: function `test18385.S.foo` cannot overload `extern(C)` function at fail_compilation/test18385.d(17)
---
*/

extern (C):

void foo(int) { }
void foo(double) { }

struct S
{
static void foo(int) {}
static void foo(double) {}
Copy link
Contributor

@jacob-carlborg jacob-carlborg Sep 22, 2020

Choose a reason for hiding this comment

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

Please remind me why this shouldn't be allowed? These two symbols are part of a struct and therefore will get mangled as D symbols, even though the linkage is extern (C).

The changelog entry says: "Note that this change is only relevant for mangling schemes that do no support
overloading". The D mangling scheme does support overloading. Obviously the implementation is not looking at the mangled name when deciding if it should issue a deprecation message or not. Make up your minds.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed above, the current behaviour of extern(C) not changing the mangling inside of a struct is a bug because the function still adheres to the C ABI.

The current implementation could cater for that special case but I don't think that it should. Having the deprecation now will allow for a smoother transition when fixing extern(C) inside structs.

}

void foo2(int) { }
extern(D) void foo2(double) { } // OK as it has a different mangling
MoonlightSentinel marked this conversation as resolved.
Show resolved Hide resolved

void foo3(int) { }
void foo3(double); // duplicate declarations are allowed
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a duplicate declaration, this is declaring an overload.

Copy link
Contributor

Choose a reason for hiding this comment

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

(so you probably wanted to write void foo3(int); ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the rational is that this is extern(C) which would mangle to the same function.

Copy link
Contributor

Choose a reason for hiding this comment

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

argh... disregard my comments on this piece. Perhaps explicitly add extern(C) in front of each function here? Makes it easier to read the testcases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced this avoid-explicit-casts use-case is worth the ugly inconsistent AST, and having to handle incompatible declarations, incl. casts, in the backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow. This PR just tries to enable a deprecation of being more restrictive than dmd is currently. It was originally intended for 2.079, but got disabled/reverted due to people already complaining about it.
I agree that the avoid-explicit-casts use-case is a pretty bad one and I would love to deprecate more ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry I meant the test; I'd rather have it check that a compatible declaration (void foo3(int);) and later definition works, instead of testing that incompatible declarations are allowed.


void foo4();
void foo4() { }

extern(D) void foo5();
extern(D) void foo5() { }