Merge branch 'develop' into stable

4 files changed, 228 insertions, 79 deletions

diff --git a/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs b/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs
new file mode 100644
index 00000000..68b5001e
--- /dev/null
+++ b/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs
@@ -0,0 +1,93 @@
+using System.Collections.Generic;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+
+namespace StardewModdingAPI.ModBuildConfig.Analyzer
+{
+    /// <summary>Provides generic utilities for SMAPI's Roslyn analyzers.</summary>
+    internal static class AnalyzerUtilities
+    {
+        /*********
+        ** Public methods
+        *********/
+        /// <summary>Get the metadata for an explicit cast or 'x as y' expression.</summary>
+        /// <param name="node">The member access expression.</param>
+        /// <param name="semanticModel">provides methods for asking semantic questions about syntax nodes.</param>
+        /// <param name="fromExpression">The expression whose value is being converted.</param>
+        /// <param name="fromType">The type being converted from.</param>
+        /// <param name="toType">The type being converted to.</param>
+        /// <returns>Returns true if the node is a matched expression, else false.</returns>
+        public static bool TryGetCastOrAsInfo(SyntaxNode node, SemanticModel semanticModel, out ExpressionSyntax fromExpression, out TypeInfo fromType, out TypeInfo toType)
+        {
+            // (type)x
+            if (node is CastExpressionSyntax cast)
+            {
+                fromExpression = cast.Expression;
+                fromType = semanticModel.GetTypeInfo(fromExpression);
+                toType = semanticModel.GetTypeInfo(cast.Type);
+                return true;
+            }
+
+            // x as y
+            if (node is BinaryExpressionSyntax binary && binary.Kind() == SyntaxKind.AsExpression)
+            {
+                fromExpression = binary.Left;
+                fromType = semanticModel.GetTypeInfo(fromExpression);
+                toType = semanticModel.GetTypeInfo(binary.Right);
+                return true;
+            }
+
+            // invalid
+            fromExpression = null;
+            fromType = default(TypeInfo);
+            toType = default(TypeInfo);
+            return false;
+        }
+
+        /// <summary>Get the metadata for a member access expression.</summary>
+        /// <param name="node">The member access expression.</param>
+        /// <param name="semanticModel">provides methods for asking semantic questions about syntax nodes.</param>
+        /// <param name="declaringType">The object type which has the member.</param>
+        /// <param name="memberType">The type of the accessed member.</param>
+        /// <param name="memberName">The name of the accessed member.</param>
+        /// <returns>Returns true if the node is a member access expression, else false.</returns>
+        public static bool TryGetMemberInfo(SyntaxNode node, SemanticModel semanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName)
+        {
+            // simple access
+            if (node is MemberAccessExpressionSyntax memberAccess)
+            {
+                declaringType = semanticModel.GetTypeInfo(memberAccess.Expression).Type;
+                memberType = semanticModel.GetTypeInfo(node);
+                memberName = memberAccess.Name.Identifier.Text;
+                return true;
+            }
+
+            // conditional access
+            if (node is ConditionalAccessExpressionSyntax conditionalAccess && conditionalAccess.WhenNotNull is MemberBindingExpressionSyntax conditionalBinding)
+            {
+                declaringType = semanticModel.GetTypeInfo(conditionalAccess.Expression).Type;
+                memberType = semanticModel.GetTypeInfo(node);
+                memberName = conditionalBinding.Name.Identifier.Text;
+                return true;
+            }
+
+            // invalid
+            declaringType = null;
+            memberType = default(TypeInfo);
+            memberName = null;
+            return false;
+        }
+
+        /// <summary>Get the class types in a type's inheritance chain, including itself.</summary>
+        /// <param name="type">The initial type.</param>
+        public static IEnumerable<ITypeSymbol> GetConcreteTypes(ITypeSymbol type)
+        {
+            while (type != null)
+            {
+                yield return type;
+                type = type.BaseType;
+            }
+        }
+    }
+}
diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs
index 915a50e8..e6766e61 100644
--- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs
+++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs
@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.Immutable;
+using System.Linq;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
@@ -21,6 +22,13 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         /// <summary>Maps net fields to their equivalent non-net properties where available.</summary>
         private readonly IDictionary<string, string> NetFieldWrapperProperties = new Dictionary<string, string>
         {
+            // AnimatedSprite
+            ["StardewValley.AnimatedSprite::currentAnimation"] = "CurrentAnimation",
+            ["StardewValley.AnimatedSprite::currentFrame"] = "CurrentFrame",
+            ["StardewValley.AnimatedSprite::sourceRect"] = "SourceRect",
+            ["StardewValley.AnimatedSprite::spriteHeight"] = "SpriteHeight",
+            ["StardewValley.AnimatedSprite::spriteWidth"] = "SpriteWidth",
+
             // Character
             ["StardewValley.Character::currentLocationRef"] = "currentLocation",
             ["StardewValley.Character::facingDirection"] = "FacingDirection",
@@ -106,7 +114,6 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
             ["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",
@@ -124,28 +131,27 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
             ["StardewValley.Tool::upgradeLevel"] = "UpgradeLevel"
         };
 
-        /// <summary>Describes the diagnostic rule covered by the analyzer.</summary>
-        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.",
-                category: "SMAPI.CommonErrors",
-                defaultSeverity: DiagnosticSeverity.Warning,
-                isEnabledByDefault: true,
-                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,
-                helpLinkUri: "https://smapi.io/buildmsg/smapi001"
-            )
-        };
+        /// <summary>The diagnostic info for an implicit net field cast.</summary>
+        private readonly DiagnosticDescriptor AvoidImplicitNetFieldCastRule = new DiagnosticDescriptor(
+            id: "AvoidImplicitNetFieldCast",
+            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/avoid-implicit-net-field-cast for details.",
+            category: "SMAPI.CommonErrors",
+            defaultSeverity: DiagnosticSeverity.Warning,
+            isEnabledByDefault: true,
+            helpLinkUri: "https://smapi.io/buildmsg/avoid-implicit-net-field-cast"
+        );
+
+        /// <summary>The diagnostic info for an avoidable net field access.</summary>
+        private readonly DiagnosticDescriptor AvoidNetFieldRule = new DiagnosticDescriptor(
+            id: "AvoidNetField",
+            title: "Avoid Netcode types when possible",
+            messageFormat: "'{0}' is a {1} field; consider using the {2} property instead. See https://smapi.io/buildmsg/avoid-net-field for details.",
+            category: "SMAPI.CommonErrors",
+            defaultSeverity: DiagnosticSeverity.Warning,
+            isEnabledByDefault: true,
+            helpLinkUri: "https://smapi.io/buildmsg/avoid-net-field"
+        );
 
 
         /*********
@@ -161,22 +167,25 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         /// <summary>Construct an instance.</summary>
         public NetFieldAnalyzer()
         {
-            this.SupportedDiagnostics = ImmutableArray.CreateRange(this.Rules.Values);
+            this.SupportedDiagnostics = ImmutableArray.CreateRange(new[] { this.AvoidNetFieldRule, this.AvoidImplicitNetFieldCastRule });
         }
 
         /// <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.AnalyzeAvoidableNetField,
-                SyntaxKind.SimpleMemberAccessExpression
+                this.AnalyzeMemberAccess,
+                SyntaxKind.SimpleMemberAccessExpression,
+                SyntaxKind.ConditionalAccessExpression
+            );
+            context.RegisterSyntaxNodeAction(
+                this.AnalyzeCast,
+                SyntaxKind.CastExpression,
+                SyntaxKind.AsExpression
             );
-
-            // SMAPI001: avoid implicit net field conversion
             context.RegisterSyntaxNodeAction(
-                this.AnalyseNetFieldConversions,
+                this.AnalyzeBinaryComparison,
                 SyntaxKind.EqualsExpression,
                 SyntaxKind.NotEqualsExpression,
                 SyntaxKind.GreaterThanExpression,
@@ -190,77 +199,126 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         /*********
         ** Private methods
         *********/
