Make `in` a first-class storage class, not a `const [scope]` alias #11474

Geod24 · 2020-07-29T07:03:13Z

Splitted off #11000 . I believe this changeset is useful on its own, but #11000 might provide an additional reasoning for why we need it.

Companion Phobos PR: dlang/phobos#7570
Spec PR / changelog pending more discussions, including #11000 .

Commit message follows:

For a long time, the 'in' storage class was only a second class citizen,
merely an alias for 'const', which is a type constructor.
While it was also documented for a long time as being 'scope',
this was removed when 'scope' actually started to have an effect
(when DIP1000 started to be implemented).
Currently, a switch (-preview=in) allows to get back this 'scope'.

However, the 'in' storage class does not really exists,
it gets lowered to 'const [scope]' at an early stage by the compiler,
which means that we expose what is essentially an implementation
detail to the user.

This led to a variety of problems for both developers and users.
The most obvious one was that functions involving `in` would show
up simply as `const` (or `const scope`), both in generated header
and error messagges, hindering the ability to do header generation
with `-preview=in` as the same value for this switch was needed
for both producer and consumer.
Another issue was that the result of `__traits(getParameterStorageClass)`
was inaccurate, giving either an empty tuple or `scope`.

For compiler developers, the `in` storage class simply couldn't be relied on,
as it was replaced by `const [scope]` rather early in the semantic phase
(in TypeFunction's semantic), which lead to some dead code (e.g. in dmangle).

After this change, the class will show up properly in error message,
and in `getParameterStorageClass`, which can be a surprising change for users.
However, it is not expected that code can break as a result,
unless they break as a result of using the `-preview=in` switch.

dlang-bot · 2020-07-29T07:03:15Z

Thanks for your pull request, @Geod24!

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

For a long time, the 'in' storage class was only a second class citizen, merely an alias for 'const', which is a type constructor. While it was also documented for a long time as being 'scope', this was removed when 'scope' actually started to have an effect (when DIP1000 started to be implemented). Currently, a switch (-preview=in) allows to get back this 'scope'. However, the 'in' storage class does not really exists, it gets lowered to 'const [scope]' at an early stage by the compiler, which means that we expose what is essentially an implementation detail to the user. There is a PR in DMD (dlang/dmd#11474) that aims to give 'in' an actual identity, instead of it being lowered to 'const [scope]'. The underlying motivation is to allow for extending 'in''s functionality, giving it the ability to pass by 'ref' when necessary, and accept rvalues. However, regardless of the second goal, having proper support for 'in' would lead to less confusing messages, better code generation, and less confusion w.r.t. the behavior of `std.traits.ParameterStorageClass`.

Geod24 · 2020-07-29T23:53:33Z

One question I have is, should in have its own mangling ? Currently it does not, so applications compiled with different values for -transition=in will not link together. It also means that demangling-based name printing (e.g. stack traces) will still be incorrect. And if it does, what value should it take ? CC @ibuclaw @rainers .

rainers · 2020-07-30T06:51:08Z

My proposal (#10526) was to add in to the mangling. It seems your version is a bit more advanced. IIRC I had some issues building phobos unittest, did you run into them, too?

Geod24 · 2020-07-30T10:29:16Z

My proposal (#10526)

Thanks, I have integrated that part now.

IIRC I had some issues building phobos unittest, did you run into them, too?

I mostly had issues with covariance (you can see that's a good chunk of the tests).
Note that I built this as part of #11000 first, and tested that PR with my project (depending on Vibe.d, D-YAML and a bunch of other things) to be confident it worked. Once it was working, carving this out was trivial.

I think the main difference with your approach is that I don't try to infer in at all, and that except for the switch rename, none of this PR is about DIP1000. It's simply about pushing the lowering of in to a later stage / keeping the STC around. The beauty of providing a mangling for in is that we completely sidestep the issue of compiling Phobos with -preview=in in its current form (obviously you forgo the safety checks for non-template, but that's not really an issue).

I just ran the unittests with the companion Phobos PR mentioned in the op and it passed (OSX).

rainers · 2020-07-30T17:06:29Z

I mostly had issues with covariance (you can see that's a good chunk of the tests).

Maybe the issues I noticed where related to that.

I think the main difference with your approach is that I don't try to infer in at all

"Inferring scope" (I guess that's what you meant) was probably not the correct term. It referred to the fact that it doesn't appear in the mangling, because it uses STC.scopeinferred. I think you might want to add it, too, to avoid linking errors with libraries compiled differently.

rainers · 2020-07-30T17:08:26Z

With the mangling added, we might want to resurrect dlang/druntime#2847

Geod24 · 2020-07-31T00:29:34Z

It referred to the fact that it doesn't appear in the mangling [...]

Neither does it here. It's either in or scope (scope and in are considered redundant by the parser).
The mangling / header / error messages are derived from the Parameter class, which keeps STC.in_ through its lifetime.
On the other hand, the checks on scope are done on the VarDeclaration, which will get STC.scope_.

rainers · 2020-07-31T05:36:18Z

The mangling / header / error messages are derived from the Parameter class, which keeps STC.in_ through its lifetime.
On the other hand, the checks on scope are done on the VarDeclaration, which will get STC.scope_.

Ah, ok. Thanks for explanation.

For a long time, the 'in' storage class was only a second class citizen, merely an alias for 'const', which is a type constructor. While it was also documented for a long time as being 'scope', this was removed when 'scope' actually started to have an effect (when DIP1000 started to be implemented). Currently, a switch (-preview=in) allows to get back this 'scope'. However, the 'in' storage class does not really exists, it gets lowered to 'const [scope]' at an early stage by the compiler, which means that we expose what is essentially an implementation detail to the user. There is a PR in DMD (dlang/dmd#11474) that aims to give 'in' an actual identity, instead of it being lowered to 'const [scope]'. The underlying motivation is to allow for extending 'in''s functionality, giving it the ability to pass by 'ref' when necessary, and accept rvalues. However, regardless of the second goal, having proper support for 'in' would lead to less confusing messages, better code generation, and less confusion w.r.t. the behavior of `std.traits.ParameterStorageClass`.

A shorter name, which also ties it to the switch name. This is in support of a later commit that changes the meaning on the switch, and would render the name extremely long if the same scheme was used ('inMeansConstScopeMaybeRefAcceptRvalue' is a bit much). Additionally, the test case was renamed for the same reason.

As explained in details in dlang/dmd#11000 and dlang/dmd#11474, 'in' has been for a very long time lowered to simply 'const', or 'scope const' when v2.092.0's '-preview=in' switch is used. DMD PR 11474 aims to change this, so 'in' is not (visibly) lowered, and shows up in any user-visible string. This includes header generation, error messages, and stringof. Since this code was testing stringof directly, it is affected by the change. To support testing of both aforementioned DMD PRs, the change is not tied to a version, but uses the (soon-to-be) old way as a fallback.

Geod24 · 2020-08-01T16:18:05Z

The Phobos fix has been merged, this should now go green on the auto-tester.
Removed some of the tests that were tied to -preview=in, in particular the covariance ones. Both the test-suite / druntime covers it already, and they are still in the other PR as well.
Vibe-core needs a fix because it is testing the exact output of a stringof call in a static assert: vibe-d/vibe-core#213

As explained in details in dlang/dmd#11000 and dlang/dmd#11474, 'in' has been for a very long time lowered to simply 'const', or 'scope const' when v2.092.0's '-preview=in' switch is used. DMD PR 11474 aims to change this, so 'in' is not (visibly) lowered, and shows up in any user-visible string. This includes header generation, error messages, and stringof. Since this code was testing stringof directly, it is affected by the change. To support testing of both aforementioned DMD PRs, the change is not tied to a version, but uses the (soon-to-be) old way as a fallback.

Geod24 · 2020-08-03T00:53:13Z

vibe-core has been fixed, this is now green.

thewilsonator

Looks fine, no tests for mangles though.

Geod24 · 2020-08-03T16:48:19Z

Added a mangling test (piggybacking on the fail_compilation test, but it works).

For a long time, the 'in' storage class was only a second class citizen, merely an alias for 'const', which is a type constructor. While it was also documented for a long time as being 'scope', this was removed when 'scope' actually started to have an effect (when DIP1000 started to be implemented). Currently, a switch (-preview=in) allows to get back this 'scope'. However, the 'in' storage class does not really exists, it gets lowered to 'const [scope]' at an early stage by the compiler, which means that we expose what is essentially an implementation detail to the user. This led to a variety of problems for both developers and users. The most obvious one was that functions involving `in` would show up simply as `const` (or `const scope`), both in generated header and error messagges, hindering the ability to do header generation with `-preview=in` as the same value for this switch was needed for both producer and consumer. Another issue was that the result of `__traits(getParameterStorageClass)` was inaccurate, giving either an empty tuple or `scope`. Lastly, the lack of mangling for `in` means that stack trace would show the wrong signature for a function, potentially confusing the users. For compiler developers, the `in` storage class simply couldn't be relied on, as it was replaced by `const [scope]` rather early in the semantic phase (in TypeFunction's semantic), which lead to some dead code (e.g. in dmangle). After this change, the class will show up properly in error message, and in `getParameterStorageClass`, which can be a surprising change for users. However, it is not expected that code can break as a result, unless they break as a result of using the `-preview=in` switch.

Geod24 · 2020-08-03T16:53:57Z

Actually, I also added a changelog to explain what happens (and one of the possible shortcomings).

UplinkCoder · 2020-08-03T17:44:07Z

src/dmd/dmangle.d

@@ -79,7 +79,7 @@ private immutable char[TMAX] mangleChar =
    Tfunction    : 'F', // D function
    Tsarray      : 'G',
    Taarray      : 'H',
-    Tident       : 'I',


I am wondering why this change does not seem to break tests.
Where was Tident used? in templates?

It isn't used (dlang/druntime#2847 (comment))

AndrejMitrovic · 2020-08-04T01:31:07Z

Seems fairly uncontroversial.

See dlang/dmd#11474 and dlang/druntime#3183

Geod24 requested review from ibuclaw and WalterBright as code owners July 29, 2020 07:03

Geod24 mentioned this pull request Jul 29, 2020

std.traits: Add support for 'in' storage class dlang/phobos#7570

Merged

Geod24 force-pushed the restore-in-identity branch from 39087e4 to d4b0800 Compare July 30, 2020 10:22

Geod24 requested a review from atilaneves July 30, 2020 12:31

rainers approved these changes Jul 31, 2020

View reviewed changes

Geod24 force-pushed the restore-in-identity branch from d4b0800 to 0ec8d21 Compare July 31, 2020 19:33

Geod24 force-pushed the restore-in-identity branch 3 times, most recently from 5940e2f to 760944d Compare August 1, 2020 15:42

Geod24 mentioned this pull request Aug 1, 2020

Change a unittest that test for 'in' => 'const' lowering vibe-d/vibe-core#213

Merged

Geod24 force-pushed the restore-in-identity branch from 760944d to 91be7f4 Compare August 1, 2020 16:17

Geod24 requested a review from thewilsonator August 3, 2020 03:28

thewilsonator approved these changes Aug 3, 2020

View reviewed changes

Geod24 force-pushed the restore-in-identity branch from 91be7f4 to 0a47e9e Compare August 3, 2020 16:48

Geod24 requested a review from UplinkCoder as a code owner August 3, 2020 16:48

Geod24 force-pushed the restore-in-identity branch from 0a47e9e to df74268 Compare August 3, 2020 16:53

UplinkCoder reviewed Aug 3, 2020

View reviewed changes

AndrejMitrovic approved these changes Aug 4, 2020

View reviewed changes

AndrejMitrovic merged commit ead2580 into dlang:master Aug 4, 2020

Geod24 deleted the restore-in-identity branch August 4, 2020 03:04

This was referenced Aug 4, 2020

Fix the mangling of 'in' to not include 'const' #11505

Merged

Add demangling support for 'in' storage class dlang/druntime#3183

Merged

Geod24 added a commit to Geod24/dlang.org that referenced this pull request Aug 4, 2020

Add mangling specification for 'in' parameters

f1360ab

See dlang/dmd#11474 and dlang/druntime#3183

Geod24 mentioned this pull request Aug 4, 2020

Add mangling specification for 'in' parameters dlang/dlang.org#2839

Merged

dlang-bot pushed a commit to dlang/dlang.org that referenced this pull request Aug 4, 2020

Add mangling specification for 'in' parameters

e5bcad1

See dlang/dmd#11474 and dlang/druntime#3183

Geod24 mentioned this pull request Aug 16, 2020

Make -preview=in means [ref] const scope and accept rvalues #11000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `in` a first-class storage class, not a `const [scope]` alias #11474

Make `in` a first-class storage class, not a `const [scope]` alias #11474

Geod24 commented Jul 29, 2020 •

edited

dlang-bot commented Jul 29, 2020

Geod24 commented Jul 29, 2020

rainers commented Jul 30, 2020

Geod24 commented Jul 30, 2020

rainers commented Jul 30, 2020

rainers commented Jul 30, 2020

Geod24 commented Jul 31, 2020

rainers commented Jul 31, 2020

Geod24 commented Aug 1, 2020

Geod24 commented Aug 3, 2020

thewilsonator left a comment

Geod24 commented Aug 3, 2020

Geod24 commented Aug 3, 2020

UplinkCoder Aug 3, 2020

Geod24 Aug 3, 2020

AndrejMitrovic commented Aug 4, 2020

Make in a first-class storage class, not a const [scope] alias #11474

Make in a first-class storage class, not a const [scope] alias #11474

Conversation

Geod24 commented Jul 29, 2020 • edited

dlang-bot commented Jul 29, 2020

Bugzilla references

Testing this PR locally

Geod24 commented Jul 29, 2020

rainers commented Jul 30, 2020

Geod24 commented Jul 30, 2020

rainers commented Jul 30, 2020

rainers commented Jul 30, 2020

Geod24 commented Jul 31, 2020

rainers commented Jul 31, 2020

Geod24 commented Aug 1, 2020

Geod24 commented Aug 3, 2020

thewilsonator left a comment

Choose a reason for hiding this comment

Geod24 commented Aug 3, 2020

Geod24 commented Aug 3, 2020

UplinkCoder Aug 3, 2020

Choose a reason for hiding this comment

Geod24 Aug 3, 2020

Choose a reason for hiding this comment

AndrejMitrovic commented Aug 4, 2020

Make `in` a first-class storage class, not a `const [scope]` alias #11474

Make `in` a first-class storage class, not a `const [scope]` alias #11474

Geod24 commented Jul 29, 2020 •

edited