Fix duplicate function detection for overloads introduced by aliases

[MoonlightSentinel]

The previous implementation compared small parts of the function mangling
to detect duplicate functions. This approach had several flaws manifesting
in the referenced issue as well as other false positives for (static)
members, `alias`ing into different scopes, ... .

Issue 21505 was caused by the assumption that all functions visited by
`overloadApply` have the same name. This doesn't hold for overloads
introduced by an `alias`.

A reliable mangling-based implementation would need to compare most of
the mangled name while also omitting severel aspects e.g. the return type
to detect `int foo()` and `void foo()`.

Hence the current implementation was replaced by actually comparing the
`FuncDeclaration`s because it allows for more fine grained checks (and
also should save some memory allocations).

---

Added the intial report as a bugzilla reference

[dlang-bot]

Thanks for your pull request and interest in making D better, @MoonlightSentinel! 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

Auto-close	Bugzilla	Severity	Description
✓	21505	regression	Function alias reported as conflicting function

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 "stable + dmd#12053"

[PetarKirov]

Yes, extern(D) preserves the mangling, but does overload resolution work well, or the user has to resort to hackery (e.g. externDFunc to traits)? If the answer is no, I think it's not a bad idea to deprecate such form of likely accidental overloading.

[MoonlightSentinel]

Using __traits(getOverloads, ...) should also work IIRC.
But thats out of scope for this PR.

EDIT: It does work e.g. for opApply IIRC

[RazvanN7]

An alternative fix would be to disallow inserting an overload in an overload list via alias this if the inserted function might conflict with any of the other overloads. Inserting a conflicting overload can never be useful.

[MoonlightSentinel]

Switchted to comparing the FuncDeclarations instead of comparing the mangling because the latter doesn't work for several cases aside from the initial bug report (unless it compares the entire mangling)

Some examples:

• the bug report
• static member funtions
• alias introducing overloads into different scopes (e.g. free function inserted into an aggregate)

EDIT: Will squash & adapt the commit message once it is ready.

[MoonlightSentinel]

@RazvanN7 You can still call the function using __traits(getOverloads):

extern (C) int puts(scope const char*) @trusted @nogc;

void func() @safe { puts("@safe"); }
void func() @nogc { puts("@nogc"); }

void main() @nogc
{
    __traits(getOverloads, M, "func")[1](); // Writes "@nogc"
}

But I can also defer those changes into a latter PR and retain the current behaviour for now.

[WalterBright]

With this behavior change, how does it affect the Spec?

[MoonlightSentinel]

The spec doesn't mention conflicting functions IIRC. That's why the proposed implementation checks only for functions that cause issues in the backend (function merging for dmd/errors for ldc/gdc).

[MoonlightSentinel]

Rebased & squashed the commits

[dkorpel]

I don't know why, but this change introduced a lot of errors in the form of address of variable x assigned to y with longer lifetime in my code. I'm working on a reduced test case.

[dkorpel]

@safe:

struct I {int* x;}
pure I gcd(I, I) {return I();}

auto foo(I func) {
	if (R n = func)
		R r = n.foo!()(n); // Error: address of variable n assigned to r with longer lifetime`
}

struct R {
@safe:
	I d;
	this(I) {}
	bool opCast() {return true;}

	R foo()(R) out {check();} do {
		gcd(d, d);
		return this;
	}

	bool check() {return true;}
}

When compiled with -dip1000, it says Error: address of variable n assigned to r with longer lifetime.
When reverting this commit, it doesn't happen. I don't know how this PR changed this, but it seems to be a side-effect: small changes to the snippet make or break the new / old compiler version. I don't even know if this is accepts-invalid or rejects-valid.

[MoonlightSentinel]

The DIP1000 implementation is very brittle, so I guess this error is probably depending on the order of semantic analysis.
I guess this PR affects your code because the parameter checks causes an earlier expansion & analysis of the function parameters.