RFC: Backwards-compatibility policy and allowed changes

[robojumper]

Rendered

Our compatibility policy so far has basically amounted to "don't break other mods". At the same time, it's very easy to construct an example mod that would be "broken" by many Highlander changes (see individual paragraphs for examples).

Recently, there has been a heated discussion [1], [2] regarding which kind of changes are acceptable.

I interpret this as a demand for a more rigorous specification of our compatibility policy.

Please use code comments to start discussions on specific parts as to keep the discussion somewhat threaded.

[Musashi1584]

Deprivatize variables and functions is a tool that let modders circumvent limitiations when dealing with sub classes of e.g actions, targeting methods and mod class overrides. Since the usage of protected and private seems completely arbitrary and inconsistent in the code base this is often necessary. Also boundaries of native functions can make this necessary.

This method is highlander agnostic so it also can and is used by modders who don't use the highlander and possibly don't know about any rules and regulations about private scopes.

It is near to impossible to monitor the code base of all mods on the workshop to detect possible incompatibilities if we change private variables or functions signatures assuming its safe to do cause we as a small team of highlander devs defined private scope as off limits for all modders out there (which is also presumptuous imo).
I also see very little benefit in changing method signatures. You can avoid this by calling new intermediate methods to achieve the same thing. You have a little bit more boilerplate code but are sure you won’t break any mods.

So i strongly recommend treating private functions and variables the same as if they were public. As a matter of fact i would even deprivatize all of them to make this clear and to solve the virtual/static dispatch issue of deprivatizing private functions locally. This also has the benefit that modders who compile against the HL don't need to create pull requests for deprivatizing stuff all the time.

[robojumper]

I can totally agree with not changing type signatures of base-game private functions/properties. I do however have an issue with

deprivatiz[ing] all of them to make this clear and to solve the virtual/static dispatch issue of deprivatizing private functions locally

1. It isn't practical.
2. It makes performance worse across the board because virtual dispatch is more expensive than static dispatch.
3. Hacking around with private functions is supposed to be difficult. Hacking around in private internals of a class runs the risk of violating invariants the modder didn't even know existed, and increases the chance of a technically not violating change (one that doesn't crash the game outright) to still break the behavior of a mod.

If you very much know what you're doing and accept the risk of violating private invariants (as Iridar said "the value was originally meant to be private, and must be used outside of own class with the necessary level of care."), you can deprivatize things locally. If you want we can even host in a separate repo a script with a disclaimer that looks at the SrcOrig folder, rudimentarily parses the UC files and strips all visibility modifiers, but adding ~18MB of source files to the repo to only deprivatize everything is not something I can get behind.

[Musashi1584]

A very specific and not well-known hack in the Unreal Virtual machine
This is an assumption and even if it is not commonly used at the moment we don't know if it will stay this way.
Every change we make now to the actual code structure could result in a future incompatibility with mods that use private members or functions or pose issue with updates by firaxis.

[Iridar]

Changing function signatures means changing parameters (type, amount, etc) or changing a function to static, changing the return type etc.

Uhhh why would anyone would ever want to do that to a vanilla function? I thought doing something like that would cause a crash on startup or something like that.

[Musashi1584]

See the discussion here: #826
Further down Xymanek posted an example where that happened and why (

X2WOTCCommunityHighlander/X2WOTCCommunityHighlander/Src/XComGame/Classes/XComGameState_CovertAction.uc

Lines 912 to 913 in 45b7370

	// Issue #436: CHL function modified: first parameter changed from "int ChanceToOccur" to "CovertActionRisk ActionRisk"
	private function int CalculateRiskChanceToOccurModifiers(CovertActionRisk ActionRisk, bool bChosenIncreaseRisks, bool bDarkEventRisk)

)

[Musashi1584]

This would crash when someone deprivatizes the function and calls or override it in a mod. This is my main issue with doing that.

[Iridar]

This would crash when someone deprivatizes the function and calls or override it in a mod. This is my main issue with doing that.

I see. What would happen if it is called through a non-private wrapper?

[Musashi1584]

You can avoid changing function signatures e.g by calling new intermediate functions. this would be save to do. Theoretical example (don't know if its actually viable here):

private function int CalculateRiskChanceToOccurModifiers(int ChanceToOccur, bool bChosenIncreaseRisks, bool bDarkEventRisk)
{
   local CovertActionRisk ActionRisk;

   ActionRisk = GetActionRisk();
   return CalculateRiskChanceToOccurModifiers_Internal(ActionRisk, bChosenIncreaseRisks, bDarkEventRisk);
}

[robojumper]

(Not viable here because this is a pure math function. Best to simply keep the old function around but not use it)

[Iridar]

I vote for more freedom and ease of access, i.e. bulk deprivatize stuff, as long as that's done via /* comment */ tags, to make it clear the value was originally meant to be private, and must be used outside of own class with the necessary level of care.

On the topic of changing function signatures, I'm not sure what that means, deprivatizing functions/methods? Adding arguments to them? I vote for maximum compatibility here.

[Musashi1584]

Changing function signatures means changing parameters (type, amount, etc) or changing a function to static, changing the return type etc.

[robojumper]

In the document I've called it "changing type signatures" because an incompatibility there almost 100% crashes the game. Visibility modifiers are part of the function signature too, but this RFC is all about the interaction between visibility and backwards-compatibility.

[Xymanek]

I've decided to abstain from this discussion for two reasons:

1. There are enough knowledgeable people in CHL team to make a "correct" decision
2. I do not believe that repeating what I said already will bring any value

However, I would recommend 2 things:

1. Please figure out whether overriding private methods works or not. Musashi and Iridar suggest that it does, while robo has shown theoretical evidence to the contrary
2. We need to adopt better review practices so that something that is reviewed, accepted and merged is not suddenly criticized a year later

[Musashi1584]

1. Please figure out whether overriding private methods works or not. Musashi and Iridar suggest that it does, while robo has shown theoretical evidence to the contrary

The difference is if the source of all calling parties are compiled or not.
e.g it worked for me cause i made the change in my SrcOrig and then compiled the HL locally. This indicates that if we deprivatize functions in the HL and all calling locations are part of the Highlander the problem is solved.

2. We need to adopt better review practices so that something that is reviewed, accepted and merged is not suddenly criticized a year later

I think this RFC is one step into this direction. Having an actual compatibility policy makes it easier to make decisions in reviews. I would also like to note that i don't meant to blame the old pull request since we never had a clear decision on this. I thought we had, but its clear now that it just existed in my head. From my opinion we should just handle things differently in the future.