Fix issue 20789: -de should disable is(T : U) if T alias-this to U is deprecated unless in deprecated scope

[FeepingCreature]

Don't indicate deprecated alias this or alias this of deprecated symbol as implicit conversion in is when -de (deprecations as errors) is on.

Example:

struct S {
    string foo() { return "foo"; }
    // conversion to string is deprecated, and -de is on
    // so string s = S.init would error
    deprecated alias foo this;
}

// but S is still indicated as convertible to string.
static assert(!is(S : string)); // Assert failed.

[dlang-bot]

Thanks for your pull request and interest in making D better, @FeepingCreature! 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
✓	20789	normal	is(T: U) doesn't exclude deprecated alias calls with -de

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#11081"

[FeepingCreature]

Hm, the Phobos error is genuine and an interesting problem. We're in a deprecated unittest block but we use -de...

edit: The real problem is that the implicitConvTo can't check if the scope is deprecated-safe, because it doesn't know what the current scope is.

edit: Maybe it has to return {Yes, No, Deprecated}? Ie. extend MATCH to be a bitfield with a deprecated_ flag. That's a bit of a bigger rework though. Ideas?

@thewilsonator probably best to remove the auto-merge, it's a nonstarter as written due to the painful deprecated unittest interaction.

[FeepingCreature]

@WalterBright Since it's a nontrivial amount of effort, let me give you a ping upfront regarding what you think of turning MATCH into a bitfield with deprecated_, so the downstream code can decide whether that's a valid implicit conversion at the point of use, where we know if our current scope is itself deprecated, or if you can think of a better way to resolve deprecated alias this interaction with implicit conversion checks.

[Geod24]

edit: Maybe it has to return {Yes, No, Deprecated}? Ie. extend MATCH to be a bitfield with a deprecated_ flag. That's a bit of a bigger rework though. Ideas?

I don't think that's going to work. You could potentially have multiple paths to convert from one type to the other, e.g. alias this in base class, and having this tri-state would mean that a deprecation message would be wrongly issue.
This happens for alias as well BTW, there is some code to prefer non-deprecated aliases when there's multiple path to a symbol.

[FeepingCreature]

I don't think that's going to work. You could potentially have multiple paths to convert from one type to the other, e.g. alias this in base class, and having this tri-state would mean that a deprecation message would be wrongly issue.

I don't think so? MATCH.deprecated_ here would just indicate that a conversion is impossible without hitting a deprecation, thus allowing the downstream IsExp to return false in -de+non-deprecated scope. This wouldn't lead to an error message in any case, because that code should be unaffected. The behavior of all but IsExp should be identical.

This happens for alias as well BTW, there is some code to prefer non-deprecated aliases when there's multiple path to a symbol.

Ie. if your base alias this is deprecated but you have an undeprecated alias this, your MATCH would be !deprecated_ because there's a nondeprecated path. Which matches the behavior you highlighted.

[Geod24]

The use case I have in mind is this:

class Foo
{
    int value;

    alias value this;
}

class Bar : Foo
{
    int otherValue;

    deprecated alias otherValue this;
}

static assert(is(Bar : int));

[FeepingCreature]

Yes, which would pass just fine under my proposal because is(Bar: int) would see that one of the conversions it has available does not set deprecated_.

[braddr]

Auto-merge toggled on

[braddr]

Auto-merge toggled off

[RazvanN7]

@FeepingCreature I can tell you that under no circumstance is @WalterBright going to accept changing the matching levels. We had this discussion before when trying to solve the overloading rules for alias this (this bug [1]) and I didn't manage to convince him to add a new level. I think that this PR makes a weaker case for changing the matching levels than [1].

[1] https://issues.dlang.org/show_bug.cgi?id=5363#

[FeepingCreature]

Okay, I mean, I'm open to ideas. But I can't think of another way to do it other than maybe passing Scope, or bool deprecated, all the way down the relevant type methods (which seems like a very special... case hack), or deprecating is(T: U) and just using __traits(compiles) everywhere. ... Maybe rewriting is(T: U) in terms of __traits(compiles); gag the compiler and try to semantic an implicit cast and return false if it fails, and remove all the explicit methods used for is(T: U)? But that'd probably increase compile times.

[RazvanN7]

One solution would be to alter the innards of the is operator to check if the type is convertible through alias this and if that is the case to call resolveAliasThis. Note that TypeStruct and TypeClass have implicitConvThoughAliasThisandimplicitConvWithoutAliasThis. Anyway, the is` operator code should have access to the scope, so I think that the patch should be done at that level.

From my experience, checking for alias this should be done only by calling resolveAliasThis which does all the necessary checks, however, the reality is that people (including myself) have used a ton of other methods similar to what you are doing in this patch. As a rule of thumb, alias this should always be resolved with a call to resolveAliasThis.

[FeepingCreature]

Implemented @RazvanN7 's suggestion, thanks!

edit: I didn't use resolveAliasThis because the problem is preventing the implicitConvTo check from matching in this case. So the special case (-de and no deprecated scope) is when alias this shouldn't be followed.

edit: I think the interop test failures are coincidence. The LLVM downloader server seems to have issues rn.

[RazvanN7]

Looks good to me.

[RazvanN7]

It's so annoying that TypeClass and TypeStruct don't have a common ancestor like TypeAggregate. This sort of check has to always be duplicated.

[FeepingCreature]

ping

edit: lemme rebase

[FeepingCreature]

Does DMD have too many CI tests? It seems like every second PR I make, some of the CI steps are broken...

And never because something like "master is broken" but because some other, external infrastructure collapsed. It kinda diminishes the usefulness of CI.

[thewilsonator]

DAutoTest Fails for unrelated causes. See slack.

[FeepingCreature]

Oh yeah, I'm on slack.

edit: I'm aware it fails for unrelated causes. That's kind of the problem...

edit: rebased