Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TEST ONLY] Enable preview=in by default #11632

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Aug 27, 2020

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

@Geod24
Copy link
Member Author

Geod24 commented Aug 31, 2020

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

@Geod24
Copy link
Member Author

Geod24 commented Aug 31, 2020

@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
Copy link
Member Author

Geod24 commented Sep 1, 2020

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

@MoonlightSentinel
Copy link
Contributor

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
Copy link
Member Author

Geod24 commented Sep 1, 2020

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
Copy link
Contributor

  • 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
Copy link
Member Author

Geod24 commented Sep 2, 2020

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
Copy link
Contributor

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 Geod24 force-pushed the enable-preview-in-default branch 2 times, most recently from 527eb70 to 8fa17e9 Compare September 5, 2020 06:35
@Geod24 Geod24 force-pushed the enable-preview-in-default branch from 8fa17e9 to 866a2da Compare September 13, 2020 04:57
@Geod24
Copy link
Member Author

Geod24 commented Sep 13, 2020

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

CyberShadow added a commit to CyberShadow/ae that referenced this pull request Sep 13, 2020
@jondegenhardt
Copy link

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
Copy link

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
Copy link
Member Author

Geod24 commented Sep 13, 2020

@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
Copy link

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
Copy link
Member Author

Geod24 commented Sep 13, 2020

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
Copy link

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.

@Geod24
Copy link
Member Author

Geod24 commented Mar 4, 2023

Looks like there's a regression in CTFE

@Geod24
Copy link
Member Author

Geod24 commented Mar 4, 2023

Regression is #13007 which I've fixed in #11545 but the test didn't check -preview=in, only regular compilation.

@Geod24 Geod24 force-pushed the enable-preview-in-default branch 2 times, most recently from fa9447e to 10bdeee Compare March 8, 2023 00:16
Geod24 added 3 commits March 14, 2023 00:45
At the moment, in dsymbolsem.d:1026, there is the following comment:
```
/* If a struct is all zeros, as a special case
 * set its initializer to the integer 0.
 * In AssignExp::toElem(), we check for this and issue
 * a memset() to initialize the struct.
 * Must do same check in interpreter.
 */
```
Turns out, the 'Must do same check in interpreter.' part was missing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants