Regression: PlayerDeathEvent custom drops are mutated into AIR by the server after the event is handled

Expected behavior

PlayerDeathEvent custom drops are not mutated in any way by the server after the event is handled.

Observed/Actual behavior

PlayerDeathEvent custom drops are mutated into AIR by the server after the event is handled.

Steps/models to reproduce

Given the following (somewhat contrived) example code:

package org.example.plugin;

import org.bukkit.Material;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;
import org.bukkit.event.entity.PlayerDeathEvent;
import org.bukkit.inventory.ItemStack;
import org.bukkit.plugin.java.JavaPlugin;

public class ExamplePlugin extends JavaPlugin implements Listener {

    @Override
    public void onEnable() {
        getServer().getPluginManager().registerEvents(this, this);
    }

    @EventHandler
    public void onPlayerDeath(PlayerDeathEvent e) {
        final ItemStack itemStack = new ItemStack(Material.DIAMOND);
        // Any custom drops added are affected by this issue
        e.getDrops().add(itemStack);

        // This one prints the diamond, it's fine.
        getLogger().info(">>> Item stack before PlayerDeathEvent: " + itemStack);
        getServer().getScheduler().runTask(this, () -> {
            // This one prints AIR, where did the diamond go?
            // If you attach a debugger you can see that the underlying NMS item is still
            // a diamond, but the item stack count is set to 0, which is why the item is treated as AIR.
            getLogger().info(">>> Item stack after PlayerDeathEvent: " + itemStack);
        });
    }

}

You should be able to observe the following output when a player dies (e.g. using /kill):

[ExamplePlugin] >>> Item stack before PlayerDeathEvent: ItemStack{DIAMOND x 1}
[ExamplePlugin] >>> Item stack after PlayerDeathEvent: ItemStack{AIR x 0}

As you can see, the item stack added as a custom drop was mutated into AIR by the server. Note that this issue only affects custom drops added, not any of the original drops that the player had in their inventory.

Plugin and Datapack List

[03:14:44 INFO]: Server Plugins (1):
[03:14:44 INFO]: Bukkit Plugins:
[03:14:44 INFO]:  - ExamplePlugin

[03:15:38 INFO]: There are 3 data pack(s) enabled: [vanilla (built-in)], [file/bukkit (world)], [paper (built-in)]
[03:15:38 INFO]: There are no more data packs available

Paper version

[03:16:13 INFO]: Checking version, please wait...
[03:16:13 INFO]: This server is running Paper version 1.21.1-128-master@d348cb8 (2024-10-21T16:23:24Z) (Implementing API version 1.21.1-R0.1-SNAPSHOT)
You are running the latest version

Other

After some testing, I can conclude that this seems to be a regression introduced in build 79 for Minecraft 1.21.1, i.e. commit https://github.com/PaperMC/paper/commit/0a53f1d10dc331ada334b61c0104ba4b4462966a. On earlier builds, the drops are not mutated into air and so both of the log statements in the example code print a diamond, as expected.

I didn't debug far enough to tell what code path is setting the item stack count to 0, but to me it seems like the regression introduced in build 79 is that the custom drop item stacks are no longer copied, so if the server mutates them then that will now affect the API user. I'm not entirely sure if the API user should guard against this by cloning the item stack or if this is a bug that should be fixed in Paper, but I'm leaning toward the latter. In which case, please see my PR #11521 for a possible patch to fix this regression.

Let me know if this seems correct or if I have perhaps misunderstood something.

While this might be a behavioral change that should be addressed, I would generally note that cloning items before you toss them into other places is a good idea, as there are 0 promises in the long term of how these items are handled

Yeah sounds fair, that's why I was not sure if this should be considered a Paper bug or API user error.

there are 0 promises in the long term of how these items are handled

So in the general case, whenever you pass an ItemStack to the API that does something with the ItemStack "later", such as adding ItemStacks to PlayerDeathEvent#getDrops(), you should expect that the server will "clobber" it, i.e. you, the caller, needs to save (clone) it if you don't want it to change? Even if the thing you're passing it to seems "harmless"? Like, in the PlayerDeathEvent drops case, my naive expectation would be that the server just has to read the ItemStack in order to drop it, not write to it (i.e. set the count to 0).

