Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 51 additions & 54 deletions data/vibe/data/bson.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ struct Bson {
A slice of the first bytes of `data` is stored, containg the data related to the value. An
exception is thrown if `data` is too short.
*/
this(Type type, bdata_t data)
this(Type type, bdata_t data) scope
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@PetarKirov PetarKirov Apr 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_;
}

Copy link
Contributor Author

@nordlow nordlow Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@nordlow nordlow Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

{
m_type = type;
m_data = data;
Expand Down Expand Up @@ -202,70 +202,71 @@ struct Bson {
/**
Initializes a new BSON value from the given D type.
*/
this(double value) { opAssign(value); }
this(double value) scope { opAssign(value); }
/// ditto
this(scope const(char)[] value, Type type = Type.string)
this(scope const(char)[] value, Type type = Type.string) scope
{
assert(type == Type.string || type == Type.code || type == Type.symbol);
opAssign(value);
m_type = type;
}
/// ditto
this(in Bson[string] value) { opAssign(value); }
this(in Bson[string] value) scope { opAssign(value); }
/// ditto
this(in Bson[] value) { opAssign(value); }
this(in Bson[] value) scope { opAssign(value); }
/// ditto
this(in BsonBinData value) { opAssign(value); }
this(in BsonBinData value) scope { opAssign(value); }
/// ditto
this(in BsonObjectID value) { opAssign(value); }
this(in BsonObjectID value) scope { opAssign(value); }
/// ditto
this(bool value) { opAssign(value); }
this(bool value) scope { opAssign(value); }
/// ditto
this(in BsonDate value) { opAssign(value); }
this(in BsonDate value) scope { opAssign(value); }
/// ditto
this(typeof(null)) { opAssign(null); }
this(typeof(null)) scope { opAssign(null); }
/// ditto
this(in BsonRegex value) { opAssign(value); }
this(in BsonRegex value) scope { opAssign(value); }
/// ditto
this(int value) { opAssign(value); }
this(int value) scope { opAssign(value); }
/// ditto
this(in BsonTimestamp value) { opAssign(value); }
this(in BsonTimestamp value) scope { opAssign(value); }
/// ditto
this(long value) { opAssign(value); }
this(long value) scope { opAssign(value); }
/// ditto
this(in Json value) { opAssign(value); }
this(in Json value) scope { opAssign(value); }
/// ditto
this(in UUID value) { opAssign(value); }
this(in UUID value) scope { opAssign(value); }

/**
Assigns a D type to a BSON value.
*/
void opAssign(const Bson other)
void opAssign(const Bson other) scope
{
m_data = other.m_data;
m_type = other.m_type;
}
/// ditto
void opAssign(double value)
void opAssign(double value) scope
{
m_data = toBsonData(value).idup;
m_type = Type.double_;
}
/// ditto
void opAssign(scope const(char)[] value)
void opAssign(scope const(char)[] value) scope
{
import std.utf;
import std.string : representation;
debug std.utf.validate(value);
auto app = appender!bdata_t();
auto app = appender!(bdata_t)();
app.put(toBsonData(cast(int)value.length+1));
app.put(value.representation);
app.put(cast(ubyte)0);
m_data = app.data;
import std.exception : assumeUnique;
() @trusted { m_data = app.data.assumeUnique; }();
m_type = Type.string;
}
/// ditto
void opAssign(in Bson[string] value)
void opAssign(in Bson[string] value) scope
{
auto app = appender!bdata_t();
foreach( k, ref v; value ){
Expand All @@ -282,7 +283,7 @@ struct Bson {
m_type = Type.object;
}
/// ditto
void opAssign(in Bson[] value)
void opAssign(in Bson[] value) scope
{
auto app = appender!bdata_t();
foreach( i, ref v; value ){
Expand All @@ -299,7 +300,7 @@ struct Bson {
m_type = Type.array;
}
/// ditto
void opAssign(in BsonBinData value)
void opAssign(in BsonBinData value) scope
{
auto app = appender!bdata_t();
app.put(toBsonData(cast(int)value.rawData.length));
Expand All @@ -310,31 +311,31 @@ struct Bson {
m_type = Type.binData;
}
/// ditto
void opAssign(in BsonObjectID value)
void opAssign(in BsonObjectID value) scope
{
m_data = value.m_bytes.idup;
m_type = Type.objectID;
}
/// ditto
void opAssign(bool value)
void opAssign(bool value) scope
{
m_data = [value ? 0x01 : 0x00];
m_type = Type.bool_;
}
/// ditto
void opAssign(in BsonDate value)
void opAssign(in BsonDate value) scope
{
m_data = toBsonData(value.m_time).idup;
m_type = Type.date;
}
/// ditto
void opAssign(typeof(null))
void opAssign(typeof(null)) scope
{
m_data = null;
m_type = Type.null_;
}
/// ditto
void opAssign(in BsonRegex value)
void opAssign(in BsonRegex value) scope
{
auto app = appender!bdata_t();
putCString(app, value.expression);
Expand All @@ -343,47 +344,47 @@ struct Bson {
m_type = type.regex;
}
/// ditto
void opAssign(int value)
void opAssign(int value) scope
{
m_data = toBsonData(value).idup;
m_type = Type.int_;
}
/// ditto
void opAssign(in BsonTimestamp value)
void opAssign(in BsonTimestamp value) scope
{
m_data = toBsonData(value.m_time).idup;
m_type = Type.timestamp;
}
/// ditto
void opAssign(long value)
void opAssign(long value) scope
{
m_data = toBsonData(value).idup;
m_type = Type.long_;
}
/// ditto
void opAssign(in Json value)
@trusted {
void opAssign(in Json value) scope @trusted
{
auto app = appender!bdata_t();
m_type = writeBson(app, value);
m_data = app.data;
}
/// ditto
void opAssign(in UUID value)
void opAssign(in UUID value) scope
{
opAssign(BsonBinData(BsonBinData.Type.uuid, value.data.idup));
}

/**
Returns the BSON type of this value.
*/
@property Type type() const { return m_type; }
@property Type type() const scope { return m_type; }

bool isNull() const { return m_type == Type.null_; }

/**
Returns the raw data representing this BSON value (not including the field name and type).
*/
@property bdata_t data() const { return m_data; }
@property bdata_t data() const return scope { return m_data; }

/**
Converts the BSON value to a D value.
Expand Down Expand Up @@ -614,8 +615,8 @@ struct Bson {

/** Converts a given JSON value to the corresponding BSON value.
*/
static Bson fromJson(in Json value)
@trusted {
static Bson fromJson(in Json value) @trusted
{
auto app = appender!bdata_t();
auto tp = writeBson(app, value);
return Bson(tp, app.data);
Expand All @@ -626,8 +627,7 @@ struct Bson {
All BSON types that cannot be exactly represented as JSON, will
be converted to a string.
*/
Json toJson()
const {
Json toJson() const scope {
switch( this.type ){
default: assert(false);
case Bson.Type.double_: return Json(get!double());
Expand Down Expand Up @@ -661,15 +661,13 @@ struct Bson {

/** Returns a string representation of this BSON value in JSON format.
*/
string toString()
const {
string toString() const scope {
auto ret = appender!string;
toString(ret);
return ret.data;
}

void toString(R)(ref R range)
const {
void toString(R)(ref R range) const {
switch (type)
{
case Bson.Type.objectID:
Expand Down Expand Up @@ -723,14 +721,14 @@ struct Bson {

Returns a null value if the specified field does not exist.
*/
inout(Bson) opIndex(string idx) inout {
inout(Bson) opIndex(string idx) inout scope {
foreach (string key, v; this.byKeyValue)
if( key == idx )
return v;
return Bson(null);
}
/// ditto
void opIndexAssign(T)(in T value, string idx){
void opIndexAssign(T)(in T value, string idx) {
// WARNING: it is ABSOLUTELY ESSENTIAL that ordering is not changed!!!
// MongoDB depends on ordering of the Bson maps.

Expand Down Expand Up @@ -796,7 +794,7 @@ struct Bson {

Returns a null value if the index is out of bounds.
*/
inout(Bson) opIndex(size_t idx) inout {
inout(Bson) opIndex(size_t idx) inout scope {
foreach (size_t i, v; this.byIndexValue)
if (i == idx)
return v;
Expand Down Expand Up @@ -928,7 +926,7 @@ struct Bson {
}

///
bool opEquals(ref const Bson other) const {
bool opEquals(ref const scope Bson other) const scope {
if( m_type != other.m_type ) return false;
if (m_type != Type.object)
return m_data == other.m_data;
Expand All @@ -948,14 +946,13 @@ struct Bson {
return true;
}
/// ditto
bool opEquals(const Bson other) const {
bool opEquals(const scope Bson other) const scope {
if( m_type != other.m_type ) return false;

return opEquals(other);
}

private void checkType(in Type[] valid_types...)
const {
private void checkType(in Type[] valid_types...) const scope {
foreach( t; valid_types )
if( m_type == t )
return;
Expand Down Expand Up @@ -1245,7 +1242,7 @@ struct BsonDate {

/** Allows relational and equality comparisons.
*/
bool opEquals(ref const BsonDate other) const { return m_time == other.m_time; }
bool opEquals(ref const scope BsonDate other) const scope { return m_time == other.m_time; }
/// ditto
int opCmp(ref const BsonDate other) const {
if( m_time == other.m_time ) return 0;
Expand Down Expand Up @@ -1914,7 +1911,7 @@ unittest
assert(t == s);
}

private string skipCString(ref bdata_t data)
private string skipCString(return scope ref bdata_t data)
@safe {
auto idx = data.countUntil(0);
enforce(idx >= 0, "Unterminated BSON C-string.");
Expand Down