-        /// <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>
+        /// <summary>Analyse a member access syntax node and add a diagnostic message if applicable.</summary>
         /// <param name="context">The analysis context.</param>
-        private void AnalyzeAvoidableNetField(SyntaxNodeAnalysisContext context)
+        /// <returns>Returns whether any warnings were added.</returns>
+        private void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context)
         {
-            try
+            this.HandleErrors(context.Node, () =>
             {
-                // check member type
-                MemberAccessExpressionSyntax node = (MemberAccessExpressionSyntax)context.Node;
-                TypeInfo memberType = context.SemanticModel.GetTypeInfo(node);
+                // get member access info
+                if (!AnalyzerUtilities.TryGetMemberInfo(context.Node, context.SemanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName))
+                    return;
                 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
-                for (ITypeSymbol type = declaringType; type != null; type = type.BaseType)
+                // warn: use property wrapper if available
+                foreach (ITypeSymbol type in AnalyzerUtilities.GetConcreteTypes(declaringType))
                 {
-                    if (this.NetFieldWrapperProperties.TryGetValue($"{type}::{propertyName}", out string suggestedPropertyName))
+                    if (this.NetFieldWrapperProperties.TryGetValue($"{type}::{memberName}", out string suggestedPropertyName))
                     {
-                        context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI002"], context.Node.GetLocation(), node, memberType.Type.Name, suggestedPropertyName));
-                        break;
+                        context.ReportDiagnostic(Diagnostic.Create(this.AvoidNetFieldRule, context.Node.GetLocation(), context.Node, memberType.Type.Name, suggestedPropertyName));
+                        return;
                     }
                 }
-            }
-            catch (Exception ex)
+
+                // warn: implicit conversion
+                if (this.IsInvalidConversion(memberType.Type, memberType.ConvertedType))
+                {
+                    context.ReportDiagnostic(Diagnostic.Create(this.AvoidImplicitNetFieldCastRule, context.Node.GetLocation(), context.Node, memberType.Type.Name, memberType.ConvertedType));
+                    return;
+                }
+            });
+        }
+
+        /// <summary>Analyse an explicit cast or 'x as y' node and add a diagnostic message if applicable.</summary>
+        /// <param name="context">The analysis context.</param>
+        /// <returns>Returns whether any warnings were added.</returns>
+        private void AnalyzeCast(SyntaxNodeAnalysisContext context)
+        {
+            // NOTE: implicit conversion within the expression is detected by the member access
+            // checks. This method is only concerned with the conversion of its final value.
+            this.HandleErrors(context.Node, () =>
             {
-                throw new InvalidOperationException($"Failed processing expression: '{context.Node}'. Exception details: {ex.ToString().Replace('\r', ' ').Replace('\n', ' ')}");
-            }
+                if (AnalyzerUtilities.TryGetCastOrAsInfo(context.Node, context.SemanticModel, out ExpressionSyntax fromExpression, out TypeInfo fromType, out TypeInfo toType))
+                {
+                    if (this.IsInvalidConversion(fromType.ConvertedType, toType.Type))
+                        context.ReportDiagnostic(Diagnostic.Create(this.AvoidImplicitNetFieldCastRule, context.Node.GetLocation(), fromExpression, fromType.ConvertedType.Name, toType.Type));
+                }
+            });
         }
 
