-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/Issue: Compile-time checks to store metadata useful for runtime debugging of AT name conflicts #7
Comments
To not leave information stuck on discord, example provided in the linked discord conversation involves the following class in a mod with no ATs: public class CustomItem extends InstrumentItem {
public CustomItem(Properties properties, TagKey<Instrument> tagKey) {
super(properties, tagKey);
}
@Override
public @NotNull ItemStack getDefaultInstance() {
return create(this, BuiltInRegistries.INSTRUMENT.getHolderOrThrow(Instruments.CALL_GOAT_HORN));
}
// making this `getInstrument` (a human readable name) in 1.20.2, with mojmaps at runtime, would cause
// the same issue - hence why this is a worry now when it was not so much in the past.
protected Optional<ResourceLocation> m_220134_(ItemStack stack) {
var tag = stack.getTag();
if (tag != null && tag.contains("instrument", Tag.TAG_STRING)) {
var instrument = tag.getString("instrument");
return Optional.ofNullable(ResourceLocation.tryParse(instrument));
} else {
return Optional.empty();
}
}
} (In mojang official mappings for 1.20.1, |
NeoForged has plans to move to Mojmaps at runtime; obviously, this offers countless advantages. However, there are some pitfalls that can arise. Most notably, this means that vanilla classes will have human-readable names at runtime. This increases the chance of a collision between an existing private vanilla method and a mod method added on a subclass. This can lead to one of two issues. Assuming that Minecraft has some class
A
with a private instance methodsomeMethod
, and that modfirstmod
creates a subclass ofA
that happens to habe a method namedsomeMethod
(this does not override the super method normally, as the super method is private), and modsecondmod
ATssomeMethod
to be public for unrelated reasons, if the descriptors of the twosomeMethod
s line up:ClassCastException
with a log that implicates neither mod involved.The only reliable way to prevent a mod from falling prey to this is to either not AT private instance methods (even on final classes, as someone else could AT the final away), which is unreasonable to enforce given that this is one of the most useful cases for ATs as it allows overriding private methods, or to not make instance methods that share names and descriptors with private vanilla methods - which is probably a good idea, though unreasonable to enforce as doing so is perfectly valid java normally.
This is likely a low priority issue - the number of situations where this pops up is likely very low. However, the chances that it will, eventually, pop up in production are not necessarily that low, given that some modders definitely copy and slightly modify private helper methods from vanilla classes, and given that you can find a reason to AT practically any method in Minecraft if you try hard enough. With this in mind, I propose the following solution, which works as a combined runtime/compile-time checker:
This solution allows for easy diagnosis of this variety of issue at runtime, while placing no burdens on mod developers, whether they use ATs or not.
To clarify, as this was apparently not well communicated elsewhere: I do not believe that this issue is likely to be anywhere near common enough that it should delay the implementation of runtime MojMaps in NeoForge, or change what ATs are allowed to do. I simply believe that the potential severity of issues that could arise, and, more importantly, the difficulty in tracking down the issue-causing mods in question (likely to be much harder than diagnosing mixin or coremod issues, in my opinion), addressing it eventually is likely a good idea.
For an example reproducing this issue (using srg names, so unlikely to ever occur at runtime given the mangled nature of the method names), see this discussion on discord: https://discord.com/channels/313125603924639766/1136320550168436798/1152344220867248189. This was a simple, proof-of-concept example designed to crash - a naturally arising example may cause unexpected, undefined behaviour instead. Notably, the stack trace produced by the crash contained no information pointing towards either of the mods in question, and the most useful debugging information ("I clicked this thing before it crashed") pointed only to the mod with the conflicting method, not the mod with the AT.
The text was updated successfully, but these errors were encountered: