Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

DIP 1028 Make @safe the default #10709

Open
wants to merge 1 commit into
base: master
from

Conversation

@WalterBright
Copy link
Member

WalterBright commented Jan 2, 2020

Implement the upcoming DIP.

https://github.com/dlang/DIPs/blob/1b705f8d4faa095d6d9e3a1b81d6cfa6d688554b/DIPs/DIP1028.md

@dlang-bot

This comment has been minimized.

Copy link
Contributor

dlang-bot commented Jan 2, 2020

Thanks for your pull request, @WalterBright!

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 fetch digger
dub run digger -- build "master + dmd#10709"
@thewilsonator

This comment has been minimized.

Copy link
Contributor

thewilsonator commented Jan 2, 2020

Could do with some tests.

@WalterBright

This comment has been minimized.

Copy link
Member Author

WalterBright commented Jan 2, 2020

Could do with some tests.

I know. I'll add them tomorrow, which is why this is WIP.

@WalterBright WalterBright changed the title make @safe the default DIP 1028 Make @safe the default Jan 2, 2020
@WalterBright WalterBright force-pushed the WalterBright:safeDefault branch 2 times, most recently from 58466a2 to 9146326 Jan 4, 2020
@WalterBright

This comment has been minimized.

@WalterBright WalterBright removed the WIP label Jan 4, 2020
@WalterBright

This comment has been minimized.

Copy link
Member Author

WalterBright commented Jan 4, 2020

Added the test cases.

@WebFreak001

This comment has been minimized.

Copy link
Member

WebFreak001 commented Jan 4, 2020

I think there might be an issue in CTFE with this here:

@safe:
import std.bitmanip;

struct X
{
        @safe:
        mixin(bitfields!(
                uint, "hello", 4,
                uint, "foo", 4
        ));
}

in theory, because it's all marked as @safe, there should be no change in behavior, however with the -preview=safedefault flag this now prints:

