diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs | 43 | ||||
| -rw-r--r-- | src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs | 208 |
2 files changed, 219 insertions, 32 deletions
diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs index d5e4ef52..b4f54234 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs @@ -16,6 +16,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests /// <summary>Sample C# code which contains a simplified representation of Stardew Valley's <c>Netcode</c> types, and sample mod code with a {{test-code}} placeholder for the code being tested.</summary> const string SampleProgram = @" using System; + using StardewValley; using Netcode; namespace Netcode @@ -29,13 +30,16 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests } } - namespace SampleMod + namespace StardewValley { class Item { public NetInt category { get; } = new NetInt { Value = 42 }; } + } + namespace SampleMod + { class ModEntry { public void Entry() @@ -45,17 +49,17 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests Item item = null; // this line should raise diagnostics - {{test-code}} // line 32 + {{test-code}} // line 36 // these lines should not - if ((int)field != 42); + if ((int)intField != 42); } } } "; /// <summary>The line number where the unit tested code is injected into <see cref="SampleProgram"/>.</summary> - private const int SampleCodeLine = 32; + private const int SampleCodeLine = 36; /// <summary>The column number where the unit tested code is injected into <see cref="SampleProgram"/>.</summary> private const int SampleCodeColumn = 25; @@ -75,7 +79,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests this.VerifyCSharpDiagnostic(test); } - /// <summary>Test that the expected diagnostic message is raised.</summary> + /// <summary>Test that the expected diagnostic message is raised for implicit net field comparisons.</summary> /// <param name="codeText">The code line to test.</param> /// <param name="column">The column within the code line where the diagnostic message should be reported.</param> /// <param name="expression">The expression which should be reported.</param> @@ -89,7 +93,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests [TestCase("if (intField != 42);", 4, "intField", "NetInt", "Int32")] [TestCase("if (refField != null);", 4, "refField", "NetRef", "Object")] [TestCase("if (item?.category != 42);", 4, "item?.category", "NetInt", "Int32")] - public void ImplicitComparisons_RaiseDiagnostics(string codeText, int column, string expression, string fromType, string toType) + public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType) { // arrange string code = UnitTests.SampleProgram.Replace("{{test-code}}", codeText); @@ -105,6 +109,31 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests this.VerifyCSharpDiagnostic(code, expected); } + /// <summary>Test that the expected diagnostic message is raised for avoidable net field references.</summary> + /// <param name="codeText">The code line to test.</param> + /// <param name="column">The column within the code line where the diagnostic message should be reported.</param> + /// <param name="expression">The expression which should be reported.</param> + /// <param name="netType">The net type name which should be reported.</param> + /// <param name="suggestedProperty">The suggested property name which should be reported.</param> + [TestCase("int category = item.category;", 15, "item.category", "NetInt", "Category")] + [TestCase("int category = (item).category;", 15, "(item).category", "NetInt", "Category")] + [TestCase("int category = ((Item)item).category;", 15, "((Item)item).category", "NetInt", "Category")] + public void AvoidNetFields_RaisesDiagnostic(string codeText, int column, string expression, string netType, string suggestedProperty) + { + // arrange + string code = UnitTests.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.", + Severity = DiagnosticSeverity.Warning, + Locations = new[] { new DiagnosticResultLocation("Test0.cs", UnitTests.SampleCodeLine, UnitTests.SampleCodeColumn + column) } + }; + + // assert + this.VerifyCSharpDiagnostic(code, expected); + } + /********* ** Helpers @@ -112,7 +141,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests /// <summary>Get the analyzer being tested.</summary> protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() { - return new ImplicitNetFieldCastAnalyzer(); + return new NetFieldAnalyzer(); } } } diff --git a/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs index d23bdc2e..d64b1486 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Collections.Immutable; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; @@ -9,7 +10,7 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer { /// <summary>Detects implicit conversion from Stardew Valley's <c>Netcode</c> types. These have very unintuitive implicit conversion rules, so mod authors should always explicitly convert the type with appropriate null checks.</summary> [DiagnosticAnalyzer(LanguageNames.CSharp)] - public class ImplicitNetFieldCastAnalyzer : DiagnosticAnalyzer + public class NetFieldAnalyzer : DiagnosticAnalyzer { /********* ** Properties @@ -17,17 +18,136 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// <summary>The namespace for Stardew Valley's <c>Netcode</c> types.</summary> private const string NetcodeNamespace = "Netcode"; + /// <summary>Maps net fields to their equivalent non-net properties where available.</summary> + private readonly IDictionary<string, string> NetFieldWrapperProperties = new Dictionary<string, string> + { + // Character + ["StardewValley.Character::currentLocationRef"] = "currentLocation", + ["StardewValley.Character::facingDirection"] = "FacingDirection", + ["StardewValley.Character::name"] = "Name", + ["StardewValley.Character::position"] = "Position", + ["StardewValley.Character::scale"] = "Scale", + ["StardewValley.Character::speed"] = "Speed", + ["StardewValley.Character::sprite"] = "Sprite", + + // Chest + ["StardewValley.Objects.Chest::tint"] = "Tint", + + // Farmer + ["StardewValley.Farmer::houseUpgradeLevel"] = "HouseUpgradeLevel", + ["StardewValley.Farmer::isMale"] = "IsMale", + ["StardewValley.Farmer::items"] = "Items", + ["StardewValley.Farmer::magneticRadius"] = "MagneticRadius", + ["StardewValley.Farmer::stamina"] = "Stamina", + ["StardewValley.Farmer::uniqueMultiplayerID"] = "UniqueMultiplayerID", + ["StardewValley.Farmer::usingTool"] = "UsingTool", + + // Forest + ["StardewValley.Locations.Forest::netTravelingMerchantDay"] = "travelingMerchantDay", + ["StardewValley.Locations.Forest::netLog"] = "log", + + // FruitTree + ["StardewValley.TerrainFeatures.FruitTree::greenHouseTileTree"] = "GreenHouseTileTree", + ["StardewValley.TerrainFeatures.FruitTree::greenHouseTree"] = "GreenHouseTree", + + // GameLocation + ["StardewValley.GameLocation::isFarm"] = "IsFarm", + ["StardewValley.GameLocation::isOutdoors"] = "IsOutdoors", + ["StardewValley.GameLocation::lightLevel"] = "LightLevel", + ["StardewValley.GameLocation::name"] = "Name", + + // Item + ["StardewValley.Item::category"] = "Category", + ["StardewValley.Item::netName"] = "Name", + ["StardewValley.Item::parentSheetIndex"] = "ParentSheetIndex", + ["StardewValley.Item::specialVariable"] = "SpecialVariable", + + // Junimo + ["StardewValley.Characters.Junimo::eventActor"] = "EventActor", + + // LightSource + ["StardewValley.LightSource::identifier"] = "Identifier", + + // Monster + ["StardewValley.Monsters.Monster::damageToFarmer"] = "DamageToFarmer", + ["StardewValley.Monsters.Monster::experienceGained"] = "ExperienceGained", + ["StardewValley.Monsters.Monster::health"] = "Health", + ["StardewValley.Monsters.Monster::maxHealth"] = "MaxHealth", + ["StardewValley.Monsters.Monster::netFocusedOnFarmers"] = "focusedOnFarmers", + ["StardewValley.Monsters.Monster::netWildernessFarmMonster"] = "wildernessFarmMonster", + ["StardewValley.Monsters.Monster::slipperiness"] = "Slipperiness", + + // NPC + ["StardewValley.NPC::age"] = "Age", + ["StardewValley.NPC::birthday_Day"] = "Birthday_Day", + ["StardewValley.NPC::birthday_Season"] = "Birthday_Season", + ["StardewValley.NPC::breather"] = "Breather", + ["StardewValley.NPC::defaultMap"] = "DefaultMap", + ["StardewValley.NPC::gender"] = "Gender", + ["StardewValley.NPC::hideShadow"] = "HideShadow", + ["StardewValley.NPC::isInvisible"] = "IsInvisible", + ["StardewValley.NPC::isWalkingTowardPlayer"] = "IsWalkingTowardPlayer", + ["StardewValley.NPC::manners"] = "Manners", + ["StardewValley.NPC::optimism"] = "Optimism", + ["StardewValley.NPC::socialAnxiety"] = "SocialAnxiety", + + // Object + ["StardewValley.Object::canBeGrabbed"] = "CanBeGrabbed", + ["StardewValley.Object::canBeSetDown"] = "CanBeSetDown", + ["StardewValley.Object::edibility"] = "Edibility", + ["StardewValley.Object::flipped"] = "Flipped", + ["StardewValley.Object::fragility"] = "Fragility", + ["StardewValley.Object::hasBeenPickedUpByFarmer"] = "HasBeenPickedUpByFarmer", + ["StardewValley.Object::isHoedirt"] = "IsHoeDirt", + ["StardewValley.Object::isOn"] = "IsOn", + ["StardewValley.Object::isRecipe"] = "IsRecipe", + ["StardewValley.Object::isSpawnedObject"] = "IsSpawnedObject", + ["StardewValley.Object::minutesUntilReady"] = "MinutesUntilReady", + ["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", + + // Projectile + ["StardewValley.Projectiles.Projectile::ignoreLocationCollision"] = "IgnoreLocationCollision", + + // Tool + ["StardewValley.Tool::currentParentTileIndex"] = "CurrentParentTileIndex", + ["StardewValley.Tool::indexOfMenuItemView"] = "IndexOfMenuItemView", + ["StardewValley.Tool::initialParentTileIndex"] = "InitialParentTileIndex", + ["StardewValley.Tool::instantUse"] = "InstantUse", + ["StardewValley.Tool::netName"] = "BaseName", + ["StardewValley.Tool::stackable"] = "Stackable", + ["StardewValley.Tool::upgradeLevel"] = "UpgradeLevel" + }; + /// <summary>Describes the diagnostic rule covered by the analyzer.</summary> - private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor( - id: "SMAPI001", - title: "Netcode types shouldn't be implicitly converted", - messageFormat: "This implicitly converts '{0}' from {1} to {2}, but {1} has unintuitive implicit conversion rules. Consider comparing against the actual value instead to avoid bugs. See https://smapi.io/buildmsg/SMAPI001 for details.", - category: "SMAPI.CommonErrors", - defaultSeverity: DiagnosticSeverity.Warning, - isEnabledByDefault: true, - description: "", - helpLinkUri: "https://smapi.io/buildmsg/SMAPI001" - ); + private readonly IDictionary<string, DiagnosticDescriptor> Rules = new Dictionary<string, DiagnosticDescriptor> + { + ["SMAPI001"] = new DiagnosticDescriptor( + id: "SMAPI001", + title: "Netcode types shouldn't be implicitly converted", + messageFormat: "This implicitly converts '{0}' from {1} to {2}, but {1} has unintuitive implicit conversion rules. Consider comparing against the actual value instead to avoid bugs. See https://smapi.io/buildmsg/SMAPI001 for details.", + category: "SMAPI.CommonErrors", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "", + helpLinkUri: "https://smapi.io/buildmsg/SMAPI001" + ), + ["SMAPI002"] = new DiagnosticDescriptor( + id: "SMAPI002", + title: "Avoid Netcode types when possible", + messageFormat: "'{0}' is a {1} field; consider using the {2} property instead. See https://smapi.io/buildmsg/SMAPI002 for details.", + category: "SMAPI.CommonErrors", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "", + helpLinkUri: "https://smapi.io/buildmsg/SMAPI001" + ) + }; /********* @@ -41,17 +161,24 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer ** Public methods *********/ /// <summary>Construct an instance.</summary> - public ImplicitNetFieldCastAnalyzer() + public NetFieldAnalyzer() { - this.SupportedDiagnostics = ImmutableArray.Create(ImplicitNetFieldCastAnalyzer.Rule); + this.SupportedDiagnostics = ImmutableArray.CreateRange(this.Rules.Values); } /// <summary>Called once at session start to register actions in the analysis context.</summary> /// <param name="context">The analysis context.</param> public override void Initialize(AnalysisContext context) { + // SMAPI002: avoid net fields if possible + context.RegisterSyntaxNodeAction( + this.AnalyzeAvoidableNetField, + SyntaxKind.SimpleMemberAccessExpression + ); + + // SMAPI001: avoid implicit net field conversion context.RegisterSyntaxNodeAction( - this.Analyse, + this.AnalyseNetFieldConversions, SyntaxKind.EqualsExpression, SyntaxKind.NotEqualsExpression, SyntaxKind.GreaterThanExpression, @@ -65,16 +192,44 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /********* ** Private methods *********/ - /// <summary>Analyse a syntax node and add a diagnostic message if applicable.</summary> + /// <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) + { + try + { + // check member type + MemberAccessExpressionSyntax node = (MemberAccessExpressionSyntax)context.Node; + TypeInfo memberType = context.SemanticModel.GetTypeInfo(node); + if (!this.IsNetType(memberType.Type)) + return; + + // get reference info + ITypeSymbol declaringType = context.SemanticModel.GetTypeInfo(node.Expression).Type; + string propertyName = node.Name.Identifier.Text; + + // suggest replacement + if (this.NetFieldWrapperProperties.TryGetValue($"{declaringType}::{propertyName}", out string suggestedPropertyName)) + { + context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI002"], context.Node.GetLocation(), node, memberType.Type.Name, suggestedPropertyName)); + } + } + 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 Analyse(SyntaxNodeAnalysisContext context) + private void AnalyseNetFieldConversions(SyntaxNodeAnalysisContext context) { try { BinaryExpressionSyntax node = (BinaryExpressionSyntax)context.Node; - bool leftHasWarning = this.Analyze(context, node.Left); + bool leftHasWarning = this.WarnIfOperandImplicitlyConvertsNetField(context, node.Left); if (!leftHasWarning) - this.Analyze(context, node.Right); + this.WarnIfOperandImplicitlyConvertsNetField(context, node.Right); } catch (Exception ex) { @@ -86,22 +241,25 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// <param name="context">The analysis context.</param> /// <param name="operand">The operand expression.</param> /// <returns>Returns whether a diagnostic message was raised.</returns> - private bool Analyze(SyntaxNodeAnalysisContext context, ExpressionSyntax operand) + private bool WarnIfOperandImplicitlyConvertsNetField(SyntaxNodeAnalysisContext context, ExpressionSyntax operand) { - const string netcodeNamespace = ImplicitNetFieldCastAnalyzer.NetcodeNamespace; - TypeInfo operandType = context.SemanticModel.GetTypeInfo(operand); - string fromNamespace = operandType.Type?.ContainingNamespace?.Name; - string toNamespace = operandType.ConvertedType?.ContainingNamespace?.Name; - if (fromNamespace == netcodeNamespace && fromNamespace != toNamespace && toNamespace != null) + if (this.IsNetType(operandType.Type) && !this.IsNetType(operandType.ConvertedType)) { string fromTypeName = operandType.Type.Name; string toTypeName = operandType.ConvertedType.Name; - context.ReportDiagnostic(Diagnostic.Create(ImplicitNetFieldCastAnalyzer.Rule, context.Node.GetLocation(), operand, fromTypeName, toTypeName)); + context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI001"], context.Node.GetLocation(), operand, fromTypeName, toTypeName)); return true; } return false; } + + /// <summary>Get whether a type symbol references a <c>Netcode</c> type.</summary> + /// <param name="typeSymbol">The type symbol.</param> + private bool IsNetType(ITypeSymbol typeSymbol) + { + return typeSymbol?.ContainingNamespace?.Name == NetFieldAnalyzer.NetcodeNamespace; + } } } |