extend PR #11846 to add symbol support for AliasAssign by WalterBright · Pull Request #12029 · dlang/dmd

WalterBright · 2020-12-16T09:04:03Z

#11846 only does alias assigns for types. As promised, this extends it for symbols, too, just like AliasDeclaration.

dlang-bot · 2020-12-16T09:04:05Z

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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

WalterBright · 2020-12-17T08:02:36Z

@atilaneves this now passes the std.meta unittests

WalterBright · 2020-12-17T08:29:40Z

Spec pull: dlang/dlang.org#2919

WalterBright · 2020-12-17T08:30:23Z

Still missing: a RHS that consists of a function literal. Will add that in next PR.

WalterBright · 2020-12-17T08:31:31Z

I also discovered there's already an AliasAssign in the grammar. I'll be doing a refactor at some point to change this to AliasReassign, which makes more sense anyway.

Geod24 · 2020-12-17T08:57:10Z

Can we put this feature behind a -preview switch ?

WalterBright · 2020-12-17T23:02:50Z

Can we put this feature behind a -preview switch ?

This feature is purely additive. It does not remove or modify any existing behavior. There's no purpose to putting it behind a switch.

Besides, I want to get it used in std.meta fairly soon, as the performance boost should be substantial.

Geod24 · 2020-12-18T00:09:46Z

As mentioned in the previous PR:

Having it straight into the language means that code might not break when the feature is first added, but will break when the design evolves to incorporate feedback.

If you're experimenting with the language, fine, but signal it properly. I found a SEGV with this feature within 5 minutes.

WalterBright · 2020-12-18T00:52:39Z

If you're experimenting with the language, fine, but signal it properly.

No more than other PRs. Still no need to hide it behind a switch.

I found a SEGV with this feature within 5 minutes.

Test case, please.

12345swordy · 2020-12-18T04:26:56Z

This is what @Geod24 is referring to.

import std.meta;

template Swap (alias A, alias B)
{
    alias C = A;
    B = A;
    A = B;
    enum Swap = true;
}

alias A = int;
alias B = string;

static assert(Swap!(A, B));

pragma(msg, A);

WalterBright · 2021-01-03T08:26:02Z

ping @Geod24

atilaneves · 2021-01-04T16:24:39Z

While it's additive, I'd hate for someone to go and write code that depends on this behaviour in case we have to revert these changes. I think a preview switch is a better way forward. Well, something that prevents what I just described, anyway.

WalterBright · 2021-01-05T06:33:27Z

We put in other additive enhancements all the time without -preview. I do not understand the trepidation about this feature - it's what people have been asking for for several years. There's no posted rationale against it. It's small and simple. If it fails, it just gets put in the deprecation cycle.

The problem with -preview is we cannot then use it to speed up Phobos, which is its primary purpose.

WalterBright · 2021-01-05T06:42:27Z

I suppose "cannot" is a bit strong. It would be just ugly.

Geod24 · 2021-01-05T06:48:14Z

We put in other additive enhancements all the time without -preview. I do not understand the trepidation about this feature - it's what people have been asking for for several years.

When is the last time we put in such a language change without -preview ?

There's no posted rationale against it.

The burden of proof should be on the proposer, not the reviewer.
Also the premise here is "let's put it in and see what happens" (which, as mentioned, is common - with -preview).

It's small and simple. If it fails, it just gets put in the deprecation cycle.

Except that it will "almost work" and there will be "just a few fixes needed" that will break code. The idea behind -preview is exactly to avoid this.

nordlow · 2021-01-05T08:38:54Z

test/runnable/aliasassign.d

