[TEST ONLY] Enable `preview=in` by default

[Geod24]

Don't mind me, just (ab)using Buildkite.

[Geod24]

I adapted Vibe.d (https://github.com/vibe-d/vibe.d/pull/2472) so when all is merged and released, it should go green on Buildkite and allow me to adapt other projects.

[Geod24]

@wilzbach : There seems to be a bug with diet-ng. It should be red and the error are showing in the log, but it's green on Buildkite... https://buildkite.com/dlang/dmd/builds/15557#e913f9b0-db63-4925-8c9f-07c056ffc206

[Geod24]

Well, the BlackEdder/ggplotd failure is interesting. The constructor turns into a copy constructor and that fails with inout.

[MoonlightSentinel]

Well, this breaks a lot of projects on BuildKite (and even more unchecked projects) - mostly due to the "redundant" in ref.

I think it would be better to change the error into a deprecation s.t. projects can integrate -preview=in more easily. AFAICT it doesn't cause issues and isn't really redundant because it enforces ref (even if it conflicts with your idea of in).

[Geod24]

I think it would be better to change the error into a deprecation s.t. projects can integrate -preview=in more easily.

There is two reasons why it's not a deprecation:

• We usually don't deprecate in the parser, because it would mean static if and version are not affected and the deprecation would show unless in a mixin;
• It's way easier to introduce an error in a -preview=in switch and get people to switch than to put a deprecation, as the deprecation period will not start until the preview is enabled by default anyways (see -dip25).

AFAICT it doesn't cause issues and isn't really redundant

It does cause one issue: If you have an overload with an in and an in ref, you will end up with two functions with the exact same signature / mangling. Preventing this would require looking up the scope to see if there's another overload that would conflict. Not something impossible to do, but something which could be very complicated to do correctly (because of forward references). And since most usages were actually doing this, I went with the error.

Note that this PR isn't meant to be merged anytime soon. I just want to get Buildkite green, and hope that'd be a good indication that people can actually use the switch.

[MoonlightSentinel]

• We usually don't deprecate in the parser, because it would mean static if and version are not affected and the deprecation would show unless in a mixin;

Agreed but that's an implementation detail(?) (which you disliked IIRC?)

• It's way easier to introduce an error in a -preview=in switch and get people to switch than to put a deprecation, as the deprecation period will not start until the preview is enabled by default anyways (see -dip25).

That's true but I would also expect that a lot more people would use -preview=in if their code still compiled and didn't break due to transitive dependencies. (Allthough this is obviously a double-edged sword).

It does cause one issue: If you have an overload with an in and an in ref, you will end up with two functions with the exact same signature / mangling.

#8429 will detect conflicting function declarations but the mangling for such functions is currently not identical. Changng the error into a deprecation for current master yields the following results:

module preview_in;

void foo(in int i) {}

void foo(in ref int i) {}

preview_in.o:     file format elf64-x86-64


Disassembly of section .text._D10preview_in3fooFIiZv:

0000000000000000 <_D10preview_in3fooFIiZv>:
   0:	c3                   	retq   
   1:	00 00                	add    %al,(%rax)
	...

Disassembly of section .text._D10preview_in3fooFIKiZv:

0000000000000000 <_D10preview_in3fooFIKiZv>:
   0:	c3                   	retq   
   1:	00 00                	add    %al,(%rax)
	...

Note that this PR isn't meant to be merged anytime soon. I just want to get Buildkite green, and hope that'd be a good indication that people can actually use the switch.

Noted, just thinking about how to make the transition to -preview=in less cumbersome.

[Geod24]

Agreed but that's an implementation detail(?) (which you disliked IIRC?)

Well it does affect tools that do parsing only. And it was made to be consistent with the other STCs. I actually tried to move it to semantic a few days ago, but hit a little snag (namely, inner functions going through semantic twice).

if their code still compiled and didn't break due to transitive dependencies.

On the other hand, I doubt there will be any library supporting both -preview=in and its absence. Most library will just switch. Phobos will have to, for another reason (it's compiled). So I was just looking to make the transition period having less step, and I've been adapting some libraries myself for the past few days.

yields the following results: [...]

People don't override in / in ref on int though. They do it on types that turns out to be ref by in's rules. Try with a type that has a postblit and you'll see the same mangling.

On another note, if you take a look at Buildkite now, it's much greener.

[MoonlightSentinel]

Well it does affect tools that do parsing only. And it was made to be consistent with the other STCs. I actually tried to move it to semantic a few days ago, but hit a little snag (namely, inner functions going through semantic twice).

Fair enough.

On the other hand, I doubt there will be any library supporting both -preview=in and its absence. Most library will just switch. Phobos will have to, for another reason (it's compiled). So I was just looking to make the transition period having less step, and I've been adapting some libraries myself for the past few days.

Your work in apdapting those libraries is much appreciated. I suppose our difference lies in how we estimate the probability that other libraries will adapt their code as well (which are not covered by BuildKite).

People don't override in / in ref on int though. They do it on types that turns out to be ref by in's rules. Try with a type that has a postblit and you'll see the same mangling.

Yeah, that was bad example. For reference, a type with postblit:

module preview_in;

struct HasPostblit
{
    this(this) {}
}

void foo(in HasPostblit i) {}

void foo(in ref HasPostblit i) {}

./generated/linux/release/64/dmd -c -preview=in preview_in.d 
preview_in.d(10): Deprecation: attribute ref is redundant with previously-applied in
preview_in.d(10): Error: function preview_in.foo(in HasPostblit i) conflicts with previous declaration at preview_in.d(8)

On another note, if you take a look at Buildkite now, it's much greener.

That's nice.

[Geod24]

Current status:

Need tags

• Style & dlang-community/D-Scanner: D-Scanner needs a new release.
• dlang/dub: Compiles and run, but the test-suite depends on pulling old dub versions. I'm just going to leave it be for a few releases and upgrade those hard-coded versions in the feature so it will "just work".
• dlang-community/std_data_json: Will make a tag myself

Left to do

• kaleidicassociates/excel-d: Needs a PR to upgrade unit-threaded
• dlang/dlang-bot: Need to raise a PR
• ikod/dlang-requests: One of its dependency's failing, wondering if @ikod would be interested to look at it.
• kaleidicassociates/lubeck: Many failures in cblas-2.1.0
• libmir/mir-optim: Many failures in cblas-2.1.0
• tjhann/imageformats: URL needs updating in Buildkite. Also internal failures, no PR raised yet.
• MartinNowak/io: Internal failures only. Any chance you could look at it @schveiguy ?
• dlang-tour/core: Has 2 failure so far

[jondegenhardt]

eBay/tsv-utils: There's only one failure, I'm hoping @jondegenhardt can help with this now that the beta is out.

@Geod24 Sure, but I need some context. I haven't been following the preview=in stuff, don't know what it's about or what it does. Can you point me to a doc that describes?

[jondegenhardt]

eBay/tsv-utils: There's only one failure, I'm hoping @jondegenhardt can help with this now that the beta is out.

@Geod24 Sure, but I need some context. I haven't been following the preview=in stuff, don't know what it's about or what it does. Can you point me to a doc that describes?

Think I found it: DMD 2.094.0 Beta change log: -preview=in. Looks easy enough. Simply change the in ref function parameter to const ref. Does that sound right? Switching to in by itself doesn't sound like it'll work correctly with versions of the compiler prior to 2.094.

[Geod24]

@jondegenhardt : Indeed, in ref => const ref is the path of least resistance. And the only path for libraries.

Switching to in by itself doesn't sound like it'll work correctly with versions of the compiler prior to 2.094.

It could work if you are really to take the hit on a lack of ref. But otherwise, I'd say wait 2 or 3 releases :)

[jondegenhardt]

Thanks for the confirmation. Making the change.

A minor note: I'm not planning on adding a version with -preview=in to the tsv-utils CI test matrix. It's unlikely I'll forget and add it back, but I'm not planning to test for it. This is something of a general issue with the preview switches. The tsv-utils CI suite already has a lot of variants, and I haven't been able to get clarity on which preview switches are definitely being added to the language and which are experiments/candidates, so I don't test with preview switches.

[Geod24]

I haven't been able to get clarity on which preview switches are definitely being added to the language and which are experiments/candidates

I think all switches but rvaluerefparams are intended to end up in the language. But the issue with -preview switch is that it is too easy to dump in progress work there, and never actually finish it. Another issue is that testing over all D projects is still too complicated (but we should automate this... At scale).

This PR already went much further than I anticipated (it's not going to be merged for a couple of years anyways), and in that time, I'm sure there will be some regressions. And as long as druntime / Phobos don't test with the switch enabled, it doesn't make sense for projects to do so.

[jondegenhardt]

And as long as druntime / Phobos don't test with the switch enabled, it doesn't make sense for projects to do so.

This is probably the right litmus test. I did pose this question in the #CI channel on slack to see if anyone else has thoughts.

[dlang-bot]

Thanks for your pull request, @Geod24!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	23763	critical	ICE on operations involving zero-initialized structs

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

[Geod24]

Time to dust this off. Next are:

• https://github.com/dlang-community/std_data_json/pull/56
• https://github.com/msgpack/msgpack-d/pull/127
• https://github.com/sociomantic-tsunami/ocean/pull/878
• https://github.com/sociomantic-tsunami/turtle/pull/86
• https://github.com/DlangScience/cblas/pull/32

[Geod24]

Looks like there's a regression in CTFE

[Geod24]

Regression is https://github.com/dlang/dmd/pull/13007 which I've fixed in https://github.com/dlang/dmd/pull/11545 but the test didn't check -preview=in, only regular compilation.