Disallow use of @Overwrite
LemmaEOF opened this issue · 6 comments
Overwrites are the primary issue with mixin mod compatibility at the moment. They're the easiest way to change a method but leave no room for forgiveness, even if it's only a couple lines that are ultimately changed. To prevent that in almost all cases, I propose this:
- disallow explicit
@Overwriteusage. Other cases called overwrites (implementing an interface method onto a targeted class) should still be allowed. - If someone wants to reimplement a whole method to change lines or flow, strongly suggest slicing or targeted injections. Failing that, instruct them to inject at head and
ci.cancel()at the end of their new reimplementation. - In the case that they want to truly overwrite a method and fundamentally change its function, add a
breakingflag to@Injectto set and prevent other injections on the same method.
The most important thing this plan provides is a barrier to entry for overwrites. Putting special caveats or rules in for when something can be overwritten will push people more towards considering how they can more respectfully transform methods, especially if they need to explicitly acknowledge that whatever they're changing will break things. The primary downside to this solution is load order issues, but I feel that it's preferable to both mixins annihilating each other because one is an improperly-used overwrite.
No, I dont think we should be limiting people.
I see you reasoning, but in some cases its the only choice, possibly adding a warning or something. I think the better longer term goal will be to try and remove the need the for the places where its required by expanding the API.
Overwrite has an irreplaceable function. We should not disallow usage of Overwrite until we have something to replace it.
I don't believe it is in the Fabric spirit to disallow breaking things in this manner. I believe we should focus on documentation and getting the message across - why Overwrite can cause issues and how to use the other available tools to avoid using it.
Yes, education could help. Mixins can be hard to get your head around if you dont understand them too well, overwrite is simple to understand, but some of the more complex parts to mixin could be very daunting.
I have seen some cases of people creating mixins where fabric-api has already got them covered, documentation on the fabric modules can be improved to try and reduce the unnecessary mixin usage.
inject at head and ci.cancel() at the end of their new reimplementation.
No, an inject at head is much worse for both compatibility and finding bugs.
Compatibility: An overwrite can be injected into, while a cancel at the method head will make other injections silently fail.
Finding bugs: Conflicting overrides clearly log which overwrites from which mod conflict, allowing the mod authors to solve the issue. An inject at head will cause other injections to just not get called making bugs very hard to find for the other mod's author! Also, there's currently no way to use priority have two injections at the same place, but priority can be used to have two overwrites that replace each other (for example, you can make a performance-only override low priority to allow it to be replaced by other mods).
An overwrite can be injected into
Really? Huh. I guess @ModifyConstant is counted as a type of overwrite, then. That's where my most recent compat issue came from.
I think a warning in the console is better, at least warn the user of incompatibilities.