Fix builtin item metatable.

[sofar]

This is a cleaned up version of #2286 , applied on top of stable-5.

[sofar]

@bell07 Can you retest this?

[bell07]

Tested, works as expected with wielded_light

[paramat]

@sofar what is your intention for this, merging to master and stable-5? I don't know how to do that so will leave merging to others.

[sofar]

The right thing to do would be:

• merge to master immediately
• merge to stable-5 if we deem it's necessary and we don't want to wait a full release

[paramat]

That sounds fine to me.

[bell07]

The related line to this issue (why it did work previosly) in minectest core is the
prototype.__index = prototype -- so that it can be used as a metatable line in core.register_entity().
I think this line needs to be removed

[paramat]

IRC comments by p_gimeno http://irc.minetest.net/minetest-dev/2019-04-03#i_5524097

21:08 p_gimeno I understand what game#2328 is doing, but not quite why it's a problem (I always prefer metatables to be in a dedicated table anyhow, but I fail to see why it's a problem in this case; the justification in game#2286 is fuzzy to me: "The reason for this fix is the wrong metatables usage that take nil values for some conditions"
21:08 ShadowBot #2328 -- Fix builtin item metatable. by sofar
21:08 ShadowBot #2286 -- fix setmetatable for builtin:item enhancement by bell07
21:09 p_gimeno )
21:23 p_gimeno hm, now that I look again, game#2286 is incomplete
21:23 ShadowBot #2286 -- fix setmetatable for builtin:item enhancement by bell07
21:24 p_gimeno err, game#2328
21:24 ShadowBot #2328 -- Fix builtin item metatable. by sofar
21:27 p_gimeno I think this line should also be removed: https://github.com/minetest/minetest/blob/4f7674d448abb9f12030c743a1a6a0d3ad07ad7c/builtin/game/register.lua#L101
21:31 p_gimeno while on that, why does core.register_item set a different metatable for each itemdef?
21:32 p_gimeno that looks like an utter waste

[paramat]

@bell07 @sofar see previous comment.

[bell07]

Using the same table as metatable to himself breaks redefinition chains.

Sample code:

-- Original mod
obj = {}
obj.a = 'A'
obj.__index = obj
print("a=", obj.a)

-- Second mod does redefine + enhance the obj
local obj2 = {}
obj2.b = 'B'
setmetatable(obj2, obj)
obj = obj2

print("a=, b=", obj.a, obj.b)

-- Third mod does redefine + enhance the obj in the same way
local obj2 = {}
obj2.c = 'C'
setmetatable(obj2, obj)
obj = obj2

print("a=, b=, c=", obj.a, obj.b, obj.c)

Output:

a=	A
a=, b=	A	B
a=, b=, c=	nil	nil	C

As you see the references to a and to b gets lost. Depending on mods loading order maybe the a and c is broken, not predicteable

[paramat]

@bell07 to be clear, are you saying this PR is fine and needed?

[bell07]

The PR is still highly recommended but not mandatory. Currently no redefinition chain is implemented in Minetest Game, but to be fair for other mods, please apply the patch.
The prototype.__index = prototype -- so that it can be used as a metatable in Minetest should be removed too to avoid such fuzy enhancements.

[paramat]

IRC http://irc.minetest.net/minetest-dev/2020-02-13

11:18 p_gimeno I still fail to see why #2328 is a problem. The example reported by bell07 is user error. http://www.formauri.es/personal/pgimeno/pastes/mtg2328.lua works fine.
11:18 ShadowBot minetest/minetest#2328 -- wrapDegrees() group of functions return incorrect values
11:18 p_gimeno sorry, game#2328
11:18 ShadowBot #2328 -- Fix builtin item metatable. by sofar
11:20 p_gimeno last line output: a=, b=, c= A B C
11:25 p_gimeno you can argue that having the metatable be the same table is bad practice, but I don't think it limits the flexibility. And if it ain't broke...
11:25 sfan5 honestly I have no idea what that change even does
11:26 p_gimeno it makes it use a dedicated metatable instead of reusing the table itself as a metatable (the table has an __index field that points to itself)
11:31 p_gimeno http://www.formauri.es/personal/pgimeno/pastes/current-vs-proposed-2328.lua
11:32 p_gimeno like that, but with the difference that the proposed change still has an __index field (for compatibility?)
11:36 sfan5 hmm

[bell07]

I think the right way is to replace __index by function that write a deprecation warning to the log but still have the meta functionality. Then in second step (after next release) the __index should be removed.

[bell07]

Will setup this change as PR tomorrow: https://github.com/bell07/minetest/commit/0d465a355b4ea3883a5cb2da5272a994bf85eb2b

[paramat]

Thanks, and sorry that most core devs do not understand this.

[bell07]

I try it ;-) minetest/minetest#9407
Any core dev did merged the prototype.__index = prototype line. Assume any of them can understand this.

[sfan5]

I looked at this again today and while both solutions work, not reusing the whole table as metatable looks like a cleaner solution to me.

[rubenwardy]

Doesn't the __index need to be removed, above?

[paramat]

This has sufficient approval to be merged, but it looks like there are still some doubts.
@sofar

[bell07]

If I should (attempt to) explain the issue again, let me know.

Current state is two merge requests:

1. Core: Deprecate wrong __index usage in core.register_entity minetest#9407 - Add the deprecation warning for the current (wrong) metatable usage.
2. Game: Fix builtin item metatable. #2328 - Fix the current (wrong) metatable usage.

The merging order does not matter. If the 1 is merged at the first, the warning is printed to the log till 2 is merged.
If 2 is merged first, the deprecation waring is not triggered by Minetest Game.

If both changes are merged, in third step the deprecation/compatibility code from 1 can be fully removed.

[SmallJoker]

Dropped items still work. LGTM.