Fix 20008 - __traits(allMembers) of packages is complete nonsense

[dkorpel]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! 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
✓	20008	normal	__traits(allMembers) of packages is complete nonsense

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

[RazvanN7]

This is silently changing the behavior of __traits(allMembers) on imports. Not sure what to do about that. Also, couldn't pkg be null?

[dkorpel]

This is silently changing the behavior of __traits(allMembers) on imports.

I sure hope nobody was relying on the nonsensical behavior of __traits(allMembers, pkg) turning into __traits(allMembers, pkg.submod), but you never know.

Also, couldn't pkg be null?

Not sure, it gets set when the module is loaded. There's this piece of code in dmd.expressionsem.symbolToExp which I based this PR's change on:

    if (Import imp = s.isImport())
    {
        if (!imp.pkg)
        {
            .error(loc, "forward reference of import `%s`", imp.toChars());
            return ErrorExp.get();
        }
        auto ie = new ScopeExp(loc, imp.pkg);
        return ie.expressionSemantic(sc);
    }

But there's no test case for that error. I added a check to be safe.

[dkorpel]

@RazvanN7 To clarify, the if (auto imp = s.isImport()) case is only taken when the argument to __traits(allMembers) is a single identifier, like core. As soon as there is a . in it, like core.sys or core.sys.linux, it resolves to a Package or Module instead of an Import. I added a test case for package modules two folders deep to ensure that works correctly as well.

Now core does not have a package module, so it's not affected by issue 20008. core.thread has a package module, but it has a ., so it was correctly resolving to a Package already. However, std has a package module and no ., so the semanticTraits function received it as whatever your first import of that package was. So if my module starts with import std.stdio, __traits(allMembers, std) will resolve its std argument as import std.stdio. This is what triggers the issue, and the only case this PR affects.

I have no idea why it works like that, but symbolToExp accounts for it, so I'll roll with it for now.

[RazvanN7]

I think that this happened because std.stdio is actually the member of your module, not std. Looking at the tests, I don't think that for this code, allMembers exhibits the correct behavior:

module pkg20008;

import pkg20008.submod;

enum T2 { y }
enum T3 { z }

static assert([__traits(allMembers, pkg20008)] == ["object", "pkg20008", "T2", "T3"]);

pkg20008 is not a member of pkg2008, rather pkg20008.submod` is.

o if my module starts with import std.stdio, __traits(allMembers, std) will resolve its std argument as import std.stdio. This is what triggers the issue, and the only case this PR affects.

Arguably, std in its entirety should not be avaible if you are simply importing std.stdio. You should be required to import std to be available to reflect on it.

[dkorpel]

I think that this happened because std.stdio is actually the member of your module, not std.

It also happens when you import std right afterwards.

Looking at the tests, I don't think that for this code, allMembers exhibits the correct behavior:

True, but this PR doesn't touch the generation of members for a module.

Arguably, std in its entirety should not be avaible if you are simply importing std.stdio. You should be required to import std to be available to reflect on it.

That's not how any other trait works.