fix Issue 20150 - -dip1000 defeated by pure

[aG0aep6G]

This is an alternative to #10924, fixing issue 20150 without introducing a rejects-valid regression.

Phobos relies on issue 20150, so this is expected to fail the test suite.

[dlang-bot]

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

[RazvanN7]

@aG0aep6G What is the status on this? Are you planing on updating phobos to pass so that this passes the test suite?

[aG0aep6G]

@aG0aep6G What is the status on this? Are you planing on updating phobos to pass so that this passes the test suite?

I had started on it, but it's tedious work tracking down the errors through templates and overloads. If I remember correctly, dup gave me some trouble, too.

So I've put it on ice for the time being. If someone else wants to give it a shot, that would be great.

[nordlow]

@RazvanN7 and @aG0aep6G: What about modifying this diagnostics to emit a deprecation message for the new case not covered by phobos and many other projects? To buy us time to adjust Phobos and many other projects before this diagnostics is turned into a new error and in the meantime have a way to detect vulnerabilities recently discussed at https://forum.dlang.org/thread/jnkdcngzytgtobihzggj@forum.dlang.org?page=1

[aG0aep6G]

What about modifying this diagnostics to emit a deprecation message for the new case not covered by phobos and many other projects?

I don't think that's a good idea. Issue 20150 is a straight-forward bug. Let's not bend over backwards to accommodate buggy code.

[nordlow]

What about modifying this diagnostics to emit a deprecation message for the new case not covered by phobos and many other projects?

I don't think that's a good idea. Issue 20150 is a straight-forward bug. Let's not bend over backwards to accommodate buggy code.

Agreed. My motive was about getting this diagnostics into the compiler without having to first correct loads of failing dub-packages in buildkite [1]. It would be nice to be able to see how many of those that break wihtout having to first correct all the failing unittests in phobos (being the one remaining).

[1] https://buildkite.com/dlang/dmd/builds/19701

[nordlow]

Have you studied the 130 dmd tests that fail with this change and the phobos error messages that are triggered by this?

For instance

@safe pure nothrow unittest
{
        assert("Hello D".array == "Hello D"d);
}

fails as

std/array.d(294): Error: `@safe` function `std.array.__unittest_L287_C20` cannot call `@system` function `std.array.array!string.array`
std/array.d(275):        `std.array.array!string.array` is declared here

The number of errors triggered is massive.

[aG0aep6G]

For instance

@safe pure nothrow unittest
{
        assert("Hello D".array == "Hello D"d);
}

fails as

std/array.d(294): Error: `@safe` function `std.array.__unittest_L287_C20` cannot call `@system` function `std.array.array!string.array`
std/array.d(275):        `std.array.array!string.array` is declared here

Going down that one rabbit hole:

1. Temporarily add @safe to std.array.array!string.array → DMD says: "scope variable str assigned to non-scope parameter s calling std.utf.toUTF32!string.toUTF32".
2. Permanently(?) add scope to std.utf.toUTF32!string.toUTF32's parameter → DMD says: "@safe function std.array.array!string.array cannot call @system function std.utf.toUTF32!string.toUTF32".
3. Temporarily add @safe to std.utf.toUTF32!string.toUTF32 → DMD says: "scope variable s assigned to non-scope parameter s calling std.utf.toUTFImpl!(dstring, string).toUTFImpl".
4. Permanently(?) add scope to std.utf.toUTFImpl!(dstring, string).toUTFImpl → DMD says: "1 modules passed unittests". Success?
5. Remove the temporary @safes, check out master DMD, run std.utf's tests. Still works. Success!
6. Make a PR for those two scopes: add scope to parameters of toUTF32 and toUTFImpl phobos#8109

2 scopes down; many, many more to go. It's a chore. Maybe there's a better way to find the places where scope is needed. What you did with the deprecation looked good.

The number of errors triggered is massive.

Yeah, issue 20150 is a serious bug.

[nordlow]

The number of errors triggered is massive.

Yeah, issue 20150 is a serious bug.

Thank you!

[nordlow]

1. Temporarily add `@safe` to `std.array.array!string.array` → DMD says: "scope variable `str` assigned to non-scope parameter `s` calling std.utf.toUTF32!string.toUTF32".

2. Permanently(?) add `scope` to `std.utf.toUTF32!string.toUTF32`'s parameter → DMD says: "`@safe` function `std.array.array!string.array` cannot call `@system` function `std.utf.toUTF32!string.toUTF32`".

3. Temporarily add `@safe` to `std.utf.toUTF32!string.toUTF32` → DMD says: "scope variable `s` assigned to non-scope parameter `s` calling std.utf.toUTFImpl!(dstring, string).toUTFImpl".

4. Permanently(?) add `scope` to `std.utf.toUTFImpl!(dstring, string).toUTFImpl` → DMD says: "1 modules passed unittests". Success?

5. Remove the temporary `@safe`s, check out master DMD, run `std.utf`'s tests. Still works. Success!

