www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - What's the deal with the massive duplication in std.meta?

reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
cc Walter, Atila

Phobos is like a toddler: you turn away for a few moments, and then back 
and something really odd has happened.

The massive duplication of declarations in Phobos is definitely 
unacceptable. That is, literally unacceptable - the pull requests 
introducing it should not have been accepted, and the issue should have 
been elevated as a major language issue.

I protested this before and the matter has been discussed maybe about 
two years ago but nothing has been done about it.

It all starts simple, like a toddler babble:

```D
alias Alias(alias a) = a;
alias Alias(T) = T;
```

The reader (including seasoned maintainer) would go like, "What? Why 
does it need to be defined twice? Wouldn't a type also fit an alias?" 
And indeed if said maintainer comments out the second declaration, a 
couple of obscure errors show up in unittesting (mind you, not 
unittesting of std.meta, but of completely unrelated modules).

Bah. Passons. Let's read further down (and skip the weird OldAlias 
thing, well it has package protection so it qualifies as dirty laundry), 
where more harbingers of bad news show up:

```D
template staticIndexOf(T, TList...)
{
     enum staticIndexOf = genericIndexOf!(T, TList);
}

template staticIndexOf(alias T, TList...)
{
     enum staticIndexOf = genericIndexOf!(T, TList);
}
```

At this point clearly there's a huge red flag waving because this is a 
public symbol, and if this duplication pop up at top level, it's a sign 
that many other public symbols will suffer the same fate. (Spoilers: 
they do. They all do.)


This kind of code is clearly, absolutely, pound-on-the-table, 
end-of-discussion not acceptable at scale. If bad code has smell, this 
is a rotten beached blue whale carcass.


This code should have NEVER been accepted upon review. Instead, the 
reviewers should have filed a TOP PRIORITY bug report to dmd "Not 
binding types to alias templates forces unscalable code duplication for 
all typelist primitives".

This needs to be fixed, and very urgently.
Apr 30 2021
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 4/30/21 6:03 PM, Andrei Alexandrescu wrote:
 The massive duplication of declarations in Phobos is definitely 
 unacceptable.
s/Phobos/std.meta/
Apr 30 2021
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 4/30/21 6:05 PM, Andrei Alexandrescu wrote:
 On 4/30/21 6:03 PM, Andrei Alexandrescu wrote:
 The massive duplication of declarations in Phobos is definitely 
 unacceptable.
s/Phobos/std.meta/
Workaround: https://github.com/dlang/phobos/pull/8024
Apr 30 2021
parent Andrei Alexandrescu <andrei erdani.com> writes:
On Saturday, 1 May 2021 at 01:06:24 UTC, Andrei Alexandrescu 
wrote:
 On 4/30/21 6:05 PM, Andrei Alexandrescu wrote:
 On 4/30/21 6:03 PM, Andrei Alexandrescu wrote:
 The massive duplication of declarations in Phobos is 
 definitely unacceptable.
s/Phobos/std.meta/
Workaround: https://github.com/dlang/phobos/pull/8024
More related: https://github.com/dlang/phobos/pull/8026
May 01 2021
prev sibling next sibling parent reply Imperatorn <johan_forsberg_86 hotmail.com> writes:
On Friday, 30 April 2021 at 22:03:41 UTC, Andrei Alexandrescu 
wrote:
 cc Walter, Atila

 Phobos is like a toddler: you turn away for a few moments, and 
 then back and something really odd has happened.

 [...]
Ouch, didn't know about that.. A question though: When we have fixed this, how do we make sure it doesn't keep happening? I'm guessing more code review, but that's after the problem has already materialized. Maybe better coding guidelines? Written by the high priest of D. I would be very happy if we could produce some, and I would gladly read and follow them.
Apr 30 2021
parent Berni44 <someone somemail.com> writes:
On Saturday, 1 May 2021 at 06:38:51 UTC, Imperatorn wrote:
 Maybe better coding guidelines? Written by the high priest of D.

 I would be very happy if we could produce some, and I would 
 gladly read and follow them.
