Entity#getScoreboardTags directly exposes the Set<String>

[Machine-Maker]

An entity will only load 1024 scoreboard tags from it's NBT, and the addTag methods on nms.Entity automatically check this and return false if it's over 1024. These methods are directly called by the API addScoreboardTag method. The problem is, the API also has getScoreboardTags which directly exposes the underlying set, no copy, no unmodifiable wrapper, nothing. So a plugin can just add strings to that set to add over 1024 tags which are then saved, and lost on a reload of the entity.

Either we copy the set to make it a mutable copy or wrap it in an unmodifiable wrapper or we do an immutable copy.

[imDaniX]

Should this be unmodifiable/a copy tho? The javadoc sentence Entities can have no more than 1024 tags. seems to imply that you can* add tags. I feel like this should be rather return a set that delegates everything to the underlying set, but overrides add and addAll methods, like

@Override
public boolean add(String tag) {
    return entity.addScoreboardTag(tag);
}

@Override
public boolean addAll(@NotNull Collection<? extends String> tags) {
    boolean edited = false;
    for (String tag : tags) {
        edited = entity.addScoreboardTag(tag) || edited;
    }
    return edited;
}

[Machine-Maker]

Not super opposed to that solution. But I'm not sure that the sentence you quoted does imply that. It just seems unlikely that was the intention when there is also an "add" method right next to the getScoreboardTags method. In general I don't think its good to have add/remove methods as well as expose a mutable set which can just take the place of those add/remove methods.

[lynxplay]

I agree with machine that the javadocs are very open to interpretation in regards to the returned set being mutable. However, this API has existed for so long and is an upstream API as well, that we obviously cannot just return an unmodifiable set.

I think we'd indeed be best of to wrap the returned set in a form of delegating wrapper. We can already start potentially printing nags that this behaviour is strongly discouraged and "deprecated for removal". Not that we can remove it until hardfork, but a heads up warning via nag may be a useful addition to danix's suggestion.