6. Make a PR for those two `scope`s: [dlang/phobos#8109](https://github.com/dlang/phobos/pull/8109)

2 scopes down; many, many more to go. It's a chore. Maybe there's a better way to find the places where scope is needed. What you did with the deprecation looked good.

Could such tedious manual labour be partially automated by an extension of #12383, @MoonlightSentinel ?

[dkorpel]

@nordlow for dlang/druntime#3480 I hacked dmd to warn about violations without giving errors or setting functions to @system: dkorpel@7d1f0c5
If you want to work on Phobos, it may be useful to you as well.

[dkorpel]

I just realized there should also be a check that the function is nothrow, or else parameters could escape using Exceptions.

import std.stdio;

void escape(string x) @safe pure {
    throw new Exception(x);
}

void main() @safe {
    static string global;
    immutable(char)[4] local;
    try {
        escape(local[]); 
    } catch(Exception e) {
        global = e.msg;
    }
    assert(&local[0] != &global[0]); // fails
}

Though that might be better to do in a follow-up PR, so we can get the fix for the more common case in quicker.

[dkorpel]

Now dlang/phobos#8113 has been merged, can you rebase this @aG0aep6G? Phobos and druntime now compile locally for me on Linux, so I wonder about other platforms and buildkite.

[aG0aep6G]

can you rebase this @aG0aep6G?

rebased

[nordlow]

Looks we have some work adjusting DUB packages.

[nordlow]

Strange error

../dmd/generated/linux/release/64/dmd  -conf= -I../druntime/import  -w -de -preview=dip1000 -preview=dtorfields -m64 -fPIC -O -release -defaultlib= -debuglib= -L-lpthread -L-lm -of=generated/linux/release/64/betterctests/betterc_module_tests -betterC -unittest test/betterc_module_tests.d std/sumtype.d
--

std/sumtype.d(998): Error: `TypeInfo` cannot be used with -betterC

at https://buildkite.com/dlang/dmd/builds/21202#25829e63-52ab-4f46-a04f-c8b88e0d27ba/456-1553.

[dkorpel]

Looks we have some work adjusting DUB packages.

First one: atilaneves/unit-threaded#254

[nordlow]

Looks we have some work adjusting DUB packages.

First one: atilaneves/unit-threaded#254

I believe @atilaneves is on vacation. :)

[dkorpel]

Strange error

Probably because an array literal used in a -betterC unittest that used to be on the stack is now GC allocated, since parameters of pure functions are not automatically scope anymore.

[nordlow]

Now that atilaneves/unit-threaded#254 is merged what remains, @dkorpel?

[dkorpel]

Now that atilaneves/unit-threaded#254 is merged what remains, @dkorpel?

I think @atilaneves needs to ensure automem uses the latest unit-threaded (with the scope fixes), and then @aG0aep6G should rebase again, and hopefully it's green then.

[nordlow]

Now that atilaneves/unit-threaded#254 is merged what remains, @dkorpel?

I think @atilaneves needs to ensure automem uses the latest unit-threaded (with the scope fixes), and then @aG0aep6G should rebase again, and hopefully it's green then.

The project kaleidicassociates/excel-d also seem to need som tlc; https://buildkite.com/dlang/dmd/builds/21202#f8d3a21a-304b-4a32-9e9e-dfa7263075d4/115-145.

[dkorpel]

Should be fixed by symmetryinvestments/excel-d#95

[nordlow]

Should be fixed by symmetryinvestments/excel-d#95

Great. Thanks.

[aG0aep6G]

Should be fixed by symmetryinvestments/excel-d#95

excel-d is now the last red project on buildkite.

[nordlow]

Should be fixed by symmetryinvestments/excel-d#95

excel-d is now the last red project on buildkite.

Seems like a trivial fix. I'll look into it.

[nordlow]

excel-d is now the last red project on buildkite.

Calling dub test -c unittest --compiler= with a dmd built with this PR in excel-d master already passes, @aG0aep6G so I don't understand why the excel-d job failed because of your forced push 22 hours ago.

@atilaneves commited a fix for this issue via symmetryinvestments/excel-d#95.

[nordlow]

Ahh, maybe excel-d just lacks a tag bump (to v0.5.10), @aG0aep6G and @atilaneves.

Can you push a new tag v0.5.10 of excel-d, @atilaneves?

Does the buildkite job automatically pick the latest tag, @aG0aep6G?

[nordlow]

• excel-d 0.5.10 has been tagged now. Can you try rebase+force-push again, @aG0aep6G?

• What about the failing FreeBSD 11.4 x64, DMD at https://github.com/dlang/dmd/pull/12010/checks?check_run_id=3349806339? Does it have to be restarted?

• Does this PR unlock any other PRs, @aG0aep6G?

• When this is merged can you take a look at https://github.com/dlang/dmd/pull/12010/files#r642995293, @dkorpel?

• Ok to close fix Issue 20150 - -dip1000 return attribute must be explicit in pure … #10924 when this merged?

FYI, @RazvanN7

[dkorpel]

* What about the failing FreeBSD 11.4 x64, DMD at https://github.com/dlang/dmd/pull/12010/checks?check_run_id=3349806339? Does it have to be restarted?

Yes, should be fixed by #12985

* When this is merged can you take a look at https://github.com/dlang/dmd/pull/12010/files#r642995293, @dkorpel?

Yes, though I consider improving scope inference first (issue 20674) to minimize the new amount of breakage it will give.

* Ok to close [fix Issue 20150 - -dip1000 return attribute must be explicit in pure … #10924](https://github.com/dlang/dmd/pull/10924) when this merged?

I've closed it now, it's clear that it's the wrong approach.

[nordlow]

Awesome.