+1 (Though I don't care who writes them as long as they are accepted by DLF.)
May 01 2021
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 4/30/2021 3:03 PM, Andrei Alexandrescu wrote:
 ```D
 template staticIndexOf(T, TList...)
 {
     enum staticIndexOf = genericIndexOf!(T, TList);
 }
 
 template staticIndexOf(alias T, TList...)
 {
     enum staticIndexOf = genericIndexOf!(T, TList);
 }
 ```
I'm curious why genericIndexOf is not renamed to staticIndexOf and the previous staticIndexOf templates removed.
 This code should have NEVER been accepted upon review. Instead, the reviewers 
 should have filed a TOP PRIORITY bug report to dmd "Not binding types to alias 
 templates forces unscalable code duplication for all typelist primitives".
 
 This needs to be fixed, and very urgently.
Please file a bug report!
May 01 2021
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 5/1/21 4:08 AM, Walter Bright wrote:
 On 4/30/2021 3:03 PM, Andrei Alexandrescu wrote:
 ```D
 template staticIndexOf(T, TList...)
 {
     enum staticIndexOf = genericIndexOf!(T, TList);
 }

 template staticIndexOf(alias T, TList...)
 {
     enum staticIndexOf = genericIndexOf!(T, TList);
 }
 ```
I'm curious why genericIndexOf is not renamed to staticIndexOf and the previous staticIndexOf templates removed.
I don't know. I speculate that the following sequence of events happened: 1. Someone writes `staticIndexOf` "reasonably" with types (could have been aliases, but I recall the first historical uses were with types): /** ... docs ... */ template staticIndexOf(T, TList...) { ... } 2. Someone else figures staticIndexOf should also work with values. They try to add "alias" to `T` in there but unittests fail due to bugs and undue limitations in the compiler. They figure the right solution is to add an "overload": /** ... docs ... */ template staticIndexOf(T, TList...) { ... } /// ditto template staticIndexOf(alias T, TList...) { ... } This does not upset the names in the documentation (they are still `T` and `TList`) so it's an entirely transparent and reasonable way to work around said bugs and undue limitations. That contributor might have thought that there may be subtle reasons for which `alias` parameters don't accept type arguments transparently, so they perhaps chose not to protest it all that much. The reviewer of the code might have accepted the same reasoning with a sigh. 3. Someone else (or the same contributor in the same PR) figures there's a way to eliminate the duplication in code: /** ... docs ... */ template staticIndexOf(T, TList...) { enum staticIndexOf = genericIndexOf!(T, TList); } /// ditto template staticIndexOf(alias T, TList...) { enum staticIndexOf = genericIndexOf!(T, TList); } // private private template genericIndexOf(args...) { ... } This does not upset the documentation and remains transparent to the user, so it seems like a reasonable thing to do. 4. Other people (or, again, the same contributor in that or other PRs) figured the same pattern can be applied to other artifacts as well. And reviews accept that code because otherwise it's impossible to make the unittests pass. 5. Now the pattern is an accepted way of doing things whenever an artifact must work with both types and aliases. The concern that the pattern scales poorly and indicates a massive problem with the language was not raised; after all, we had working code using the pattern. This has happened before, and will happen again unless we do something about it.
 This code should have NEVER been accepted upon review. Instead, the 
 reviewers should have filed a TOP PRIORITY bug report to dmd "Not 
 binding types to alias templates forces unscalable code duplication 
 for all typelist primitives".

 This needs to be fixed, and very urgently.
Please file a bug report!
Apparently Nick Treleaven proposed a solution or a part of it: https://github.com/dlang/dmd/pull/11320 The bug report related to the process is more subtle and does not belong in bugzilla.
May 02 2021
next sibling parent reply Herringway <lookitupyourself example.com> writes:
On Sunday, 2 May 2021 at 12:21:53 UTC, Andrei Alexandrescu wrote:
 On 5/1/21 4:08 AM, Walter Bright wrote:
 On 4/30/2021 3:03 PM, Andrei Alexandrescu wrote:
 This code should have NEVER been accepted upon review. 
 Instead, the reviewers should have filed a TOP PRIORITY bug 
 report to dmd "Not binding types to alias templates forces 
 unscalable code duplication for all typelist primitives".

 This needs to be fixed, and very urgently.
Please file a bug report!
Here's the proposed patch and its review. I believe you may be able to understand why the reviewer didn't do those things. https://issues.dlang.org/show_bug.cgi?id=2996
May 02 2021
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 5/2/2021 8:07 AM, Herringway wrote:
 On Sunday, 2 May 2021 at 12:21:53 UTC, Andrei Alexandrescu wrote:
 On 5/1/21 4:08 AM, Walter Bright wrote:
 On 4/30/2021 3:03 PM, Andrei Alexandrescu wrote:
 This code should have NEVER been accepted upon review. Instead, the 
 reviewers should have filed a TOP PRIORITY bug report to dmd "Not binding 
 types to alias templates forces unscalable code duplication for all typelist 
 primitives".

 This needs to be fixed, and very urgently.
Please file a bug report!
Here's the proposed patch and its review. I believe you may be able to understand why the reviewer didn't do those things. https://issues.dlang.org/show_bug.cgi?id=2996
Ah, thank you!
May 02 2021
parent reply Andrei Alexandrescu <andrei erdani.com> writes:
On Sunday, 2 May 2021 at 18:18:43 UTC, Walter Bright wrote:
 On 5/2/2021 8:07 AM, Herringway wrote:
 On Sunday, 2 May 2021 at 12:21:53 UTC, Andrei Alexandrescu 
 wrote:
 On 5/1/21 4:08 AM, Walter Bright wrote:
 On 4/30/2021 3:03 PM, Andrei Alexandrescu wrote:
 This code should have NEVER been accepted upon review. 
 Instead, the reviewers should have filed a TOP PRIORITY bug 
 report to dmd "Not binding types to alias templates forces 
 unscalable code duplication for all typelist primitives".

 This needs to be fixed, and very urgently.
Please file a bug report!
Here's the proposed patch and its review. I believe you may be able to understand why the reviewer didn't do those things. https://issues.dlang.org/show_bug.cgi?id=2996
Ah, thank you!
I also submitted this: https://issues.dlang.org/show_bug.cgi?id=21889 It is related but not identical.
May 03 2021
parent reply Andrei Alexandrescu <andrei erdani.com> writes:
I put together a bunch other related pull requests:

https://github.com/dlang/phobos/pulls/andralex

Using alias assignment with good measure leads to much simpler 
and clearer code. I also measured memory consumption 
improvements, but for the Phobos unittest they are not dramatic.
May 04 2021
parent reply Q. Schroll <qs.il.paperinik gmail.com> writes:
On Tuesday, 4 May 2021 at 16:54:13 UTC, Andrei Alexandrescu wrote:
 I put together a bunch other related pull requests:

 https://github.com/dlang/phobos/pulls/andralex

 Using alias assignment with good measure leads to much simpler 
 and clearer code. I also measured memory consumption 
 improvements, but for the Phobos unittest they are not dramatic.
Is there some resource where the rules for alias assignments are spelled out? Googling '"alias assignment" site:dlang.org' produces no meaningful results. Having procedural aspects in a declarative space looks a bit odd to me. The change logs revealed nothing.
May 04 2021
parent reply Paul Backus <snarwin gmail.com> writes:
On Tuesday, 4 May 2021 at 17:39:54 UTC, Q. Schroll wrote:
 On Tuesday, 4 May 2021 at 16:54:13 UTC, Andrei Alexandrescu 
 wrote:
 I put together a bunch other related pull requests:

 https://github.com/dlang/phobos/pulls/andralex

 Using alias assignment with good measure leads to much simpler 
 and clearer code. I also measured memory consumption 
 improvements, but for the Phobos unittest they are not 
 dramatic.
Is there some resource where the rules for alias assignments are spelled out? Googling '"alias assignment" site:dlang.org' produces no meaningful results. Having procedural aspects in a declarative space looks a bit odd to me. The change logs revealed nothing.
There's an open spec PR: https://github.com/dlang/dlang.org/pull/2919
May 04 2021
parent reply Q. Schroll <qs.il.paperinik gmail.com> writes:
On Tuesday, 4 May 2021 at 17:45:53 UTC, Paul Backus wrote:
 On Tuesday, 4 May 2021 at 17:39:54 UTC, Q. Schroll wrote:
 On Tuesday, 4 May 2021 at 16:54:13 UTC, Andrei Alexandrescu 
 wrote:
 I put together a bunch other related pull requests:

 https://github.com/dlang/phobos/pulls/andralex

 Using alias assignment with good measure leads to much 
 simpler and clearer code. I also measured memory consumption 
 improvements, but for the Phobos unittest they are not 
 dramatic.
Is there some resource where the rules for alias assignments are spelled out? Googling '"alias assignment" site:dlang.org' produces no meaningful results. Having procedural aspects in a declarative space looks a bit odd to me. The change logs revealed nothing.
There's an open spec PR: https://github.com/dlang/dlang.org/pull/2919
How does something like this not require a DIP?
May 04 2021
next sibling parent Q. Schroll <qs.il.paperinik gmail.com> writes:
On Tuesday, 4 May 2021 at 18:19:10 UTC, Q. Schroll wrote:
 On Tuesday, 4 May 2021 at 17:45:53 UTC, Paul Backus wrote:
 On Tuesday, 4 May 2021 at 17:39:54 UTC, Q. Schroll wrote:
 On Tuesday, 4 May 2021 at 16:54:13 UTC, Andrei Alexandrescu 
 wrote:
 I put together a bunch other related pull requests:

 https://github.com/dlang/phobos/pulls/andralex

 Using alias assignment with good measure leads to much 
 simpler and clearer code. I also measured memory consumption 
 improvements, but for the Phobos unittest they are not 
 dramatic.
Is there some resource where the rules for alias assignments are spelled out? Googling '"alias assignment" site:dlang.org' produces no meaningful results. Having procedural aspects in a declarative space looks a bit odd to me. The change logs revealed nothing.
There's an open spec PR: https://github.com/dlang/dlang.org/pull/2919
How does something like this not require a DIP?
For example, `staticMap` can be implemented iteratively like this: ```D alias staticMap(alias f, args...) = mixin(() { string result = "AliasSeq!("; static foreach (i; 0 .. args.length) result ~= mixin(`"f!(args[`, i, `])"`); return result ~ ")"; }()); ``` I agree it leads to simpler code. I just don't know if it's worth it.
May 04 2021
prev sibling parent Paul Backus <snarwin gmail.com> writes:
On Tuesday, 4 May 2021 at 18:19:10 UTC, Q. Schroll wrote:
 On Tuesday, 4 May 2021 at 17:45:53 UTC, Paul Backus wrote:
 There's an open spec PR:

 https://github.com/dlang/dlang.org/pull/2919
How does something like this not require a DIP?
Beats me. Seems like at the very least it should have a `-preview` switch. I suspect the actual answer is "it's Walter's PR, and Walter can get away with ignoring the official process." If you click through to the DMD PR you can see some discussion about this.
May 04 2021
prev sibling parent Petar Kirov [ZombineDev] <petar.p.kirov gmail.com> writes:
On Sunday, 2 May 2021 at 12:21:53 UTC, Andrei Alexandrescu wrote:
 [..]
 I don't know. I speculate that the following sequence of events 
 happened:
 [..]
Don't speculate, use git instead :P * https://github.com/dlang/phobos/commit/bcfdd75970977e7dd4819c32497e3d74be4a263b * https://github.com/dlang/phobos/commit/ff6acb6e3ea5b0527d5a91fcc04c5ae885f3e4ae * https://github.com/dlang/phobos/commit/0c142994d9b5cb9f379eca28f3a625c749370e4a mentions the 2.032 changelog: * https://dlang.org/changelog/2.032, which under "Bugs Fixed" lists issue 2996: * https://issues.dlang.org/show_bug.cgi?id=2996
May 02 2021
prev sibling next sibling parent reply Nick Treleaven <nick geany.org> writes:
On Friday, 30 April 2021 at 22:03:41 UTC, Andrei Alexandrescu 
wrote:
 the reviewers should have filed a TOP PRIORITY bug report to 
 dmd "Not binding types to alias templates forces unscalable 
 code duplication for all typelist primitives".
I think that did get fixed. At the time, I tried to remove the overloads in std.meta, but ran into another bug: https://github.com/dlang/phobos/pull/7513#issuecomment-638946029 I made a dmd pull to fix that bug, but I wasn't too confident about its correctness, and the fix didn't get merged.
May 01 2021
parent reply Nick Treleaven <nick geany.org> writes:
On Saturday, 1 May 2021 at 10:23:26 UTC, Nick Treleaven wrote:
 I made a dmd pull to fix that bug, but I wasn't too confident 
 about its correctness, and the fix didn't get merged.
https://github.com/dlang/dmd/pull/11320
May 01 2021
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 5/1/21 6:26 AM, Nick Treleaven wrote:
 On Saturday, 1 May 2021 at 10:23:26 UTC, Nick Treleaven wrote:
 I made a dmd pull to fix that bug, but I wasn't too confident about 
 its correctness, and the fix didn't get merged.
https://github.com/dlang/dmd/pull/11320
Thanks! This is important work. I see Iain mentioned it breaks existing behavior. What is the policy for breaking broken behavior? We shouldn't deprecate it, but there should be a compatibility switch or something. Walter, what's the plan here?
May 02 2021
parent Walter Bright <newshound2 digitalmars.com> writes:
On 5/2/2021 4:55 AM, Andrei Alexandrescu wrote:
 On 5/1/21 6:26 AM, Nick Treleaven wrote:
 On Saturday, 1 May 2021 at 10:23:26 UTC, Nick Treleaven wrote:
 I made a dmd pull to fix that bug, but I wasn't too confident about its 
 correctness, and the fix didn't get merged.
https://github.com/dlang/dmd/pull/11320
Thanks! This is important work. I see Iain mentioned it breaks existing behavior. What is the policy for breaking broken behavior? We shouldn't deprecate it, but there should be a compatibility switch or something. Walter, what's the plan here?
It's definitely a bug and needs fixing. The idea is to correctly diagnose any code that breaks, suggesting the user fix it or use the revert switch.
May 02 2021
prev sibling parent Steven Schveighoffer <schveiguy gmail.com> writes:
On 4/30/21 6:03 PM, Andrei Alexandrescu wrote:

 It all starts simple, like a toddler babble:
 
 ```D
 alias Alias(alias a) = a;
 alias Alias(T) = T;
 ```
 
 The reader (including seasoned maintainer) would go like, "What? Why 
 does it need to be defined twice? Wouldn't a type also fit an alias?" 
 And indeed if said maintainer comments out the second declaration, a 
 couple of obscure errors show up in unittesting (mind you, not 
 unittesting of std.meta, but of completely unrelated modules).
I'm going to take a guess that this is because older compilers would not accept keywords as alias parameters (so e.g. Alias!int would not work without the T overload). There are likely leftover workarounds, either of this type, or of the form of: ```d template Alias(T...) if (T.length == 1) { alias Alias = T[0]; } ``` I think if we can get rid of all of the overloads/workarounds, we should. -Steve
May 01 2021