dworkin / dgd

Dworkin's Game Driver, an object-oriented database management system originally used to run MUDs.
https://www.dworkin.nl/dgd/
GNU Affero General Public License v3.0
103 stars 31 forks source link

Follow up: value of assignment to array #52

Closed shentino closed 3 years ago

shentino commented 3 years ago

A recent issue exposing a bug in ({ var, ... }) = was filed.

At the time the issue was resolved by having the overall result of the assignment have a value of nil, which was better than having it crash.

However, I think it would be an even better idea to have it follow suit with C style chained assignment expressions to preserve semantic compatibility, by preserving the array rvalue after spreading to the syntactic lvalue, and having said rvalue be the overall value of the "assign to array" expression.

Example:

float x0, y0, x1, y1, x2, y2;
object monster;

monster = SpawnRandomMonster();

({ x0, y0 }) = ({ x1, y1 }) = ({ x2, y2 }) = GetPositionOf(monster);

// this would mean the same thing

({ x0, y0 }) = ( ({ x1, y1 }) = ( ({ x2, y2 }) = GetPositionOf(monster) ) );

At the very least it would preserve conformity to C style assignment semantics, and allowing assignments to be chained in this manner may save time if the same rvalue needs spread more than once in the same statement.

I tried to mention this during the other issue but my clarity in explaining what I meant left much to be desired.

dworkin commented 3 years ago

This is worth considering. For reference, the original issue referenced is https://github.com/dworkin/dgd/issues/46 raised by @nyankers.

dworkin commented 3 years ago

Note that aggregate assignment does not have a value of nil, as nil is an actual value. It would be more correct to say that the value is void.

Also note that ({ x0, y0 }) = ({ x1, y1 }) = ... has never worked in LPC, so there is no legacy code to take into account.

shentino commented 3 years ago

Note that aggregate assignment does not have a value of nil, as nil is an actual value. It would be more correct to say that the value is void.

I was unaware of the distinction.

Also note that ({ x0, y0 }) = ({ x1, y1 }) = ... has never worked in LPC, so there is no legacy code to take into account.

It's not legacy code I'm worried about. It's the potential to use this "chained aggregate assignment" feature in the future and have it work as expected. I can even foresee potential performance and/or code tidiness implications.

I did remember in the previous issue that you were uncertain as to what the value of an aggregate assignment should even be in the first place.

nyankers commented 3 years ago

The distinction's pretty big actually. nil is an acceptable false-y value for strings, objects, arrays, mappings, and of course mixed (I kind of wish there was a "no-nils-allowed" type modifier, besides type object on an object-scope variable of course, e.g. some keyword or symbol @ where void print(string @x) demands x be a non-nil string, int @*ids; initializes to an empty array and can never be assigned nil, etc. but that's a total aside).

On the other hand, void will throw a compile-time error if you try to use it in any way as a value, because it's not one.

Making it return a value would allow it to be used in other ways, such as if (({a, b}) = c) { /* do something */ } (though this would be a bit pointless as currently the assignment gives a runtime error if c is not an array with sufficient values to populate every lvalue) or return ({a, b}) = c; (which would have some uses in read-evaluate-print stuff like cloudlib's code command).

I don't mind how it is now, a compile-time error is far better than a fatal crash, but I do agree that it's somewhat peculiar to have an operator conditionally return void in a C-style language (the only other time I know this can happen is the ?: ternary operator, and even there it only happens if you use void expressions as arguments).

shentino commented 3 years ago

As for void vs nil I think I got confused by nil being the "return value" of a void function ^.^*

dworkin commented 3 years ago

I'm not convinced by the "similar to C" argument. Chaining of aggregate assignments in the same statement does not seem like a great benefit to me, when you can already do

    a = ({ 1, 2 });
    ({ x0, y0 }) = a, ({ x1, y1 }) = a, ({ x2, y2 }) = a;

As for the "strange that an operator can conditionally return void" argument, the () (function call) operator conditionally returns void.

Sidestepping a discussion of the merits, yesterday I had a look at how difficult it would be to implement this.

It turns out to be quite easy for Hydra and DGD, although that would require another VM version number update. However, for the JIT compiler module it would be a major rewrite, since values which were already used out of order on the stack (which the decompiler can just barely handle) would now also have to persist beyond the scope of the aggregate assignment (which the decompiler definitely cannot handle).

I don't have the time for a decompiler rewrite.

I could release my changes to aggregate assignment in a separate branch, but I'm not going to merge it until the decompiler issue has been resolved.

dworkin commented 3 years ago

It turns out that there is a fairly simple workaround for the JIT compiler issue.

Since letting the assignment have a value is a feature that several have requested, I decided to support it. It is in the latest commit on the master branch. Let me know how it works out.

shentino commented 3 years ago

Confirmed as working as expected.

Source code:

inherit "/lib/string/sprint";

void test()
{
    int a, b, c, d, e, f;

    ACCESS_CHECK(TEST());

    ({ a, b }) = ({ c, d }) = ({ e, f }) = ({ 1, 2 });

    LOGD->post_message(
        "system", LOG_NOTICE,
        "({ a, b, c, d, e, f }) == " + mixed_sprint(
            ({ a, b, c, d, e, f })
        )
    );
}

Output on stderr

Aug 14 20:10:18 ** ({ a, b, c, d, e, f }) == ({ 1, 2, 1, 2, 1, 2 })