From 6d8cf614a24ab69baffa89c351b9a22776741442 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Sat, 14 Apr 2018 19:51:50 -0400 Subject: [PATCH] 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 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)) {