+template staticMap(alias F, T...)
+{
+    alias A = AliasSeq!();
+    static foreach (t; T)


This is still gonna create T.length - 1 number of intermediate instances of AliasSeq. Despite AliasSeq being special cased in the compiler, it would be interesting to see how the compilation performance of this compares to an implementation using alias functions, @UplinkCoder.

This is still gonna create T.length - 1 number of intermediate instances of AliasSeq.

Yup. A lot less than T.length-1 number of template instantiations of staticMap, which are far more expensive.

WalterBright · 2021-01-05T09:22:27Z

Except that it will "almost work" and there will be "just a few fixes needed" that will break code.

There's no evidence that it will break code. It's additive. It's no more risk than any of many other PRs sailing through. Sticking it behind -preview means it will be years before the performance improvements to things like staticMap will be available.

When is the last time we put in such a language change without -preview ?

Last week? Heck, one went through in less than ONE DAY with no Spec pull, no Bugzilla issue, and no changelog. #11833

The burden of proof should be on the proposer, not the reviewer.

I can't prove a negative. We do know that nothing broke in the test suite, druntime, phobos, buildkite, etc.

The idea behind -preview is exactly to avoid this.

The idea is we use it for features we know will break code. Otherwise, every single PR would have to be behind -preview.

ibuclaw · 2021-01-05T13:32:11Z

Except that it will "almost work" and there will be "just a few fixes needed" that will break code.

There's no evidence that it will break code. It's additive. It's no more risk than any of many other PRs sailing through. Sticking it behind -preview means it will be years before the performance improvements to things like staticMap will be available.

Consider it an incubator for new features. As this feature is clearly still incomplete, it shouldn't be on by default until it's ready to be presented to the world.

When is the last time we put in such a language change without -preview ?

Last week? Heck, one went through in less than ONE DAY with no Spec pull, no Bugzilla issue, and no changelog. #11833

There is a bugzilla issue, it's referenced in the PR - and the language change is hidden behind a -preview, so that isn't answering the question at all.

The idea behind -preview is exactly to avoid this.

The idea is we use it for features we know will break code. Otherwise, every single PR would have to be behind -preview.

It's also there for experiments that may prove volatile and allows removal without ever bothering with any deprecation cycle (though apparently some things like -preview=rvaluerefparam appear to be more tricky than initially thought).

Having a -preview= would have prevented @[UDA] being introduced as both a new feature and deprecated syntax in the same release.

WalterBright · 2021-01-05T20:46:32Z

As this feature is clearly still incomplete

How so?

Having a -preview= would have prevented @[UDA] being introduced as both a new feature and deprecated syntax in the same release.

On the other hand, hiding it behind a switch would have meant it would have taken several years to adopt it, whereas it was immediately successful. The immediacy and eagerness of its adoption justified it.

The point of this PR is to speed up staticMap and some other Phobos templates, where people have been begging for solutions for years. It's a topic of every DConf and every quarterly meeting. This is not going to happen for staticMap for years more if it is behind a preview switch. Nobody has identified any problems with this addition. It passes all the tests.

This feature gives D a significant competitive advantage. It's immediately useful to anyone who uses std.meta.

Geod24

So identity assignment doesn't work anymore ?

template Identity (alias T)
{
    T = T;
    alias Identity = T;
}
pragma(msg, Identity!int);

Results in:

aliasassign.d(5): Error: `T` must have same parent `Identity!int` as alias `T`

Geod24 · 2021-01-06T04:37:50Z

test/fail_compilation/aliasassign.d

+fail_compilation/aliasassign.d(13): Error: `B` must have same parent `Swap!(int, string)` as alias `B`
+fail_compilation/aliasassign.d(14): Error: `A` must have same parent `Swap!(int, string)` as alias `A`


This error message doesn't make much sense. "B must have same parent as B"... Well it does, doesn't it ?
And changing the outter B (L19) doesn't help.

If you name all your symbols the same, no, it isn't going to make that much sense. If you have a better idea, let me know.

Note that in your example the parameter T is a different declaration than the target T of the aliasassign, and they have different parents. They have to have the same parent to be accepted.

If you name all your symbols the same, no, it isn't going to make that much sense. If you have a better idea, let me know.

That's why I mentioned changing the outer B:

template Swap (alias A, alias B) { alias C = A; B = A; A = B; enum Swap = true; } alias OuterA = int; alias OuterB = string; static assert(Swap!(OuterA, OuterB));

This produces the same error message.

Note that in your example the parameter T is a different declaration than the target T of the aliasassign, and they have different parents. They have to have the same parent to be accepted.

How can they be different, there is no other alias declaration with the same name ?
Looking at the code again, did you mean something like:

fail_compilation/aliasassign.d(14): Error: alias `A` must be declared inside `Swap!(int, string)`

Likewise, the following would be more user-friendly:

fail_compilation/aliasassign.d(14): Error: alias parameter `A` cannot be re-assigned (errorSupplemental) Only aliases declared inside a template can be re-assigned

RazvanN7 · 2021-01-08T15:17:36Z

We put in other additive enhancements all the time without -preview. I do not understand the trepidation about this feature

The fact that this happens is not a reason to continue to do so. Besides the technical shortcomings that have been pointed out by other reviewers, one fundamental issue with this approach is that it is unfair to other contributors. We have rules that are clearly stated and the fact that those rules were not respected in this case brings a lot of damage to the community.

For example, @UplinkCoder 's type functions: while I do agree that a DIP is necessary in that situation, it is hard to impose that rule when we get examples such as this PR.

From my perspective, both AliasAssign and type functions should be behind a preview switch and both should be presenting a DIP so that everyone gets to analyze them.

atilaneves · 2021-01-08T16:12:02Z

I talked to @WalterBright about this today.

The idea is to not put it behind -preview but instead:

Move forward with enough changes in the compiler to enable us to write a different version of staticMap that we can then use to benchmark compile time for large codebases that use it.
Iff we see compile-time improvements we keep it. Otherwise we revert the changes.

The idea is to use the beta period for this and if necessary revert before the release. In any case we wouldn't update the changelog until we're sure this is the direction we're going in. How does that sound?

UplinkCoder · 2021-01-08T16:43:20Z

@atilaneves in that case we would do the same with typefunctions?

atilaneves · 2021-01-11T10:12:41Z

@atilaneves in that case we would do the same with typefunctions?

This is a false equivalency.

RazvanN7 · 2021-01-12T09:00:50Z

@atilaneves I still don't understand what is wrong with putting it behind a preview switch. If we decide to go on with this we can just eliminate the preview, if not then we can revert it in a clean way. It is not clear why we should flout the rules in this case and implement the feature directly in the language without a DIP.

Geod24 · 2021-01-12T09:08:36Z

The idea is to use the beta period for this and if necessary revert before the release. In any case we wouldn't update the changelog until we're sure this is the direction we're going in. How does that sound?

Well the previous PR already made it into a release. And as @RazvanN7 noted, if the idea is to test between releases, a -preview switch is not a problem.

atilaneves · 2021-01-12T10:23:04Z

Because the whole point of this is to test changing the implementation of staticMap and we'd have to use the preview flag when building Phobos itself.

Either way, I don't think it's a big deal. Reasonable people may of course disagree.

12345swordy · 2021-01-14T16:47:02Z

@WalterBright is using a preview switch a huge deal breaker here?

WalterBright · 2021-01-15T08:10:34Z

@12345swordy yes. Atila explained it, so have I.

Geod24 · 2021-01-15T09:17:57Z

Either way, I don't think it's a big deal. Reasonable people may of course disagree.

Seems like everyone but the two of you not only disagree, but agree on an alternate course.

nordlow · 2021-01-15T10:21:05Z

What's the plan on updating druntime and phobos, such as staticMap to make use of this?

atilaneves · 2021-01-18T10:20:44Z

What's the plan on updating druntime and phobos, such as staticMap to make use of this?

The plan is to try it out, benchmark it, and revert it if it's not faster to compile. If it is, we make a PR for Phobos.

WalterBright added the Review:WIP Work In Progress - not ready for review or pulling label Dec 16, 2020

WalterBright requested review from RazvanN7, UplinkCoder, ibuclaw and kinke as code owners December 16, 2020 09:04

WalterBright added the Review:Needs Work label Dec 16, 2020

WalterBright force-pushed the AliasAssign2 branch from 4ef16b6 to 875627f Compare December 17, 2020 08:01

dlang-bot removed the Review:Needs Work label Dec 17, 2020

WalterBright mentioned this pull request Dec 17, 2020

add AliasAssign declarations #11846

Merged

WalterBright removed the Review:WIP Work In Progress - not ready for review or pulling label Dec 17, 2020

PetarKirov added the language-experiment label Dec 17, 2020

WalterBright force-pushed the AliasAssign2 branch 4 times, most recently from 09eafc3 to f37f028 Compare January 3, 2021 07:41

extend PR dlang#11846 to add symbol support for AliasAssign

9176a12

WalterBright force-pushed the AliasAssign2 branch from f37f028 to 9176a12 Compare January 5, 2021 06:35

nordlow reviewed Jan 5, 2021

View reviewed changes

Geod24 reviewed Jan 6, 2021

View reviewed changes

UplinkCoder mentioned this pull request Jan 8, 2021

Preview typefunctions #12101

Closed

atilaneves approved these changes Jan 15, 2021

View reviewed changes

atilaneves added the Merge:auto-merge label Jan 15, 2021

dlang-bot merged commit a85d0d6 into dlang:master Jan 15, 2021

WalterBright deleted the AliasAssign2 branch January 20, 2021 08:11

WalterBright mentioned this pull request Jan 20, 2021

Rewrite std.meta.staticMap using AliasAssign dlang/phobos#7756

Closed

		fail_compilation/aliasassign.d(13): Error: `B` must have same parent `Swap!(int, string)` as alias `B`
		fail_compilation/aliasassign.d(14): Error: `A` must have same parent `Swap!(int, string)` as alias `A`

Uh oh!

Conversation

WalterBright commented Dec 16, 2020

Uh oh!

dlang-bot commented Dec 16, 2020

Bugzilla references

Testing this PR locally

Uh oh!

WalterBright commented Dec 17, 2020

Uh oh!

WalterBright commented Dec 17, 2020

Uh oh!

WalterBright commented Dec 17, 2020

Uh oh!

WalterBright commented Dec 17, 2020

Uh oh!

Geod24 commented Dec 17, 2020

Uh oh!

WalterBright commented Dec 17, 2020

Uh oh!

Geod24 commented Dec 18, 2020

Uh oh!

WalterBright commented Dec 18, 2020

Uh oh!

12345swordy commented Dec 18, 2020

Uh oh!

WalterBright commented Jan 3, 2021

Uh oh!

atilaneves commented Jan 4, 2021

Uh oh!

WalterBright commented Jan 5, 2021

Uh oh!

WalterBright commented Jan 5, 2021

Uh oh!

Geod24 commented Jan 5, 2021

Uh oh!

nordlow Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

WalterBright commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WalterBright commented Jan 5, 2021

Uh oh!

Geod24 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geod24 Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

WalterBright Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

WalterBright Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

Geod24 Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

WalterBright Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Jan 8, 2021

Uh oh!

atilaneves commented Jan 8, 2021

Uh oh!

UplinkCoder commented Jan 8, 2021

Uh oh!

atilaneves commented Jan 11, 2021

Uh oh!

RazvanN7 commented Jan 12, 2021

Uh oh!

nordlow Jan 5, 2021 •

edited

Loading

WalterBright commented Jan 5, 2021 •

edited

Loading

ibuclaw commented Jan 5, 2021 •

edited

Loading

Geod24 left a comment •

edited

Loading

nordlow commented Jan 15, 2021 •

edited

Loading