From 54e7fb7a0bcd994f6d49348c879cb96902dbe07b Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Wed, 26 Aug 2020 22:32:59 -0400 Subject: [PATCH 1/3] fix some broken field references not detected --- docs/release-notes.md | 1 + .../ModLoading/Finders/ReferenceToMissingMemberFinder.cs | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/release-notes.md b/docs/release-notes.md index 7e928aed..21f9d213 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -15,6 +15,7 @@ * Fixed broken URL in update alerts for unofficial versions. * Fixed rare error when a mod adds/removes event handlers asynchronously. * Fixed rare issue where the console showed incorrect colors when mods wrote to it asynchronously. + * Fixed SMAPI not always detecting broken field references in mod code. * Removed the experimental `RewriteInParallel` option added in SMAPI 3.6 (it was already disabled by default). Unfortunately this caused intermittent and unpredictable errors when enabled. * For modders: diff --git a/src/SMAPI/Framework/ModLoading/Finders/ReferenceToMissingMemberFinder.cs b/src/SMAPI/Framework/ModLoading/Finders/ReferenceToMissingMemberFinder.cs index 9afd1de0..75575c97 100644 --- a/src/SMAPI/Framework/ModLoading/Finders/ReferenceToMissingMemberFinder.cs +++ b/src/SMAPI/Framework/ModLoading/Finders/ReferenceToMissingMemberFinder.cs @@ -1,5 +1,4 @@ using System.Collections.Generic; -using System.Linq; using Mono.Cecil; using Mono.Cecil.Cil; using StardewModdingAPI.Framework.ModLoading.Framework; @@ -35,8 +34,8 @@ namespace StardewModdingAPI.Framework.ModLoading.Finders FieldReference fieldRef = RewriteHelper.AsFieldReference(instruction); if (fieldRef != null && this.ShouldValidate(fieldRef.DeclaringType)) { - FieldDefinition target = fieldRef.DeclaringType.Resolve()?.Fields.FirstOrDefault(p => p.Name == fieldRef.Name); - if (target == null) + FieldDefinition target = fieldRef.Resolve(); + if (target == null || target.HasConstant) { this.MarkFlag(InstructionHandleResult.NotCompatible, $"reference to {fieldRef.DeclaringType.FullName}.{fieldRef.Name} (no such field)"); return false; From 0bf692addc3e309a8448de9ffb2a41cb701cfddf Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Wed, 26 Aug 2020 23:11:41 -0400 Subject: [PATCH 2/3] add heuristic rewrite for field => const changes --- docs/release-notes.md | 12 ++-- .../ModLoading/Framework/RewriteHelper.cs | 18 +++++ .../Rewriters/HeuristicFieldRewriter.cs | 69 ++++++++++++++----- .../Rewriters/HeuristicMethodRewriter.cs | 20 +----- .../StaticFieldToConstantRewriter.cs | 65 ----------------- src/SMAPI/Metadata/InstructionMetadata.cs | 3 - 6 files changed, 78 insertions(+), 109 deletions(-) delete mode 100644 src/SMAPI/Framework/ModLoading/Rewriters/StaticFieldToConstantRewriter.cs diff --git a/docs/release-notes.md b/docs/release-notes.md index 21f9d213..554b9518 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -26,12 +26,12 @@ * For SMAPI developers: * The web API now returns an update alert in two new cases: any newer unofficial update (previously only shown if the mod was incompatible), and a newer prerelease version if the installed non-prerelease version is broken (previously only shown if the installed version was prerelease). - * Internal refactoring to simplify game updates: - * Reorganised SMAPI core to reduce coupling to `Game1` and make it easier to navigate. - * Added rewriter for any method broken due to new optional parameters. - * Added rewriter for any field which was replaced by a property. - * `FieldReplaceRewriter` now supports mapping to a different target type. - * Internal refactoring to simplify future game updates. + * Reorganised SMAPI core to reduce coupling to `Game1`, make it easier to navigate, and simplify future game updates. + * SMAPI now automatically fixes code broken by these changes in game code, so manual rewriters are no longer needed: + * reference to a method with new optional parameters; + * reference to a field replaced by a property; + * reference to a field replaced by a `const` field. + * `FieldReplaceRewriter` now supports mapping to a different target type. ## 3.6.2 Released 02 August 2020 for Stardew Valley 1.4.1 or later. diff --git a/src/SMAPI/Framework/ModLoading/Framework/RewriteHelper.cs b/src/SMAPI/Framework/ModLoading/Framework/RewriteHelper.cs index 36058b86..4b88148f 100644 --- a/src/SMAPI/Framework/ModLoading/Framework/RewriteHelper.cs +++ b/src/SMAPI/Framework/ModLoading/Framework/RewriteHelper.cs @@ -59,6 +59,24 @@ namespace StardewModdingAPI.Framework.ModLoading.Framework : null; } + /// Get the CIL instruction to load a value onto the stack. + /// The constant value to inject. + /// Returns the instruction, or null if the value type isn't supported. + public static Instruction GetLoadValueInstruction(object rawValue) + { + return rawValue switch + { + null => Instruction.Create(OpCodes.Ldnull), + bool value => Instruction.Create(value ? OpCodes.Ldc_I4_1 : OpCodes.Ldc_I4_0), + int value => Instruction.Create(OpCodes.Ldc_I4, value), // int32 + long value => Instruction.Create(OpCodes.Ldc_I8, value), // int64 + float value => Instruction.Create(OpCodes.Ldc_R4, value), // float32 + double value => Instruction.Create(OpCodes.Ldc_R8, value), // float64 + string value => Instruction.Create(OpCodes.Ldstr, value), + _ => null + }; + } + /// Get whether a type matches a type reference. /// The defined type. /// The type reference. diff --git a/src/SMAPI/Framework/ModLoading/Rewriters/HeuristicFieldRewriter.cs b/src/SMAPI/Framework/ModLoading/Rewriters/HeuristicFieldRewriter.cs index 5a088ed8..ca04205c 100644 --- a/src/SMAPI/Framework/ModLoading/Rewriters/HeuristicFieldRewriter.cs +++ b/src/SMAPI/Framework/ModLoading/Rewriters/HeuristicFieldRewriter.cs @@ -6,7 +6,7 @@ using StardewModdingAPI.Framework.ModLoading.Framework; namespace StardewModdingAPI.Framework.ModLoading.Rewriters { - /// Automatically fix references to fields that have been replaced by a property. + /// Automatically fix references to fields that have been replaced by a property or const field. internal class HeuristicFieldRewriter : BaseInstructionHandler { /********* @@ -36,23 +36,16 @@ namespace StardewModdingAPI.Framework.ModLoading.Rewriters return false; // skip if not broken - if (fieldRef.Resolve() != null) + FieldDefinition fieldDefinition = fieldRef.Resolve(); + if (fieldDefinition != null && !fieldDefinition.HasConstant) return false; - // get equivalent property - PropertyDefinition property = fieldRef.DeclaringType.Resolve().Properties.FirstOrDefault(p => p.Name == fieldRef.Name); - MethodDefinition method = instruction.OpCode == OpCodes.Ldsfld || instruction.OpCode == OpCodes.Ldfld - ? property?.GetMethod - : property?.SetMethod; - if (method == null) - return false; - - // rewrite field to property - instruction.OpCode = OpCodes.Call; - instruction.Operand = module.ImportReference(method); - - this.Phrases.Add($"{fieldRef.DeclaringType.Name}.{fieldRef.Name} (field => property)"); - return this.MarkRewritten(); + // rewrite if possible + TypeDefinition declaringType = fieldRef.DeclaringType.Resolve(); + bool isRead = instruction.OpCode == OpCodes.Ldsfld || instruction.OpCode == OpCodes.Ldfld; + return + this.TryRewriteToProperty(module, instruction, fieldRef, declaringType, isRead) + || this.TryRewriteToConstField(instruction, fieldDefinition); } @@ -65,5 +58,49 @@ namespace StardewModdingAPI.Framework.ModLoading.Rewriters { return type != null && this.RewriteReferencesToAssemblies.Contains(type.Scope.Name); } + + /// Try rewriting the field into a matching property. + /// The assembly module containing the instruction. + /// The CIL instruction to rewrite. + /// The field reference. + /// The type on which the field was defined. + /// Whether the field is being read; else it's being written to. + private bool TryRewriteToProperty(ModuleDefinition module, Instruction instruction, FieldReference fieldRef, TypeDefinition declaringType, bool isRead) + { + // get equivalent property + PropertyDefinition property = declaringType.Properties.FirstOrDefault(p => p.Name == fieldRef.Name); + MethodDefinition method = isRead ? property?.GetMethod : property?.SetMethod; + if (method == null) + return false; + + // rewrite field to property + instruction.OpCode = OpCodes.Call; + instruction.Operand = module.ImportReference(method); + + this.Phrases.Add($"{fieldRef.DeclaringType.Name}.{fieldRef.Name} (field => property)"); + return this.MarkRewritten(); + } + + /// Try rewriting the field into a matching const field. + /// The CIL instruction to rewrite. + /// The field definition. + private bool TryRewriteToConstField(Instruction instruction, FieldDefinition field) + { + // must have been a static field read, and the new field must be const + if (instruction.OpCode != OpCodes.Ldsfld || field?.HasConstant != true) + return false; + + // get opcode for value type + Instruction loadInstruction = RewriteHelper.GetLoadValueInstruction(field.Constant); + if (loadInstruction == null) + return false; + + // rewrite to constant + instruction.OpCode = loadInstruction.OpCode; + instruction.Operand = loadInstruction.Operand; + + this.Phrases.Add($"{field.DeclaringType.Name}.{field.Name} (field => const)"); + return this.MarkRewritten(); + } } } diff --git a/src/SMAPI/Framework/ModLoading/Rewriters/HeuristicMethodRewriter.cs b/src/SMAPI/Framework/ModLoading/Rewriters/HeuristicMethodRewriter.cs index 21b42e12..e133b6fa 100644 --- a/src/SMAPI/Framework/ModLoading/Rewriters/HeuristicMethodRewriter.cs +++ b/src/SMAPI/Framework/ModLoading/Rewriters/HeuristicMethodRewriter.cs @@ -64,7 +64,7 @@ namespace StardewModdingAPI.Framework.ModLoading.Rewriters // get instructions to inject parameter values var loadInstructions = method.Parameters.Skip(methodRef.Parameters.Count) - .Select(p => this.GetLoadValueInstruction(p.Constant)) + .Select(p => RewriteHelper.GetLoadValueInstruction(p.Constant)) .ToArray(); if (loadInstructions.Any(p => p == null)) return false; // SMAPI needs to load the value onto the stack before the method call, but the optional parameter type wasn't recognized @@ -105,23 +105,5 @@ namespace StardewModdingAPI.Framework.ModLoading.Rewriters return true; } - - /// Get the CIL instruction to load a value onto the stack. - /// The constant value to inject. - /// Returns the instruction, or null if the value type isn't supported. - private Instruction GetLoadValueInstruction(object rawValue) - { - return rawValue switch - { - null => Instruction.Create(OpCodes.Ldnull), - bool value => Instruction.Create(value ? OpCodes.Ldc_I4_1 : OpCodes.Ldc_I4_0), - int value => Instruction.Create(OpCodes.Ldc_I4, value), // int32 - long value => Instruction.Create(OpCodes.Ldc_I8, value), // int64 - float value => Instruction.Create(OpCodes.Ldc_R4, value), // float32 - double value => Instruction.Create(OpCodes.Ldc_R8, value), // float64 - string value => Instruction.Create(OpCodes.Ldstr, value), - _ => null - }; - } } } diff --git a/src/SMAPI/Framework/ModLoading/Rewriters/StaticFieldToConstantRewriter.cs b/src/SMAPI/Framework/ModLoading/Rewriters/StaticFieldToConstantRewriter.cs deleted file mode 100644 index f34d4943..00000000 --- a/src/SMAPI/Framework/ModLoading/Rewriters/StaticFieldToConstantRewriter.cs +++ /dev/null @@ -1,65 +0,0 @@ -using System; -using Mono.Cecil; -using Mono.Cecil.Cil; -using StardewModdingAPI.Framework.ModLoading.Framework; - -namespace StardewModdingAPI.Framework.ModLoading.Rewriters -{ - /// Rewrites static field references into constant values. - /// The constant value type. - internal class StaticFieldToConstantRewriter : BaseInstructionHandler - { - /********* - ** Fields - *********/ - /// The type containing the field to which references should be rewritten. - private readonly Type Type; - - /// The field name to which references should be rewritten. - private readonly string FromFieldName; - - /// The constant value to replace with. - private readonly TValue Value; - - - /********* - ** Public methods - *********/ - /// Construct an instance. - /// The type whose field to which references should be rewritten. - /// The field name to rewrite. - /// The constant value to replace with. - public StaticFieldToConstantRewriter(Type type, string fieldName, TValue value) - : base(defaultPhrase: $"{type.FullName}.{fieldName} field") - { - this.Type = type; - this.FromFieldName = fieldName; - this.Value = value; - } - - /// - public override bool Handle(ModuleDefinition module, ILProcessor cil, Instruction instruction) - { - // get field reference - FieldReference fieldRef = RewriteHelper.AsFieldReference(instruction); - if (!RewriteHelper.IsFieldReferenceTo(fieldRef, this.Type.FullName, this.FromFieldName)) - return false; - - // rewrite to constant - if (typeof(TValue) == typeof(int)) - { - instruction.OpCode = OpCodes.Ldc_I4; - instruction.Operand = this.Value; - } - else if (typeof(TValue) == typeof(string)) - { - instruction.OpCode = OpCodes.Ldstr; - instruction.Operand = this.Value; - } - else - throw new NotSupportedException($"Rewriting to constant values of type {typeof(TValue)} isn't currently supported."); - - return this.MarkRewritten(); - } - } -} diff --git a/src/SMAPI/Metadata/InstructionMetadata.cs b/src/SMAPI/Metadata/InstructionMetadata.cs index 86e16e1e..09a199f9 100644 --- a/src/SMAPI/Metadata/InstructionMetadata.cs +++ b/src/SMAPI/Metadata/InstructionMetadata.cs @@ -35,9 +35,6 @@ namespace StardewModdingAPI.Metadata if (platformChanged) yield return new MethodParentRewriter(typeof(SpriteBatch), typeof(SpriteBatchFacade)); - // rewrite for Stardew Valley 1.3 - yield return new StaticFieldToConstantRewriter(typeof(Game1), "tileSize", Game1.tileSize); - // heuristic rewrites yield return new HeuristicFieldRewriter(this.ValidateReferencesToAssemblies); yield return new HeuristicMethodRewriter(this.ValidateReferencesToAssemblies); From e57fc0eb15ea3976072949453a831d493b9225db Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Wed, 26 Aug 2020 23:27:32 -0400 Subject: [PATCH 3/3] fix old comment --- src/SMAPI/Framework/Events/EventManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SMAPI/Framework/Events/EventManager.cs b/src/SMAPI/Framework/Events/EventManager.cs index b5a12a6e..9092669f 100644 --- a/src/SMAPI/Framework/Events/EventManager.cs +++ b/src/SMAPI/Framework/Events/EventManager.cs @@ -11,7 +11,7 @@ namespace StardewModdingAPI.Framework.Events internal class EventManager { /********* - ** Events (new) + ** Events *********/ /**** ** Display