WIP: Annotate with return or return scope to please dlang/dmd#12520

[nordlow]

Closed because this has been superceeded by a set of sub-pull-requests listed below.

Unblocks dlang/dmd#12520.

make -f posix.mak unittest passes locally for me now when using dlang/dmd#12520.

There are a couple of places where I had to qualify as @trusted to silence. I've commented those places with a TODO.

Furthermore, there are several places where I have no idea how to explain to the compiler that the code should compile when lambdas inside map and file. For instance,

@safe unittest
{
    string[] listdir(string pathname)
    {
        import std.algorithm;
        import std.array;
        import std.file;
        import std.path;

        return std.file.dirEntries(pathname, SpanMode.shallow)
            .filter!(a => a.isFile)
            .map!(a => std.path.baseName(a.name))
            .array;
    }

    void main(string[] args)
    {
        import std.stdio;

        string[] files = listdir(args[1]);
        writefln("%s", files);
     }
}

erroring as

std/file.d(4987): Error: returning `baseName(a.name())` escapes a reference to parameter `a`
std/file.d(4987):        perhaps annotate the parameter with `return`
std/algorithm/iteration.d(524):        instantiated from here: `MapResult!(__lambda3, FilterResult!(__lambda2, DirIterator))`
std/file.d(4987):        instantiated from here: `map!(FilterResult!(__lambda2, DirIterator))`

Not even qualifying listdir as @trusted silences the diagnostics. Shouldn't it?

[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

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

[nordlow]

make -f posix.mak unittest passes locally for me now when using dlang/dmd#12520. There are still disabled tests I'm uncertain how to make the compiler accept. Feel free to take a look, @dkorpel

I don't know why https://app.circleci.com/pipelines/github/dlang/phobos/4197/workflows/22bf7ec0-e91a-4bdf-aec8-6422abb5f439/jobs/13744?invite=true#step-104-95 fails considering the fact that that line has been disabled using version(none) for now.

[dkorpel]

A general comment about BigInt:

A lot of the functions look like this (pseudo code):

BigInt opBinary(BigInt other) {
    if (we're lucky) {
        return [bigint with slice of the existing big digits]
    } else {
        return [newly allocated big digits with computation result]
    }
}

There's two ways to approach this:

• annotate the function return scope so returning a BigInt with a slice of this.data is allowed, but putting an extra burden on the caller (a breaking change)
• annotate the function with scope and defensively .dup this.data in the lucky cases, as the common case has to re-allocate anyway.

I'd say: use return scope when the function is already @nogc (e.g. opUnary!"-") do scope + .dup otherwise.

[dkorpel]

A general comment about this PR:

It gives a neat overview, but please split it up at some point into smaller PR's (one per module / part) so it can be reviewed more easily and regressions can be bisected with more granularity. I happen to have familiarity with bigint, but can't help you with most other modules, and adding return and especially @trusted without a good understanding of each module can be risky.

[dkorpel]

I don't follow. When should these problems arise? I've already verified locally that #8076 builds and runs all the unittests without errors with dlang/dmd#12520.

But does it work when you apply dlang/dmd#10924 or dlang/dmd#12010 to make it an error?
I'm afraid that your deprecations variant dlang/dmd#12520 is not thorough enough, since e.g. you didn't stumble on dlang/druntime#3472 like I did when applying aG0aep6G's fix.

[nordlow]

But does it work when you apply dlang/dmd#10924 or dlang/dmd#12010 to make it an error?

My initial phobos branch buillt without any deprecations when using https://github.com/dlang/dmd/pull/10924/files, yes. As will all the sub-PRs. The local build stopped on a deprecation message.

[WalterBright]

@nordlow I pulled out a couple of files from this PR into separate PRs to make it smaller and more tractable.

[nordlow]

@nordlow I pulled out a couple of files from this PR into separate PRs to make it smaller and more tractable.

Thanks. Note that I already had pulled out the bigint part from this PR at #8081, though.

[RazvanN7]

Does this depend solely on #8085 now?

[nordlow]

Closing this because of sub-PR has been created and out of those only #8085 remains. After that has been merged this PR adds no extra changes.