I'm not that familiar with the server internals but I understand if "0 promises" has to be the case for one reason or another. Still, I can't help but feel that it's a bit unintuitive, and a potential pitfall for newcomers to the API. Do you have any long-term plans to address this, i.e. in terms of documentation or improved API (some kind of immutable ItemStack?). I'm just a bit curious, that's all.

Edit: Hmm, the more I think about it, the more I get this feeling that yeah, why should you expect that the server does not modify ItemStacks you pass to it? It kind of makes sense that if ItemStacks are mutable and "live" in the server, the server will probably end up mutating them at some point unless copies are made at every point. So I think I kind of get it. Still feels like a pitfall though.

I will also add that I'm maintaining an old plugin that someone else wrote, so I have no idea just how many potential pitfalls are hiding in the codebase if "0 promises" is the case. I guess up until now I've just been relying on the API to copy/clone things for me, and then a few days ago I noticed this particular bug with PlayerDeathEvent (which took me a while to get to the bottom of, I must say). I guess it's just "tough luck" for me, having to maintain code that someone else wrote. I don't suppose you have any ideas on how I could more easily discover more of these cases (if PlayerDeathEvent drops is not the only one)?

the server just has to read the ItemStack in order to drop it

There are 0 promises that the server will blindly clone an ItemStack you put into the world for you, a lot of the old behavior here was somewhat a mixture of happenchance or the fact that ItemStacks often API driven which is no longer the case; The behavioral change should probably be fixed, but, long run, you should be cloning any stack you don't want to be modified outside of your control when you're putting it into the world

The only thing we could ever do here is start blindly cloning ItemStack instances all around the API, but there is no real great consistency in the internals or the API here in terms of how this sort of thing is handled, and there is a lot of reliance on the fact that ItemStacks are not blindly cloned all over the place to prevent these issues

Yeah this is starting to click in my head now, thanks for the input. I think part of my confusion comes from these API inconsistencies and "hidden" happenchance behaviors around ItemStack. Even though it seems obvious in hindsight, I hadn't really considered that a consequence of adding the ItemStack to the drops is that it becomes "live" in the world. And I feel like it's very common for the API to hide this fact because it in fact does copy the ItemStack in many cases.

I also think it's interesting to compare ItemStack to Block and Entity. For the latter two, I think it is quite intuitive to understand that they are "live" and mutable objects in the world, so you should expect them to change. And both also have immutable snapshot variants (BlockState and EntitySnapshot) that you can use if you want just that, an immutable snapshot (which is also an intuitive concept).

But ItemStack is this weird middle ground where it can be "live" in the world, but it can also be "disconnected" from the world if you just create it in the API and don't add it to the world. Although since it always can be added to the world it seems like in the general case you should assume that it is live in the world. Also, in contrast to Block/BlockState and Entity/EntitySnapshot, ItemStack has no snapshot variant (to my knowledge), so it is always mutable.

I can make ItemStack clones, and will probably do so in more places now that I've become aware of these issues. But I feel like clones only protect me "short-term" and not "long-term" in the sense that if I accidentally leak a cloned ItemStack somewhere, it might become live and change again. Having an immutable ItemStack snapshot would allow me to protect against such mistakes.

Given all the above, two questions come to mind, if you don't mind answering:

Are there any (long-term) plans to add API for ItemStack snapshots? Just curious, I understand that it might take quite a bit of work.
The Javadocs for Block have a warning that reads:

This is a live object, and only one Block may exist for any given location in a world. The state of the block may change concurrently to your own handling of it; use block.getState() to get a snapshot state of a block which will not be modified.

What are your thoughts on adding a similar warning to ItemStack, if you agree that in the general case, one should assume that an ItemStack is live in the world?

Edit: After thinking some more I realize that BlockState is not immutable, so I guess in a way it's more similar to cloning an ItemStack than it is to EntitySnapshot. But even though a BlockState is mutable, the BlockState object itself cannot become "live" in the world, so I still feel like it protects me from having it accidentally becoming live if I leak it to the server somehere. The same cannot be said if I clone an ItemStack.

PaperMC / Paper