WIP (do not merge): Make in mean scope const in DIP1000

[atilaneves]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @atilaneves! 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
✗	20362	normal	dmd fails to infer scope parameter for delegate

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

[PetarKirov]

It looks like some of the builds are failing because of link errors. The reason for that is likely that one object file was compiled with -dip1000 (and so functions with in parameters change their mangling) and another without (in which case in doesn't imply scope).

Of course this can be fixed by compiling all object files with the same flags on our side (upstream dlang), but it's likely that users will experience similar problems in the wild. For example, if we ship druntime and phobos compiled with -dip1000, but a user is building his project without, it's likely that if he calls druntime/phobos functions with in parameters he'll get link errors because the linker will expect to find functions without scope in the mangled name, while druntime/phobos will have the same function with scope in the mangled name. Even if we ship multiple versions of druntime/phobos, the same issue can appear when using third-party libraries.

I think the best solution is to have in always imply scope const (and have the same mangled name regardless of whether -dip1000 is used), but to ensure that the addition (compared to a previous release of dmd) of scope doesn't break code if -dip1000 is not used.

[rainers]

It might work if you also add STC.scopeinferred so scope won't be visible in the mangling.

I'd prefer if that transformation would be done during the semantic phase, though, so the parse tree is unmodified. For example dumping a header file will not keep the in modifier,

[Geod24]

It looks like some of the builds are failing because of link errors.

Wait I think that bug was fixed ? It was a prerequisite to make Phobos compiler with -dip1000.

[atilaneves]

I'd prefer if that transformation would be done during the semantic phase, though, so the parse tree is unmodified. For example dumping a header file will not keep the in modifier,

I tried doing it then and failed miserably to find where the information was being passed from the declared storage class to meaning const (as it is now). I asked @WalterBright for help and he said the easiest would be to change it in the parser.

[atilaneves]

@rainers:

It might work if you also add STC.scopeinferred so scope won't be visible in the mangling.

It doesn't work. STC.scopeinferred ends up not adding scope.

I'd prefer if that transformation would be done during the semantic phase, though, so the parse tree is unmodified. For example dumping a header file will not keep the in modifier,

Could you point me in the right direction, please? I still haven't been able to find where the handover happens.

[atilaneves]

Currently blocked by this Phobos PR.

[thewilsonator]

src/core/sys/freebsd/dlfcn.d(97): Error: static assert:  `is(extern (C) void* function(scope const(char*), int) nothrow @nogc == extern (C) void* function(const(char*), int) nothrow @nogc)` is false

[rainers]

It doesn't work. STC.scopeinferred ends up not adding scope.

I think it adds it to the semantics, but it is then suppressed in stcToBuffer:

dmd/src/dmd/hdrgen.d

Lines 2668 to 2669 in 83fde70

	if (stc & STC.scopeinferred)
	stc &= ~(STC.scope_ | STC.scopeinferred);

I noticed another reason why it is not enough to modify the parser: STC.in_ is used in some lowerings, too, e.g.

dmd/src/dmd/statementsem.d

Line 1746 in 83fde70

params.push(new Parameter(STC.in_, Type.tsize_t, null, null, null));

Could you point me in the right direction, please? I still haven't been able to find where the handover happens.

The current transformation in->const is here:

dmd/src/dmd/mtype.d

Lines 1949 to 1959 in 83fde70

	Type addStorageClass(StorageClass stc)
	{
	/* Just translate to MOD bits and let addMod() do the work
	*/
	MOD mod = 0;
	if (stc & STC.immutable_)
	mod = MODFlags.immutable_;
	else
	{
	if (stc & (STC.const_ | STC.in_))
	mod |= MODFlags.const_;

But that's the wrong place to add a storage class.

I'd try adding

                    if (fparam.storageClass & STC.in_)
                        stc |= STC.scope_ | STC.scopeinferred;
                    else

before

dmd/src/dmd/semantic3.d

Line 461 in 83fde70

if (funcdecl.flags & FUNCFLAG.inferScope && !(fparam.storageClass & STC.scope_))

[atilaneves]

@thewilsonator:

src/core/sys/freebsd/dlfcn.d(97): Error: static assert:  `is(extern (C) void* function(scope const(char*), int) nothrow @nogc == extern (C) void* function(const(char*), int) nothrow @nogc)` is false

I saw that. But at least it passed on a few OSs now.

[PetarKirov]

@atilaneves this PR should fix the BuildKite dub failure: dlang/dub#1791.

[rainers]

I'd try adding

That didn't work, here's a possible solution: #10526

[PetarKirov]

@rainers @atilaneves now that dlang/dub#1791 was merged, dub is now build-able with this branch of dmd (as well as dmd >= 2.088.1 with and without -preview=dip1000) and so we get buildkite to reach a bit further, but still fail while building the https://github.com/dlang/tools repo. The exact failure is this:

../dmd/generated/linux/release/64/dmd -I../druntime/import -I../phobos -L-L../phobos/generated/linux/release/64 -m64 -fPIC -dip25 -w -de -ofgenerated/linux/64/dget dget.d
dget.d(113): Error: function std.net.curl.HTTP.onReceiveHeader(void delegate(scope const(char[]) key, scope const(char[]) value) callback) is not callable using argument types (void)
dget.d(113):        cannot pass argument __lambda2 of type void to parameter void delegate(scope const(char[]) key, scope const(char[]) value) callback

And the failing code in question is:
https://github.com/dlang/tools/blob/11b1d12e3fee80c8af9611f60ba2be3286a5b18d/dget.d#L113-L117

Whereas onReceiveHeader (as expected) is defined as:

https://github.com/dlang/phobos/blob/07e975542c39bedd4f8b8df9ee7a341e2b61f87b/std/net/curl.d#L3137-L3141

Here's a minimal test case:

struct HTTP
{
    @property void onReceiveHeader(
        void delegate(scope const char[] key, scope const char[] value) callback)
    {
    }
}

void main()
{
    import std.conv : to;
    size_t contentLength;
    HTTP http;
    http.onReceiveHeader((k, v)
    {
        if (k == "content-length")
            contentLength = to!size_t(v);
    });
}

(you can test it here: https://run.dlang.io/gist/run-dlang/f2d2f7141d1a8466bc7eacce3bc9cbb2?compiler=dmd)

It compiles fine with -dip1000 (on all dmd versions since 2.074.0) and fails without it. In our case the tools repo is compiled without -preview=dip1000 and presumably that's why it fails to compile.
This looks like a regular bug that needs to be fixed, regardless of the faith of in.

@rainers My understanding is that #10526 changes in to mean const @and_maybe_scope, because otherwise it should fail to compile dget.d. While I agree with you that in shouldn't be replaced during parsing as this has problems with header generation, among other things, I'm not sure that STC.scopeinferred is the right choice either. My understanding is that this attribute is used only for local variables, nested functions and templates. I am not sure if having it on the interface level makes sense.

[PetarKirov]

It compiles fine with -dip1000 (on all dmd versions since 2.074.0) and fails without it. In our case the tools repo is compiled without -preview=dip1000 and presumably that's why it fails to compile.
This looks like a regular bug that needs to be fixed, regardless of the faith of in.

dlang/tools#383

While I don't think this PR (#10506) should be merged as it is, I'm still interested to see how far we can go with this approach and see how it fares in the real world (or at least in BuildKite, past the Build step). dlang/tools#383 will help us get there.

[rainers]

An more reduced test case:

void onReceiveHeader(void delegate(scope int* key) callback) {}

void main()
{
    onReceiveHeader((k) {});
}

onlineapp.d(5): Error: function onlineapp.onReceiveHeader(void delegate(scope int* key) callback) is not callable using argument types (void)
onlineapp.d(5):        cannot pass argument __lambda1 of type void to parameter void delegate(scope int* key) callback

Seems like dmd is unable to deduce the function literal type as soon as scope is involved.

[rainers]

I am not sure if having it on the interface level makes sense.

I'm unsure, too, especially since in isn't written to the mangling which causes the function type of foo(in char*s) s to be identical to foo(const char*s), but not foo(scope const char*s).

[PetarKirov]

I'm unsure, too, especially since in isn't written to the mangling which causes the function type of foo(in chars) s to be identical to foo(const chars), but not foo(scope const char*s).

Yep. Here's an odd idea: introduce a new mangling for in parameters. This way it shouldn't matter at link time whether in means const, scope const, or even auto ref scope const (as I (and probably many others before me) proposed here: [1], [2]). This way, we limit the code breakage to -preview=dip1000 and have in really mean scope const.

[rainers]

Yep. Here's an odd idea: introduce a new mangling for in parameters.

Yeah, that's what I thought, too. We'd have to avoid removing STC.in_ from the storage_class member as in https://github.com/dlang/dmd/pull/10526/files#diff-64d4abe08ba2355c2b95cb22e46b8572L1510, though

[atilaneves]

Even more reduced test case to highlight the Windows failure (which is due to the druntime tests there not being compiled with -preview=dip1000):

void stringify(scope void delegate(scope const char[]) sink) {
    sink("oops");
}

void main() {
    string str;
    stringify((chars) {str ~= chars; });  // won't compile
}

With -preview=dip1000 this compiles. Without, it doesn't. Adding scope manually (i.e. (scope chars)) to the delegate also compiles.

[atilaneves]

I filed a dmd bug.

[dkgroot]

Regarding:

src/core/sys/freebsd/dlfcn.d(97): Error: static assert:  `is(extern (C) void* function(scope const(char*), int) nothrow @nogc == extern (C) void* function(const(char*), int) nothrow @nogc)` is false

Replacing :

static assert(is(typeof(&dlopen)  == __externC!(void*, const char*, int)));
static assert(is(typeof(&dlsym)   == __externC!(void*, void*, const char*)));

With:

static assert(is(typeof(&dlopen)  == void* function(scope const(char*), int) nothrow @nogc ));
static assert(is(typeof(&dlsym)   == void* function(void *, scope const(char*)) nothrow @nogc));

Works.

However:

static assert(is(typeof(&dlopen)  == __externC!(void*, scope const(char*), int)));

Does not. Not sure why the __externC wrapper in this case doesn't work.
Note: it would help if there was a Version(DIP1000) to find out if we need to compensate or not.

[Geod24]

I filed a dmd bug.

@WalterBright : This is one of the bugs blocking @safe as default.
I added the safe keyword on Bugzilla.
Note that I tried to make some progress on Vibe.d (and its dependencies) and having in means scope const would really help a lot.

[WalterBright]

Note that I tried to make some progress on Vibe.d (and its dependencies) and having in means scope const would really help a lot.

Why? Why is s/in/scope const/ hard?

[Geod24]

Why? Why is s/in/scope const/ hard?

For starter, it's not possible to do it automatically. So I ended up grepping for "in " and going over the many lines of results (between comments and opIn calls).
Second it's just plain annoying. in was always advertised as scope const and I have yet to encounter a place where in was misused to mean just const.
Last, knowing that this PR is up (and hopefully will get accepted), this means that at one point we could go back to change those scope const to in.

It's not difficult, it is a worthless use of time.

[WalterBright]

So I ended up grepping for "in " and going over the many lines of results (between comments and opIn calls).

Ouch!

grep -w in *.d

should do the trick (case sensitive and words only). You can go further with:

me `grep -l -w in *.d`

and this will send a list of files with those matches to your editor (my editor is me). Even further, you can do a query-search-replace in the editor with a regex for in that is case sensitive and words only, that should save you enormous time over the way you are doing it. (I do this all the time.)

[WalterBright]

`in` was always advertised as `scope const`

yes except scope changed its meaning over time. As long as dip1000 is a -preview feature, we shouldn't be in a big hurry to upset existing code.

[Geod24]

grep -w in *.d

I didn't knew about the -w switch, but it doesn't change the result so much: it still matches comments "and in this situation we should avoid..." and opIn (if (auto x = i in aa)).
The point was, there's no mechanical way to do this.

yes except scope changed its meaning over time. As long as dip1000 is a -preview feature, we shouldn't be in a big hurry to upset existing code.

We have-preview=dip1000 that changes the meaning of scope, but there's no upsetting existing code because it's hidden behind a -preview switch, which we can use to make code compliant (which is exactly what I was trying to do).
But why doesn't the same reasoning apply to in ? People wouldn't be more upset about in being enforced than they would be about scope being enforced, provided in has always meant to imply scope.

[atilaneves]

Wrt to the regexp, this is far better:

find ~/.dub/packages/vibe-d-0.8.6 -name '*.d' | xargs grep -P '\(.*?\bin \w.*?\)'       

But, still has a lot of ifs, pragmas, asserts, etc. There's no easy way to grep for a function, you need a parser.

Regarding scope changing meaning, I think that's a good thing.

[atilaneves]

Closed in favour of #10769