Pocketmine-mp: Crack the "magic numbers" nut

[dktapps]

A few weeks ago I jokingly floated an idea for a way to solve the problem of plugin developers blindly hardcoding item magic number IDs. In the time since then, I've seen this problem crop up frequently enough to make me consider it for legitimate integration.

Problem

Plugin devs, no matter how much we tell them not to, hardcode block and item IDs everywhere. I've been warning for months that these IDs would eventually die and be replaced with stringy IDs, but this takes work and we're not there yet. Additionally, a transition to stringy IDs only solves this problem once.

Immediate term solution

Introduce a "constant value randomization". This works by having a global constant defined somewhere which is used as a randomization base for all constants. This would force plugin developers to use appropriate constants instead of hardcoding magic numbers. The theory is to make this as annoying as possible so that using item constants becomes the less painful solution.

Difficulty

This is relatively straightforward to implement, but requires a PHP extension to define the CVR base to ensure that it's always the same across threads, otherwise strange things could occur when moving items between threads.

Wider scope

It's possible, maybe even wise, to apply this concept more widely wherever magic numbers are used which have no real significance.

[95CivicSi]

While I agree that helping plugin developers to learn better coding standards is a good idea, this would potentially introduce confusion about where it's acceptable to use hard-coded numbers and where its not. It also puts unnecessary burden on server owners who rely heavily on free plugins that may not be well maintained.

An example situation where this would get confusing, would be the creation of splash potions. In the current state, developers would be required to use a constant for the item ID but not the meta when creating a splash potion. Avoiding situations like this would require constants to be created for all meta conditions.

As this would introduce a significant impact on how plugins work, this should probably target 4.0.0 unless it was integrated in such a way that allowed for existing plugins to continue functioning but throw a warning that support for this will end in 4.0.0.

[dktapps]

I think the unmaintained plugins thing is a red herring. Everything's guaranteed to catch fire in the future as I've been warning for many months now, no matter whether this gets implemented or not. I'm floating this now in case the revamp doesn't happen in time for 4.0 release.

This situation with magic numbers for metadata values as an unfortunate one indeed. This ultimately stems from poor API design and also because of poor design from Mojang. It's a similar problem we have with NBT where plugins hardcode tags. Terrible design has led to Mojang internals being exposed to API and to users. However I don't think that should stand in the way of something like this. The solution is not to continue allowing magic numbers, but to make sure that there are no magic numbers.

The concept of this is to make using magic numbers very unreliable. It won't emit any warning, it will just give you unexpected results. Unfortunately it's not possible to detect the usage of magic numbers, it's only possible to deter their usage by making them unreliable. As such, this is impossible to do in a backwards compatible manner, although plugins that are already using constants would work just fine with the change anyway. It's technically backwards compatible as long as you didn't rely on the IDs being the same anyway :)

[dktapps]

I'd also like to point out the if Mojang did decide to change the numeric IDs for some reason you'd all be royally fucked whether you liked it or not - unless you used constants :wink:
Such a change would not be pushed to a major version since it would be considered protocol related.

[95CivicSi]

To solve the problem with meta numbers, would it be feasible to introduce additional constants in the specific classes where that meta data was needed? With the splash potions example, constants would be added to the SplashPotion class for each variant.

As for the "impossible to do in a backwards compatible manor", what makes this urgent or critical enough to push it into the 3.X branches, disregarding backwards compatibility? From all appearances, PocketMine-MP itself doesn't really benefit from this internally as it already uses constants in most places, and the places where it doesn't could be updated to use them.

I'm not saying this shouldn't be added, just that it makes the most sense to be in the 4.0.0 branch.

[dktapps]

Your example is actually invalid because constants already exist for this. I should reiterate that this is a backwards incompatible change for a lot of plugins and as such it can't be integrated for a minor version. As I said in my previous comments I'm floating this idea now in case a revamp doesn't happen in time for the next major release. We have somewhat planned timelines for the next major release, and I don't want to put it on hold indefinitely because of something that isn't done that isn't critically necessary.

[HimbeersaftLP]

Unfortunately it's not possible to detect the usage of magic numbers, it's only possible to deter their usage by making them unreliable.

You could make the "compiler" (DevTools/Poggit) throw a Warning?

[dktapps]

This relies on users using tools that give them such warnings, and also people will happily ignore warnings if they don't do anything.

In addition, it would not be possible to analyze things like

$id = $args[1];
ItemFactory::get($id);

I have no intention whatsoever of writing a static analyzer, and the constant value randomization solution is much easier to implement.

[dktapps]

On 4.0, API changes are in progress which will remove the usage of IDs and magic numbers from the general API entirely, so this issue no longer serves any purpose.