-        /// <summary>Analyse a syntax node and add a diagnostic message if it implicitly converts a net field.</summary>
+        /// <summary>Analyse a binary comparison syntax node and add a diagnostic message if applicable.</summary>
         /// <param name="context">The analysis context.</param>
-        private void AnalyseNetFieldConversions(SyntaxNodeAnalysisContext context)
+        /// <returns>Returns whether any warnings were added.</returns>
+        private void AnalyzeBinaryComparison(SyntaxNodeAnalysisContext context)
         {
-            try
+            // NOTE: implicit conversion within an operand is detected by the member access checks.
+            // This method is only concerned with the conversion of each side's final value.
+            this.HandleErrors(context.Node, () =>
             {
-                BinaryExpressionSyntax binaryExpression = (BinaryExpressionSyntax)context.Node;
-                foreach (var pair in new[] { Tuple.Create(binaryExpression.Left, binaryExpression.Right), Tuple.Create(binaryExpression.Right, binaryExpression.Left) })
+                BinaryExpressionSyntax expression = (BinaryExpressionSyntax)context.Node;
+                foreach (var pair in new[] { Tuple.Create(expression.Left, expression.Right), Tuple.Create(expression.Right, expression.Left) })
                 {
                     // get node info
                     ExpressionSyntax curExpression = pair.Item1; // the side of the comparison being examined
                     ExpressionSyntax otherExpression = pair.Item2; // the other side
-                    TypeInfo typeInfo = context.SemanticModel.GetTypeInfo(curExpression);
-                    if (!this.IsNetType(typeInfo.Type))
+                    TypeInfo curType = context.SemanticModel.GetTypeInfo(curExpression);
+                    TypeInfo otherType = context.SemanticModel.GetTypeInfo(otherExpression);
+                    if (!this.IsNetType(curType.ConvertedType))
                         continue;
 
-                    // warn for implicit conversion
-                    if (!this.IsNetType(typeInfo.ConvertedType))
-                    {
-                        context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI001"], context.Node.GetLocation(), curExpression, typeInfo.Type.Name, typeInfo.ConvertedType));
-                        break;
-                    }
-
                     // warn for comparison to null
                     // An expression like `building.indoors != null` will sometimes convert `building.indoors` to NetFieldBase instead of object before comparison. Haven't reproduced this in unit tests yet.
                     Optional<object> otherValue = context.SemanticModel.GetConstantValue(otherExpression);
                     if (otherValue.HasValue && otherValue.Value == null)
                     {
-                        context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI001"], context.Node.GetLocation(), curExpression, typeInfo.Type.Name, "null"));
+                        context.ReportDiagnostic(Diagnostic.Create(this.AvoidImplicitNetFieldCastRule, context.Node.GetLocation(), curExpression, curType.Type.Name, "null"));
+                        break;
+                    }
+
+                    // warn for implicit conversion
+                    if (!this.IsNetType(otherType.ConvertedType))
+                    {
+                        context.ReportDiagnostic(Diagnostic.Create(this.AvoidImplicitNetFieldCastRule, context.Node.GetLocation(), curExpression, curType.Type.Name, curType.ConvertedType));
                         break;
                     }
                 }
+            });
+        }
+
+        /// <summary>Handle exceptions raised while analyzing a node.</summary>
+        /// <param name="node">The node being analysed.</param>
+        /// <param name="action">The callback to invoke.</param>
+        private void HandleErrors(SyntaxNode node, Action action)
+        {
+            try
+            {
+                action();
             }
             catch (Exception ex)
             {
-                throw new InvalidOperationException($"Failed processing expression: '{context.Node}'. Exception details: {ex.ToString().Replace('\r', ' ').Replace('\n', ' ')}");
+                throw new InvalidOperationException($"Failed processing expression: '{node}'. Exception details: {ex.ToString().Replace('\r', ' ').Replace('\n', ' ')}");
             }
         }
 
