diff --git a/docs/mod-build-config.md b/docs/mod-build-config.md index 71a2518a..44242160 100644 --- a/docs/mod-build-config.md +++ b/docs/mod-build-config.md @@ -182,6 +182,15 @@ Suggested fix: if (item != null && item.category == 0) ``` +### SMAPI002 +**Avoid net fields when possible:** +> '{{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. + +Suggested fix: access the suggested property name instead. + ## Troubleshoot ### "Failed to find the game install path" That error means the package couldn't find your game. You can specify the game path yourself; see diff --git a/docs/screenshots/code-analyzer-example.png b/docs/screenshots/code-analyzer-example.png index 2723b164..3b930dc5 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.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 /// Sample C# code which contains a simplified representation of Stardew Valley's Netcode types, and sample mod code with a {{test-code}} placeholder for the code being tested. 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); } } } "; /// The line number where the unit tested code is injected into . - private const int SampleCodeLine = 32; + private const int SampleCodeLine = 36; /// The column number where the unit tested code is injected into . private const int SampleCodeColumn = 25; @@ -75,7 +79,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests this.VerifyCSharpDiagnostic(test); } - /// Test that the expected diagnostic message is raised. + /// Test that the expected diagnostic message is raised for implicit net field comparisons. /// The code line to test. /// The column within the code line where the diagnostic message should be reported. /// The expression which should be reported. @@ -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); } + /// 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. + /// The expression which should be reported. + /// The net type name which should be reported. + /// The suggested property name which should be reported. + [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 /// Get the analyzer being tested. 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 { /// Detects implicit conversion from Stardew Valley's Netcode types. These have very unintuitive implicit conversion rules, so mod authors should always explicitly convert the type with appropriate null checks. [DiagnosticAnalyzer(LanguageNames.CSharp)] - public class ImplicitNetFieldCastAnalyzer : DiagnosticAnalyzer + public class NetFieldAnalyzer : DiagnosticAnalyzer { /********* ** Properties @@ -17,17 +18,136 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /// The namespace for Stardew Valley's Netcode types. private const string NetcodeNamespace = "Netcode"; + /// Maps net fields to their equivalent non-net properties where available. + private readonly IDictionary NetFieldWrapperProperties = new Dictionary + { + // 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" + }; + /// Describes the diagnostic rule covered by the analyzer. - 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 Rules = new Dictionary + { + ["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 *********/ /// Construct an instance. - public ImplicitNetFieldCastAnalyzer() + public NetFieldAnalyzer() { - this.SupportedDiagnostics = ImmutableArray.Create(ImplicitNetFieldCastAnalyzer.Rule); + this.SupportedDiagnostics = ImmutableArray.CreateRange(this.Rules.Values); } /// Called once at session start to register actions in the analysis context. /// The analysis context. public override void Initialize(AnalysisContext context) { + // SMAPI002: avoid net fields if possible context.RegisterSyntaxNodeAction( - this.Analyse, + this.AnalyzeAvoidableNetField, + SyntaxKind.SimpleMemberAccessExpression + ); + + // SMAPI001: avoid implicit net field conversion + context.RegisterSyntaxNodeAction( + this.AnalyseNetFieldConversions, SyntaxKind.EqualsExpression, SyntaxKind.NotEqualsExpression, SyntaxKind.GreaterThanExpression, @@ -65,16 +192,44 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer /********* ** Private methods *********/ - /// Analyse a syntax node and add a diagnostic message if applicable. + /// 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 Analyse(SyntaxNodeAnalysisContext context) + 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', ' ')}"); + } + } + + /// 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 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 /// The analysis context. /// The operand expression. /// Returns whether a diagnostic message was raised. - 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; } + + /// Get whether a type symbol references a Netcode type. + /// The type symbol. + private bool IsNetType(ITypeSymbol typeSymbol) + { + return typeSymbol?.ContainingNamespace?.Name == NetFieldAnalyzer.NetcodeNamespace; + } } }