rewrite analyzers to match more cases, use readable warning IDs (#471)

7 files changed, 93 insertions, 96 deletions

diff --git a/docs/mod-build-config.md b/docs/mod-build-config.md
index 99a567f2..d942beeb 100644
--- a/docs/mod-build-config.md
+++ b/docs/mod-build-config.md
@@ -153,8 +153,8 @@ You can hide the warnings...
 
 See below for help with each specific warning.
 
-### SMAPI001
-**Implicit net field conversion:**
+### Avoid implicit net field cast
+Warning text:
 > This implicitly converts '{{expression}}' from {{net type}} to {{other type}}, but
 > {{net type}} has unintuitive implicit conversion rules. Consider comparing against the actual
 > value instead to avoid bugs.
@@ -185,8 +185,8 @@ Suggested fix:
   if (item != null && item.category.Value == 0)
   ```
 
-### SMAPI002
-**Avoid net fields when possible:**
+### Avoid net field
+Warning text:
 > '{{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
@@ -194,8 +194,8 @@ field has an equivalent non-net property that avoids those issues.
 
 Suggested fix: access the suggested property name instead.
 
-### SMAPI003
-**Avoid obsolete fields:**
+### Avoid obsolete field
+Warning text:
 > The '{{old field}}' field is obsolete and should be replaced with '{{new field}}'.
 
 Your code accesses a field which is obsolete or no longer works. Use the suggested field instead.
diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs
index 101f4c21..79ce9263 100644
--- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs
+++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs
@@ -91,8 +91,8 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
             string code = NetFieldAnalyzerTests.SampleProgram.Replace("{{test-code}}", codeText);
             DiagnosticResult expected = new DiagnosticResult
             {
-                Id = "SMAPI001",
-                Message = $"This implicitly converts '{expression}' from {fromType} to {toType}, but {fromType} has unintuitive implicit conversion rules. Consider comparing against the actual value instead to avoid bugs. See https://smapi.io/buildmsg/smapi001 for details.",
+                Id = "AvoidImplicitNetFieldCast",
+                Message = $"This implicitly converts '{expression}' from {fromType} to {toType}, but {fromType} 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.",
                 Severity = DiagnosticSeverity.Warning,
                 Locations = new[] { new DiagnosticResultLocation("Test0.cs", NetFieldAnalyzerTests.SampleCodeLine, NetFieldAnalyzerTests.SampleCodeColumn + column) }
             };
@@ -117,8 +117,8 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
             string code = NetFieldAnalyzerTests.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.",
+                Id = "AvoidNetField",
+                Message = $"'{expression}' is a {netType} field; consider using the {suggestedProperty} property instead. See https://smapi.io/buildmsg/avoid-net-field for details.",
                 Severity = DiagnosticSeverity.Warning,
                 Locations = new[] { new DiagnosticResultLocation("Test0.cs", NetFieldAnalyzerTests.SampleCodeLine, NetFieldAnalyzerTests.SampleCodeColumn + column) }
             };
diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/ObsoleteFieldAnalyzerTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/ObsoleteFieldAnalyzerTests.cs
index dc7476ef..102a80d1 100644
--- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/ObsoleteFieldAnalyzerTests.cs
+++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/ObsoleteFieldAnalyzerTests.cs
@@ -59,14 +59,15 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests
         /// <param name="oldName">The old field name which should be reported.</param>
         /// <param name="newName">The new field name which should be reported.</param>
         [TestCase("var x = new Farmer().friendships;", 8, "StardewValley.Farmer.friendships", "friendshipData")]
+        [TestCase("var x = new Farmer()?.friendships;", 8, "StardewValley.Farmer.friendships", "friendshipData")]
         public void AvoidObsoleteField_RaisesDiagnostic(string codeText, int column, string oldName, string newName)
         {
             // arrange
             string code = ObsoleteFieldAnalyzerTests.SampleProgram.Replace("{{test-code}}", codeText);
             DiagnosticResult expected = new DiagnosticResult
             {
-                Id = "SMAPI003",
-                Message = $"The '{oldName}' field is obsolete and should be replaced with '{newName}'. See https://smapi.io/buildmsg/smapi003 for details.",
+                Id = "AvoidObsoleteField",
+                Message = $"The '{oldName}' field is obsolete and should be replaced with '{newName}'. See https://smapi.io/buildmsg/avoid-obsolete-field for details.",
                 Severity = DiagnosticSeverity.Warning,
                 Locations = new[] { new DiagnosticResultLocation("Test0.cs", ObsoleteFieldAnalyzerTests.SampleCodeLine, ObsoleteFieldAnalyzerTests.SampleCodeColumn + column) }
             };
diff --git a/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs b/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs
new file mode 100644
index 00000000..77e7812f
--- /dev/null
+++ b/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs
@@ -0,0 +1,46 @@
+using Microsoft.CodeAnalysis;
+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 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 GetMemberInfo(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;
+        }
+    }
+}
diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs
index 915a50e8..895eebf0 100644
--- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs
+++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.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
@@ -127,23 +126,23 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         /// <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",
+            ["AvoidImplicitNetFieldCast"] = 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/smapi001 for details.",
+                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/smapi001"
+                helpLinkUri: "https://smapi.io/buildmsg/avoid-implicit-net-field-cast"
             ),
-            ["SMAPI002"] = new DiagnosticDescriptor(
-                id: "SMAPI002",
+            ["AvoidNetField"] = 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/smapi002 for details.",
+                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/smapi001"
+                helpLinkUri: "https://smapi.io/buildmsg/avoid-net-field"
             )
         };
 
@@ -170,19 +169,9 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         {
             // SMAPI002: avoid net fields if possible
             context.RegisterSyntaxNodeAction(
-                this.AnalyzeAvoidableNetField,
-                SyntaxKind.SimpleMemberAccessExpression
-            );
-
-            // SMAPI001: avoid implicit net field conversion
-            context.RegisterSyntaxNodeAction(
-                this.AnalyseNetFieldConversions,
-                SyntaxKind.EqualsExpression,
-                SyntaxKind.NotEqualsExpression,
-                SyntaxKind.GreaterThanExpression,
-                SyntaxKind.GreaterThanOrEqualExpression,
-                SyntaxKind.LessThanExpression,
-                SyntaxKind.LessThanOrEqualExpression
+                this.AnalyzeMemberAccess,
+                SyntaxKind.SimpleMemberAccessExpression,
+                SyntaxKind.ConditionalAccessExpression
             );
         }
 
@@ -192,68 +181,30 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
         *********/
         /// <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 AnalyzeAvoidableNetField(SyntaxNodeAnalysisContext context)
+        private void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context)
         {
             try
             {
-                // check member type
-                MemberAccessExpressionSyntax node = (MemberAccessExpressionSyntax)context.Node;
-                TypeInfo memberType = context.SemanticModel.GetTypeInfo(node);
+                // get member access info
+                if (!AnalyzerUtilities.GetMemberInfo(context.Node, context.SemanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName))
+                    return;
                 if (!this.IsNetType(memberType.Type))
                     return;
+                bool isConverted = !this.IsNetType(memberType.ConvertedType);
 
-                // get reference info
-                ITypeSymbol declaringType = context.SemanticModel.GetTypeInfo(node.Expression).Type;
-                string propertyName = node.Name.Identifier.Text;
-
-                // suggest replacement
+                // warn: use property wrapper if available
                 for (ITypeSymbol type = declaringType; type != null; type = type.BaseType)
                 {
-                    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.Rules["AvoidNetField"], context.Node.GetLocation(), context.Node, memberType.Type.Name, suggestedPropertyName));
+                        return;
                     }
                 }
-            }
-            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 binaryExpression = (BinaryExpressionSyntax)context.Node;
-                foreach (var pair in new[] { Tuple.Create(binaryExpression.Left, binaryExpression.Right), Tuple.Create(binaryExpression.Right, binaryExpression.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))
-                        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"));
-                        break;
-                    }
-                }
+                // warn: implicit conversion
+                if (isConverted)
+                    context.ReportDiagnostic(Diagnostic.Create(this.Rules["AvoidImplicitNetFieldCast"], context.Node.GetLocation(), context.Node, memberType.Type.Name, memberType.ConvertedType));
             }
             catch (Exception ex)
             {
diff --git a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs
index 00565329..dc21e505 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"
             )
         };
 
@@ -60,7 +59,8 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer
             // SMAPI003: avoid obsolete fields
             context.RegisterSyntaxNodeAction(
                 this.AnalyzeObsoleteFields,
-                SyntaxKind.SimpleMemberAccessExpression
+                SyntaxKind.SimpleMemberAccessExpression,
+                SyntaxKind.ConditionalAccessExpression
             );
         }
 
@@ -75,16 +75,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.GetMemberInfo(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)
                 {
-                    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/package.nuspec b/src/SMAPI.ModBuildConfig/package.nuspec
index 92e7e81e..2512c4d6 100644
--- a/src/SMAPI.ModBuildConfig/package.nuspec
+++ b/src/SMAPI.ModBuildConfig/package.nuspec
@@ -2,7 +2,7 @@
 <package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
   <metadata>
     <id>Pathoschild.Stardew.ModBuildConfig</id>
-    <version>2.1-alpha20180410</version>
+    <version>2.1-alpha20180414</version>
     <title>Build package for SMAPI mods</title>
     <authors>Pathoschild</authors>
     <owners>Pathoschild</owners>