Qualify some members as scope to avoid deprecation warnings when building with dmd master

[nordlow]

No description provided.

[nordlow]

Ready for review.

[nordlow]

Why is CI jobs waiting for status to be reported, @Geod24? Does #2726 need to merged first?

[s-ludwig]

CI checks needed to be approved manually.

[s-ludwig]

I honestly find these annotations quite ridiculous. Why would a struct in D ever have non-scope methods? Since structs can always be moved, storing a pointer to them is never safe outside of a defined scope. Actually, I began fixing similar deprecation warnings before stopping and throwing the diff away, because this just can't be how this is supposed to work.

@Geod24, do you possibly have more insight here?

[PetarKirov]

Since structs can always be moved, storing a pointer to them is never safe

I'm not 100% sure, but since C++ interop was considered important a few years back (way before all new importC stuff), supporting self-references was considered important enough (e.g. for certain C++ STL data type implementations, but also for certain niche D applications, IIRC Weka), that first opPostBlit - DIP1014 was accepted (but never implemented IIRC), later to be replaced by copy constructors - DIP1018.

I don't if this interaction with DIP1000 was specifically considered, but in general my understanding is that struct moving with self references is supposed to be supported (albeit maybe not fully implemented?).

[s-ludwig]

Do copy constructors have any influence on move operations? My impression was that they are purely a generalization of the postblit mechanics. Also, performing a move is specifically done to avoid calling a copy constructor, so that would be a pretty strange choice of semantics.

But apart from the question of always having struct methods be scope, shouldn't scope at least be inferred here?

And then of course also why the compiler thinks that the this pointer is getting escaped by calling a method like this in the first place (which I assumed is the warning that is getting fixed by these changes):

void opAssign(int value)
{
	m_data = toBsonData(value).idup;
	m_type = Type.int_;
}

[nordlow]

This fixes reduces deprecation warnings in our builds. Can't we just approve these for now. Afaict, it doesn't cost anything.

[RazvanN7]

Why would a struct in D ever have non-scope methods?

The plan was to also introduce move constructors to enable the use of interior pointers in structs. From that point of view having non-scope struct methods makes sense. Also, I remember that at Weka they used to have a scenario where they would register structs in a global array and perform some operations on them, but because dmd moves structs freely they were having problems and that was what led to the opPostMove proposal (which was abandoned because it has the same flaws as the postblit).

Do copy constructors have any influence on move operations?

The copy constructor, currently, is orthogonal to moving. If you are trying to make a copy of an lvalue, the copy constructor will be called; if you are trying to make a copy of an rvalue, a move is performed (even if the copy constructor is disabled).

But apart from the question of always having struct methods be scope, shouldn't scope at least be inferred here?

In short, if we impose that struct methods are always scope then we basically give up on interior pointers. If we plan on having a move constructor that will finally enable the use of such pointers, then having them by default as non-scope would make sense because if we automatically consider them scope there is no way for the user to mark a method as non-scope. However, indeed, I think that inferring scope for struct methods is the way to go.

[s-ludwig]

Okay, so that confirms what I understood of the situation. Under the current regime, a struct shouldn't be able to be @safe and non-scope, so we really shouldn't have this deprecation warning in the first place, regardless of the technical reason why it is triggered here. What I haven't yet understood, though, is why this randomly occurs in some places, but not every time a method is called on a scope struct instance*.

The problem with just going ahead adding scope everywhere is that

1. this code will in practice linger there basically forever, adding permanent clutter when the fix really should be in the compiler
2. this is just the tip of the iceberg - there are tons of methods in other places where the same warning occurs, which is why I stopped going this route earlier

Both together make me really reluctant to just follow the warnings here, although I can certainly understand the urge to just get rid of them as quickly as possible.

* If nobody has an explanation for this, I'd start a dustmite run and see where that leads.

[nordlow]

Does this mean that we should postpone this until the adjustments to dmd has been performed?

[WalterBright]

a struct shouldn't be able to be @safe and non-scope

struct S
{
    int s;
    void abc(int i) @safe
    {
        s = i;
    }
}

compiles.

The thing about struct member functions is they are equivalent to a static member function having the first parameter be ref this, as in:

struct S
{
    void abc(ref S this) { ... }
}

If you could give a specific example of a member function that exhibits baffling behavior, I can help.

[WalterBright]

Perhaps this will help:

struct S {
    int* p;
    void abc(int i) @safe { }
}

void test() @safe {
    int i;
    S s;
    s.p = &i;
    s.abc(1);
}

Compiling it yields:

test.d(13): Error: scope variable `s` assigned to non-scope parameter `this` calling `abc`

but this compiles without error:

struct S {
    int* p;
    void abc(int i) @safe { }
}

void test() @safe {
    int i;
    S s;
//    s.p = &i;
    s.abc(1);
}

So what's going on? s is a local variable. The member p is a pointer. The pointer is assigned the address of a local variable i. This then makes s a scope variable.

Now s is passed to abc() by ref. The ref prevents the address of s from escaping abc(). But s is wrapping a pointer to a local, so the escape of the contents of s must be protected as well. Making abc() a scope function will accomplish this. Sure enough, adding scope to abc() causes the error to go away.

This is semantically no different from:

@safe void abc(ref int* p, int i) { }
@safe void test() {
    int i;
    int* p;
    p = &i;
    abc(p, 1);
}

[s-ludwig]

@WalterBright, thank you for looking into this! I'll try to look out for places that match this pattern.

I've also seen the Bugzilla issue for rephrasing the warning message, which will help a lot to make it more clear what happens at the innermost place. However, one crucial piece of information still missing from the deprecation message is the instantiation chain, in case the offending function/method is templated. Also, the fact that function/method names are not qualified can make it difficult to to match them against the code base. With all of these fixed, it should be pretty straight forward to find the most appropriate fix.

But in general, the semantics still taste somewhat odd to me. Making scope the default for this feels more like the right approach (using escape or similar to explicitly let the reference escape), but I guess that the way scope propagates to member variables would make that even more more intrusive. I'll have to play more with the current implementation to make any qualified suggestions, though, since I lost track of the developments that came after DIP1000.

[nordlow]

Code adjusted for all comments.

[s-ludwig]

How do you have to compile this to trigger the warnings? I've just fixed some deprecations triggered by formattedWrite in conjunction with a scope range (#2732) and would like to see if the ones here can also be fixed in a more isolated place.