/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(64): Error: cast from char[] to string not allowed in safe code
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(118):        called from here: myToString(8388607LU)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(176): Error: template instance std.bitmanip.createAccessors!("_fraction_exponent_sign", uint, "fraction", 23LU, 0LU) error instantiating
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(169):        instantiated from here: createFields!("_fraction_exponent_sign", 0LU, uint, "fraction", 23, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(252):        instantiated from here: createStorageAndFields!(uint, "fraction", 23, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(702):        instantiated from here: bitfields!(uint, "fraction", 23, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(118): Error: CTFE failed because of previous errors in myToString
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(176): Error: template instance std.bitmanip.createAccessors!("_fraction_exponent_sign", ubyte, "exponent", 8LU, 23LU) error instantiating
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(177):        instantiated from here: createFields!("_fraction_exponent_sign", 23LU, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(169):        instantiated from here: createFields!("_fraction_exponent_sign", 0LU, uint, "fraction", 23, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(252):        instantiated from here: createStorageAndFields!(uint, "fraction", 23, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(702):        instantiated from here: bitfields!(uint, "fraction", 23, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(107): Error: CTFE failed because of previous errors in myToString
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(176): Error: template instance std.bitmanip.createAccessors!("_fraction_exponent_sign", bool, "sign", 1LU, 31LU) error instantiating
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(177):        instantiated from here: createFields!("_fraction_exponent_sign", 31LU, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(177):        instantiated from here: createFields!("_fraction_exponent_sign", 23LU, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(169):        instantiated from here: createFields!("_fraction_exponent_sign", 0LU, uint, "fraction", 23, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(252):        instantiated from here: createStorageAndFields!(uint, "fraction", 23, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(702):        instantiated from here: bitfields!(uint, "fraction", 23, ubyte, "exponent", 8, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(118): Error: CTFE failed because of previous errors in myToString
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(176): Error: template instance std.bitmanip.createAccessors!("_fraction_exponent_sign", ulong, "fraction", 52LU, 0LU) error instantiating
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(169):        instantiated from here: createFields!("_fraction_exponent_sign", 0LU, ulong, "fraction", 52, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(252):        instantiated from here: createStorageAndFields!(ulong, "fraction", 52, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(783):        instantiated from here: bitfields!(ulong, "fraction", 52, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(118): Error: CTFE failed because of previous errors in myToString
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(176): Error: template instance std.bitmanip.createAccessors!("_fraction_exponent_sign", ushort, "exponent", 11LU, 52LU) error instantiating
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(177):        instantiated from here: createFields!("_fraction_exponent_sign", 52LU, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(169):        instantiated from here: createFields!("_fraction_exponent_sign", 0LU, ulong, "fraction", 52, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(252):        instantiated from here: createStorageAndFields!(ulong, "fraction", 52, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(783):        instantiated from here: bitfields!(ulong, "fraction", 52, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(107): Error: CTFE failed because of previous errors in myToString
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(176): Error: template instance std.bitmanip.createAccessors!("_fraction_exponent_sign", bool, "sign", 1LU, 63LU) error instantiating
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(177):        instantiated from here: createFields!("_fraction_exponent_sign", 63LU, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(177):        instantiated from here: createFields!("_fraction_exponent_sign", 52LU, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(169):        instantiated from here: createFields!("_fraction_exponent_sign", 0LU, ulong, "fraction", 52, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(252):        instantiated from here: createStorageAndFields!(ulong, "fraction", 52, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(783):        instantiated from here: bitfields!(ulong, "fraction", 52, ushort, "exponent", 11, bool, "sign", 1)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(118): Error: CTFE failed because of previous errors in myToString
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(176): Error: template instance std.bitmanip.createAccessors!("_hello_foo", uint, "hello", 4LU, 0LU) error instantiating
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(169):        instantiated from here: createFields!("_hello_foo", 0LU, uint, "hello", 4, uint, "foo", 4)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(252):        instantiated from here: createStorageAndFields!(uint, "hello", 4, uint, "foo", 4)
a.d(7):        instantiated from here: bitfields!(uint, "hello", 4, uint, "foo", 4)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(118): Error: CTFE failed because of previous errors in myToString
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(176): Error: template instance std.bitmanip.createAccessors!("_hello_foo", uint, "foo", 4LU, 4LU) error instantiating
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(177):        instantiated from here: createFields!("_hello_foo", 4LU, uint, "foo", 4)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(169):        instantiated from here: createFields!("_hello_foo", 0LU, uint, "hello", 4, uint, "foo", 4)
/tmp/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/bitmanip.d(252):        instantiated from here: createStorageAndFields!(uint, "hello", 4, uint, "foo", 4)
a.d(7):        instantiated from here: bitfields!(uint, "hello", 4, uint, "foo", 4)
@WebFreak001

This comment has been minimized.

Copy link
Member

WebFreak001 commented Jan 4, 2020

delegates don't have the @safe attribute by default with this (maybe out of scope as not defined in DIP?)

void foo(void delegate() x)
{
        x();
}

gives

a.d(9): Error: @safe function a.foo cannot call @system delegate x

advantages of this:

  • fixes existing toString functions with sink parameter being used

disadvantages of this:

  • breaks previous functions with multiple overloads, one being @safe delegate and the other being a normal delegate (e.g. vibe.d)
@WalterBright WalterBright force-pushed the WalterBright:safeDefault branch from 9146326 to 0ab7e90 Jan 6, 2020
@WalterBright

This comment has been minimized.

Copy link
Member Author

WalterBright commented Jan 6, 2020

@WebFreak001 fixed it.

@Geod24

This comment has been minimized.

Copy link
Member

Geod24 commented Jan 8, 2020

The commit name is quite confusing, it should mention the introduction of a switch, not "Make @safe the default".

As discussed, a simple "Hello World" (using writeln) will not work without work on druntime/phobos:

Undefined symbols for architecture x86_64:
  "__D3std5stdio14fputc_unlockedFNbNiNfiPS4core4stdcQBq7__sFILEZi", referenced from:
      __D3std5stdio4File17LockingTextWriter__T3putTyaZQiMFNfyaZ12trustedFPUTCFNbNiNeiPS4core4stdcQDg7__sFILEZi in helloworld.o
      __D3std5stdio4File17LockingTextWriter__T3putTaZQhMFNfaZ12trustedFPUTCFNbNiNeiPS4core4stdcQDe7__sFILEZi in helloworld.o
  "__D3std5stdio15fputwc_unlockedFNbNiNfwPS4core4stdcQBr7__sFILEZi", referenced from:
      __D3std5stdio4File17LockingTextWriter__T3putTyaZQiMFNfyaZ13trustedFPUTWCFNbNiNewPS4core4stdcQDh7__sFILEZi in helloworld.o
      __D3std5stdio4File17LockingTextWriter__T3putTaZQhMFNfaZ13trustedFPUTWCFNbNiNewPS4core4stdcQDf7__sFILEZi in helloworld.o
  "__D6object8TypeInfo8postblitMxFNfPvZv", referenced from:
      __D6object__T12_getPostblitTyaZQsFNaNbNiNeZDFNaNbNiNfKyaZv in helloworld.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Error: linker exited with status 1

Likewise, any call to printf (and most C functions) will need to be wrapped in a @trusted delegate. That's a big hit for usability, or anyone using D primarily to interface with C/C++, something D has been praised for.

Trying to compile druntime with this switch results in:

src/object.d(170): Error: cast from const(Object) to object.Object not allowed in safe code
src/object.d(170): Error: cast from const(Object) to object.Object not allowed in safe code
src/object.d(356): Error: cast from void* to byte* not allowed in safe code
src/object.d(357): Error: cast from void* to byte* not allowed in safe code
src/object.d(358): Error: cast from void* to byte* not allowed in safe code
src/object.d(417): Error: scope variable p1 assigned to non-scope parameter p1 calling object.TypeInfo.equals
src/object.d(417): Error: scope variable p2 assigned to non-scope parameter p2 calling object.TypeInfo.equals
src/object.d(418): Error: scope variable p1 assigned to non-scope parameter p1 calling object.TypeInfo.compare
src/object.d(418): Error: scope variable p2 assigned to non-scope parameter p2 calling object.TypeInfo.compare
src/object.d(472): Error: cast from const(void*) to void** not allowed in safe code
src/object.d(472): Error: cast from const(void*) to void** not allowed in safe code
src/object.d(477): Error: cast from const(void*) to void** not allowed in safe code
src/object.d(477): Error: cast from const(void*) to void** not allowed in safe code
src/object.d(479): Error: cast from const(void*) to void** not allowed in safe code
src/object.d(479): Error: cast from const(void*) to void** not allowed in safe code
src/object.d(497): Error: cast from void* to void** not allowed in safe code
src/object.d(498): Error: cast from void* to void** not allowed in safe code
src/object.d(499): Error: cast from void* to void** not allowed in safe code
src/object.d(528): Error: cast from const(void*) to void[]* not allowed in safe code
src/object.d(529): Error: cast from const(void*) to void[]* not allowed in safe code

And since we're limited to 20 errors, I suspect this is just the tip of the iceberg.
If we want this to be useful, it would be great to fix at least druntime before we move forward.

OS: Mac OSX 10.15
HOST_DMD: 2.090.0 (via Homebrew)
Commands (with the proper git checkout and make clean in between):

$ cd projects/dlang/dmd
$ git checkout walter/safeDefault
$ make -f posix.mak -j8 install
$ cd ../druntime
# At this point, one can choose to insert `-preview=safedefault` to compiler druntime
$ make -f posix.mak -j8 install
$ cd ../phobos/
# Likewise, `-preview=safedefault` in DFLAGS (didn't get that far because druntime didn't compile)
$ make -f posix.mak -j8 install
$ cd ../install
$ echo 'import std.stdio; void main() { writeln("Hello World"); }' > helloworld.d
$ ./osx/bin/dmd -preview=safedefault helloworld.d

By the way, the point where I inserted the -preview=safedefault flag was here for druntime and here for Phobos.

@WalterBright

This comment has been minimized.

Copy link
Member Author

WalterBright commented Jan 9, 2020

@Geod24 I expect there'll be fixes needed in druntime/phobos. I don't mind doing that work, but I don't see much point until this PR is merged.

@Geod24

This comment has been minimized.

Copy link
Member

Geod24 commented Jan 9, 2020

The point was to be able to use, and by extension test, this switch (since at the moment absolutely nothing compiles when turned on, I can't even run an hello world).

@atilaneves

This comment has been minimized.

Copy link
Contributor

atilaneves commented Jan 9, 2020

@Geod24 I think the point is things can't be fixed until this is merged. It needs a command-line switch to turn the functionality on anyway.

@Geod24

This comment has been minimized.

Copy link
Member

Geod24 commented Jan 14, 2020

@atilaneves : Nothing prevents druntime and Phobos to be fixed without merging this PR. Anyone working on DMD routinely needs to test a feature / deprecation / bug fix with at least druntime / phobos, and quite frequently other projects as well.
Doing that work upfront would give us a much better idea of how the feature will play out in practice. Plus, it wouldn't be lost work, because I believe we want to make our core libraries @safe, so even if DIP1028 was to be rejected (which I doubt it will), it wouldn't be lost work.

But that's just my 0.02$, and if you two think it's better to proceed with this now, let's get it merged. However, could @WalterBright fix the commit message to "Introduce -preview=safeDefault" or something similar ?

@Geod24

This comment has been minimized.

Copy link
Member

Geod24 commented Jan 14, 2020

I didn't want to overcrowd the last message, but there's a few things I'd like to add regarding DIP1028.

The "valuable feedback" I mentioned previously might include, but not be limited to:

  • "oh we can't link things together when using this switch";
  • "hum that doesn't play well with delegates because we now require @safe delegate everywhere";
  • "oops that doesn't work so well with anything OOP either";
  • "ahem maybe we should exclude extern(...) declarations because they really shouldn't be @safe by default";

And this is just from the top of my head. Most of my issues with DIP1028 is that you cannot live in a @system world with a bit of @safety. @safe by default won't fix that, it will just make life worse for people that like their @system world. Because suddenly the delegate I pass to Throwable.toString needs to be @safe, but I'm calling printf in that function. Or because my override of opCmp needs to be @safe, but I'm calling qsort_r.

@WalterBright WalterBright force-pushed the WalterBright:safeDefault branch from 0ab7e90 to 43649c1 Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.