Fail if player instance is accessed after leaving/replacing

[S-S-X]

Problem

Currently player instances are always valid, disconnected players are not returned by get_connected_players but if reference to instance is available then it will be valid.

Compatibility with simple tests

Allow player instances to be used before joining just like before, do not change that.

Handling invalidated

Fail with clear error message if player instance is used after player is disconnected by calling mineunit:execute_on_leaveplayer(player). This allows properly validating cases where player instance might be used after player disconnected from game.

Also handle replaced instances

Also make replaced player invalid. Currently when new Player instance is created it will just replace old one but wont invalidate previous instance. At this point control over previous instance is lost (but another ref might very well be available within mod, spec or fixture code). Invalidate this old instance immediately, similar way how leaving game is handled.

Implementation

Should be enough to set some flag (so that instance can be restored when player joins, metadata stays) and error when trying to access index. This also allows access using rawget and rawset if needed for some weird unknown reason.

[fluxionary]

Fail with clear error message if player instance is used after player is disconnected by calling mineunit:execute_on_leaveplayer(player).

I think the game just returns "nil" for every method of a player object corresponding to a disconnected player. Perhaps such access could error if such access is made in "strict" mode?

[S-S-X]

What engine returns does not matter, while it should not be done mod can store instances locally.

Also another case (which probably would be a lot harder to test properly as it is engine implementation detail) in some cases engine returns valid object instance where some data is already freed. This case is basically type(player)=="userdata" and player:get_meta()==nil. However this might not be the case for every method, player class instance is still valid and data stored directly on player class instance might still be available.

It gets complicated but while it does work as expected and return nil in most cases it does not seem to be guaranteed that nil is always returned. Anyway even if it would be guaranteed this is still needed to catch cases where instances are incorrectly stored Lua side.

[fluxionary]

What engine returns does not matter

Then I fundamentally misunderstood what this project was intended to be. I wanted to be able to test against the behavior of the engine w/out the actual engine, but that doesn't seem to be the goal here.

[S-S-X]

What engine returns does not matter, while it should not be done mod can store instances locally.

[fluxionary]

What engine returns does not matter, while it should not be done mod can store instances locally.

Possibly we have a communication problem, I'm not sure your first language is English. To me, this means:

1. The behavior of the engine isn't important
2. Mods have access to scopes that can hold on to references to player objects after the player has left the server
3. Mods should not do this

Point 3 is the point I disagree with the most. Mods do this all the time, because they can do this. Lua is built on capturing state this way. The game allows mods to make use of this language mechanic. Mods still need to check that the reference is valid, if they do this - which can be done by seeing whether the result of any arbitrary function call is nil. That the function call itself errors out is the locus of the bug - other mods should not be assuming that various API functions are not present. Should every API call be gated by a check to see if the function is available? Maybe, if lua 5.1 or luajit provided a language feature like C#'s "null-forgiving" operator - but they don't.