warn when directly using a net field that has a non-net wrapper (#471)

docs/mod-build-config.md

@@ -182,6 +182,15 @@ Suggested fix:
   if (item != null && item.category == 0)
   ```
 
+### SMAPI002
+**Avoid net fields when possible:**
+> '{{expression}}' is a {{net type}} field; consider using the {{property name}} property instead.
+
+Your code accesses a net field, which has some unusual behavior (see [SMAPI001](#SMAPI001)). This
+field has an equivalent non-net property that avoids those issues.
+
+Suggested fix: access the suggested property name instead.
+
 ## Troubleshoot
 ### "Failed to find the game install path"
 That error means the package couldn't find your game. You can specify the game path yourself; see

src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs

@@ -16,6 +16,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
         /// <summary>Sample C# code which contains a simplified representation of Stardew Valley's <c>Netcode</c> types, and sample mod code with a {{test-code}} placeholder for the code being tested.</summary>
         const string SampleProgram = @"
             using System;
+            using StardewValley;
             using Netcode;
 
             namespace Netcode
@@ -29,13 +30,16 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
                 }
             }
 
-            namespace SampleMod
+            namespace StardewValley
             {
                 class Item
                 {
                     public NetInt category { get; } = new NetInt { Value = 42 };
                 }
+            }
 
+            namespace SampleMod
+            {
                 class ModEntry
                 {
                     public void Entry()
@@ -45,17 +49,17 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
                         Item item = null;
 
                         // this line should raise diagnostics
-                        {{test-code}} // line 32
+                        {{test-code}} // line 36
 
                         // these lines should not
-                        if ((int)field != 42);
+                        if ((int)intField != 42);
                     }
                 }
             }
         ";
 
         /// <summary>The line number where the unit tested code is injected into <see cref="SampleProgram"/>.</summary>
-        private const int SampleCodeLine = 32;
+        private const int SampleCodeLine = 36;
 
         /// <summary>The column number where the unit tested code is injected into <see cref="SampleProgram"/>.</summary>
         private const int SampleCodeColumn = 25;
@@ -75,7 +79,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
             this.VerifyCSharpDiagnostic(test);
         }
 
-        /// <summary>Test that the expected diagnostic message is raised.</summary>
+        /// <summary>Test that the expected diagnostic message is raised for implicit net field comparisons.</summary>
         /// <param name="codeText">The code line to test.</param>
         /// <param name="column">The column within the code line where the diagnostic message should be reported.</param>
         /// <param name="expression">The expression which should be reported.</param>
@@ -89,7 +93,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
         [TestCase("if (intField != 42);", 4, "intField", "NetInt", "Int32")]
         [TestCase("if (refField != null);", 4, "refField", "NetRef", "Object")]
         [TestCase("if (item?.category != 42);", 4, "item?.category", "NetInt", "Int32")]
-        public void ImplicitComparisons_RaiseDiagnostics(string codeText, int column, string expression, string fromType, string toType)
+        public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType)
         {
             // arrange
             string code = UnitTests.SampleProgram.Replace("{{test-code}}", codeText);
@@ -105,6 +109,31 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
             this.VerifyCSharpDiagnostic(code, expected);
         }
 
+        /// <summary>Test that the expected diagnostic message is raised for avoidable net field references.</summary>
+        /// <param name="codeText">The code line to test.</param>
+        /// <param name="column">The column within the code line where the diagnostic message should be reported.</param>
+        /// <param name="expression">The expression which should be reported.</param>
+        /// <param name="netType">The net type name which should be reported.</param>
+        /// <param name="suggestedProperty">The suggested property name which should be reported.</param>
+        [TestCase("int category = item.category;", 15, "item.category", "NetInt", "Category")]
+        [TestCase("int category = (item).category;", 15, "(item).category", "NetInt", "Category")]
+        [TestCase("int category = ((Item)item).category;", 15, "((Item)item).category", "NetInt", "Category")]
+        public void AvoidNetFields_RaisesDiagnostic(string codeText, int column, string expression, string netType, string suggestedProperty)
+        {
+            // arrange
+            string code = UnitTests.SampleProgram.Replace("{{test-code}}", codeText);
+            DiagnosticResult expected = new DiagnosticResult
+            {
+                Id = "SMAPI002",
+                Message = $"'{expression}' is a {netType} field; consider using the {suggestedProperty} property instead. See https://smapi.io/buildmsg/SMAPI002 for details.",
+                Severity = DiagnosticSeverity.Warning,
+                Locations = new[] { new DiagnosticResultLocation("Test0.cs", UnitTests.SampleCodeLine, UnitTests.SampleCodeColumn + column) }
+            };
+
+            // assert
+            this.VerifyCSharpDiagnostic(code, expected);
+        }
+
 
         /*********
         ** Helpers
@@ -112,7 +141,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
         /// <summary>Get the analyzer being tested.</summary>
         protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer()
         {
-            return new ImplicitNetFieldCastAnalyzer();
+            return new NetFieldAnalyzer();
         }
     }
 }

src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.Collections.Immutable;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
@@ -9,7 +10,7 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
 {
     /// <summary>Detects implicit conversion from Stardew Valley's <c>Netcode</c> types. These have very unintuitive implicit conversion rules, so mod authors should always explicitly convert the type with appropriate null checks.</summary>
     [DiagnosticAnalyzer(LanguageNames.CSharp)]
-    public class ImplicitNetFieldCastAnalyzer : DiagnosticAnalyzer
+    public class NetFieldAnalyzer : DiagnosticAnalyzer
     {
         /*********
         ** Properties
@@ -17,8 +18,116 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         /// <summary>The namespace for Stardew Valley's <c>Netcode</c> types.</summary>
         private const string NetcodeNamespace = "Netcode";
 
+        /// <summary>Maps net fields to their equivalent non-net properties where available.</summary>
+        private readonly IDictionary<string, string> NetFieldWrapperProperties = new Dictionary<string, string>
+        {
+            // Character
+            ["StardewValley.Character::currentLocationRef"] = "currentLocation",
+            ["StardewValley.Character::facingDirection"] = "FacingDirection",
+            ["StardewValley.Character::name"] = "Name",
+            ["StardewValley.Character::position"] = "Position",
+            ["StardewValley.Character::scale"] = "Scale",
+            ["StardewValley.Character::speed"] = "Speed",
+            ["StardewValley.Character::sprite"] = "Sprite",
+
+            // Chest
+            ["StardewValley.Objects.Chest::tint"] = "Tint",
+
+            // Farmer
+            ["StardewValley.Farmer::houseUpgradeLevel"] = "HouseUpgradeLevel",
+            ["StardewValley.Farmer::isMale"] = "IsMale",
+            ["StardewValley.Farmer::items"] = "Items",
+            ["StardewValley.Farmer::magneticRadius"] = "MagneticRadius",
+            ["StardewValley.Farmer::stamina"] = "Stamina",
+            ["StardewValley.Farmer::uniqueMultiplayerID"] = "UniqueMultiplayerID",
+            ["StardewValley.Farmer::usingTool"] = "UsingTool",
+
+            // Forest
+            ["StardewValley.Locations.Forest::netTravelingMerchantDay"] = "travelingMerchantDay",
+            ["StardewValley.Locations.Forest::netLog"] = "log",
+
+            // FruitTree
+            ["StardewValley.TerrainFeatures.FruitTree::greenHouseTileTree"] = "GreenHouseTileTree",
+            ["StardewValley.TerrainFeatures.FruitTree::greenHouseTree"] = "GreenHouseTree",
+
+            // GameLocation
+            ["StardewValley.GameLocation::isFarm"] = "IsFarm",
+            ["StardewValley.GameLocation::isOutdoors"] = "IsOutdoors",
+            ["StardewValley.GameLocation::lightLevel"] = "LightLevel",
+            ["StardewValley.GameLocation::name"] = "Name",
+
+            // Item
+            ["StardewValley.Item::category"] = "Category",
+            ["StardewValley.Item::netName"] = "Name",
+            ["StardewValley.Item::parentSheetIndex"] = "ParentSheetIndex",
+            ["StardewValley.Item::specialVariable"] = "SpecialVariable",
+
+            // Junimo
+            ["StardewValley.Characters.Junimo::eventActor"] = "EventActor",
+
+            // LightSource
+            ["StardewValley.LightSource::identifier"] = "Identifier",
+
+            // Monster
+            ["StardewValley.Monsters.Monster::damageToFarmer"] = "DamageToFarmer",
+            ["StardewValley.Monsters.Monster::experienceGained"] = "ExperienceGained",
+            ["StardewValley.Monsters.Monster::health"] = "Health",
+            ["StardewValley.Monsters.Monster::maxHealth"] = "MaxHealth",
+            ["StardewValley.Monsters.Monster::netFocusedOnFarmers"] = "focusedOnFarmers",
+            ["StardewValley.Monsters.Monster::netWildernessFarmMonster"] = "wildernessFarmMonster",
+            ["StardewValley.Monsters.Monster::slipperiness"] = "Slipperiness",
+
+            // NPC
+            ["StardewValley.NPC::age"] = "Age",
+            ["StardewValley.NPC::birthday_Day"] = "Birthday_Day",
+            ["StardewValley.NPC::birthday_Season"] = "Birthday_Season",
+            ["StardewValley.NPC::breather"] = "Breather",
+            ["StardewValley.NPC::defaultMap"] = "DefaultMap",
+            ["StardewValley.NPC::gender"] = "Gender",
+            ["StardewValley.NPC::hideShadow"] = "HideShadow",
+            ["StardewValley.NPC::isInvisible"] = "IsInvisible",
+            ["StardewValley.NPC::isWalkingTowardPlayer"] = "IsWalkingTowardPlayer",
+            ["StardewValley.NPC::manners"] = "Manners",
+            ["StardewValley.NPC::optimism"] = "Optimism",
+            ["StardewValley.NPC::socialAnxiety"] = "SocialAnxiety",
+
+            // Object
+            ["StardewValley.Object::canBeGrabbed"] = "CanBeGrabbed",
+            ["StardewValley.Object::canBeSetDown"] = "CanBeSetDown",
+            ["StardewValley.Object::edibility"] = "Edibility",
+            ["StardewValley.Object::flipped"] = "Flipped",
+            ["StardewValley.Object::fragility"] = "Fragility",
+            ["StardewValley.Object::hasBeenPickedUpByFarmer"] = "HasBeenPickedUpByFarmer",
+            ["StardewValley.Object::isHoedirt"] = "IsHoeDirt",
+            ["StardewValley.Object::isOn"] = "IsOn",
+            ["StardewValley.Object::isRecipe"] = "IsRecipe",
+            ["StardewValley.Object::isSpawnedObject"] = "IsSpawnedObject",
+            ["StardewValley.Object::minutesUntilReady"] = "MinutesUntilReady",
+            ["StardewValley.Object::netName"] = "name",
+            ["StardewValley.Object::price"] = "Price",
+            ["StardewValley.Object::quality"] = "Quality",
+            ["StardewValley.Object::scale"] = "Scale",
+            ["StardewValley.Object::stack"] = "Stack",
+            ["StardewValley.Object::tileLocation"] = "TileLocation",
+            ["StardewValley.Object::type"] = "Type",
+
+            // Projectile
+            ["StardewValley.Projectiles.Projectile::ignoreLocationCollision"] = "IgnoreLocationCollision",
+
+            // Tool
+            ["StardewValley.Tool::currentParentTileIndex"] = "CurrentParentTileIndex",
+            ["StardewValley.Tool::indexOfMenuItemView"] = "IndexOfMenuItemView",
+            ["StardewValley.Tool::initialParentTileIndex"] = "InitialParentTileIndex",
+            ["StardewValley.Tool::instantUse"] = "InstantUse",
+            ["StardewValley.Tool::netName"] = "BaseName",
+            ["StardewValley.Tool::stackable"] = "Stackable",
+            ["StardewValley.Tool::upgradeLevel"] = "UpgradeLevel"
+        };
+
         /// <summary>Describes the diagnostic rule covered by the analyzer.</summary>
-        private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(
+        private readonly IDictionary<string, DiagnosticDescriptor> Rules = new Dictionary<string, DiagnosticDescriptor>
+        {
+            ["SMAPI001"] = new DiagnosticDescriptor(
                 id: "SMAPI001",
                 title: "Netcode types shouldn't be implicitly converted",
                 messageFormat: "This implicitly converts '{0}' from {1} to {2}, but {1} has unintuitive implicit conversion rules. Consider comparing against the actual value instead to avoid bugs. See https://smapi.io/buildmsg/SMAPI001 for details.",
@@ -27,7 +136,18 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
                 isEnabledByDefault: true,
                 description: "",
                 helpLinkUri: "https://smapi.io/buildmsg/SMAPI001"
-        );
+            ),
+            ["SMAPI002"] = new DiagnosticDescriptor(
+                id: "SMAPI002",
+                title: "Avoid Netcode types when possible",
+                messageFormat: "'{0}' is a {1} field; consider using the {2} property instead. See https://smapi.io/buildmsg/SMAPI002 for details.",
+                category: "SMAPI.CommonErrors",
+                defaultSeverity: DiagnosticSeverity.Warning,
+                isEnabledByDefault: true,
+                description: "",
+                helpLinkUri: "https://smapi.io/buildmsg/SMAPI001"
+            )
+        };
 
 
         /*********
@@ -41,17 +161,24 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         ** Public methods
         *********/
         /// <summary>Construct an instance.</summary>
-        public ImplicitNetFieldCastAnalyzer()
+        public NetFieldAnalyzer()
         {
-            this.SupportedDiagnostics = ImmutableArray.Create(ImplicitNetFieldCastAnalyzer.Rule);
+            this.SupportedDiagnostics = ImmutableArray.CreateRange(this.Rules.Values);
         }
 
         /// <summary>Called once at session start to register actions in the analysis context.</summary>
         /// <param name="context">The analysis context.</param>
         public override void Initialize(AnalysisContext context)
         {
+            // SMAPI002: avoid net fields if possible
             context.RegisterSyntaxNodeAction(
-                this.Analyse,
+                this.AnalyzeAvoidableNetField,
+                SyntaxKind.SimpleMemberAccessExpression
+            );
+
+            // SMAPI001: avoid implicit net field conversion
+            context.RegisterSyntaxNodeAction(
+                this.AnalyseNetFieldConversions,
                 SyntaxKind.EqualsExpression,
                 SyntaxKind.NotEqualsExpression,
                 SyntaxKind.GreaterThanExpression,
@@ -65,16 +192,44 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         /*********
         ** Private methods
         *********/
-        /// <summary>Analyse a syntax node and add a diagnostic message if applicable.</summary>
+        /// <summary>Analyse a syntax node and add a diagnostic message if it references a net field when there's a non-net equivalent available.</summary>
         /// <param name="context">The analysis context.</param>
-        private void Analyse(SyntaxNodeAnalysisContext context)
+        private void AnalyzeAvoidableNetField(SyntaxNodeAnalysisContext context)
+        {
+            try
+            {
+                // check member type
+                MemberAccessExpressionSyntax node = (MemberAccessExpressionSyntax)context.Node;
+                TypeInfo memberType = context.SemanticModel.GetTypeInfo(node);
+                if (!this.IsNetType(memberType.Type))
+                    return;
+
+                // get reference info
+                ITypeSymbol declaringType = context.SemanticModel.GetTypeInfo(node.Expression).Type;
+                string propertyName = node.Name.Identifier.Text;
+
+                // suggest replacement
+                if (this.NetFieldWrapperProperties.TryGetValue($"{declaringType}::{propertyName}", out string suggestedPropertyName))
+                {
+                    context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI002"], context.Node.GetLocation(), node, memberType.Type.Name, suggestedPropertyName));
+                }
+            }
+            catch (Exception ex)
+            {
+                throw new InvalidOperationException($"Failed processing expression: '{context.Node}'. Exception details: {ex.ToString().Replace('\r', ' ').Replace('\n', ' ')}");
+            }
+        }
+
+        /// <summary>Analyse a syntax node and add a diagnostic message if it implicitly converts a net field.</summary>
+        /// <param name="context">The analysis context.</param>
+        private void AnalyseNetFieldConversions(SyntaxNodeAnalysisContext context)
         {
             try
             {
                 BinaryExpressionSyntax node = (BinaryExpressionSyntax)context.Node;
-                bool leftHasWarning = this.Analyze(context, node.Left);
+                bool leftHasWarning = this.WarnIfOperandImplicitlyConvertsNetField(context, node.Left);
                 if (!leftHasWarning)
-                    this.Analyze(context, node.Right);
+                    this.WarnIfOperandImplicitlyConvertsNetField(context, node.Right);
             }
             catch (Exception ex)
             {
@@ -86,22 +241,25 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         /// <param name="context">The analysis context.</param>
         /// <param name="operand">The operand expression.</param>
         /// <returns>Returns whether a diagnostic message was raised.</returns>
-        private bool Analyze(SyntaxNodeAnalysisContext context, ExpressionSyntax operand)
+        private bool WarnIfOperandImplicitlyConvertsNetField(SyntaxNodeAnalysisContext context, ExpressionSyntax operand)
         {
-            const string netcodeNamespace = ImplicitNetFieldCastAnalyzer.NetcodeNamespace;
-
             TypeInfo operandType = context.SemanticModel.GetTypeInfo(operand);
-            string fromNamespace = operandType.Type?.ContainingNamespace?.Name;
-            string toNamespace = operandType.ConvertedType?.ContainingNamespace?.Name;
-            if (fromNamespace == netcodeNamespace && fromNamespace != toNamespace && toNamespace != null)
+            if (this.IsNetType(operandType.Type) && !this.IsNetType(operandType.ConvertedType))
             {
                 string fromTypeName = operandType.Type.Name;
                 string toTypeName = operandType.ConvertedType.Name;
-                context.ReportDiagnostic(Diagnostic.Create(ImplicitNetFieldCastAnalyzer.Rule, context.Node.GetLocation(), operand, fromTypeName, toTypeName));
+                context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI001"], context.Node.GetLocation(), operand, fromTypeName, toTypeName));
                 return true;
             }
 
             return false;
         }
+
+        /// <summary>Get whether a type symbol references a <c>Netcode</c> type.</summary>
+        /// <param name="typeSymbol">The type symbol.</param>
+        private bool IsNetType(ITypeSymbol typeSymbol)
+        {
+            return typeSymbol?.ContainingNamespace?.Name == NetFieldAnalyzer.NetcodeNamespace;
+        }
     }
 }