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

std.socket: Fix some -dip1000 compilable issues #6204

Closed
wants to merge 1 commit into from
Closed

std.socket: Fix some -dip1000 compilable issues #6204

wants to merge 1 commit into from

Conversation

carblue
Copy link
Contributor

@carblue carblue commented Feb 21, 2018

Fixes these errors:

std/socket.d(2458): Error: function std.socket.softUnittest(void delegate() @safe test, int line = __LINE__) is not callable using argument types (void function() @system)
std/socket.d(2458):        cannot pass argument __lambda1 of type void function() @system to parameter void delegate() @safe test
std/socket.d(3667): Error: scope variable s assigned to non-scope parameter this calling std.socket.Socket.close

https://github.com/dlang/DIPs/blob/master/DIPs/DIP1000.md#scope-function-returns
In that section, the examples that have comment "applies to 'this' reference".

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @carblue! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

@trusted ptrdiff_t receive(void[] buf, SocketFlags flags) { assert(0); }
@safe ptrdiff_t receive(void[] buf) { assert(0); }
@trusted ptrdiff_t receive(void[] buf, SocketFlags flags) scope { assert(0); }
@safe ptrdiff_t receive(void[] buf) scope { assert(0); }
Copy link
Member

@CyberShadow CyberShadow Feb 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot change this class. It is part of a unit test, so by changing it, you are changing a test that has been put in place to guard against breaking user code. This class is here to test for inadvertently breaking user classes which derive from Socket. Any changes here need to have a strong rationale for potentially breaking user code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a conflict of objectives:
To the best of my knowledge - with current DIP1000.md and it's implementation (scope is a part of (member) function's signature/mangling) - I don't see a way to have std.socket -dip1000 compilable without breaking user code (i.e. change class Socket). According to https://github.com/dlang/DIPs/blob/master/DIPs/DIP1000.md#breaking-changes--deprecation-process it's known there will be breaking changes; IMHO this PR is an example for coming breakages, but I now understand - supposed my PR is correct - it shouldn't be merged without a deprecation process in concert with -transition=safe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @andralex @MartinNowak @WalterBright
What's the strategy for such errors resulting from DIP1000 upgrades?
Do we need to introduce some @__future like behavior in the compiler to issue deprecations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It simply means that Socket classes and sub-classes cannot be used as scoped classes with DIP1000, because it's unclear whether they escape sth. or not. It should remain fully useable with GC allocated Socket classes though.
As @CyberShadow said, if Socket is not final we cannot suddenly require all derivatives to obey to not escaping sth.
That's unfortunate but a big restriction with open interface&classes in APIs.
I asked for @future to become useful for such deprecations as well, but it made it to a mentioning in https://github.com/dlang/DIPs/blob/master/DIPs/DIP1007.md#user-content-proposal under 6..
I don't think exempting Socket from DIP1000 is a big issue, the whole module is outdated (by todays D standards) and class based sockets are questionable. Here is a possible replacement https://github.com/MartinNowak/io (though I still need to get return scope correct).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no two-tier memory safety.
D can't ever claim memory safety of @safe code, if it starts to exempt some phobos module(s) from memory checks which possibly may then escape pointers to expired stack frames or alike.
Walter said at DConf2017, he believes there is a tsunami coming and predicted, it will kill the C language. That's my opinion too ref. growing importance of memory safety for language selection.
And D still isn't ahead of the caravan. https://www.youtube.com/watch?v=Lo6Q2vB9AAg

Was there a survey, whether users put up with code breakage mitigated by a reasonable deprecation duration, when they get memory safety in return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take this as PR rejection and will close it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about simply making std.socket @System so it will compile with -dip1000 and move on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!! It's as simple as that to avoid changing class socket and be -dip1000 compilable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it, then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> #6384

@WalterBright
Copy link
Member

Since this is congruent with the D vision, I added the Vision label and this is a priority issue. https://wiki.dlang.org/Vision/2018H1

@WalterBright WalterBright added the Vision Vision Plan https://wiki.dlang.org/Vision/2018H1 label Mar 22, 2018
@wilzbach
Copy link
Member

(I opened #6384 to keep this approach in the GitHub history.)

@wilzbach wilzbach closed this Mar 29, 2018
DIP1000 automation moved this from In progress to Done Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Vision Vision Plan https://wiki.dlang.org/Vision/2018H1
Projects
No open projects
DIP1000
  
Done
6 participants