https://issues.dlang.org/show_bug.cgi?id=21065

Mathias LANG <pro.mathias.lang@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pro.mathias.lang@gmail.com

--- Comment #1 from Mathias LANG <pro.mathias.lang@gmail.com> ---
Regarding your proposed implementation:

> T __makeClassObject(T, A...)(auto ref A);

This would have the effect of producing a potentially large amount of template instances, as the `auto ref` variadic will match the parameters *exactly* - something we don't really want, e.g. when instantiating `class Foo { this (int) {} }` and passing `short`.

Additionally, with this change the implicit conversions don't work anymore (e.g. passing `ulong.init` will give it a `ulong` which will error when attempting to implicitly convert it to `int`).

What I found to be working very well when doing this kind of forwarding was to instead *generate* an overload set and let the compiler do the heavy lifting for me. In this case, the definition would become:

> template __makeClassObject (T)

And have multiple `__makeClassObject (args)` functions defined in it, each one accepting exactly the parameter of one constructor definition.

Alternatively, just leave the initialization where it currently is (the compiler) and just templatize `newclass` to sidestep this issue.

--

https://issues.dlang.org/show_bug.cgi?id=21065

--- Comment #2 from Andrei Alexandrescu <andrei@erdani.com> ---
(In reply to Mathias LANG from comment #1)
> Regarding your proposed implementation:
> 
> > T __makeClassObject(T, A...)(auto ref A);
> 
> This would have the effect of producing a potentially large amount of template instances, as the `auto ref` variadic will match the parameters *exactly* - something we don't really want, e.g. when instantiating `class Foo { this (int) {} }` and passing `short`.

Not to worry about that at all - the instantiations are limited by the number of calls to new in source code, which is a small fraction of the code in the project. (In the Pool pull request, __FILE__ and __LINE__ are part of the instantiation so literally there's one instantiation per use.) So we're looking at hundreds to thousands. By comparison recursive templates are liable to generate orders of magnitude more.

> Additionally, with this change the implicit conversions don't work anymore (e.g. passing `ulong.init` will give it a `ulong` which will error when attempting to implicitly convert it to `int`).

Hmmm, this would be a problem. Can you please explain a bit?

> What I found to be working very well when doing this kind of forwarding was to instead *generate* an overload set and let the compiler do the heavy lifting for me. In this case, the definition would become:
> 
> > template __makeClassObject (T)
> 
> And have multiple `__makeClassObject (args)` functions defined in it, each one accepting exactly the parameter of one constructor definition.
>
> Alternatively, just leave the initialization where it currently is (the compiler) and just templatize `newclass` to sidestep this issue.

As a matter of principle, the more compiler magic we need the more problems with the language we're sweeping under the rug. Both of these seem to be in that category.

All high level constructs should lower to a small core language of data structures and functions.

--

https://issues.dlang.org/show_bug.cgi?id=21065

--- Comment #3 from Mathias LANG <pro.mathias.lang@gmail.com> ---
> the instantiations are limited by the number of calls to new in source code, which is a small fraction of the code in the project

That is a good point, however this report is about the downside of runtime solution disadvantage. We should also consider the disadvantages that come with a compile time solution: code duplication leading to bigger binaries, slowing down compile time, potential cache thrashing due to the increased amount of instructions. However, as explained after, those are not even the primary concern with a variadic approach.

> Hmmm, this would be a problem. Can you please explain a bit?

Consider the following code:
```
class Klass { this (int value) {} }
void main ()
{
    scope c = new Klass(ulong.init);
}
```

This currently compiles just fine.
A naive implementation of `makeClassObject` would be (per your OP):
```
class Klass { this (int value) {} }
void main ()
{
    auto c = makeClassObject!Klass(ulong.init);
}
T makeClassObject (T, Args...) (auto ref Args args)
{
    return new T(args);
}
```

Which would give you:
```
foo.d(8): Error: constructor foo.Klass.this(int value) is not callable using
argument types (ulong)
foo.d(8):        cannot pass argument _param_0 of type ulong to parameter int
value
foo.d(4): Error: template instance foo.makeClassObject!(Klass, ulong) error
instantiating
```

This is because the function parameters are disconnected from their usage. We let the compiler infer the template arguments based on what is given to the function, then try to pass them to an overload set.

Another example that will fail is:
```
class Klass
{
    this (int value) {}
    this (ref int value) { assert(0); }
}
void main ()
{
    // scope c = new Klass(0);
    auto c = makeClassObject!Klass(0); // rvalue promoted to lvalue
}
```

Currently, this does not trigger the `assert`, but with the signature you suggested, it does.

> As a matter of principle, the more compiler magic we need the more problems with the language we're sweeping under the rug. Both of these seem to be in that category.

There is nothing magic about the suggestion though, it relies on a very basic principle: forwarding should use the same set of constraints.

Using variadics + auto ref as a way to provide a proxy for an overload set is a common, yet fundamentally flawed approach, because it disables some of the machinery in place for overload resolution (e.g. implicit conversion). This set of rules the compiler applies when doing a call is well understood by developers and very intuitive. But by using variadics, the proxy provides the loosest of filter for arguments, then tries to fit those into a tighter filter.

Trying to fix this issue while retaining the variadics + auto ref approach would require re-engineering some of the compiler machinery. In the case of the `ulong.init`, a runtime switch will do it, while for `ref` one needs to inspect the type of parameter to call the right function. There might be cases it's not even possible (for example, mutable => immutable conversion when the argument is a call to a strongly pure function).

This is an issue which can be seen in `Algebraic`, where it doesn't let you pass a `const(int)` to an Algebraic accepting `int` (I mentioned it here: https://forum.dlang.org/post/wvhlrgbtdbrvaubflabu@forum.dlang.org). Had it generated an overload set based on the allowed types, this issue would be gone.

A basic PoC for `makeClassObject` would be this implementation:
```
import std.stdio;

class Klass
{
    this (int value) { writeln("rvalue"); }
    this (ref int value) { writeln("lvalue"); }
}
void main ()
{
    auto c = makeClassObject!Klass(0);
    int lvalue;
    auto d = makeClassObject!Klass(lvalue);
    auto e = makeClassObject!Klass(ulong.min);
}

template makeClassObject (T)
{
    import std.traits;
    static foreach (ovrld; __traits(getOverloads, T, "__ctor"))
    {
        T makeClassObject (Parameters!ovrld args)
        {
            scope ctor = &ovrld; // Need this to avoid promoting rvalues to
lvalues
            return ctor(args);
        }
    }
}
```

This *might* have issues with some storage classes, are those are usually finicky to work with, but it will avoid most of the common (if not trivial) issues an unbound variadic + auto ref templates will trigger.



TL;DR: If you want to forward to an overload set, do it by exposing an overload set.

--

https://issues.dlang.org/show_bug.cgi?id=21065

--- Comment #4 from Andrei Alexandrescu <andrei@erdani.com> ---
(In reply to Mathias LANG from comment #3)
> TL;DR: If you want to forward to an overload set, do it by exposing an overload set.

Cool, I'm convinced. Overload set inside a template using the eponymous trick is the way to go. Thanks!

--

https://issues.dlang.org/show_bug.cgi?id=21065

Iain Buclaw <ibuclaw@gdcproject.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P1                          |P4

--

https://issues.dlang.org/show_bug.cgi?id=21065

--- Comment #5 from dlangBugzillaToGithub <robert.schadek@posteo.de> ---
THIS ISSUE HAS BEEN MOVED TO GITHUB

https://github.com/dlang/dmd/issues/19749

DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB

--