Disallow public introduction of symbols by mixins

[rikkimax]

Both string mixins and mixin templates should not be allowed to introduce symbols.

A good example of why this should be the case:

    mixin(bitfields!(
        uint, "x",    2,
        int,  "y",    3,
        uint, "z",    2,
        bool, "flag", 1));

Tooling cannot see the symbols x, y, z, or flag.

We should not be recommending this for a user experience.

EDIT: To clarify to introduce means to create new symbols that do not already exist but may not be implemented.

interface I {
    void method();
}

class C : I {
    mixin("void method() { }");
}

This is allowed because method is introduced by the interface, but the class string mixin implements it. Tooling can still see the method. Just not its implementation.

[LightBender]

I agree with this, and we should also approve this DIP: https://forum.dlang.org/thread/[email protected]

Approving this DIP obsoletes this whole concept and we can just never port this to Phobos 3.

[rikkimax]

String mixins means that you need CTFE, so that's pretty much a no-go.

For conditional compilation, you can match against all branches, so that is possible, but it does all need to be verified.

[jmdavis]

I would expect any tool that needs to properly determine which symbols exist would need to actually do some level of compilation and that that would include doing CTFE. If it doesn't, it has no way of determining what any conditional compilation is actually doing. I don't see how such a tool could work in a reliable way if it doesn't do at least partial compilation. Maybe it could be "good enough" for many cases, but it's going to miss stuff if it doesn't actually compile code.

[rikkimax]

It works reliably enough as long as you don't go introducing symbols with string mixins.

It is simply too far outside of our IDE tooling capability.

Conditionally compiled code I've found to be ok-ish, it can usually see what you need it to.
But it won't see the exact symbol for a given target.

[jmdavis]

Well, it sounds like the tooling needs to be improved then, but it's not common that we need to introduce public symbols via string mixins, so it shouldn't be a problem to minimize that particular form of conditional compilation.

That being said, I don't know if we want to actually kill std.bitmanip.bitfields given that it's looking like we're not going to get bitfields in the language where you actually have a guaranteed layout across compilers and architectures, meaning that some use cases will need something like std.bitmanip.bitfields (and Walter has been saying that those use cases should just use the Phobos solution). So, we're probably going to have to decide whether those use cases matter enough to include std.bitmanip.bitfields or whether they're rare enough that it's reasonable to drop it. But given that some folks keep arguing with Walter to try to get him to make the built-in bitfields not be C-compatible, they're either just doing it on principle (in which case, it doesn't really matter in practice), or the use cases where we'd need a guaranteed bitfield layout are common enough that we should include some form of std.bitmanip.bitfields in Phobos v3. At that point, code that used bitfields but didn't need a guaranteed layout could use the language solution, and code like serializers could use the Phobos one, in which case, it wouldn't be a common problem for folks using tools like DCD.

So, we may ultimately want bitfields in Phobos v3 even if it doesn't work well with tooling, but even if we do, the principle does stand that it's less desirable to introduce symbols via string mixins (even if the tooling handles it well, it's often harder to follow what the code is doing), so we probably should restrict them to the cases where they really are the best solution, which should be rare.

So, I'm totally fine with saying that we should prefer to not introduce symbols via string mixins and that they should be restricted to the cases where they're clearly the solution that makes the most sense, but I don't agree that we should ban them outright. If they're clearly the best solution to a particular problem, then we should be able to use them. They just should only be used if they're clearly the best option, which should be rare.

[rikkimax]

If we can prove that it is the only good way to do it, and no language addition would be a good solution then yes I agree.

The bar can be high enough that it is a practical ban, without actually being a ban.

[atilaneves]

If it's about the tooling, then I think the tooling should be improved.

[rikkimax]

Timon is working on tuples, even if it is taking a while to go into the language so we can check that one off as not being brought over.

AutoImplement does not introduce symbols, it implements them, so that isn't an issue for tooling, even if we'd like to have the location of the implement also IDE aware.

wrap yes that one has no good alternatives and because you can specify the type you want at the end, that has a workaround. That I have no problem with.

Now imported is an interesting case, as it is itself a symbol which tooling can see, but it can't see into it. So it is very much a case of it depends upon how it is used.

[rikkimax]

doesn't exist for DCD based IDE's currently
Incorrect.

Both DCD and dscanner do not support taking in those arguments.

https://github.com/dlang-community/DCD/blob/master/src/dcd/client/client.d#L86

Only -I, no targets, no versions nothing for this.

https://github.com/dlang-community/D-Scanner/blob/master/src/dscanner/main.d#L87

A target must be provided
I don't see how that's necessary, the tooling should compile the module in question.

If you do not have the target you do not have the ability to perform the lookups wrt. conditional compilation.

We are not rewriting IDE plugins to support this capability even with a rewritten DCD/dscanner this decade. It is going to take a lot longer than that to get us there.

[pbackus]

AutoImplement does not introduce symbols, it implements them

Here is the mixin that introduces public symbols in AutoImplement: https://github.com/dlang/phobos/blob/v2.108.0/std/typecons.d#L5552

[rikkimax]

We may be using different definitions because I have read it and don't see an issue with it.

Introduction here means a symbol (such as a method in a class) that was not previously declared by a superclass or interface.

Implementation is that string mixin on line 5552, BUT it's wrapped in the class AutoImplement which inherits from the base which tooling can see both it and its inheritance structure so it isn't an issue even with incomplete tooling.

[pbackus]

Understood; thanks for clarifying.