From 6be4d5abe03932a7f6a638816c2206388dc18983 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sat, 28 Apr 2018 16:07:41 -0400 Subject: [PATCH] detect conversions due to explicit casts or 'x as y' expressions (#471) --- .../NetFieldAnalyzerTests.cs | 6 +- .../AnalyzerUtilities.cs | 37 +++++- .../NetFieldAnalyzer.cs | 115 +++++++++++------- .../ObsoleteFieldAnalyzer.cs | 2 +- 4 files changed, 113 insertions(+), 47 deletions(-) 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) - { - 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. + /// 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, () => + { + 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 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