PersistentIConfigNode patch can cause deprecated parts to show up (enum parsing becomes case-sensitive?)

[JonnyOThan]

As described here: https://forum.kerbalspaceprogram.com/topic/204002-18-112-kspcommunityfixes-bugfixes-and-qol-tweaks/?do=findComment&comment=4315970

Describe your problem with KSPCF : When the PersistentIConfigNode patch is enabled, many deprecated parts from Tantares will appear in the VAB. This does not occur when the PersistentIConfigNode patch is disabled. The root of the issue seems to be that these parts have their category set to None instead of none, but the stock code seems to accept either one (or possibly there's some other mechanism that sets the category to none if it fails to parse. Notably none is -1 instead of 0, and enums in C# will default to 0.

KSP version : 1.12.5
Link to your KSP.log file : https://drive.google.com/drive/folders/1AbMk8hD8jh2PLdSfEBo5I2TBCTrFqlIY?usp=sharing

[gotmachine]

So...

When parsing an enum, the KSPCF parser will actually catch exceptions rising when the config value is undefined in the enum, with the end result being the enum set to its default value, which in the case of the PartCategories enum will be Propulsion, since as you noted, the none option has a value of -1.

In stock, the exception isn't catched immediately, resulting in the whole part failing to load :

[ERR 16:14:24.259] PartLoader: Encountered exception during compilation. System.ArgumentException: Requested value 'None' was not found.
  at System.Enum+EnumResult.SetFailure (System.Enum+ParseFailureKind failure, System.String failureMessageID, System.Object failureMessageFormatArgument) [0x00023] in <9577ac7a62ef43179789031239ba8798>:0 
  at System.Enum.TryParseEnum (System.Type enumType, System.String value, System.Boolean ignoreCase, System.Enum+EnumResult& parseResult) [0x0017a] in <9577ac7a62ef43179789031239ba8798>:0 
  at System.Enum.Parse (System.Type enumType, System.String value, System.Boolean ignoreCase) [0x00010] in <9577ac7a62ef43179789031239ba8798>:0 
  at System.Enum.Parse (System.Type enumType, System.String value) [0x00000] in <9577ac7a62ef43179789031239ba8798>:0 
  at ConfigNode.ParseEnum (System.Type enumType, System.String vectorString) [0x00000] in <39c0323fb6b449a4aaf3465c00ed3c8d>:0 
  at ConfigNode.ReadValue (System.Type fieldType, System.String value) [0x00516] in <39c0323fb6b449a4aaf3465c00ed3c8d>:0 
  at ConfigNode.ReadObject (System.Object obj, ConfigNode node) [0x00143] in <39c0323fb6b449a4aaf3465c00ed3c8d>:0 
  at ConfigNode.LoadObjectFromConfig (System.Object obj, ConfigNode node, System.Int32 pass, System.Boolean removeAfterUse) [0x0001a] in <39c0323fb6b449a4aaf3465c00ed3c8d>:0 
  at PartLoader.ParsePart (UrlDir+UrlConfig urlConfig, ConfigNode node) [0x001a9] in <39c0323fb6b449a4aaf3465c00ed3c8d>:0 
  at (wrapper dynamic-method) PartLoader+<CompileParts>d__56.PartLoader+<CompileParts>d__56.MoveNext_Patch0(PartLoader/<CompileParts>d__56)
	UnityEngine.DebugLogHandler:LogFormat(LogType, Object, String, Object[])
	ModuleManager.UnityLogHandle.InterceptLogHandler:LogFormat(LogType, Object, String, Object[])
	UnityEngine.Debug:LogError(Object)
	<CompileParts>d__56:PartLoader+<CompileParts>d__56.MoveNext_Patch0(<CompileParts>d__56)
	KSPCommunityFixes.Performance.<FrameUnlockedCoroutine>d__62:MoveNext() (at C:/Users/helen/Source/Repos/KSPModdingLibs/KSPCommunityFixes/KSPCommunityFixes/Performance/FastLoader.cs:1917)
	KSPCommunityFixes.Performance.<PartLoader_CompileAll>d__58:MoveNext() (at C:/Users/helen/Source/Repos/KSPModdingLibs/KSPCommunityFixes/KSPCommunityFixes/Performance/FastLoader.cs:1785)
	UnityEngine.SetupCoroutine:InvokeMoveNext(IEnumerator, IntPtr)
[ERR 16:14:24.260] PartCompiler: Cannot compile part

While this indeed results in a different behavior, I'd say that the issue is on the mod author. I guess the intent was to soft-depreciate some parts, hidding them in the part list but avoiding to break existing crafts, which isn't achieved in stock as a result of the config typo.

On the KSPCF side, I guess we should add some logging when an enum parsing fails, but the behavior of try/catching early sounds better than letting it propagate, see the current code for reference :

KSPCommunityFixes/KSPCommunityFixes/Modding/PersistentIConfigNode.cs

Lines 625 to 633 in 081c3bd

	case DataType.ValueEnum:
	try
	{
	return Enum.Parse(fieldType, value);
	}
	catch
	{
	return null;
	}

[JonnyOThan]

Nice catch. While I agree the soft-deprecation doesn’t work without KSPCF, it’s definitely a worse experience for the user when KSPCF is installed. What do you think about changing enum parsing to be case-insensitive? That would fix this issue and possibly other similar ones….but quite a weird hack.

[gotmachine]

I'd say that the user experience problem has more to do with broken mods configs and little with KSPCF, and changing enum parsing to case-insensitive is just as likely to break stuff, in any case we're in undefined behavior territory. This being said, I agree that swallowing the exception silently is bad, and that definitely warrants adding some logging, I think this should do :

[LOG 17:14:56.263] PartLoader: Compiling Part 'Squad/Parts/Utility/parachuteMk25/parachuteMk25/parachuteDrogue'
[WRN 17:14:56.273] [KSPCF] Couldn't parse value 'None' for enum 'PartCategories', default value 'Propulsion' will be used.
Valid values are Propulsion, Control, Structural, Aero, Utility, Science, Pods, FuelTank, Engine, Communication, Electrical, Ground, Thermal, Payload, Coupling, Cargo, Robotics, none
[WRN 17:14:56.274] PartLoader Warning: Variable  not found in Part

Besides, this only happens in a very specific code path (the ObjectFrom/ToConfig serializers), which is mainly intended for automatic serialization and deserialization of custom non-unity objects, and rarely used in anything that is intended to be edited manually.

[JonnyOThan]

I dunno, I feel pretty strongly that KSPCF shouldn't do what it's currently doing here. A mod author might never notice the exception in the logs if everything in game seems fine, and they shouldn't have to test their mod with KSPCF installed to see if it might break. Possible options:

• failing to compile the part like stock does
• setting the category to none if it failed to parse (this seems difficult since it's so specific and the parsing code is so general). But the choice of defaulting to propulsion is essentially completely arbitrary; none is just as arbitrary and likely always better.
• using case-insensitive enum parsing (though I totally agree that is a kludge and could be risky).

[gotmachine]

Well, if we take the current example, the parts wrongly being present in the propulsion category is a clear in-game indication that something isn't fine, whereas the stock behavior resulting in part compilation had a less immediately visible effect. I'd argue that in practice the KSPCF behavior allowed a bug in that mod to be detected and hopefully reported.

Letting the exception propagate isn't a good idea. For all other types, the behavior for a value parsing failing is to ignore that value, this notably has the benefit of providing a stable behavior for changes in the data model.

[gotmachine]

I don't intend to adress this more than the logging I added. I something arise, feel free to open a new issue.