WIP: fix Issue 20150 - -dip1000 return attribute must be explicit in pure (use deprecation instead of error)

[nordlow]

Adjust #10924 to emit a deprecation instead of error and see how that goes.

After this gets merged the adding of @trusted qualifiers in the changes at

• Annotate std/json.d to please dlang/dmd#12520 phobos#8088

need to be reverted.

[dlang-bot]

Thanks for your pull request and interest in making D better, @nordlow! 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

Auto-close	Bugzilla	Severity	Description
✓	20150	major	-dip1000 defeated by pure

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

[thewilsonator]

This a a serious bug, please target stable.

[nordlow]

WARNING: fail_compilation/retscope6.d has multiple `TEST_OUTPUT` blocks and can't be auto-updated

Ok to split retscope6.d into 7,8,9, etc, so AUTO_UPDATE works again. @thewilsonator ?

[thewilsonator]

yes

[nordlow]

yes

Good, I'll fix.

[nordlow]

What to do about druntime failing?

src/core/thread/osthread.d(988): Deprecation: `pure` function `toThread` returns parameter `t`, annotate with `return`
src/core/thread/osthread.d(988): Deprecation: `pure` function `toThread` returns parameter `t`, annotate with `return`

Annotate druntime in another PR before we can merge this PR right?

[thewilsonator]

Annotate druntime in another PR before we can merge this PR right?

Seems so, yes.

[nordlow]

Phobos now triggers

std/bigint.d(460): Deprecation: `pure` function `opOpAssign` returns parameter `this`, annotate with `return`
std/bigint.d(481): Error: template instance `std.bigint.BigInt.opOpAssign!("*", BigInt)` error instantiating
std/bigint.d(2335):        instantiated from here: `opBinary!("*", BigInt)`
std/bigint.d(460): Deprecation: `pure` function `opOpAssign` returns parameter `this`, annotate with `return`
std/bigint.d(481): Error: template instance `std.bigint.BigInt.opOpAssign!("%", BigInt)` error instantiating
std/bigint.d(2338):        instantiated from here: `opBinary!("%", BigInt)`
std/bigint.d(460): Deprecation: `pure` function `opOpAssign` returns parameter `this`, annotate with `return`
std/bigint.d(481): Error: template instance `std.bigint.BigInt.opOpAssign!("*", BigInt)` error instantiating
std/bigint.d(2335):        instantiated from here: `opBinary!("*", BigInt)`
std/bigint.d(310): Deprecation: `pure` function `opOpAssign` returns parameter `this`, annotate with `return`
std/bigint.d(350): Deprecation: `pure` function `opOpAssign` returns parameter `this`, annotate with `return`
std/bigint.d(2340): Error: template instance `std.bigint.BigInt.opOpAssign!(">>", int)` error instantiating
std/bigint.d(460): Deprecation: `pure` function `opOpAssign` returns parameter `this`, annotate with `return`
std/bigint.d(481): Error: template instance `std.bigint.BigInt.opOpAssign!("%", BigInt)` error instantiating
std/bigint.d(2338):        instantiated from here: `opBinary!("%", BigInt)`
std/bitmanip.d(1622): Deprecation: `pure` function `reverse` returns parameter `this`, annotate with `return`
std/bitmanip.d(1682): Deprecation: `pure` function `sort` returns parameter `this`, annotate with `return`
std/bigint.d(310): Deprecation: `pure` function `opOpAssign` returns parameter `this`, annotate with `return`
std/bigint.d(350): Deprecation: `pure` function `opOpAssign` returns parameter `this`, annotate with `return`
std/bigint.d(2340): Error: template instance `std.bigint.BigInt.opOpAssign!(">>", int)` error instantiating
std/bitmanip.d(1622): Deprecation: `pure` function `reverse` returns parameter `this`, annotate with `return`
std/bitmanip.d(1682): Deprecation: `pure` function `sort` returns parameter `this`, annotate with `return`
std/algorithm/searching.d(1608): Deprecation: `pure` function `trustedMemchr` returns parameter `haystack`, annotate with `return`
std/algorithm/searching.d(1608): Deprecation: `pure` function `trustedMemchr` returns parameter `haystack`, annotate with `return`
std/algorithm/searching.d(1608): Deprecation: `pure` function `trustedMemchr` returns parameter `haystack`, annotate with `return`
std/algorithm/searching.d(1608): Deprecation: `pure` function `trustedMemchr` returns parameter `haystack`, annotate with `return`
std/format/internal/write.d(610): Error: template instance `std.algorithm.searching.find!("a == b", string, const(char))` error instantiating
std/format/write.d(1239):        instantiated from here: `formatValueImpl!(void delegate(const(char)[]) pure nothrow @safe, const(real), char)`
std/complex.d(164):        instantiated from here: `formatValue!(void delegate(const(char)[]) pure nothrow @safe, const(real), char)`
std/complex.d(135):        instantiated from here: `toString!(void delegate(const(char)[]) pure nothrow @safe, char)`
std/complex.d(1231):        instantiated from here: `Complex!real`
std/format/internal/write.d(610): Error: template instance `std.algorithm.searching.find!("a == b", string, const(char))` error instantiating
std/format/write.d(1239):        instantiated from here: `formatValueImpl!(void delegate(const(char)[]) pure nothrow @safe, const(real), char)`
std/complex.d(164):        instantiated from here: `formatValue!(void delegate(const(char)[]) pure nothrow @safe, const(real), char)`
std/complex.d(135):        instantiated from here: `toString!(void delegate(const(char)[]) pure nothrow @safe, char)`
std/complex.d(1231):        instantiated from here: `Complex!real`
std/file.d(3976): Deprecation: `pure` function `name` returns parameter `this`, annotate with `return`
std/file.d(3976): Deprecation: `pure` function `name` returns parameter `this`, annotate with `return`
std/json.d(159): Deprecation: `pure` function `str` returns parameter `this`, annotate with `return`
std/json.d(159): Deprecation: `pure` function `str` returns parameter `this`, annotate with `return`
std/json.d(165): Deprecation: `pure` function `str` returns parameter `v`, annotate with `return`
std/json.d(165): Deprecation: `pure` function `str` returns parameter `v`, annotate with `return`
std/json.d(288): Deprecation: `pure` function `object` returns parameter `v`, annotate with `return`
std/json.d(288): Deprecation: `pure` function `object` returns parameter `v`, annotate with `return`
std/json.d(334): Deprecation: `pure` function `array` returns parameter `v`, annotate with `return`
std/json.d(334): Deprecation: `pure` function `array` returns parameter `v`, annotate with `return`
std/json.d(641): Deprecation: `pure` function `opIndex` returns parameter `k`, annotate with `return`
std/json.d(641): Deprecation: `pure` function `opIndex` returns parameter `k`, annotate with `return`
make: *** [posix.mak:342: generated/linux/release/64/libphobos2.a] Fel 1

