Add scope decorations to std.socket

[schveiguy]

This is to fix issues with compiling projects with dip1000.

I'm not sure how to add appropriate tests, since dip1000 is a compiler switch.

[dlang-bot]

Thanks for your pull request, @schveiguy!

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 + phobos#8438"

[schveiguy]

Is there a way to check if dip1000 compiles phobos? We must be checking this somewhere...

[adamdruppe]

Worth noting that this is a breaking change so it will need to be called out in the documentation, but the transition is easy so it is surely worth the minor hassle.

[iK4tsu]

Is there a way to check if dip1000 compiles phobos?

I think it adds the flag automatically already: https://github.com/dlang/phobos/blob/master/posix.mak#L138

[schveiguy]

For reference, I discovered this when trying to build mysql-native with my new safe patch with dip1000 turned on. Adam is right that this is a breaking change. I have it in draft until I can think about how to properly do the deprecation (if it's even possible).

[iK4tsu]

Why is it a breaking change? Without dip1000 scope does nothing here right?

[schveiguy]

Because if you derive from Socket (e.g. to make a SSL socket), then you have to repeat the scope attributes, or it breaks. So if you haven't already done that, then your code won't compile with this version.

And there is actually a buildkite project that breaks for this exact reason: https://buildkite.com/dlang/phobos/builds/6817#314ce653-7c99-423b-83dc-a28ac09cfc32

[adamdruppe]

On Fri, Apr 22, 2022 at 01:58:45PM -0700, Steven Schveighoffer wrote: Because if you derive from Socket (e.g. to make a SSL socket), then you have to repeat the scope attributes, or it breaks. So if you haven't already done that, then your code won't compile with this version.

Worth noting this is quite minor breakage that you can probably just rip the band-aid off.

[thewilsonator]

cc @ikod

[ikod]

cc @ikod

Thanks!

Sorry if this is wrong place to ask. If I understand correctly I only have to change 'override' Socket calls and this should work?

diff --git a/source/requests/streams.d b/source/requests/streams.d
index b1e54d0..b78e044 100644
--- a/source/requests/streams.d
+++ b/source/requests/streams.d
@@ -898,17 +898,17 @@ else {
             return this;
         }
         @trusted
-        override ptrdiff_t send(const(void)[] buf, SocketFlags flags) scope {
+        override ptrdiff_t send(scope const(void)[] buf, SocketFlags flags) scope {
             return openssl.SSL_write(ssl, buf.ptr, cast(uint) buf.length);
         }
-        override ptrdiff_t send(const(void)[] buf) scope {
+        override ptrdiff_t send(scope const(void)[] buf) scope {
             return send(buf, SocketFlags.NONE);
         }
         @trusted
         override ptrdiff_t receive(void[] buf, SocketFlags flags) scope {
             return openssl.SSL_read(ssl, buf.ptr, cast(int)buf.length);
         }
-        override ptrdiff_t receive(void[] buf) scope {
+        override ptrdiff_t receive(scope void[] buf) scope {
             return receive(buf, SocketFlags.NONE);
         }
         this(AddressFamily af, SocketType type = SocketType.STREAM, SSLOptions opts = SSLOptions()) {

[adamdruppe]

On Sat, Apr 23, 2022 at 12:36:49AM -0700, ikod wrote: Sorry if this is wrong place to ask. If I understand correctly I only have to change overrided Socket calls and this should work?

Yes, and it should compile now, even without the phobos pr, meaning you can make the change any time without breaking old compiler compatibility. I gotta do the same thing to my lib.

[ikod]

On Sat, Apr 23, 2022 at 12:36:49AM -0700, ikod wrote: Sorry if this is wrong place to ask. If I understand correctly I only have to change overrided Socket calls and this should work?
Yes, and it should compile now, even without the phobos pr, meaning you can make the change any time without breaking old compiler compatibility. I gotta do the same thing to my lib.

fixed in v2.0.5

[thewilsonator]

fixed in v2.0.5

Thanks for the quick response!

[schveiguy]

Damn, still broken, I think some functions got missed. @ikod I'll see if I can make a PR.

[ikod]

Damn, still broken, I think some functions got missed. @ikod I'll see if I can make a PR.

sorry! merged, released 2.0.6

[schveiguy]

Thanks @ikod for your help! Hopefully all done. I had to force push, as buildkite seemed to be stalled.

[schveiguy]

should be green now (dlang-requests now passing). I need either a bug report or a changelog now...

[RazvanN7]

Looks good! A changelog will suffice.

[schveiguy]

Please review changelog entry before merging.

[CyberShadow]

Flagging just to make sure we're on the same page, thanks @schveiguy .

We've previously rejected pull requests which cause breakage of this type. In particular TestSocket is considered "holy" and may not be changed, as the need of doing so signals that the rest of the changes are likely to be breaking.

Is there anything special about this case that's different from the previous ones, or is it time to reconsider those decisions too?

[PetarKirov]

If making the interface defined by Socket more strict is deemed an unacceptable breaking change in general, are there any blockers to defining a new derived class (say SafeSocket or RefinedSocket (well, not the best name)), that would have a more strict interface (perhaps that class could even be final)?

[schveiguy]

Wait, so we can just merge this as-is? I was going to put in the template check, but would rather not...

[CyberShadow]

It's guarded by debug (std_socket), so I'm not sure what that is for.

See the comment at the top of the file:

phobos/std/socket.d

Lines 3 to 5 in 3443e00

	// NOTE: When working on this module, be sure to run tests with -debug=std_socket
	// E.g.: dmd -version=StdUnittest -debug=std_socket -unittest -main -run socket
	// This will enable some tests which are too slow or flaky to run as part of CI.

[schveiguy]

FYI, the hunt-labs socket which would be affected is no more, they removed it in 2019. So we can take that off the list of affected projects.

[CyberShadow]

Wait, so we can just merge this as-is?

Well, the reason that CI is succeeding is because this PR changes the test code which should not be changed (because then it's not testing for what it is supposed to be testing for).

[schveiguy]

OK, I think that test would not pass if compiled with dip1000. I think that's a coincidence, since the debug switch was added 2 years ago (probably long before dip1000 testing was added to phobos).

[schveiguy]

OK, so give me a path forward. My current plan was to change the virtual functions to skinny wrappers with the "correct" attributes around the real implementation, based on detection of dip1000. I'd have to do that to the unittest wrapper socket as well.

Is that satisfactory?

[CyberShadow]

OK, I think that test would not pass if compiled with dip1000

Without looking at the specifics, I think the intention of debug(std_socket) is to test (our bindings to) network functionality, not things like lifetime or const-correctness. So, it might be a bug (or at least something fixable) in the test, or then again it might be that the test is representative of code using std.socket.

Looking at the specifics, it looks a lot like the second case. Sending a single byte is a common pattern to e.g. wake up an event loop, and a static array makes sense for that.

My current plan was to change the virtual functions to skinny wrappers with the "correct" attributes around the real implementation, based on detection of dip1000. I'd have to do that to the unittest wrapper socket as well.

I can't picture how this would look like and work. Could you make a simple example with one method?

The idea of the TestSocket class is that it represents a worst-case scenario of someone subclassing Socket in their project. Any changes that need to be done to TestSocket would therefore (in this worst-case scenario) have to be done in users' code. I think ideally we would want to look for a solution which doesn't require overly complicated changes on behalf of users.

I can do the same for my proposal in #8438 (comment) if you want.

[CyberShadow]

I think we should change all debug(std_socket) blocks to always compile the code, but never run it unless debug=std_socket.

[schveiguy]

OK, can you re-instate your request changes so this doesn't get accidentally merged while meddling with it? It won't let me put it back in draft or "request changes" on my own PR.

[CyberShadow]

OK, here it is. (It's forcing me to enter a comment.)

[schveiguy]

Will see if it builds, but checkout second commit. If it works, I'll do it to all them. I have issues testing phobos locally, so relying on CI here.

[CyberShadow]

It does work, but not if I revert the changes done to TestSocket.

[schveiguy]

Wait, I don't think this is going to work. Scope is in the mangling. So if Phobos is compiled with dip1000, but when you compile against it, you have dip1000 off, it won't link.

So that idea is no good... damn.

[schveiguy]

And overloading on scope doesn't work. So it has to be a new class, or we break code, as already discussed.

[schveiguy]

I can do the same for my proposal in #8438 (comment) if you want.

Rereading that comment, I'm not loving this idea. If Socket is final, then you can't derive from it, and get all the enforcement of attributes that we would put there. You'd have to derive from BaseSocket, and then add those attributes manually yourself.

It also would break any code that accepts a Socket, but you want to pass in your special flavor of socket to. All that code would have to change to accepting BaseSocket, which is now breaking a lot more code than just socket derivatives.

[schveiguy]

Not sure if this approach will work with all relevant language changes; I think it should at least work with const, @nogc, and scope.

I don't know that const or @nogc are appropriate for sockets. E.g. a buffered socket derivative might want to use the GC to allocate buffers, and store them in the socket instance.

However scope is a declaration that the socket class won't squirrel away a reference to the buffer you hand it, which has always been implied for sockets.

[CyberShadow]

Rereading that comment, I'm not loving this idea. If Socket is final, then you can't derive from it, and get all the enforcement of attributes that we would put there. You'd have to derive from BaseSocket, and then add those attributes manually yourself.

Yes. But that simultaneously gives us the freedom to keep tightening the Socket interface.

It also would break any code that accepts a Socket, but you want to pass in your special flavor of socket to. All that could would have to change to accepting BaseSocket, which is now breaking a lot more code than just socket derivatives.

Yes, I mentioned this as well in my comment above. The benefit is that this breaking change will happen once.

I don't know that const or @nogc are appropriate for sockets. E.g. a buffered socket derivative might want to use the GC to allocate buffers, and store them in the socket instance.

This is a good point.

If we don't foresee many other necessary changes to Sockets interface, then simply changing it does make the most sense.

There is also #5496, which tried to add const to some Address parameters, which I think still does make sense.

[schveiguy]

There is also #5496, which tried to add const to some Address parameters, which I think still does make sense.

It does make sense, and it also doesn't. But not because of Address (all methods have a const overload requirement), but because of the pain of using const class references in D. Since all Address methods are const, you can assume that Address is essentially a const class.

Overloads based on const are possible, but of course, you'd have to overload both, so it still breaks using that path.

But I'm not sure it's important to do. If not for that single name method, then it wouldn't even be a problem to cast away const in order to call the functions. And the amount of code that keeps a const Address is probably near zero.

[CyberShadow]

I think it's much more likely that Address will be const by transitivity (e.g. you have some struct Context which you have no business writing to, and you're given a const pointer to it, but you can't use context.address in the socket code).

[schveiguy]

yeah, good point. Perhaps worth doing now, which means we would have to have the current derivers update their types again.

[atilaneves]

Good points about breakage and what got rejected earlier, but adding @nogc is very different I think since it's not going to be the default, whereas dip1000 is. I'm in favour, especially after @adamdruppe 's investigation into possible breakage.

[CyberShadow]

Right, so I think this is good to go as-is.

[schveiguy]

Thanks for the thoughtful discussion. This really opened my eyes as to what is coming when dip1000 becomes the default, we need to tread cautiously!