+        /// <summary>Get whether a net field was converted in an error-prone way.</summary>
+        /// <param name="fromType">The source type.</param>
+        /// <param name="toType">The target type.</param>
+        private bool IsInvalidConversion(ITypeSymbol fromType, ITypeSymbol toType)
+        {
+            // no conversion
+            if (!this.IsNetType(fromType) || this.IsNetType(toType))
+                return false;
+
+            // conversion to implemented interface is OK
+            if (fromType.AllInterfaces.Contains(toType))
+                return false;
+
+            // avoid any other conversions
+            return true;
+        }
+
         /// <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)
diff --git a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs
index 00565329..3d353e52 100644
--- a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs
+++ b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs
@@ -3,7 +3,6 @@ using System.Collections.Generic;
 using System.Collections.Immutable;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
-using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
 
 namespace StardewModdingAPI.ModBuildConfig.Analyzer
@@ -25,14 +24,14 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         /// <summary>Describes the diagnostic rule covered by the analyzer.</summary>
         private readonly IDictionary<string, DiagnosticDescriptor> Rules = new Dictionary<string, DiagnosticDescriptor>
         {
-            ["SMAPI003"] = new DiagnosticDescriptor(
-                id: "SMAPI003",
+            ["AvoidObsoleteField"] = new DiagnosticDescriptor(
+                id: "AvoidObsoleteField",
                 title: "Reference to obsolete field",
-                messageFormat: "The '{0}' field is obsolete and should be replaced with '{1}'. See https://smapi.io/buildmsg/smapi003 for details.",
+                messageFormat: "The '{0}' field is obsolete and should be replaced with '{1}'. See https://smapi.io/buildmsg/avoid-obsolete-field for details.",
                 category: "SMAPI.CommonErrors",
                 defaultSeverity: DiagnosticSeverity.Warning,
                 isEnabledByDefault: true,
-                helpLinkUri: "https://smapi.io/buildmsg/smapi003"
+                helpLinkUri: "https://smapi.io/buildmsg/avoid-obsolete-field"
             )
         };
 
@@ -57,10 +56,10 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         /// <param name="context">The analysis context.</param>
         public override void Initialize(AnalysisContext context)
         {
-            // SMAPI003: avoid obsolete fields
             context.RegisterSyntaxNodeAction(
                 this.AnalyzeObsoleteFields,
-                SyntaxKind.SimpleMemberAccessExpression
+                SyntaxKind.SimpleMemberAccessExpression,
+                SyntaxKind.ConditionalAccessExpression
             );
         }
 
@@ -75,16 +74,15 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
             try
             {
                 // get reference info
-                MemberAccessExpressionSyntax node = (MemberAccessExpressionSyntax)context.Node;
-                ITypeSymbol declaringType = context.SemanticModel.GetTypeInfo(node.Expression).Type;
-                string propertyName = node.Name.Identifier.Text;
+                if (!AnalyzerUtilities.TryGetMemberInfo(context.Node, context.SemanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName))
+                    return;
 
                 // suggest replacement
-                for (ITypeSymbol type = declaringType; type != null; type = type.BaseType)
+                foreach (ITypeSymbol type in AnalyzerUtilities.GetConcreteTypes(declaringType))
                 {
-                    if (this.ReplacedFields.TryGetValue($"{type}::{propertyName}", out string replacement))
+                    if (this.ReplacedFields.TryGetValue($"{type}::{memberName}", out string replacement))
                     {
-                        context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI003"], context.Node.GetLocation(), $"{type}.{propertyName}", replacement));
+                        context.ReportDiagnostic(Diagnostic.Create(this.Rules["AvoidObsoleteField"], context.Node.GetLocation(), $"{type}.{memberName}", replacement));
                         break;
                     }
                 }
diff --git a/src/SMAPI.ModBuildConfig.Analyzer/StardewModdingAPI.ModBuildConfig.Analyzer.csproj b/src/SMAPI.ModBuildConfig.Analyzer/StardewModdingAPI.ModBuildConfig.Analyzer.csproj
index c32343e3..9d3f6d5b 100644
--- a/src/SMAPI.ModBuildConfig.Analyzer/StardewModdingAPI.ModBuildConfig.Analyzer.csproj
+++ b/src/SMAPI.ModBuildConfig.Analyzer/StardewModdingAPI.ModBuildConfig.Analyzer.csproj
@@ -12,7 +12,7 @@
   </ItemGroup>
    
   <ItemGroup>
-    <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="2.4.0" PrivateAssets="all" />
+    <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="2.8.2" PrivateAssets="all" />
     <PackageReference Update="NETStandard.Library" PrivateAssets="all" />
   </ItemGroup>