To alleviate, shall I qualify as return or return scope, @thewilsonator?

For the class members I shall only use return, right? And return scope for structs members?

[thewilsonator]

To alleviate, shall I qualify as return or return scope

as I understand it return scope is a stronger restriction/guarantee than return, so use return scope where you can and return where return scope would not be appropriate.

For the class members I shall only use return, right? And return scope for structs members?

don't know.

[nordlow]

Will qualifying as return scope instead of return increase the risk of other projects failing to use phobos?

[thewilsonator]

As I understand, return scope is an onus on the callee not the caller.

[nordlow]

As I understand, return scope is an onus on the callee not the caller.

Indeed. So in fact qualifying members as return scope instead of return should indeed allow more code to call them.

[dkorpel]

Indeed. So in fact qualifying members as return scope instead of return should indeed allow more code to call them.

While I can't find anything explicitly indicating it, to me it seems like return scope is equal to return. It's not like just return allows you to escape the reference through global variables or non-scope parameters or anything. Can anyone provide an example on how they differ?

[nordlow]

While I can't find anything explicitly indicating it, to me it seems like return scope is equal to return. It's not like just return allows you to escape the reference through global variables or non-scope parameters or anything. Can anyone provide an example on how they differ?

I only this for now https://github.com/dlang/DIPs/blob/master/DIPs/other/DIP1000.md

[dkorpel]

When I first saw you suggest to use deprecations, I didn't think it was a good idea, and I agree with aG0aep6G: #12010 (comment)
However, it seems to do an excellent job at pin-pointing spots in Phobos that need to be updated, unlike applying the bug-fix directly which results in a lot of "this @safe unittest can't call function X which is now inferred @system because 8 templates deep some @safe inference failed". So thanks for making this PR.

[nordlow]

However, it seems to do an excellent job at pin-pointing spots in Phobos that need to be updated, unlike applying the bug-fix directly which results in a lot of "this @safe unittest can't call function X which is now inferred @system because 8 templates deep some @safe inference failed". So thanks for making this PR.

Great. I'm currently sifting through adjusting Phobos at dlang/phobos#8076. I'd be very happy to help figure if this is possible or not. You can look at the diff and search for TODO to get to the tricky parts.

[nordlow]

Gonna target master until all things work and then change to stable.

[RazvanN7]

Gonna target master until all things work and then change to stable.

That's not gonna work because your PRs from phobos targeted master, not stable.

[nordlow]

That's not gonna work because your PRs from phobos targeted master, not stable.

So I guess I should merge to dmd master first, @RazvanN7 ?

[RazvanN7]

Yes.

[nordlow]

Rebased and squashed.

Removed WP from title.

Let's see if third-party libraries in Circle CI trigger any deprecations. If they do but they pass is it ok to merge anyway?

[nordlow]

Seems Walter has recently updated the PR #10924 this PR is based upon. I'll notify him. That phobos is ready now.

[aG0aep6G]

Let's see if third-party libraries in Circle CI trigger any deprecations. If they do but they pass is it ok to merge anyway?

In my opinion, this is not ok to merge at all. Just like #10924, this introduces a rejects-valid regression.

[nordlow]

Hmm, the dmd tests fail. Need to investigate.

[nordlow]

This PR introduces a regression in scope analysis in fail20108.d.

I update the test to make the diff highlight how the diagnostics is missing the line

fail_compilation/fail20108.d(16): Error: scope variable `x` may not be returned

I could try to adjust the changes in escape.d but that's not my territory. Would it be possible for you to have a look , @WalterBright ?

[nordlow]

In my opinion, this is not ok to merge at all. Just like #10924, this introduces a rejects-valid regression.

Ok. Anyway the druntime and phobos have all been updated to allow compilation via this dmd. Should I work on making druntime and phobos build with your #12010 aswell, @aG0aep6G ?

Moroever, does #12010 completely superceede #10924?

[aG0aep6G]

Ok. Anyway the druntime and phobos have all been updated to allow compilation via this dmd. Should I work on making druntime and phobos build with your #12010 aswell, @aG0aep6G ?

That would be great.

By the way, if you mention someone in an edit, they don't get an email notification. I only noticed this because I had it open in the browser. If you want to reach someone, better post a new comment.

Moroever, does #12010 completely superceede #10924?

yes

[nordlow]

Closing because this has been superceeded by #12010.

Right, @dkorpel?

[dkorpel]

Right, @dkorpel?

Yes