From c2cb76b79919261c4d7eab107c5cb77ec6c6a81c Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sat, 14 Apr 2018 17:53:58 -0400 Subject: rewrite analyzers to match more cases, use readable warning IDs (#471) --- .../AnalyzerUtilities.cs | 46 +++++++++++ .../NetFieldAnalyzer.cs | 95 ++++++---------------- .../ObsoleteFieldAnalyzer.cs | 21 +++-- 3 files changed, 79 insertions(+), 83 deletions(-) create mode 100644 src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs (limited to 'src/SMAPI.ModBuildConfig.Analyzer') 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 +{ + /// Provides generic utilities for SMAPI's Roslyn analyzers. + internal static class AnalyzerUtilities + { + /********* + ** Public methods + *********/ + /// Get the metadata for a member access expression. + /// The member access expression. + /// provides methods for asking semantic questions about syntax nodes. + /// The object type which has the member. + /// The type of the accessed member. + /// The name of the accessed member. + /// Returns true if the node is a member access expression, else false. + 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 /// Describes the diagnostic rule covered by the analyzer. private readonly IDictionary Rules = new Dictionary { - ["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 *********/ /// Analyse a syntax node and add a diagnostic message if it references a net field when there's a non-net equivalent available. /// The analysis context. - 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', ' ')}"); - } - } - /// Analyse a syntax node and add a diagnostic message if it implicitly converts a net field. - /// The analysis context. - 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 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 /// Describes the diagnostic rule covered by the analyzer. private readonly IDictionary Rules = new Dictionary { - ["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; } } -- cgit From 6d8cf614a24ab69baffa89c351b9a22776741442 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sat, 14 Apr 2018 19:51:50 -0400 Subject: don't warn for NetList conversion to implemented interface (#471) --- .../Mock/Netcode/NetList.cs | 9 +++++++ .../Mock/Netcode/NetObjectList.cs | 6 +++++ .../Mock/StardewValley/Farmer.cs | 11 +++++++- .../NetFieldAnalyzerTests.cs | 19 +++++++++++++- .../AnalyzerUtilities.cs | 12 +++++++++ .../NetFieldAnalyzer.cs | 30 +++++++++++++++++++--- .../ObsoleteFieldAnalyzer.cs | 2 +- 7 files changed, 83 insertions(+), 6 deletions(-) create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetList.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetObjectList.cs (limited to 'src/SMAPI.ModBuildConfig.Analyzer') diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetList.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetList.cs new file mode 100644 index 00000000..1699f71c --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetList.cs @@ -0,0 +1,9 @@ +// ReSharper disable CheckNamespace -- matches Stardew Valley's code +using System.Collections; +using System.Collections.Generic; + +namespace Netcode +{ + /// A simplified version of Stardew Valley's Netcode.NetObjectList for unit testing. + public class NetList : List, IList, ICollection, IEnumerable, IEnumerable { } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetObjectList.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetObjectList.cs new file mode 100644 index 00000000..7814e7d6 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetObjectList.cs @@ -0,0 +1,6 @@ +// ReSharper disable CheckNamespace -- matches Stardew Valley's code +namespace Netcode +{ + /// A simplified version of Stardew Valley's Netcode.NetObjectList for unit testing. + public class NetObjectList : NetList { } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs index e0f0e30c..54e91682 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs @@ -1,11 +1,20 @@ // ReSharper disable CheckNamespace, InconsistentNaming -- matches Stardew Valley's code +#pragma warning disable 649 // (never assigned) -- only used to test type conversions using System.Collections.Generic; +using Netcode; namespace StardewValley { /// A simplified version of Stardew Valley's StardewValley.Farmer class for unit testing. internal class Farmer { - public IDictionary friendships; + /// A sample field which should be replaced with a different property. + public readonly IDictionary friendships; + + /// A sample net list. + public readonly NetList eventsSeen; + + /// A sample net object list. + public readonly NetObjectList netObjectList; } } diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs index 79ce9263..15bcadcd 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs @@ -19,6 +19,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests using StardewValley; using Netcode; using SObject = StardewValley.Object; + using SFarmer = StardewValley.Farmer; namespace SampleMod { @@ -33,7 +34,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests "; /// The line number where the unit tested code is injected into . - private const int SampleCodeLine = 13; + private const int SampleCodeLine = 14; /// The column number where the unit tested code is injected into . private const int SampleCodeColumn = 25; @@ -85,6 +86,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests [TestCase("SObject obj = null; if (obj.netRefField != null);", 24, "obj.netRefField", "NetRef", "object")] [TestCase("SObject obj = null; if (obj.netRefProperty == null);", 24, "obj.netRefProperty", "NetRef", "object")] [TestCase("SObject obj = null; if (obj.netRefProperty != null);", 24, "obj.netRefProperty", "NetRef", "object")] + [TestCase("SFarmer farmer = new SFarmer(); object list = farmer.eventsSeen;", 46, "farmer.eventsSeen", "NetList", "object")] // ↓ NetList field converted to a non-interface type public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType) { // arrange @@ -101,6 +103,21 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests this.VerifyCSharpDiagnostic(code, expected); } + /// Test that the net field analyzer doesn't raise any warnings for safe member access. + /// The code line to test. + [TestCase("SFarmer farmer = new SFarmer(); System.Collections.IEnumerable list = farmer.eventsSeen;")] + [TestCase("SFarmer farmer = new SFarmer(); System.Collections.Generic.IEnumerable list = farmer.eventsSeen;")] + [TestCase("SFarmer farmer = new SFarmer(); System.Collections.Generic.IList list = farmer.eventsSeen;")] + [TestCase("SFarmer farmer = new SFarmer(); System.Collections.Generic.IList list = farmer.netObjectList;")] // subclass of NetList + public void AvoidImplicitNetFieldComparisons_AllowsSafeAccess(string codeText) + { + // arrange + string code = NetFieldAnalyzerTests.SampleProgram.Replace("{{test-code}}", codeText); + + // assert + this.VerifyCSharpDiagnostic(code); + } + /// Test that the expected diagnostic message is raised for avoidable net field references. /// The code line to test. /// The column within the code line where the diagnostic message should be reported. diff --git a/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs b/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs index 77e7812f..e0c0cd63 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs @@ -1,3 +1,4 @@ +using System.Collections.Generic; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp.Syntax; @@ -42,5 +43,16 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer memberName = null; return false; } + + /// Get the class types in a type's inheritance chain, including itself. + /// The initial type. + public static IEnumerable 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 895eebf0..7c8b804e 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -1,6 +1,8 @@ using System; +using System.Collections; using System.Collections.Generic; using System.Collections.Immutable; +using System.Linq; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.Diagnostics; @@ -17,6 +19,9 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// The namespace for Stardew Valley's Netcode types. private const string NetcodeNamespace = "Netcode"; + /// The full name for Stardew Valley's Netcode.NetList type. + private readonly string NetListTypeFullName = "Netcode.NetList"; + /// Maps net fields to their equivalent non-net properties where available. private readonly IDictionary NetFieldWrapperProperties = new Dictionary { @@ -190,10 +195,9 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer return; if (!this.IsNetType(memberType.Type)) return; - bool isConverted = !this.IsNetType(memberType.ConvertedType); // warn: use property wrapper if available - for (ITypeSymbol type = declaringType; type != null; type = type.BaseType) + foreach (ITypeSymbol type in AnalyzerUtilities.GetConcreteTypes(declaringType)) { if (this.NetFieldWrapperProperties.TryGetValue($"{type}::{memberName}", out string suggestedPropertyName)) { @@ -203,7 +207,7 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer } // warn: implicit conversion - if (isConverted) + if (this.IsInvalidConversion(memberType)) context.ReportDiagnostic(Diagnostic.Create(this.Rules["AvoidImplicitNetFieldCast"], context.Node.GetLocation(), context.Node, memberType.Type.Name, memberType.ConvertedType)); } catch (Exception ex) @@ -212,6 +216,26 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer } } + /// Get whether a net field was converted in an error-prone way. + /// The member access type info. + private bool IsInvalidConversion(TypeInfo typeInfo) + { + // no conversion + if (!this.IsNetType(typeInfo.Type) || this.IsNetType(typeInfo.ConvertedType)) + return false; + + // list conversion to an implemented interface is OK + if (AnalyzerUtilities.GetConcreteTypes(typeInfo.Type).Any(p => p.ToString().StartsWith(this.NetListTypeFullName))) // StartsWith to ignore generics + { + string toType = typeInfo.ConvertedType.ToString(); + if (toType.StartsWith(typeof(IEnumerable<>).Namespace) || toType == typeof(IEnumerable).FullName) + return false; + } + + // avoid any other conversions + return true; + } + /// Get whether a type symbol references a Netcode type. /// The type symbol. private bool IsNetType(ITypeSymbol typeSymbol) diff --git a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs index dc21e505..943d0350 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs @@ -79,7 +79,7 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer return; // suggest replacement - for (ITypeSymbol type = declaringType; type != null; type = type.BaseType) + foreach (ITypeSymbol type in AnalyzerUtilities.GetConcreteTypes(declaringType)) { if (this.ReplacedFields.TryGetValue($"{type}::{memberName}", out string replacement)) { -- cgit From 1848abe7d57e32207db9535c9c83d96dbda64ced Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sat, 14 Apr 2018 20:14:31 -0400 Subject: don't warn for NetCollection conversion to implemented interface (#471) --- .../Mock/Netcode/NetCollection.cs | 10 ++++++++++ .../Mock/StardewValley/Farmer.cs | 7 ------- .../Mock/StardewValley/Item.cs | 9 +++++++++ .../NetFieldAnalyzerTests.cs | 12 +++++++----- src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs | 11 +++++++++++ 5 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetCollection.cs (limited to 'src/SMAPI.ModBuildConfig.Analyzer') diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetCollection.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetCollection.cs new file mode 100644 index 00000000..d160610e --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetCollection.cs @@ -0,0 +1,10 @@ +// ReSharper disable CheckNamespace -- matches Stardew Valley's code +using System.Collections; +using System.Collections.Generic; +using System.Collections.ObjectModel; + +namespace Netcode +{ + /// A simplified version of Stardew Valley's Netcode.NetCollection for unit testing. + public class NetCollection : Collection, IList, ICollection, IEnumerable, IEnumerable { } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs index 54e91682..13fab069 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs @@ -1,7 +1,6 @@ // ReSharper disable CheckNamespace, InconsistentNaming -- matches Stardew Valley's code #pragma warning disable 649 // (never assigned) -- only used to test type conversions using System.Collections.Generic; -using Netcode; namespace StardewValley { @@ -10,11 +9,5 @@ namespace StardewValley { /// A sample field which should be replaced with a different property. public readonly IDictionary friendships; - - /// A sample net list. - public readonly NetList eventsSeen; - - /// A sample net object list. - public readonly NetObjectList netObjectList; } } diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs index 386767d7..1b6317c1 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs @@ -20,5 +20,14 @@ namespace StardewValley /// A generic net ref property with no equivalent non-net property. public NetRef netRefProperty { get; } = new NetRef(); + + /// A sample net list. + public readonly NetList netList = new NetList(); + + /// A sample net object list. + public readonly NetObjectList netObjectList = new NetObjectList(); + + /// A sample net collection. + public readonly NetCollection netCollection = new NetCollection(); } } diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs index 15bcadcd..7b410085 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs @@ -86,7 +86,8 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests [TestCase("SObject obj = null; if (obj.netRefField != null);", 24, "obj.netRefField", "NetRef", "object")] [TestCase("SObject obj = null; if (obj.netRefProperty == null);", 24, "obj.netRefProperty", "NetRef", "object")] [TestCase("SObject obj = null; if (obj.netRefProperty != null);", 24, "obj.netRefProperty", "NetRef", "object")] - [TestCase("SFarmer farmer = new SFarmer(); object list = farmer.eventsSeen;", 46, "farmer.eventsSeen", "NetList", "object")] // ↓ NetList field converted to a non-interface type + [TestCase("Item item = new Item(); object list = item.netList;", 38, "item.netList", "NetList", "object")] // ↓ NetList field converted to a non-interface type + [TestCase("Item item = new Item(); object list = item.netCollection;", 38, "item.netCollection", "NetCollection", "object")] public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType) { // arrange @@ -105,10 +106,11 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests /// Test that the net field analyzer doesn't raise any warnings for safe member access. /// The code line to test. - [TestCase("SFarmer farmer = new SFarmer(); System.Collections.IEnumerable list = farmer.eventsSeen;")] - [TestCase("SFarmer farmer = new SFarmer(); System.Collections.Generic.IEnumerable list = farmer.eventsSeen;")] - [TestCase("SFarmer farmer = new SFarmer(); System.Collections.Generic.IList list = farmer.eventsSeen;")] - [TestCase("SFarmer farmer = new SFarmer(); System.Collections.Generic.IList list = farmer.netObjectList;")] // subclass of NetList + [TestCase("Item item = new Item(); System.Collections.IEnumerable list = farmer.eventsSeen;")] + [TestCase("Item item = new Item(); System.Collections.Generic.IEnumerable list = farmer.netList;")] + [TestCase("Item item = new Item(); System.Collections.Generic.IList list = farmer.netList;")] + [TestCase("Item item = new Item(); System.Collections.Generic.ICollection list = farmer.netCollection;")] + [TestCase("Item item = new Item(); System.Collections.Generic.IList list = farmer.netObjectList;")] // subclass of NetList public void AvoidImplicitNetFieldComparisons_AllowsSafeAccess(string codeText) { // arrange diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index 7c8b804e..72d3bbf8 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -22,6 +22,9 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// The full name for Stardew Valley's Netcode.NetList type. private readonly string NetListTypeFullName = "Netcode.NetList"; + /// The full name for Stardew Valley's Netcode.NetCollection type. + private readonly string NetCollectionTypeFullName = "Netcode.NetCollection"; + /// Maps net fields to their equivalent non-net properties where available. private readonly IDictionary NetFieldWrapperProperties = new Dictionary { @@ -232,6 +235,14 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer return false; } + // collection conversion to an implemented interface is OK + if (AnalyzerUtilities.GetConcreteTypes(typeInfo.Type).Any(p => p.ToString().StartsWith(this.NetCollectionTypeFullName))) // StartsWith to ignore generics + { + string toType = typeInfo.ConvertedType.ToString(); + if (toType.StartsWith(typeof(IEnumerable<>).Namespace) || toType == typeof(IEnumerable).FullName) + return false; + } + // avoid any other conversions return true; } -- cgit From 97120c6df2f59a12eef0129f13d773bdde305184 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sat, 14 Apr 2018 20:33:43 -0400 Subject: update references to old warning IDs (#471) --- docs/mod-build-config.md | 12 ++++++------ docs/screenshots/code-analyzer-example.png | Bin 4022 -> 3473 bytes src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs | 1 - .../ObsoleteFieldAnalyzer.cs | 1 - src/SMAPI/Framework/SGame.cs | 2 +- 5 files changed, 7 insertions(+), 9 deletions(-) (limited to 'src/SMAPI.ModBuildConfig.Analyzer') diff --git a/docs/mod-build-config.md b/docs/mod-build-config.md index d942beeb..74ee34e4 100644 --- a/docs/mod-build-config.md +++ b/docs/mod-build-config.md @@ -140,11 +140,11 @@ To enable it, add this above the first `` in your `.csproj`: The NuGet package adds code warnings in Visual Studio specific to Stardew Valley. For example: ![](screenshots/code-analyzer-example.png) -You can hide the warnings... +You can hide the warnings using the warning ID (shown under 'code' in the Error List). See... * [for specific code](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives/preprocessor-pragma-warning); * for a method using this attribute: ```cs - [System.Diagnostics.CodeAnalysis.SuppressMessage("SMAPI.CommonErrors", "SMAPI001")] // implicit net field conversion + [System.Diagnostics.CodeAnalysis.SuppressMessage("SMAPI.CommonErrors", "AvoidNetField")] ``` * for an entire project: 1. Expand the _References_ node for the project in Visual Studio. @@ -163,11 +163,11 @@ Stardew Valley uses net types (like `NetBool` and `NetInt`) to handle multiplaye can implicitly convert to their equivalent normal values (like `bool x = new NetBool()`), but their conversion rules are unintuitive and error-prone. For example, `item?.category == null && item?.category != null` can both be true at once, and -`building.indoors != null` will be true for a null value in some cases. +`building.indoors != null` can be true for a null value. Suggested fix: * Some net fields have an equivalent non-net property like `monster.Health` (`int`) instead of - `monster.health` (`NetInt`). The package will add a separate [SMAPI002](#smapi002) warning for + `monster.health` (`NetInt`). The package will add a separate [AvoidNetField](#avoid-net-field) warning for these. Use the suggested property instead. * For a reference type (i.e. one that can contain `null`), you can use the `.Value` property: ```c# @@ -189,8 +189,8 @@ Suggested fix: 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 -field has an equivalent non-net property that avoids those issues. +Your code accesses a net field, which has some unusual behavior (see [AvoidImplicitNetFieldCast](#avoid-implicit-net-field-cast)). +This field has an equivalent non-net property that avoids those issues. Suggested fix: access the suggested property name instead. diff --git a/docs/screenshots/code-analyzer-example.png b/docs/screenshots/code-analyzer-example.png index 3b930dc5..de38f643 100644 Binary files a/docs/screenshots/code-analyzer-example.png and b/docs/screenshots/code-analyzer-example.png differ diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index 72d3bbf8..e3c92617 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -175,7 +175,6 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// The analysis context. public override void Initialize(AnalysisContext context) { - // SMAPI002: avoid net fields if possible context.RegisterSyntaxNodeAction( this.AnalyzeMemberAccess, SyntaxKind.SimpleMemberAccessExpression, diff --git a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs index 943d0350..a770f47d 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs @@ -56,7 +56,6 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// The analysis context. public override void Initialize(AnalysisContext context) { - // SMAPI003: avoid obsolete fields context.RegisterSyntaxNodeAction( this.AnalyzeObsoleteFields, SyntaxKind.SimpleMemberAccessExpression, diff --git a/src/SMAPI/Framework/SGame.cs b/src/SMAPI/Framework/SGame.cs index 67390882..be98aeb1 100644 --- a/src/SMAPI/Framework/SGame.cs +++ b/src/SMAPI/Framework/SGame.cs @@ -633,7 +633,7 @@ namespace StardewModdingAPI.Framework [SuppressMessage("ReSharper", "RedundantCast", Justification = "copied from game code as-is")] [SuppressMessage("ReSharper", "RedundantExplicitNullableCreation", Justification = "copied from game code as-is")] [SuppressMessage("ReSharper", "RedundantTypeArgumentsOfMethod", Justification = "copied from game code as-is")] - [SuppressMessage("SMAPI.CommonErrors", "SMAPI002", Justification = "copied from game code as-is")] + [SuppressMessage("SMAPI.CommonErrors", "AvoidNetField", Justification = "copied from game code as-is")] private void DrawImpl(GameTime gameTime) { if (Game1.debugMode) -- cgit From 83f89c6ef31b783bd6afa4782df14cbbace0f022 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Thu, 26 Apr 2018 23:18:53 -0400 Subject: don't warn when converting net fields to an interface they implement --- .../NetFieldAnalyzer.cs | 25 +++------------------- src/SMAPI.ModBuildConfig/package.nuspec | 2 +- 2 files changed, 4 insertions(+), 23 deletions(-) (limited to 'src/SMAPI.ModBuildConfig.Analyzer') diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index e3c92617..0e9154a1 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -1,5 +1,4 @@ using System; -using System.Collections; using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; @@ -19,12 +18,6 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// The namespace for Stardew Valley's Netcode types. private const string NetcodeNamespace = "Netcode"; - /// The full name for Stardew Valley's Netcode.NetList type. - private readonly string NetListTypeFullName = "Netcode.NetList"; - - /// The full name for Stardew Valley's Netcode.NetCollection type. - private readonly string NetCollectionTypeFullName = "Netcode.NetCollection"; - /// Maps net fields to their equivalent non-net properties where available. private readonly IDictionary NetFieldWrapperProperties = new Dictionary { @@ -226,21 +219,9 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer if (!this.IsNetType(typeInfo.Type) || this.IsNetType(typeInfo.ConvertedType)) return false; - // list conversion to an implemented interface is OK - if (AnalyzerUtilities.GetConcreteTypes(typeInfo.Type).Any(p => p.ToString().StartsWith(this.NetListTypeFullName))) // StartsWith to ignore generics - { - string toType = typeInfo.ConvertedType.ToString(); - if (toType.StartsWith(typeof(IEnumerable<>).Namespace) || toType == typeof(IEnumerable).FullName) - return false; - } - - // collection conversion to an implemented interface is OK - if (AnalyzerUtilities.GetConcreteTypes(typeInfo.Type).Any(p => p.ToString().StartsWith(this.NetCollectionTypeFullName))) // StartsWith to ignore generics - { - string toType = typeInfo.ConvertedType.ToString(); - if (toType.StartsWith(typeof(IEnumerable<>).Namespace) || toType == typeof(IEnumerable).FullName) - return false; - } + // conversion to implemented interface is OK + if (typeInfo.Type.AllInterfaces.Contains(typeInfo.ConvertedType)) + return false; // avoid any other conversions return true; diff --git a/src/SMAPI.ModBuildConfig/package.nuspec b/src/SMAPI.ModBuildConfig/package.nuspec index 5e0b479e..6bb7736c 100644 --- a/src/SMAPI.ModBuildConfig/package.nuspec +++ b/src/SMAPI.ModBuildConfig/package.nuspec @@ -2,7 +2,7 @@ Pathoschild.Stardew.ModBuildConfig - 2.1.0-beta + 2.1.0-beta-20180426 Build package for SMAPI mods Pathoschild Pathoschild -- cgit From e1eca00c66464005512127a80012b861b5c7c22e Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sat, 28 Apr 2018 15:10:54 -0400 Subject: fix net field analyzers not detecting implicit conversions via binary expressions (#471) --- .../NetFieldAnalyzer.cs | 59 +++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) (limited to 'src/SMAPI.ModBuildConfig.Analyzer') diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index 0e9154a1..39b9abc1 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -4,6 +4,7 @@ using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; namespace StardewModdingAPI.ModBuildConfig.Analyzer @@ -173,14 +174,24 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer SyntaxKind.SimpleMemberAccessExpression, SyntaxKind.ConditionalAccessExpression ); + context.RegisterSyntaxNodeAction( + this.AnalyzeBinaryComparison, + SyntaxKind.EqualsExpression, + SyntaxKind.NotEqualsExpression, + SyntaxKind.GreaterThanExpression, + SyntaxKind.GreaterThanOrEqualExpression, + SyntaxKind.LessThanExpression, + SyntaxKind.LessThanOrEqualExpression + ); } /********* ** Private methods *********/ - /// Analyse a syntax node and add a diagnostic message if it references a net field when there's a non-net equivalent available. + /// Analyse a member access syntax node and add a diagnostic message if it references a net field when there's a non-net equivalent available. /// The analysis context. + /// Returns whether any warnings were added. private void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context) { try @@ -203,7 +214,53 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer // warn: implicit conversion if (this.IsInvalidConversion(memberType)) + { context.ReportDiagnostic(Diagnostic.Create(this.Rules["AvoidImplicitNetFieldCast"], context.Node.GetLocation(), context.Node, memberType.Type.Name, memberType.ConvertedType)); + return; + } + } + catch (Exception ex) + { + throw new InvalidOperationException($"Failed processing expression: '{context.Node}'. Exception details: {ex.ToString().Replace('\r', ' ').Replace('\n', ' ')}"); + } + } + + /// Analyse a binary comparison syntax node and add a diagnostic message if it references a net field when there's a non-net equivalent available. + /// The analysis context. + /// Returns whether any warnings were added. + private void AnalyzeBinaryComparison(SyntaxNodeAnalysisContext context) + { + // 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. + try + { + 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 curType = context.SemanticModel.GetTypeInfo(curExpression); + TypeInfo otherType = context.SemanticModel.GetTypeInfo(otherExpression); + if (!this.IsNetType(curType.ConvertedType)) + continue; + + // 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 otherValue = context.SemanticModel.GetConstantValue(otherExpression); + if (otherValue.HasValue && otherValue.Value == null) + { + context.ReportDiagnostic(Diagnostic.Create(this.Rules["AvoidImplicitNetFieldCast"], context.Node.GetLocation(), curExpression, curType.Type.Name, "null")); + break; + } + + // warn for implicit conversion + if (!this.IsNetType(otherType.ConvertedType)) + { + context.ReportDiagnostic(Diagnostic.Create(this.Rules["AvoidImplicitNetFieldCast"], context.Node.GetLocation(), curExpression, curType.Type.Name, curType.ConvertedType)); + break; + } + } } catch (Exception ex) { -- cgit From 6be4d5abe03932a7f6a638816c2206388dc18983 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sat, 28 Apr 2018 16:07:41 -0400 Subject: detect conversions due to explicit casts or 'x as y' expressions (#471) --- .../NetFieldAnalyzerTests.cs | 6 +- .../AnalyzerUtilities.cs | 37 ++++++- .../NetFieldAnalyzer.cs | 113 +++++++++++++-------- .../ObsoleteFieldAnalyzer.cs | 2 +- 4 files changed, 112 insertions(+), 46 deletions(-) (limited to 'src/SMAPI.ModBuildConfig.Analyzer') diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs index ab3f2c5e..6f8c8b9b 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs @@ -59,7 +59,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests /// The expression which should be reported. /// The source type name which should be reported. /// The target type name which should be reported. - [TestCase("Item item = null; if (item.netIntField < 42);", 22, "item.netIntField", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntField < 42);", 22, "item.netIntField", "NetInt", "int")] // ↓ implicit conversion [TestCase("Item item = null; if (item.netIntField <= 42);", 22, "item.netIntField", "NetInt", "int")] [TestCase("Item item = null; if (item.netIntField > 42);", 22, "item.netIntField", "NetInt", "int")] [TestCase("Item item = null; if (item.netIntField >= 42);", 22, "item.netIntField", "NetInt", "int")] @@ -79,7 +79,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests [TestCase("Item item = null; if (item.netRefField != null);", 22, "item.netRefField", "NetRef", "object")] [TestCase("Item item = null; if (item.netRefProperty == null);", 22, "item.netRefProperty", "NetRef", "object")] [TestCase("Item item = null; if (item.netRefProperty != null);", 22, "item.netRefProperty", "NetRef", "object")] - [TestCase("SObject obj = null; if (obj.netIntField != 42);", 24, "obj.netIntField", "NetInt", "int")] // ↓ same as above, but inherited from base class + [TestCase("SObject obj = null; if (obj.netIntField != 42);", 24, "obj.netIntField", "NetInt", "int")] // ↓ implicit conversion for parent field [TestCase("SObject obj = null; if (obj.netIntProperty != 42);", 24, "obj.netIntProperty", "NetInt", "int")] [TestCase("SObject obj = null; if (obj.netRefField == null);", 24, "obj.netRefField", "NetRef", "object")] [TestCase("SObject obj = null; if (obj.netRefField != null);", 24, "obj.netRefField", "NetRef", "object")] @@ -87,6 +87,8 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests [TestCase("SObject obj = null; if (obj.netRefProperty != null);", 24, "obj.netRefProperty", "NetRef", "object")] [TestCase("Item item = new Item(); object list = item.netList;", 38, "item.netList", "NetList", "object")] // ↓ NetList field converted to a non-interface type [TestCase("Item item = new Item(); object list = item.netCollection;", 38, "item.netCollection", "NetCollection", "object")] + [TestCase("Item item = new Item(); int x = (int)item.netIntField;", 32, "item.netIntField", "NetInt", "int")] // ↓ explicit conversion to invalid type + [TestCase("Item item = new Item(); int x = item.netRefField as object;", 32, "item.netRefField", "NetRef", "object")] public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType) { // arrange diff --git a/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs b/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs index e0c0cd63..68b5001e 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/AnalyzerUtilities.cs @@ -1,5 +1,6 @@ using System.Collections.Generic; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; namespace StardewModdingAPI.ModBuildConfig.Analyzer @@ -10,6 +11,40 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /********* ** Public methods *********/ + /// Get the metadata for an explicit cast or 'x as y' expression. + /// The member access expression. + /// provides methods for asking semantic questions about syntax nodes. + /// The expression whose value is being converted. + /// The type being converted from. + /// The type being converted to. + /// Returns true if the node is a matched expression, else false. + 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; + } + /// Get the metadata for a member access expression. /// The member access expression. /// provides methods for asking semantic questions about syntax nodes. @@ -17,7 +52,7 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// The type of the accessed member. /// The name of the accessed member. /// Returns true if the node is a member access expression, else false. - public static bool GetMemberInfo(SyntaxNode node, SemanticModel semanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName) + public static bool TryGetMemberInfo(SyntaxNode node, SemanticModel semanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName) { // simple access if (node is MemberAccessExpressionSyntax memberAccess) diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index 39b9abc1..d1e5c59a 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -125,28 +125,27 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer ["StardewValley.Tool::upgradeLevel"] = "UpgradeLevel" }; - /// Describes the diagnostic rule covered by the analyzer. - private readonly IDictionary Rules = new Dictionary - { - ["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/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" - ), - ["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/avoid-net-field for details.", - category: "SMAPI.CommonErrors", - defaultSeverity: DiagnosticSeverity.Warning, - isEnabledByDefault: true, - helpLinkUri: "https://smapi.io/buildmsg/avoid-net-field" - ) - }; + /// The diagnostic info for an implicit net field cast. + 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" + ); + + /// The diagnostic info for an avoidable net field access. + 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" + ); /********* @@ -162,7 +161,7 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// Construct an instance. public NetFieldAnalyzer() { - this.SupportedDiagnostics = ImmutableArray.CreateRange(this.Rules.Values); + this.SupportedDiagnostics = ImmutableArray.CreateRange(new[] { this.AvoidNetFieldRule, this.AvoidImplicitNetFieldCastRule }); } /// Called once at session start to register actions in the analysis context. @@ -174,6 +173,11 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer SyntaxKind.SimpleMemberAccessExpression, SyntaxKind.ConditionalAccessExpression ); + context.RegisterSyntaxNodeAction( + this.AnalyzeCast, + SyntaxKind.CastExpression, + SyntaxKind.AsExpression + ); context.RegisterSyntaxNodeAction( this.AnalyzeBinaryComparison, SyntaxKind.EqualsExpression, @@ -189,15 +193,15 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /********* ** Private methods *********/ - /// Analyse a member access syntax node and add a diagnostic message if it references a net field when there's a non-net equivalent available. + /// Analyse a member access syntax node and add a diagnostic message if applicable. /// The analysis context. /// Returns whether any warnings were added. private void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context) { - try + this.HandleErrors(context.Node, () => { // get member access info - if (!AnalyzerUtilities.GetMemberInfo(context.Node, context.SemanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName)) + if (!AnalyzerUtilities.TryGetMemberInfo(context.Node, context.SemanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName)) return; if (!this.IsNetType(memberType.Type)) return; @@ -207,32 +211,45 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer { if (this.NetFieldWrapperProperties.TryGetValue($"{type}::{memberName}", out string suggestedPropertyName)) { - context.ReportDiagnostic(Diagnostic.Create(this.Rules["AvoidNetField"], context.Node.GetLocation(), context.Node, memberType.Type.Name, suggestedPropertyName)); + context.ReportDiagnostic(Diagnostic.Create(this.AvoidNetFieldRule, context.Node.GetLocation(), context.Node, memberType.Type.Name, suggestedPropertyName)); return; } } // warn: implicit conversion - if (this.IsInvalidConversion(memberType)) + if (this.IsInvalidConversion(memberType.Type, memberType.ConvertedType)) { - context.ReportDiagnostic(Diagnostic.Create(this.Rules["AvoidImplicitNetFieldCast"], context.Node.GetLocation(), context.Node, memberType.Type.Name, memberType.ConvertedType)); + context.ReportDiagnostic(Diagnostic.Create(this.AvoidImplicitNetFieldCastRule, context.Node.GetLocation(), context.Node, memberType.Type.Name, memberType.ConvertedType)); return; } - } - catch (Exception ex) + }); + } + + /// Analyse an explicit cast or 'x as y' node and add a diagnostic message if applicable. + /// The analysis context. + /// Returns whether any warnings were added. + 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)); + } + }); } - /// Analyse a binary comparison syntax node and add a diagnostic message if it references a net field when there's a non-net equivalent available. + /// Analyse a binary comparison syntax node and add a diagnostic message if applicable. /// The analysis context. /// Returns whether any warnings were added. private void AnalyzeBinaryComparison(SyntaxNodeAnalysisContext context) { // 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. - try + this.HandleErrors(context.Node, () => { BinaryExpressionSyntax expression = (BinaryExpressionSyntax)context.Node; foreach (var pair in new[] { Tuple.Create(expression.Left, expression.Right), Tuple.Create(expression.Right, expression.Left) }) @@ -250,34 +267,46 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer Optional otherValue = context.SemanticModel.GetConstantValue(otherExpression); if (otherValue.HasValue && otherValue.Value == null) { - context.ReportDiagnostic(Diagnostic.Create(this.Rules["AvoidImplicitNetFieldCast"], context.Node.GetLocation(), curExpression, curType.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.Rules["AvoidImplicitNetFieldCast"], context.Node.GetLocation(), curExpression, curType.Type.Name, curType.ConvertedType)); + context.ReportDiagnostic(Diagnostic.Create(this.AvoidImplicitNetFieldCastRule, context.Node.GetLocation(), curExpression, curType.Type.Name, curType.ConvertedType)); break; } } + }); + } + + /// Handle exceptions raised while analyzing a node. + /// The node being analysed. + /// The callback to invoke. + 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', ' ')}"); } } /// Get whether a net field was converted in an error-prone way. - /// The member access type info. - private bool IsInvalidConversion(TypeInfo typeInfo) + /// The source type. + /// The target type. + private bool IsInvalidConversion(ITypeSymbol fromType, ITypeSymbol toType) { // no conversion - if (!this.IsNetType(typeInfo.Type) || this.IsNetType(typeInfo.ConvertedType)) + if (!this.IsNetType(fromType) || this.IsNetType(toType)) return false; // conversion to implemented interface is OK - if (typeInfo.Type.AllInterfaces.Contains(typeInfo.ConvertedType)) + if (fromType.AllInterfaces.Contains(toType)) return false; // avoid any other conversions diff --git a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs index a770f47d..3d353e52 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs @@ -74,7 +74,7 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer try { // get reference info - if (!AnalyzerUtilities.GetMemberInfo(context.Node, context.SemanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName)) + if (!AnalyzerUtilities.TryGetMemberInfo(context.Node, context.SemanticModel, out ITypeSymbol declaringType, out TypeInfo memberType, out string memberName)) return; // suggest replacement -- cgit From 46fe7a86a7020d9170ab5a05047a3f7a6cc4b3c0 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sun, 29 Apr 2018 11:35:01 -0400 Subject: add a few more avoidable net fields (#471) --- src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs | 7 +++++++ src/SMAPI.ModBuildConfig/package.nuspec | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) (limited to 'src/SMAPI.ModBuildConfig.Analyzer') diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index d1e5c59a..fad9251b 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -22,6 +22,13 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// Maps net fields to their equivalent non-net properties where available. private readonly IDictionary NetFieldWrapperProperties = new Dictionary { + // 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", diff --git a/src/SMAPI.ModBuildConfig/package.nuspec b/src/SMAPI.ModBuildConfig/package.nuspec index 6bb7736c..fa26875b 100644 --- a/src/SMAPI.ModBuildConfig/package.nuspec +++ b/src/SMAPI.ModBuildConfig/package.nuspec @@ -2,7 +2,7 @@ Pathoschild.Stardew.ModBuildConfig - 2.1.0-beta-20180426 + 2.1.0-beta-20180428 Build package for SMAPI mods Pathoschild Pathoschild -- cgit From 05f81cb85f09c8e82ea125f520e4a264abfc3869 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Fri, 4 May 2018 18:46:46 -0400 Subject: update net field list --- src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs | 1 - 1 file changed, 1 deletion(-) (limited to 'src/SMAPI.ModBuildConfig.Analyzer') diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index fad9251b..e6766e61 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -114,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", -- cgit From 1c10e54d050b0b3c869992848b912962da0b8304 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sun, 24 Jun 2018 18:55:04 -0400 Subject: update analyzer packages --- .../SMAPI.ModBuildConfig.Analyzer.Tests.csproj | 2 +- .../StardewModdingAPI.ModBuildConfig.Analyzer.csproj | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'src/SMAPI.ModBuildConfig.Analyzer') diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/SMAPI.ModBuildConfig.Analyzer.Tests.csproj b/src/SMAPI.ModBuildConfig.Analyzer.Tests/SMAPI.ModBuildConfig.Analyzer.Tests.csproj index 956e6cf3..c6241ecb 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/SMAPI.ModBuildConfig.Analyzer.Tests.csproj +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/SMAPI.ModBuildConfig.Analyzer.Tests.csproj @@ -5,7 +5,7 @@ - + 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 @@ - + -- cgit