[RFC] Add deprecation message for extern functions with default safety

[pbackus]

This message is needed to avoid silently introducing safety violations
to existing code when the default is changed from @system to @safe. It
should become an error when @safe is made the default.

---

This is an attempt to provide a compiler diagnostic for the potential safety violations that would be introduced by DIP 1028, as it is currently written. (See e.g. [1] for a more detailed explanation.)

I've implemented it as a deprecation, with the recommendation that it become an error when -preview=safedefault is enabled. If necessary, it can be downgraded to a warning--the important thing is to ensure that safety is not violated silently.

[1] https://forum.dlang.org/post/ra8t3k$q09$1@digitalmars.com

[dlang-bot]

Thanks for your pull request and interest in making D better, @pbackus! 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 coverage diff by visiting the details link of the codecov check)
• 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.

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

[MoonlightSentinel]

Interesting idea. This could theoretically exclude extern(D) functions because missmatched safety levels would cause a linker error. But having consistent rules independent of linkage type is probably the way to go.

[PetarKirov]

Thanks @pbackus, I think the best way to go it to require non-extern (D) function declarations to be explicitly marked with @system or @trusted. In the worst case, if this change is not accepted to the language, this pull request is still valuable, as it allows us to see the potential effect on the ecosystem of extern (C) functions implicitly becoming @safe.

[pbackus]

@PetarKirov Whether @safe or @trusted ought to be used for non-extern (D) functions was a matter of some debate in the DIP 1028 discussion thread [1][2]. Reasonable arguments can be made on both sides.

Personally, I think the value of this PR is independent of the outcome of that debate, so I've decided to punt on the issue for now.

[1] https://forum.dlang.org/post/r5hfe5$u8g$1@digitalmars.com
[2] https://forum.dlang.org/post/mailman.2442.1585203310.31109.digitalmars-d@puremagic.com

[atilaneves]

extern(D) declarations should probably be deprecated as well. Definitions on the other hand, are checked by the compiler.

[pbackus]

@atilaneves This PR applies to all extern function declarations without a body, including extern (D). I'll add an extern (D) example to the test-case to clarify that.

[thewilsonator]

ERROR: src/dmd/backend/code.d(276): Deprecation: extern function src/dmd.backend.code.seg_data_isCode should be marked explicitly as @safe, @system, or @trusted
src/dmd/backend/code.d(319): Deprecation: extern function src/dmd.backend.code.FuncParamRegs_create should be marked explicitly as @safe, @system, or @trusted
src/dmd/backend/code.d(320): Deprecation: extern function src/dmd.backend.code.FuncParamRegs_alloc should be marked explicitly as @safe, @system, or @trusted
/home/circleci/src/dmd/src/src/dmd/backend/elfobj.d(56): Deprecation: extern function src/dmd.backend.elfobj.symbol_iscomdat2 should be marked explicitly as @safe, @system, or @trusted

[pbackus]

@thewilsonator #11179

[pbackus]

Update: it turns out there are a lot of extern (C) functions that don't actually have the extern storage class. So we need to check linkage attributes too.

[thewilsonator]

src/core/sys/freebsd/sys/link_elf.d(76): Deprecation: `extern` function `core.sys.freebsd.sys.link_elf.dl_iterate_phdr` should be marked explicitly as `@safe`, `@system`, or `@trusted`
src/core/sys/freebsd/sys/link_elf.d(77): Deprecation: `extern` function `core.sys.freebsd.sys.link_elf.dl_iterate_phdr` should be marked explicitly as `@safe`, `@system`, or `@trusted`
src/core/sys/freebsd/sys/link_elf.d(78): Deprecation: `extern` function `core.sys.freebsd.sys.link_elf._rtld_addr_phdr` should be marked explicitly as `@safe`, `@system`, or `@trusted`
src/core/sys/freebsd/sys/link_elf.d(79): Deprecation: `extern` function `core.sys.freebsd.sys.link_elf._rtld_get_stack_prot` should be marked explicitly as `@safe`, `@system`, or `@trusted`
src/core/sys/freebsd/sys/link_elf.d(76): Deprecation: `extern` function `core.sys.freebsd.sys.link_elf.dl_iterate_phdr` should be marked explicitly as `@safe`, `@system`, or `@trusted`
src/core/sys/freebsd/sys/link_elf.d(77): Deprecation: `extern` function `core.sys.freebsd.sys.link_elf.dl_iterate_phdr` should be marked explicitly as `@safe`, `@system`, or `@trusted`
src/core/sys/freebsd/sys/link_elf.d(78): Deprecation: `extern` function `core.sys.freebsd.sys.link_elf._rtld_addr_phdr` should be marked explicitly as `@safe`, `@system`, or `@trusted`
src/core/sys/freebsd/sys/link_elf.d(79): Deprecation: `extern` function `core.sys.freebsd.sys.link_elf._rtld_get_stack_prot` should be marked explicitly as `@safe`, `@system`, or `@trusted`

[Geod24]

You are missing quite a lot of functions. E.g. everything in this module (the backend has a lot of those):

dmd/src/dmd/backend/cgcv.d

Lines 22 to 24 in a0af9fa

	extern (C++):
	@nogc:
	nothrow:

[pbackus]

@Geod24 I've updated the check; it should catch everything now.

Test-suite changes are split into a separate commit to make reviewing easier. There are still un-annotated declarations in druntime and phobos that need to be updated to make the full test-suite pass. PRs for both forthcoming.

[WalterBright]

Given the large number of changes this requires, it looks impractical.

[pbackus]

@WalterBright The changes in question are required by DIP 1028, regardless of whether this is merged or not. Since you yourself recently submitted one such change in dlang/druntime#3009, it seems we are in agreement about that.

All this message does is makes the declarations that need updating easier to find.

[pbackus]

Closing this now that DIP 1028 has been withdrawn.