From 22965604bfa5858a089d842173cdebe6aaed0ed8 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Mon, 9 Apr 2018 19:30:17 -0400 Subject: add support for build message URLs (#471) --- src/SMAPI.ModBuildConfig/build/smapi.targets | 2 +- .../Framework/RewriteRules/RedirectToUrlRule.cs | 20 ++++++++------------ src/SMAPI.Web/Startup.cs | 1 + 3 files changed, 10 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/src/SMAPI.ModBuildConfig/build/smapi.targets b/src/SMAPI.ModBuildConfig/build/smapi.targets index d2e37101..a177840c 100644 --- a/src/SMAPI.ModBuildConfig/build/smapi.targets +++ b/src/SMAPI.ModBuildConfig/build/smapi.targets @@ -142,7 +142,7 @@ - + diff --git a/src/SMAPI.Web/Framework/RewriteRules/RedirectToUrlRule.cs b/src/SMAPI.Web/Framework/RewriteRules/RedirectToUrlRule.cs index 0719e311..4bae0b4c 100644 --- a/src/SMAPI.Web/Framework/RewriteRules/RedirectToUrlRule.cs +++ b/src/SMAPI.Web/Framework/RewriteRules/RedirectToUrlRule.cs @@ -12,11 +12,8 @@ namespace StardewModdingAPI.Web.Framework.RewriteRules /********* ** Properties *********/ - /// A predicate which indicates when the rule should be applied. - private readonly Func ShouldRewrite; - - /// The new URL to which to redirect. - private readonly string NewUrl; + /// Get the new URL to which to redirect (or null to skip). + private readonly Func NewUrl; /********* @@ -27,8 +24,7 @@ namespace StardewModdingAPI.Web.Framework.RewriteRules /// The new URL to which to redirect. public RedirectToUrlRule(Func shouldRewrite, string url) { - this.ShouldRewrite = shouldRewrite ?? (req => true); - this.NewUrl = url; + this.NewUrl = req => shouldRewrite(req) ? url : null; } /// Construct an instance. @@ -37,8 +33,7 @@ namespace StardewModdingAPI.Web.Framework.RewriteRules public RedirectToUrlRule(string pathRegex, string url) { Regex regex = new Regex(pathRegex, RegexOptions.IgnoreCase | RegexOptions.Compiled); - this.ShouldRewrite = req => req.Path.HasValue && regex.IsMatch(req.Path.Value); - this.NewUrl = url; + this.NewUrl = req => req.Path.HasValue ? regex.Replace(req.Path.Value, url) : null; } /// Applies the rule. Implementations of ApplyRule should set the value for (defaults to RuleResult.ContinueRules). @@ -47,14 +42,15 @@ namespace StardewModdingAPI.Web.Framework.RewriteRules { HttpRequest request = context.HttpContext.Request; - // check condition - if (!this.ShouldRewrite(request)) + // check rewrite + string newUrl = this.NewUrl(request); + if (newUrl == null || newUrl == request.Path.Value) return; // redirect request HttpResponse response = context.HttpContext.Response; response.StatusCode = (int)HttpStatusCode.Redirect; - response.Headers["Location"] = this.NewUrl; + response.Headers["Location"] = newUrl; context.Result = RuleResult.EndResponse; } } diff --git a/src/SMAPI.Web/Startup.cs b/src/SMAPI.Web/Startup.cs index 47102e5c..6c7ccecd 100644 --- a/src/SMAPI.Web/Startup.cs +++ b/src/SMAPI.Web/Startup.cs @@ -155,6 +155,7 @@ namespace StardewModdingAPI.Web // shortcut redirects redirects.Add(new RedirectToUrlRule(@"^/compat\.?$", "https://stardewvalleywiki.com/Modding:SMAPI_compatibility")); redirects.Add(new RedirectToUrlRule(@"^/docs\.?$", "https://stardewvalleywiki.com/Modding:Index")); + redirects.Add(new RedirectToUrlRule(@"^/buildmsg(?:/?(.*))$", "https://github.com/Pathoschild/SMAPI/blob/develop/docs/mod-build-config.md#$1")); // redirect legacy canimod.com URLs var wikiRedirects = new Dictionary -- cgit From f52f7ca36f2ecf3d4478c5bc1e9cd95e5ff53929 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Mon, 9 Apr 2018 19:32:00 -0400 Subject: add mod code analyzers to detect implicit net field conversion issues (#471) --- build/prepare-nuget-package.targets | 20 ++ docs/mod-build-config.md | 63 +++++- docs/screenshots/code-analyzer-example.png | Bin 0 -> 5696 bytes docs/technical-docs.md | 29 +++ .../Framework/DiagnosticResult.cs | 88 ++++++++ .../Framework/DiagnosticVerifier.Helper.cs | 171 ++++++++++++++ .../Framework/DiagnosticVerifier.cs | 248 +++++++++++++++++++++ .../SMAPI.ModBuildConfig.Analyzer.Tests.csproj | 18 ++ .../UnitTests.cs | 118 ++++++++++ .../ImplicitNetFieldCastAnalyzer.cs | 107 +++++++++ .../Properties/AssemblyInfo.cs | 4 + ...tardewModdingAPI.ModBuildConfig.Analyzer.csproj | 23 ++ .../tools/install.ps1 | 58 +++++ .../tools/uninstall.ps1 | 65 ++++++ .../StardewModdingAPI.ModBuildConfig.csproj | 1 + src/SMAPI.ModBuildConfig/package.nuspec | 24 +- src/SMAPI.Tests/StardewModdingAPI.Tests.csproj | 3 + src/SMAPI.sln | 33 +++ 18 files changed, 1045 insertions(+), 28 deletions(-) create mode 100644 build/prepare-nuget-package.targets create mode 100644 docs/screenshots/code-analyzer-example.png create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticResult.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.Helper.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/SMAPI.ModBuildConfig.Analyzer.Tests.csproj create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer/Properties/AssemblyInfo.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer/StardewModdingAPI.ModBuildConfig.Analyzer.csproj create mode 100644 src/SMAPI.ModBuildConfig.Analyzer/tools/install.ps1 create mode 100644 src/SMAPI.ModBuildConfig.Analyzer/tools/uninstall.ps1 (limited to 'src') diff --git a/build/prepare-nuget-package.targets b/build/prepare-nuget-package.targets new file mode 100644 index 00000000..5dbc5508 --- /dev/null +++ b/build/prepare-nuget-package.targets @@ -0,0 +1,20 @@ + + + + + $(SolutionDir)\..\bin\Pathoschild.Stardew.ModBuildConfig + + + + + + + + + + diff --git a/docs/mod-build-config.md b/docs/mod-build-config.md index 0d72e4d9..71a2518a 100644 --- a/docs/mod-build-config.md +++ b/docs/mod-build-config.md @@ -3,15 +3,16 @@ for SMAPI mods. The package... -* lets your code compile on any computer (Linux/Mac/Windows) without needing to change the assembly - references or game path. -* packages the mod into the game's `Mods` folder when you rebuild the code (configurable). -* configures Visual Studio so you can debug into the mod code when the game is running (_Windows - only_). +* detects your game install path; +* adds the assembly references you need (with automatic support for Linux/Mac/Windows); +* packages the mod into your `Mods` folder when you rebuild the code (configurable); +* configures Visual Studio to enable debugging into the code when the game is running (_Windows only_); +* adds C# analyzers to warn for Stardew Valley-specific issues. ## Contents * [Install](#install) * [Configure](#configure) +* [Code analysis warnings](#code-analysis-warnings) * [Troubleshoot](#troubleshoot) * [Release notes](#release-notes) @@ -121,7 +122,7 @@ The configuration will check your custom path first, then fall back to the defau still compile on a different computer). ### Unit test projects -**(upcoming in 2.0.3)** +**(upcoming in 2.1)** You can use the package in unit test projects too. Its optional unit test mode... @@ -134,15 +135,63 @@ To enable it, add this above the first `` in your `.csproj`: True ``` +## Code warnings +### Overview +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... +* [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 + ``` +* for an entire project: + 1. Expand the _References_ node for the project in Visual Studio. + 2. Right-click on _Analyzers_ and choose _Open Active Rule Set_. + 4. Expand _StardewModdingAPI.ModBuildConfig.Analyzer_ and uncheck the warnings you want to hide. + +See below for help with each specific warning. + +### SMAPI001 +**Implicit net field conversion:** +> This implicitly converts '{{expression}}' from {{net type}} to {{other type}}, but +> {{net type}} has unintuitive implicit conversion rules. Consider comparing against the actual +> value instead to avoid bugs. + +Stardew Valley uses a set of net fields (like `NetBool` or `NetInt`) to handle multiplayer sync. +These types can implicitly convert to their equivalent normal values (like `bool x = new NetBool()`), +but their conversion rules can be 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. + +Suggested fix: +* For a reference type (i.e. one that can contain `null`), you can use the `.Value` property: + ```c# + if (building.indoors.Value == null) + ``` + Or convert the value before comparison: + ```c# + GameLocation indoors = building.indoors; + if(indoors == null) + // ... + ``` +* For a value type (i.e. one that can't contain `null`), make sure to check for null first if + applicable: + ```cs + if (item != null && item.category == 0) + ``` + ## 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 _[Game path](#game-path)_ above. ## Release notes -### 2.0.3 alpha +### 2.1 alpha * Added support for Stardew Valley 1.3. * Added support for unit test projects. +* Added C# analyzers to warn about implicit conversions of Netcode fields in Stardew Valley 1.3. ### 2.0.2 * Fixed compatibility issue on Linux. diff --git a/docs/screenshots/code-analyzer-example.png b/docs/screenshots/code-analyzer-example.png new file mode 100644 index 00000000..2723b164 Binary files /dev/null and b/docs/screenshots/code-analyzer-example.png differ diff --git a/docs/technical-docs.md b/docs/technical-docs.md index 37ec7f69..9e1a49e7 100644 --- a/docs/technical-docs.md +++ b/docs/technical-docs.md @@ -20,6 +20,7 @@ mods, this section isn't relevant to you; see the main README to use or create m * [Development](#development-2) * [Local development](#local-development) * [Deploying to Amazon Beanstalk](#deploying-to-amazon-beanstalk) +* [Mod build config package](#mod-build-config-package) # SMAPI ## Development @@ -222,3 +223,31 @@ property name | description `LogParser:SectionUrl` | The root URL of the log page, like `https://log.smapi.io/`. `ModUpdateCheck:GitHubPassword` | The password with which to authenticate to GitHub when fetching release info. `ModUpdateCheck:GitHubUsername` | The username with which to authenticate to GitHub when fetching release info. + +## Mod build config package +### Overview +The mod build config package is a NuGet package that mods reference to automatically set up +references, configure the build, and add analyzers specific to Stardew Valley mods. + +This involves three projects: + +project | purpose +------------------------------------------------- | ---------------- +`StardewModdingAPI.ModBuildConfig` | Configures the build (references, deploying the mod files, setting up debugging, etc). +`StardewModdingAPI.ModBuildConfig.Analyzer` | Adds C# analyzers which show code warnings in Visual Studio. +`StardewModdingAPI.ModBuildConfig.Analyzer.Tests` | Unit tests for the C# analyzers. + +When the projects are built, the relevant files are copied into `bin/Pathoschild.Stardew.ModBuildConfig`. + +### Preparing a build +To prepare a build of the NuGet package: +1. Install the [NuGet CLI](https://docs.microsoft.com/en-us/nuget/install-nuget-client-tools#nugetexe-cli). +1. Change the version and release notes in `package.nuspec`. +2. Rebuild the solution in _Release_ mode. +3. Open a terminal in the `bin/Pathoschild.Stardew.ModBuildConfig` package and run this command: + ```bash + nuget.exe pack + ``` + +That will create a `Pathoschild.Stardew.ModBuildConfig-.nupkg` file in the same directory +which can be uploaded to NuGet or referenced directly. diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticResult.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticResult.cs new file mode 100644 index 00000000..896c2cb8 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticResult.cs @@ -0,0 +1,88 @@ +// +using Microsoft.CodeAnalysis; +using System; + +namespace SMAPI.ModBuildConfig.Analyzer.Tests.Framework +{ + /// + /// Location where the diagnostic appears, as determined by path, line number, and column number. + /// + public struct DiagnosticResultLocation + { + public DiagnosticResultLocation(string path, int line, int column) + { + if (line < -1) + { + throw new ArgumentOutOfRangeException(nameof(line), "line must be >= -1"); + } + + if (column < -1) + { + throw new ArgumentOutOfRangeException(nameof(column), "column must be >= -1"); + } + + this.Path = path; + this.Line = line; + this.Column = column; + } + + public string Path { get; } + public int Line { get; } + public int Column { get; } + } + + /// + /// Struct that stores information about a Diagnostic appearing in a source + /// + public struct DiagnosticResult + { + private DiagnosticResultLocation[] locations; + + public DiagnosticResultLocation[] Locations + { + get + { + if (this.locations == null) + { + this.locations = new DiagnosticResultLocation[] { }; + } + return this.locations; + } + + set + { + this.locations = value; + } + } + + public DiagnosticSeverity Severity { get; set; } + + public string Id { get; set; } + + public string Message { get; set; } + + public string Path + { + get + { + return this.Locations.Length > 0 ? this.Locations[0].Path : ""; + } + } + + public int Line + { + get + { + return this.Locations.Length > 0 ? this.Locations[0].Line : -1; + } + } + + public int Column + { + get + { + return this.Locations.Length > 0 ? this.Locations[0].Column : -1; + } + } + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.Helper.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.Helper.cs new file mode 100644 index 00000000..d33455fc --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.Helper.cs @@ -0,0 +1,171 @@ +// +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Text; +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; + +namespace SMAPI.ModBuildConfig.Analyzer.Tests.Framework +{ + /// + /// Class for turning strings into documents and getting the diagnostics on them + /// All methods are static + /// + public abstract partial class DiagnosticVerifier + { + private static readonly MetadataReference CorlibReference = MetadataReference.CreateFromFile(typeof(object).Assembly.Location); + private static readonly MetadataReference SystemCoreReference = MetadataReference.CreateFromFile(typeof(Enumerable).Assembly.Location); + private static readonly MetadataReference CSharpSymbolsReference = MetadataReference.CreateFromFile(typeof(CSharpCompilation).Assembly.Location); + private static readonly MetadataReference CodeAnalysisReference = MetadataReference.CreateFromFile(typeof(Compilation).Assembly.Location); + + internal static string DefaultFilePathPrefix = "Test"; + internal static string CSharpDefaultFileExt = "cs"; + internal static string VisualBasicDefaultExt = "vb"; + internal static string TestProjectName = "TestProject"; + + #region Get Diagnostics + + /// + /// Given classes in the form of strings, their language, and an IDiagnosticAnalyzer to apply to it, return the diagnostics found in the string after converting it to a document. + /// + /// Classes in the form of strings + /// The language the source classes are in + /// The analyzer to be run on the sources + /// An IEnumerable of Diagnostics that surfaced in the source code, sorted by Location + private static Diagnostic[] GetSortedDiagnostics(string[] sources, string language, DiagnosticAnalyzer analyzer) + { + return GetSortedDiagnosticsFromDocuments(analyzer, GetDocuments(sources, language)); + } + + /// + /// Given an analyzer and a document to apply it to, run the analyzer and gather an array of diagnostics found in it. + /// The returned diagnostics are then ordered by location in the source document. + /// + /// The analyzer to run on the documents + /// The Documents that the analyzer will be run on + /// An IEnumerable of Diagnostics that surfaced in the source code, sorted by Location + protected static Diagnostic[] GetSortedDiagnosticsFromDocuments(DiagnosticAnalyzer analyzer, Document[] documents) + { + var projects = new HashSet(); + foreach (var document in documents) + { + projects.Add(document.Project); + } + + var diagnostics = new List(); + foreach (var project in projects) + { + var compilationWithAnalyzers = project.GetCompilationAsync().Result.WithAnalyzers(ImmutableArray.Create(analyzer)); + var diags = compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync().Result; + foreach (var diag in diags) + { + if (diag.Location == Location.None || diag.Location.IsInMetadata) + { + diagnostics.Add(diag); + } + else + { + for (int i = 0; i < documents.Length; i++) + { + var document = documents[i]; + var tree = document.GetSyntaxTreeAsync().Result; + if (tree == diag.Location.SourceTree) + { + diagnostics.Add(diag); + } + } + } + } + } + + var results = SortDiagnostics(diagnostics); + diagnostics.Clear(); + return results; + } + + /// + /// Sort diagnostics by location in source document + /// + /// The list of Diagnostics to be sorted + /// An IEnumerable containing the Diagnostics in order of Location + private static Diagnostic[] SortDiagnostics(IEnumerable diagnostics) + { + return diagnostics.OrderBy(d => d.Location.SourceSpan.Start).ToArray(); + } + + #endregion + + #region Set up compilation and documents + /// + /// Given an array of strings as sources and a language, turn them into a project and return the documents and spans of it. + /// + /// Classes in the form of strings + /// The language the source code is in + /// A Tuple containing the Documents produced from the sources and their TextSpans if relevant + private static Document[] GetDocuments(string[] sources, string language) + { + if (language != LanguageNames.CSharp && language != LanguageNames.VisualBasic) + { + throw new ArgumentException("Unsupported Language"); + } + + var project = CreateProject(sources, language); + var documents = project.Documents.ToArray(); + + if (sources.Length != documents.Length) + { + throw new InvalidOperationException("Amount of sources did not match amount of Documents created"); + } + + return documents; + } + + /// + /// Create a Document from a string through creating a project that contains it. + /// + /// Classes in the form of a string + /// The language the source code is in + /// A Document created from the source string + protected static Document CreateDocument(string source, string language = LanguageNames.CSharp) + { + return CreateProject(new[] { source }, language).Documents.First(); + } + + /// + /// Create a project using the inputted strings as sources. + /// + /// Classes in the form of strings + /// The language the source code is in + /// A Project created out of the Documents created from the source strings + private static Project CreateProject(string[] sources, string language = LanguageNames.CSharp) + { + string fileNamePrefix = DefaultFilePathPrefix; + string fileExt = language == LanguageNames.CSharp ? CSharpDefaultFileExt : VisualBasicDefaultExt; + + var projectId = ProjectId.CreateNewId(debugName: TestProjectName); + + var solution = new AdhocWorkspace() + .CurrentSolution + .AddProject(projectId, TestProjectName, TestProjectName, language) + .AddMetadataReference(projectId, CorlibReference) + .AddMetadataReference(projectId, SystemCoreReference) + .AddMetadataReference(projectId, CSharpSymbolsReference) + .AddMetadataReference(projectId, CodeAnalysisReference); + + int count = 0; + foreach (var source in sources) + { + var newFileName = fileNamePrefix + count + "." + fileExt; + var documentId = DocumentId.CreateNewId(projectId, debugName: newFileName); + solution = solution.AddDocument(documentId, newFileName, SourceText.From(source)); + count++; + } + return solution.GetProject(projectId); + } + #endregion + } +} + diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.cs new file mode 100644 index 00000000..edaaabd4 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.cs @@ -0,0 +1,248 @@ +// +using System.Collections.Generic; +using System.Linq; +using System.Text; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Framework; + +namespace SMAPI.ModBuildConfig.Analyzer.Tests.Framework +{ + /// + /// Superclass of all Unit Tests for DiagnosticAnalyzers + /// + public abstract partial class DiagnosticVerifier + { + #region To be implemented by Test classes + /// + /// Get the CSharp analyzer being tested - to be implemented in non-abstract class + /// + protected virtual DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() + { + return null; + } + + /// + /// Get the Visual Basic analyzer being tested (C#) - to be implemented in non-abstract class + /// + protected virtual DiagnosticAnalyzer GetBasicDiagnosticAnalyzer() + { + return null; + } + #endregion + + #region Verifier wrappers + + /// + /// Called to test a C# DiagnosticAnalyzer when applied on the single inputted string as a source + /// Note: input a DiagnosticResult for each Diagnostic expected + /// + /// A class in the form of a string to run the analyzer on + /// DiagnosticResults that should appear after the analyzer is run on the source + protected void VerifyCSharpDiagnostic(string source, params DiagnosticResult[] expected) + { + VerifyDiagnostics(new[] { source }, LanguageNames.CSharp, GetCSharpDiagnosticAnalyzer(), expected); + } + + /// + /// Called to test a C# DiagnosticAnalyzer when applied on the inputted strings as a source + /// Note: input a DiagnosticResult for each Diagnostic expected + /// + /// An array of strings to create source documents from to run the analyzers on + /// DiagnosticResults that should appear after the analyzer is run on the sources + protected void VerifyCSharpDiagnostic(string[] sources, params DiagnosticResult[] expected) + { + VerifyDiagnostics(sources, LanguageNames.CSharp, GetCSharpDiagnosticAnalyzer(), expected); + } + + /// + /// General method that gets a collection of actual diagnostics found in the source after the analyzer is run, + /// then verifies each of them. + /// + /// An array of strings to create source documents from to run the analyzers on + /// The language of the classes represented by the source strings + /// The analyzer to be run on the source code + /// DiagnosticResults that should appear after the analyzer is run on the sources + private void VerifyDiagnostics(string[] sources, string language, DiagnosticAnalyzer analyzer, params DiagnosticResult[] expected) + { + var diagnostics = GetSortedDiagnostics(sources, language, analyzer); + VerifyDiagnosticResults(diagnostics, analyzer, expected); + } + + #endregion + + #region Actual comparisons and verifications + /// + /// Checks each of the actual Diagnostics found and compares them with the corresponding DiagnosticResult in the array of expected results. + /// Diagnostics are considered equal only if the DiagnosticResultLocation, Id, Severity, and Message of the DiagnosticResult match the actual diagnostic. + /// + /// The Diagnostics found by the compiler after running the analyzer on the source code + /// The analyzer that was being run on the sources + /// Diagnostic Results that should have appeared in the code + private static void VerifyDiagnosticResults(IEnumerable actualResults, DiagnosticAnalyzer analyzer, params DiagnosticResult[] expectedResults) + { + int expectedCount = expectedResults.Count(); + int actualCount = actualResults.Count(); + + if (expectedCount != actualCount) + { + string diagnosticsOutput = actualResults.Any() ? FormatDiagnostics(analyzer, actualResults.ToArray()) : " NONE."; + + Assert.IsTrue(false, + string.Format("Mismatch between number of diagnostics returned, expected \"{0}\" actual \"{1}\"\r\n\r\nDiagnostics:\r\n{2}\r\n", expectedCount, actualCount, diagnosticsOutput)); + } + + for (int i = 0; i < expectedResults.Length; i++) + { + var actual = actualResults.ElementAt(i); + var expected = expectedResults[i]; + + if (expected.Line == -1 && expected.Column == -1) + { + if (actual.Location != Location.None) + { + Assert.IsTrue(false, + string.Format("Expected:\nA project diagnostic with No location\nActual:\n{0}", + FormatDiagnostics(analyzer, actual))); + } + } + else + { + VerifyDiagnosticLocation(analyzer, actual, actual.Location, expected.Locations.First()); + var additionalLocations = actual.AdditionalLocations.ToArray(); + + if (additionalLocations.Length != expected.Locations.Length - 1) + { + Assert.IsTrue(false, + string.Format("Expected {0} additional locations but got {1} for Diagnostic:\r\n {2}\r\n", + expected.Locations.Length - 1, additionalLocations.Length, + FormatDiagnostics(analyzer, actual))); + } + + for (int j = 0; j < additionalLocations.Length; ++j) + { + VerifyDiagnosticLocation(analyzer, actual, additionalLocations[j], expected.Locations[j + 1]); + } + } + + if (actual.Id != expected.Id) + { + Assert.IsTrue(false, + string.Format("Expected diagnostic id to be \"{0}\" was \"{1}\"\r\n\r\nDiagnostic:\r\n {2}\r\n", + expected.Id, actual.Id, FormatDiagnostics(analyzer, actual))); + } + + if (actual.Severity != expected.Severity) + { + Assert.IsTrue(false, + string.Format("Expected diagnostic severity to be \"{0}\" was \"{1}\"\r\n\r\nDiagnostic:\r\n {2}\r\n", + expected.Severity, actual.Severity, FormatDiagnostics(analyzer, actual))); + } + + if (actual.GetMessage() != expected.Message) + { + Assert.IsTrue(false, + string.Format("Expected diagnostic message to be \"{0}\" was \"{1}\"\r\n\r\nDiagnostic:\r\n {2}\r\n", + expected.Message, actual.GetMessage(), FormatDiagnostics(analyzer, actual))); + } + } + } + + /// + /// Helper method to VerifyDiagnosticResult that checks the location of a diagnostic and compares it with the location in the expected DiagnosticResult. + /// + /// The analyzer that was being run on the sources + /// The diagnostic that was found in the code + /// The Location of the Diagnostic found in the code + /// The DiagnosticResultLocation that should have been found + private static void VerifyDiagnosticLocation(DiagnosticAnalyzer analyzer, Diagnostic diagnostic, Location actual, DiagnosticResultLocation expected) + { + var actualSpan = actual.GetLineSpan(); + + Assert.IsTrue(actualSpan.Path == expected.Path || (actualSpan.Path != null && actualSpan.Path.Contains("Test0.") && expected.Path.Contains("Test.")), + string.Format("Expected diagnostic to be in file \"{0}\" was actually in file \"{1}\"\r\n\r\nDiagnostic:\r\n {2}\r\n", + expected.Path, actualSpan.Path, FormatDiagnostics(analyzer, diagnostic))); + + var actualLinePosition = actualSpan.StartLinePosition; + + // Only check line position if there is an actual line in the real diagnostic + if (actualLinePosition.Line > 0) + { + if (actualLinePosition.Line + 1 != expected.Line) + { + Assert.IsTrue(false, + string.Format("Expected diagnostic to be on line \"{0}\" was actually on line \"{1}\"\r\n\r\nDiagnostic:\r\n {2}\r\n", + expected.Line, actualLinePosition.Line + 1, FormatDiagnostics(analyzer, diagnostic))); + } + } + + // Only check column position if there is an actual column position in the real diagnostic + if (actualLinePosition.Character > 0) + { + if (actualLinePosition.Character + 1 != expected.Column) + { + Assert.IsTrue(false, + string.Format("Expected diagnostic to start at column \"{0}\" was actually at column \"{1}\"\r\n\r\nDiagnostic:\r\n {2}\r\n", + expected.Column, actualLinePosition.Character + 1, FormatDiagnostics(analyzer, diagnostic))); + } + } + } + #endregion + + #region Formatting Diagnostics + /// + /// Helper method to format a Diagnostic into an easily readable string + /// + /// The analyzer that this verifier tests + /// The Diagnostics to be formatted + /// The Diagnostics formatted as a string + private static string FormatDiagnostics(DiagnosticAnalyzer analyzer, params Diagnostic[] diagnostics) + { + var builder = new StringBuilder(); + for (int i = 0; i < diagnostics.Length; ++i) + { + builder.AppendLine("// " + diagnostics[i].ToString()); + + var analyzerType = analyzer.GetType(); + var rules = analyzer.SupportedDiagnostics; + + foreach (var rule in rules) + { + if (rule != null && rule.Id == diagnostics[i].Id) + { + var location = diagnostics[i].Location; + if (location == Location.None) + { + builder.AppendFormat("GetGlobalResult({0}.{1})", analyzerType.Name, rule.Id); + } + else + { + Assert.IsTrue(location.IsInSource, + $"Test base does not currently handle diagnostics in metadata locations. Diagnostic in metadata: {diagnostics[i]}\r\n"); + + string resultMethodName = diagnostics[i].Location.SourceTree.FilePath.EndsWith(".cs") ? "GetCSharpResultAt" : "GetBasicResultAt"; + var linePosition = diagnostics[i].Location.GetLineSpan().StartLinePosition; + + builder.AppendFormat("{0}({1}, {2}, {3}.{4})", + resultMethodName, + linePosition.Line + 1, + linePosition.Character + 1, + analyzerType.Name, + rule.Id); + } + + if (i != diagnostics.Length - 1) + { + builder.Append(','); + } + + builder.AppendLine(); + break; + } + } + } + return builder.ToString(); + } + #endregion + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/SMAPI.ModBuildConfig.Analyzer.Tests.csproj b/src/SMAPI.ModBuildConfig.Analyzer.Tests/SMAPI.ModBuildConfig.Analyzer.Tests.csproj new file mode 100644 index 00000000..8d3815c3 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/SMAPI.ModBuildConfig.Analyzer.Tests.csproj @@ -0,0 +1,18 @@ + + + + netcoreapp2.0 + + + + + + + + + + + + + + diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs new file mode 100644 index 00000000..d5e4ef52 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs @@ -0,0 +1,118 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Framework; +using SMAPI.ModBuildConfig.Analyzer.Tests.Framework; +using StardewModdingAPI.ModBuildConfig.Analyzer; + +namespace SMAPI.ModBuildConfig.Analyzer.Tests +{ + /// Unit tests for the C# analyzers. + [TestFixture] + public class UnitTests : DiagnosticVerifier + { + /********* + ** Properties + *********/ + /// 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 Netcode; + + namespace Netcode + { + public class NetInt : NetFieldBase { } + public class NetRef : NetFieldBase { } + public class NetFieldBase where TSelf : NetFieldBase + { + public T Value { get; set; } + public static implicit operator T(NetFieldBase field) => field.Value; + } + } + + namespace SampleMod + { + class Item + { + public NetInt category { get; } = new NetInt { Value = 42 }; + } + + class ModEntry + { + public void Entry() + { + NetInt intField = new NetInt { Value = 42 }; + NetRef refField = new NetRef { Value = null }; + Item item = null; + + // this line should raise diagnostics + {{test-code}} // line 32 + + // these lines should not + if ((int)field != 42); + } + } + } + "; + + /// The line number where the unit tested code is injected into . + private const int SampleCodeLine = 32; + + /// The column number where the unit tested code is injected into . + private const int SampleCodeColumn = 25; + + + /********* + ** Unit tests + *********/ + /// Test that no diagnostics are raised for an empty code block. + [TestCase] + public void EmptyCode_HasNoDiagnostics() + { + // arrange + string test = @""; + + // assert + this.VerifyCSharpDiagnostic(test); + } + + /// Test that the expected diagnostic message is raised. + /// 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 source type name which should be reported. + /// The target type name which should be reported. + [TestCase("if (intField < 42);", 4, "intField", "NetInt", "Int32")] + [TestCase("if (intField <= 42);", 4, "intField", "NetInt", "Int32")] + [TestCase("if (intField > 42);", 4, "intField", "NetInt", "Int32")] + [TestCase("if (intField >= 42);", 4, "intField", "NetInt", "Int32")] + [TestCase("if (intField == 42);", 4, "intField", "NetInt", "Int32")] + [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) + { + // arrange + string code = UnitTests.SampleProgram.Replace("{{test-code}}", codeText); + DiagnosticResult expected = new DiagnosticResult + { + Id = "SMAPI001", + Message = $"This implicitly converts '{expression}' from {fromType} to {toType}, but {fromType} has unintuitive implicit conversion rules. Consider comparing against the actual value instead to avoid bugs. See https://smapi.io/buildmsg/SMAPI001 for details.", + Severity = DiagnosticSeverity.Warning, + Locations = new[] { new DiagnosticResultLocation("Test0.cs", UnitTests.SampleCodeLine, UnitTests.SampleCodeColumn + column) } + }; + + // assert + this.VerifyCSharpDiagnostic(code, expected); + } + + + /********* + ** Helpers + *********/ + /// Get the analyzer being tested. + protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() + { + return new ImplicitNetFieldCastAnalyzer(); + } + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs new file mode 100644 index 00000000..d23bdc2e --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs @@ -0,0 +1,107 @@ +using System; +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +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 + { + /********* + ** Properties + *********/ + /// The namespace for Stardew Valley's Netcode types. + private const string NetcodeNamespace = "Netcode"; + + /// 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" + ); + + + /********* + ** Accessors + *********/ + /// The descriptors for the diagnostics that this analyzer is capable of producing. + public override ImmutableArray SupportedDiagnostics { get; } + + + /********* + ** Public methods + *********/ + /// Construct an instance. + public ImplicitNetFieldCastAnalyzer() + { + this.SupportedDiagnostics = ImmutableArray.Create(ImplicitNetFieldCastAnalyzer.Rule); + } + + /// Called once at session start to register actions in the analysis context. + /// The analysis context. + public override void Initialize(AnalysisContext context) + { + context.RegisterSyntaxNodeAction( + this.Analyse, + SyntaxKind.EqualsExpression, + SyntaxKind.NotEqualsExpression, + SyntaxKind.GreaterThanExpression, + SyntaxKind.GreaterThanOrEqualExpression, + SyntaxKind.LessThanExpression, + SyntaxKind.LessThanOrEqualExpression + ); + } + + + /********* + ** Private methods + *********/ + /// Analyse a syntax node and add a diagnostic message if applicable. + /// The analysis context. + private void Analyse(SyntaxNodeAnalysisContext context) + { + try + { + BinaryExpressionSyntax node = (BinaryExpressionSyntax)context.Node; + bool leftHasWarning = this.Analyze(context, node.Left); + if (!leftHasWarning) + this.Analyze(context, node.Right); + } + catch (Exception ex) + { + throw new InvalidOperationException($"Failed processing expression: '{context.Node}'. Exception details: {ex.ToString().Replace('\r', ' ').Replace('\n', ' ')}"); + } + } + + /// Analyse one operand in a binary expression (like a and b in a == b) and add a diagnostic message if applicable. + /// The analysis context. + /// The operand expression. + /// Returns whether a diagnostic message was raised. + private bool Analyze(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) + { + string fromTypeName = operandType.Type.Name; + string toTypeName = operandType.ConvertedType.Name; + context.ReportDiagnostic(Diagnostic.Create(ImplicitNetFieldCastAnalyzer.Rule, context.Node.GetLocation(), operand, fromTypeName, toTypeName)); + return true; + } + + return false; + } + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer/Properties/AssemblyInfo.cs b/src/SMAPI.ModBuildConfig.Analyzer/Properties/AssemblyInfo.cs new file mode 100644 index 00000000..1cc41000 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer/Properties/AssemblyInfo.cs @@ -0,0 +1,4 @@ +using System.Reflection; + +[assembly: AssemblyTitle("SMAPI.ModBuildConfig.Analyzer")] +[assembly: AssemblyDescription("")] diff --git a/src/SMAPI.ModBuildConfig.Analyzer/StardewModdingAPI.ModBuildConfig.Analyzer.csproj b/src/SMAPI.ModBuildConfig.Analyzer/StardewModdingAPI.ModBuildConfig.Analyzer.csproj new file mode 100644 index 00000000..c32343e3 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer/StardewModdingAPI.ModBuildConfig.Analyzer.csproj @@ -0,0 +1,23 @@ + + + + netstandard1.3 + false + false + bin + + + + + + + + + + + + + + + + diff --git a/src/SMAPI.ModBuildConfig.Analyzer/tools/install.ps1 b/src/SMAPI.ModBuildConfig.Analyzer/tools/install.ps1 new file mode 100644 index 00000000..ff051759 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer/tools/install.ps1 @@ -0,0 +1,58 @@ +param($installPath, $toolsPath, $package, $project) + +if($project.Object.SupportsPackageDependencyResolution) +{ + if($project.Object.SupportsPackageDependencyResolution()) + { + # Do not install analyzers via install.ps1, instead let the project system handle it. + return + } +} + +$analyzersPaths = Join-Path (Join-Path (Split-Path -Path $toolsPath -Parent) "analyzers") * -Resolve + +foreach($analyzersPath in $analyzersPaths) +{ + if (Test-Path $analyzersPath) + { + # Install the language agnostic analyzers. + foreach ($analyzerFilePath in Get-ChildItem -Path "$analyzersPath\*.dll" -Exclude *.resources.dll) + { + if($project.Object.AnalyzerReferences) + { + $project.Object.AnalyzerReferences.Add($analyzerFilePath.FullName) + } + } + } +} + +# $project.Type gives the language name like (C# or VB.NET) +$languageFolder = "" +if($project.Type -eq "C#") +{ + $languageFolder = "cs" +} +if($project.Type -eq "VB.NET") +{ + $languageFolder = "vb" +} +if($languageFolder -eq "") +{ + return +} + +foreach($analyzersPath in $analyzersPaths) +{ + # Install language specific analyzers. + $languageAnalyzersPath = join-path $analyzersPath $languageFolder + if (Test-Path $languageAnalyzersPath) + { + foreach ($analyzerFilePath in Get-ChildItem -Path "$languageAnalyzersPath\*.dll" -Exclude *.resources.dll) + { + if($project.Object.AnalyzerReferences) + { + $project.Object.AnalyzerReferences.Add($analyzerFilePath.FullName) + } + } + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer/tools/uninstall.ps1 b/src/SMAPI.ModBuildConfig.Analyzer/tools/uninstall.ps1 new file mode 100644 index 00000000..4bed3337 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer/tools/uninstall.ps1 @@ -0,0 +1,65 @@ +param($installPath, $toolsPath, $package, $project) + +if($project.Object.SupportsPackageDependencyResolution) +{ + if($project.Object.SupportsPackageDependencyResolution()) + { + # Do not uninstall analyzers via uninstall.ps1, instead let the project system handle it. + return + } +} + +$analyzersPaths = Join-Path (Join-Path (Split-Path -Path $toolsPath -Parent) "analyzers") * -Resolve + +foreach($analyzersPath in $analyzersPaths) +{ + # Uninstall the language agnostic analyzers. + if (Test-Path $analyzersPath) + { + foreach ($analyzerFilePath in Get-ChildItem -Path "$analyzersPath\*.dll" -Exclude *.resources.dll) + { + if($project.Object.AnalyzerReferences) + { + $project.Object.AnalyzerReferences.Remove($analyzerFilePath.FullName) + } + } + } +} + +# $project.Type gives the language name like (C# or VB.NET) +$languageFolder = "" +if($project.Type -eq "C#") +{ + $languageFolder = "cs" +} +if($project.Type -eq "VB.NET") +{ + $languageFolder = "vb" +} +if($languageFolder -eq "") +{ + return +} + +foreach($analyzersPath in $analyzersPaths) +{ + # Uninstall language specific analyzers. + $languageAnalyzersPath = join-path $analyzersPath $languageFolder + if (Test-Path $languageAnalyzersPath) + { + foreach ($analyzerFilePath in Get-ChildItem -Path "$languageAnalyzersPath\*.dll" -Exclude *.resources.dll) + { + if($project.Object.AnalyzerReferences) + { + try + { + $project.Object.AnalyzerReferences.Remove($analyzerFilePath.FullName) + } + catch + { + + } + } + } + } +} diff --git a/src/SMAPI.ModBuildConfig/StardewModdingAPI.ModBuildConfig.csproj b/src/SMAPI.ModBuildConfig/StardewModdingAPI.ModBuildConfig.csproj index d2e06e05..2e3ba356 100644 --- a/src/SMAPI.ModBuildConfig/StardewModdingAPI.ModBuildConfig.csproj +++ b/src/SMAPI.ModBuildConfig/StardewModdingAPI.ModBuildConfig.csproj @@ -57,4 +57,5 @@ + \ No newline at end of file diff --git a/src/SMAPI.ModBuildConfig/package.nuspec b/src/SMAPI.ModBuildConfig/package.nuspec index d24e15be..a280a268 100644 --- a/src/SMAPI.ModBuildConfig/package.nuspec +++ b/src/SMAPI.ModBuildConfig/package.nuspec @@ -2,7 +2,7 @@ Pathoschild.Stardew.ModBuildConfig - 2.0.3-alpha20180325 + 2.1-alpha20180409 Build package for SMAPI mods Pathoschild Pathoschild @@ -12,28 +12,10 @@ https://raw.githubusercontent.com/Pathoschild/SMAPI/develop/src/SMAPI.ModBuildConfig/assets/nuget-icon.png Automates the build configuration for crossplatform Stardew Valley SMAPI mods. - 2.0: - - Added: mods are now copied into the `Mods` folder automatically (configurable). - - Added: release zips are now created automatically in your build output folder (configurable). - - Added: mod deploy and release zips now exclude Json.NET automatically, since it's provided by SMAPI. - - Added mod's version to release zip filename. - - Improved errors to simplify troubleshooting. - - Fixed release zip not having a mod folder. - - Fixed release zip failing if mod name contains characters that aren't valid in a filename. - - 2.0.1: - - Fixed mod deploy failing to create subfolders if they don't already exist. - - 2.0.2: - - Fixed compatibility issue on Linux. - - 2.0.3: + 2.1: - Added support for Stardew Valley 1.3. - Added support for unit test projects. + - Added C# analyzers to warn about implicit conversions of Netcode fields in Stardew Valley 1.3. - - - - diff --git a/src/SMAPI.Tests/StardewModdingAPI.Tests.csproj b/src/SMAPI.Tests/StardewModdingAPI.Tests.csproj index e4b2c8c6..858a0d4a 100644 --- a/src/SMAPI.Tests/StardewModdingAPI.Tests.csproj +++ b/src/SMAPI.Tests/StardewModdingAPI.Tests.csproj @@ -66,6 +66,9 @@ StardewModdingAPI + + + \ No newline at end of file diff --git a/src/SMAPI.sln b/src/SMAPI.sln index 6577b42e..56898a32 100644 --- a/src/SMAPI.sln +++ b/src/SMAPI.sln @@ -45,9 +45,17 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "build", "build", "{09CF91E5 ..\build\common.targets = ..\build\common.targets ..\build\GlobalAssemblyInfo.cs = ..\build\GlobalAssemblyInfo.cs ..\build\prepare-install-package.targets = ..\build\prepare-install-package.targets + ..\build\prepare-nuget-package.targets = ..\build\prepare-nuget-package.targets EndProjectSection EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "StardewModdingAPI.ModBuildConfig", "SMAPI.ModBuildConfig\StardewModdingAPI.ModBuildConfig.csproj", "{EA4F1E80-743F-4A1D-9757-AE66904A196A}" + ProjectSection(ProjectDependencies) = postProject + {80AD8528-AA49-4731-B4A6-C691845815A1} = {80AD8528-AA49-4731-B4A6-C691845815A1} + EndProjectSection +EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "StardewModdingAPI.ModBuildConfig.Analyzer", "SMAPI.ModBuildConfig.Analyzer\StardewModdingAPI.ModBuildConfig.Analyzer.csproj", "{80AD8528-AA49-4731-B4A6-C691845815A1}" +EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SMAPI.ModBuildConfig.Analyzer.Tests", "SMAPI.ModBuildConfig.Analyzer.Tests\SMAPI.ModBuildConfig.Analyzer.Tests.csproj", "{0CF97929-B0D0-4D73-B7BF-4FF7191035F9}" EndProject Global GlobalSection(SharedMSBuildProjectFiles) = preSolution @@ -137,6 +145,30 @@ Global {EA4F1E80-743F-4A1D-9757-AE66904A196A}.Release|Mixed Platforms.Build.0 = Release|x86 {EA4F1E80-743F-4A1D-9757-AE66904A196A}.Release|x86.ActiveCfg = Release|x86 {EA4F1E80-743F-4A1D-9757-AE66904A196A}.Release|x86.Build.0 = Release|x86 + {80AD8528-AA49-4731-B4A6-C691845815A1}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Debug|Any CPU.Build.0 = Debug|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Debug|Mixed Platforms.ActiveCfg = Debug|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Debug|Mixed Platforms.Build.0 = Debug|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Debug|x86.ActiveCfg = Debug|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Debug|x86.Build.0 = Debug|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Release|Any CPU.ActiveCfg = Release|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Release|Any CPU.Build.0 = Release|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Release|Mixed Platforms.ActiveCfg = Release|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Release|Mixed Platforms.Build.0 = Release|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Release|x86.ActiveCfg = Release|Any CPU + {80AD8528-AA49-4731-B4A6-C691845815A1}.Release|x86.Build.0 = Release|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Debug|Any CPU.Build.0 = Debug|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Debug|Mixed Platforms.ActiveCfg = Debug|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Debug|Mixed Platforms.Build.0 = Debug|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Debug|x86.ActiveCfg = Debug|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Debug|x86.Build.0 = Debug|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Release|Any CPU.ActiveCfg = Release|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Release|Any CPU.Build.0 = Release|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Release|Mixed Platforms.ActiveCfg = Release|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Release|Mixed Platforms.Build.0 = Release|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Release|x86.ActiveCfg = Release|Any CPU + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -147,6 +179,7 @@ Global {2AA02FB6-FF03-41CF-A215-2EE60AB4F5DC} = {82D22ED7-A0A7-4D64-8E92-4B6A5E74ED11} {EB35A917-67B9-4EFA-8DFC-4FB49B3949BB} = {86C452BE-D2D8-45B4-B63F-E329EB06CEDA} {09CF91E5-5BAB-4650-A200-E5EA9A633046} = {86C452BE-D2D8-45B4-B63F-E329EB06CEDA} + {0CF97929-B0D0-4D73-B7BF-4FF7191035F9} = {82D22ED7-A0A7-4D64-8E92-4B6A5E74ED11} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {70143042-A862-47A8-A677-7C819DDC90DC} -- cgit From 4f5f463bd227a81c26224a81b178e2221946b9b2 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Mon, 9 Apr 2018 22:33:45 -0400 Subject: warn when directly using a net field that has a non-net wrapper (#471) --- docs/mod-build-config.md | 9 + docs/screenshots/code-analyzer-example.png | Bin 5696 -> 4022 bytes .../UnitTests.cs | 43 ++++- .../ImplicitNetFieldCastAnalyzer.cs | 208 ++++++++++++++++++--- 4 files changed, 228 insertions(+), 32 deletions(-) (limited to 'src') 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.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 *********/ - /// 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 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 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 /// 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; + } } } -- cgit From 416e1c3c1b884fb9b932968e72895babb6151d2f Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Mon, 9 Apr 2018 22:34:11 -0400 Subject: rename file to match new scope (#471) --- .../ImplicitNetFieldCastAnalyzer.cs | 265 --------------------- .../NetFieldAnalyzer.cs | 265 +++++++++++++++++++++ 2 files changed, 265 insertions(+), 265 deletions(-) delete mode 100644 src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs (limited to 'src') diff --git a/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs deleted file mode 100644 index d64b1486..00000000 --- a/src/SMAPI.ModBuildConfig.Analyzer/ImplicitNetFieldCastAnalyzer.cs +++ /dev/null @@ -1,265 +0,0 @@ -using System; -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 -{ - /// 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 NetFieldAnalyzer : DiagnosticAnalyzer - { - /********* - ** Properties - *********/ - /// 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 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" - ) - }; - - - /********* - ** Accessors - *********/ - /// The descriptors for the diagnostics that this analyzer is capable of producing. - public override ImmutableArray SupportedDiagnostics { get; } - - - /********* - ** Public methods - *********/ - /// Construct an instance. - public NetFieldAnalyzer() - { - 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.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 - ); - } - - - /********* - ** 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. - /// The analysis 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.WarnIfOperandImplicitlyConvertsNetField(context, node.Left); - if (!leftHasWarning) - this.WarnIfOperandImplicitlyConvertsNetField(context, node.Right); - } - catch (Exception ex) - { - throw new InvalidOperationException($"Failed processing expression: '{context.Node}'. Exception details: {ex.ToString().Replace('\r', ' ').Replace('\n', ' ')}"); - } - } - - /// Analyse one operand in a binary expression (like a and b in a == b) and add a diagnostic message if applicable. - /// The analysis context. - /// The operand expression. - /// Returns whether a diagnostic message was raised. - private bool WarnIfOperandImplicitlyConvertsNetField(SyntaxNodeAnalysisContext context, ExpressionSyntax operand) - { - TypeInfo operandType = context.SemanticModel.GetTypeInfo(operand); - if (this.IsNetType(operandType.Type) && !this.IsNetType(operandType.ConvertedType)) - { - string fromTypeName = operandType.Type.Name; - string toTypeName = operandType.ConvertedType.Name; - 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; - } - } -} diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs new file mode 100644 index 00000000..d64b1486 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -0,0 +1,265 @@ +using System; +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 +{ + /// 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 NetFieldAnalyzer : DiagnosticAnalyzer + { + /********* + ** Properties + *********/ + /// 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 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" + ) + }; + + + /********* + ** Accessors + *********/ + /// The descriptors for the diagnostics that this analyzer is capable of producing. + public override ImmutableArray SupportedDiagnostics { get; } + + + /********* + ** Public methods + *********/ + /// Construct an instance. + public NetFieldAnalyzer() + { + 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.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 + ); + } + + + /********* + ** 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. + /// The analysis 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.WarnIfOperandImplicitlyConvertsNetField(context, node.Left); + if (!leftHasWarning) + this.WarnIfOperandImplicitlyConvertsNetField(context, node.Right); + } + catch (Exception ex) + { + throw new InvalidOperationException($"Failed processing expression: '{context.Node}'. Exception details: {ex.ToString().Replace('\r', ' ').Replace('\n', ' ')}"); + } + } + + /// Analyse one operand in a binary expression (like a and b in a == b) and add a diagnostic message if applicable. + /// The analysis context. + /// The operand expression. + /// Returns whether a diagnostic message was raised. + private bool WarnIfOperandImplicitlyConvertsNetField(SyntaxNodeAnalysisContext context, ExpressionSyntax operand) + { + TypeInfo operandType = context.SemanticModel.GetTypeInfo(operand); + if (this.IsNetType(operandType.Type) && !this.IsNetType(operandType.ConvertedType)) + { + string fromTypeName = operandType.Type.Name; + string toTypeName = operandType.ConvertedType.Name; + 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; + } + } +} -- cgit From c8db771e11a0e5224aa3b0766134afc8e733896e Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Mon, 9 Apr 2018 23:25:10 -0400 Subject: tweak message output and unit tests (#471) --- .../UnitTests.cs | 24 +++++++++++----------- .../NetFieldAnalyzer.cs | 4 +--- 2 files changed, 13 insertions(+), 15 deletions(-) (limited to 'src') diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs index b4f54234..92fc9074 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs @@ -34,7 +34,9 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests { class Item { - public NetInt category { get; } = new NetInt { Value = 42 }; + public NetInt category { get; } = new NetInt { Value = 42 }; // SMAPI002: use Category instead + public NetInt type { get; } = new NetInt { Value = 42 }; + public NetRef refField { get; } = null; } } @@ -44,15 +46,13 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests { public void Entry() { - NetInt intField = new NetInt { Value = 42 }; - NetRef refField = new NetRef { Value = null }; Item item = null; // this line should raise diagnostics {{test-code}} // line 36 // these lines should not - if ((int)intField != 42); + if (item.type.Value != 42); } } } @@ -85,14 +85,14 @@ 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("if (intField < 42);", 4, "intField", "NetInt", "Int32")] - [TestCase("if (intField <= 42);", 4, "intField", "NetInt", "Int32")] - [TestCase("if (intField > 42);", 4, "intField", "NetInt", "Int32")] - [TestCase("if (intField >= 42);", 4, "intField", "NetInt", "Int32")] - [TestCase("if (intField == 42);", 4, "intField", "NetInt", "Int32")] - [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")] + [TestCase("if (item.type < 42);", 4, "item.type", "NetInt", "int")] + [TestCase("if (item.type <= 42);", 4, "item.type", "NetInt", "int")] + [TestCase("if (item.type > 42);", 4, "item.type", "NetInt", "int")] + [TestCase("if (item.type >= 42);", 4, "item.type", "NetInt", "int")] + [TestCase("if (item.type == 42);", 4, "item.type", "NetInt", "int")] + [TestCase("if (item.type != 42);", 4, "item.type", "NetInt", "int")] + [TestCase("if (item.refField != null);", 4, "item.refField", "NetRef", "object")] + [TestCase("if (item?.type != 42);", 4, "item?.type", "NetInt", "int")] public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType) { // arrange diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index d64b1486..a9987733 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -246,9 +246,7 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer TypeInfo operandType = context.SemanticModel.GetTypeInfo(operand); if (this.IsNetType(operandType.Type) && !this.IsNetType(operandType.ConvertedType)) { - string fromTypeName = operandType.Type.Name; - string toTypeName = operandType.ConvertedType.Name; - context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI001"], context.Node.GetLocation(), operand, fromTypeName, toTypeName)); + context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI001"], context.Node.GetLocation(), operand, operandType.Type.Name, operandType.ConvertedType)); return true; } -- cgit From 971ed1a17559064ee0ee42a83e786b3e3076059f Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Mon, 9 Apr 2018 23:43:13 -0400 Subject: fix net field replacements not reported for a subclass reference (#471) --- src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs | 8 ++++++-- src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs index 92fc9074..f3919f78 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs @@ -38,6 +38,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests public NetInt type { get; } = new NetInt { Value = 42 }; public NetRef refField { get; } = null; } + class SObject : Item { } } namespace SampleMod @@ -47,9 +48,10 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests public void Entry() { Item item = null; + SObject obj = null; // this line should raise diagnostics - {{test-code}} // line 36 + {{test-code}} // line 38 // these lines should not if (item.type.Value != 42); @@ -59,7 +61,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests "; /// The line number where the unit tested code is injected into . - private const int SampleCodeLine = 36; + private const int SampleCodeLine = 38; /// The column number where the unit tested code is injected into . private const int SampleCodeColumn = 25; @@ -93,6 +95,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests [TestCase("if (item.type != 42);", 4, "item.type", "NetInt", "int")] [TestCase("if (item.refField != null);", 4, "item.refField", "NetRef", "object")] [TestCase("if (item?.type != 42);", 4, "item?.type", "NetInt", "int")] + [TestCase("if (obj.type != 42);", 4, "obj.type", "NetInt", "int")] public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType) { // arrange @@ -118,6 +121,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests [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")] + [TestCase("int category = obj.category;", 15, "obj.category", "NetInt", "Category")] public void AvoidNetFields_RaisesDiagnostic(string codeText, int column, string expression, string netType, string suggestedProperty) { // arrange diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index a9987733..2cb1ac4c 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -209,9 +209,13 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer string propertyName = node.Name.Identifier.Text; // suggest replacement - if (this.NetFieldWrapperProperties.TryGetValue($"{declaringType}::{propertyName}", out string suggestedPropertyName)) + for (ITypeSymbol type = declaringType; type != null; type = type.BaseType) { - context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI002"], context.Node.GetLocation(), node, memberType.Type.Name, suggestedPropertyName)); + if (this.NetFieldWrapperProperties.TryGetValue($"{type}::{propertyName}", out string suggestedPropertyName)) + { + context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI002"], context.Node.GetLocation(), node, memberType.Type.Name, suggestedPropertyName)); + break; + } } } catch (Exception ex) -- cgit From 9e5c3912b6cfce0348aa2d88d30a11fe5b410e9c Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Tue, 10 Apr 2018 18:22:16 -0400 Subject: move mock classes out of sample code (#471) --- .../Framework/DiagnosticVerifier.Helper.cs | 2 ++ .../Mock/Netcode/NetFieldBase.cs | 16 +++++++++++++ .../Mock/Netcode/NetInt.cs | 6 +++++ .../Mock/Netcode/NetRef.cs | 6 +++++ .../Mock/StardewValley/Item.cs | 18 +++++++++++++++ .../Mock/StardewValley/Object.cs | 6 +++++ .../UnitTests.cs | 27 +++------------------- 7 files changed, 57 insertions(+), 24 deletions(-) create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetFieldBase.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetInt.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetRef.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Object.cs (limited to 'src') diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.Helper.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.Helper.cs index d33455fc..0247288e 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.Helper.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Framework/DiagnosticVerifier.Helper.cs @@ -20,6 +20,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests.Framework private static readonly MetadataReference SystemCoreReference = MetadataReference.CreateFromFile(typeof(Enumerable).Assembly.Location); private static readonly MetadataReference CSharpSymbolsReference = MetadataReference.CreateFromFile(typeof(CSharpCompilation).Assembly.Location); private static readonly MetadataReference CodeAnalysisReference = MetadataReference.CreateFromFile(typeof(Compilation).Assembly.Location); + private static readonly MetadataReference SelfReference = MetadataReference.CreateFromFile(typeof(DiagnosticVerifier).Assembly.Location); internal static string DefaultFilePathPrefix = "Test"; internal static string CSharpDefaultFileExt = "cs"; @@ -150,6 +151,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests.Framework var solution = new AdhocWorkspace() .CurrentSolution .AddProject(projectId, TestProjectName, TestProjectName, language) + .AddMetadataReference(projectId, DiagnosticVerifier.SelfReference) .AddMetadataReference(projectId, CorlibReference) .AddMetadataReference(projectId, SystemCoreReference) .AddMetadataReference(projectId, CSharpSymbolsReference) diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetFieldBase.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetFieldBase.cs new file mode 100644 index 00000000..1684229a --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetFieldBase.cs @@ -0,0 +1,16 @@ +// ReSharper disable CheckNamespace -- matches Stardew Valley's code +namespace Netcode +{ + /// A simplified version of Stardew Valley's Netcode.NetFieldBase for unit testing. + /// The type of the synchronised value. + /// The type of the current instance. + public class NetFieldBase where TSelf : NetFieldBase + { + /// The synchronised value. + public T Value { get; set; } + + /// Implicitly convert a net field to the its type. + /// The field to convert. + public static implicit operator T(NetFieldBase field) => field.Value; + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetInt.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetInt.cs new file mode 100644 index 00000000..b3abc467 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetInt.cs @@ -0,0 +1,6 @@ +// ReSharper disable CheckNamespace -- matches Stardew Valley's code +namespace Netcode +{ + /// A simplified version of Stardew Valley's Netcode.NetInt for unit testing. + public class NetInt : NetFieldBase { } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetRef.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetRef.cs new file mode 100644 index 00000000..714c4a8d --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetRef.cs @@ -0,0 +1,6 @@ +// ReSharper disable CheckNamespace -- matches Stardew Valley's code +namespace Netcode +{ + /// A simplified version of Stardew Valley's Netcode.NetRef for unit testing. + public class NetRef : NetFieldBase { } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs new file mode 100644 index 00000000..88723a56 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs @@ -0,0 +1,18 @@ +// ReSharper disable CheckNamespace, InconsistentNaming -- matches Stardew Valley's code +using Netcode; + +namespace StardewValley +{ + /// A simplified version of Stardew Valley's StardewValley.Item class for unit testing. + public class Item + { + /// A net int field with an equivalent non-net Category property. + public NetInt category { get; } = new NetInt { Value = 42 }; + + /// A net int field with no equivalent non-net property. + public NetInt type { get; } = new NetInt { Value = 42 }; + + /// A net reference field. + public NetRef refField { get; } = null; + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Object.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Object.cs new file mode 100644 index 00000000..498c38c1 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Object.cs @@ -0,0 +1,6 @@ +// ReSharper disable CheckNamespace -- matches Stardew Valley's code +namespace StardewValley +{ + /// A simplified version of Stardew Valley's StardewValley.Object class for unit testing. + public class Object : Item { } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs index f3919f78..c12bb839 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs @@ -18,28 +18,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests using System; using StardewValley; using Netcode; - - namespace Netcode - { - public class NetInt : NetFieldBase { } - public class NetRef : NetFieldBase { } - public class NetFieldBase where TSelf : NetFieldBase - { - public T Value { get; set; } - public static implicit operator T(NetFieldBase field) => field.Value; - } - } - - namespace StardewValley - { - class Item - { - public NetInt category { get; } = new NetInt { Value = 42 }; // SMAPI002: use Category instead - public NetInt type { get; } = new NetInt { Value = 42 }; - public NetRef refField { get; } = null; - } - class SObject : Item { } - } + using SObject = StardewValley.Object; namespace SampleMod { @@ -51,7 +30,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests SObject obj = null; // this line should raise diagnostics - {{test-code}} // line 38 + {{test-code}} // these lines should not if (item.type.Value != 42); @@ -61,7 +40,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests "; /// The line number where the unit tested code is injected into . - private const int SampleCodeLine = 38; + private const int SampleCodeLine = 17; /// The column number where the unit tested code is injected into . private const int SampleCodeColumn = 25; -- cgit From 35c2e5968579ea578cf04e7550ab43bc5a4b8074 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Tue, 10 Apr 2018 18:22:34 -0400 Subject: expand analyzer unit tests (#471) --- .../Mock/Netcode/NetRef.cs | 2 +- .../Mock/StardewValley/Item.cs | 16 ++++--- .../Mock/StardewValley/Object.cs | 10 ++++- .../UnitTests.cs | 52 +++++++++++++--------- 4 files changed, 51 insertions(+), 29 deletions(-) (limited to 'src') diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetRef.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetRef.cs index 714c4a8d..be2459cc 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetRef.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/Netcode/NetRef.cs @@ -2,5 +2,5 @@ namespace Netcode { /// A simplified version of Stardew Valley's Netcode.NetRef for unit testing. - public class NetRef : NetFieldBase { } + public class NetRef : NetFieldBase> where T : class { } } diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs index 88723a56..386767d7 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Item.cs @@ -7,12 +7,18 @@ namespace StardewValley public class Item { /// A net int field with an equivalent non-net Category property. - public NetInt category { get; } = new NetInt { Value = 42 }; + public readonly NetInt category = new NetInt { Value = 42 }; - /// A net int field with no equivalent non-net property. - public NetInt type { get; } = new NetInt { Value = 42 }; + /// A generic net int field with no equivalent non-net property. + public readonly NetInt netIntField = new NetInt { Value = 42 }; - /// A net reference field. - public NetRef refField { get; } = null; + /// A generic net ref field with no equivalent non-net property. + public readonly NetRef netRefField = new NetRef(); + + /// A generic net int property with no equivalent non-net property. + public NetInt netIntProperty = new NetInt { Value = 42 }; + + /// A generic net ref property with no equivalent non-net property. + public NetRef netRefProperty { get; } = new NetRef(); } } diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Object.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Object.cs index 498c38c1..3dd66a6d 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Object.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Object.cs @@ -1,6 +1,12 @@ -// ReSharper disable CheckNamespace -- matches Stardew Valley's code +// ReSharper disable CheckNamespace, InconsistentNaming -- matches Stardew Valley's code +using Netcode; + namespace StardewValley { /// A simplified version of Stardew Valley's StardewValley.Object class for unit testing. - public class Object : Item { } + public class Object : Item + { + /// A net int field with an equivalent non-net property. + public NetInt type = new NetInt { Value = 42 }; + } } diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs index c12bb839..51e0b059 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs @@ -26,21 +26,14 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests { public void Entry() { - Item item = null; - SObject obj = null; - - // this line should raise diagnostics {{test-code}} - - // these lines should not - if (item.type.Value != 42); } } } "; /// The line number where the unit tested code is injected into . - private const int SampleCodeLine = 17; + private const int SampleCodeLine = 13; /// The column number where the unit tested code is injected into . private const int SampleCodeColumn = 25; @@ -66,15 +59,32 @@ 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("if (item.type < 42);", 4, "item.type", "NetInt", "int")] - [TestCase("if (item.type <= 42);", 4, "item.type", "NetInt", "int")] - [TestCase("if (item.type > 42);", 4, "item.type", "NetInt", "int")] - [TestCase("if (item.type >= 42);", 4, "item.type", "NetInt", "int")] - [TestCase("if (item.type == 42);", 4, "item.type", "NetInt", "int")] - [TestCase("if (item.type != 42);", 4, "item.type", "NetInt", "int")] - [TestCase("if (item.refField != null);", 4, "item.refField", "NetRef", "object")] - [TestCase("if (item?.type != 42);", 4, "item?.type", "NetInt", "int")] - [TestCase("if (obj.type != 42);", 4, "obj.type", "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")] + [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")] + [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 != null);", 22, "item?.netIntField", "NetInt", "object")] + [TestCase("Item item = null; if (item.netIntProperty < 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty <= 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty > 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty >= 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty == 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty != 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item?.netIntProperty != 42);", 22, "item?.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item?.netIntProperty != null);", 22, "item?.netIntProperty", "NetInt", "object")] + [TestCase("Item item = null; if (item.netRefField == null);", 22, "item.netRefField", "NetRef", "object")] + [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.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")] + [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")] public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType) { // arrange @@ -97,10 +107,10 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests /// 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")] - [TestCase("int category = obj.category;", 15, "obj.category", "NetInt", "Category")] + [TestCase("Item item = null; int category = item.category;", 33, "item.category", "NetInt", "Category")] + [TestCase("Item item = null; int category = (item).category;", 33, "(item).category", "NetInt", "Category")] + [TestCase("Item item = null; int category = ((Item)item).category;", 33, "((Item)item).category", "NetInt", "Category")] + [TestCase("SObject obj = null; int category = obj.category;", 35, "obj.category", "NetInt", "Category")] public void AvoidNetFields_RaisesDiagnostic(string codeText, int column, string expression, string netType, string suggestedProperty) { // arrange -- cgit From 1fb625dc42a7cf5fdf74329454bc3ecde806ae10 Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Tue, 10 Apr 2018 18:23:08 -0400 Subject: fix some net field comparisons to null not flagged (#471) --- .../NetFieldAnalyzer.cs | 46 ++++++++++++---------- 1 file changed, 26 insertions(+), 20 deletions(-) (limited to 'src') diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index 2cb1ac4c..ae646fd8 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -230,10 +230,32 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer { try { - BinaryExpressionSyntax node = (BinaryExpressionSyntax)context.Node; - bool leftHasWarning = this.WarnIfOperandImplicitlyConvertsNetField(context, node.Left); - if (!leftHasWarning) - this.WarnIfOperandImplicitlyConvertsNetField(context, node.Right); + 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; + } + } } catch (Exception ex) { @@ -241,22 +263,6 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer } } - /// Analyse one operand in a binary expression (like a and b in a == b) and add a diagnostic message if applicable. - /// The analysis context. - /// The operand expression. - /// Returns whether a diagnostic message was raised. - private bool WarnIfOperandImplicitlyConvertsNetField(SyntaxNodeAnalysisContext context, ExpressionSyntax operand) - { - TypeInfo operandType = context.SemanticModel.GetTypeInfo(operand); - if (this.IsNetType(operandType.Type) && !this.IsNetType(operandType.ConvertedType)) - { - context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI001"], context.Node.GetLocation(), operand, operandType.Type.Name, operandType.ConvertedType)); - return true; - } - - return false; - } - /// Get whether a type symbol references a Netcode type. /// The type symbol. private bool IsNetType(ITypeSymbol typeSymbol) -- cgit From c6c2302baf830a441b269ef7402068bd9dece22e Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Tue, 10 Apr 2018 18:23:39 -0400 Subject: tweak analyzer code & documentation (#471) --- docs/mod-build-config.md | 17 ++++++++++------- src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs | 4 ++-- src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs | 10 ++++------ 3 files changed, 16 insertions(+), 15 deletions(-) (limited to 'src') diff --git a/docs/mod-build-config.md b/docs/mod-build-config.md index 44242160..00f9a356 100644 --- a/docs/mod-build-config.md +++ b/docs/mod-build-config.md @@ -159,13 +159,16 @@ See below for help with each specific warning. > {{net type}} has unintuitive implicit conversion rules. Consider comparing against the actual > value instead to avoid bugs. -Stardew Valley uses a set of net fields (like `NetBool` or `NetInt`) to handle multiplayer sync. -These types can implicitly convert to their equivalent normal values (like `bool x = new NetBool()`), -but their conversion rules can be unintuitive and error-prone. For example, +Stardew Valley uses net types (like `NetBool` and `NetInt`) to handle multiplayer sync. These types +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. 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 + these. Use the suggested property instead. * For a reference type (i.e. one that can contain `null`), you can use the `.Value` property: ```c# if (building.indoors.Value == null) @@ -176,17 +179,17 @@ Suggested fix: if(indoors == null) // ... ``` -* For a value type (i.e. one that can't contain `null`), make sure to check for null first if - applicable: +* For a value type (i.e. one that can't contain `null`), check if the object is null (if applicable) + and compare with `.Value`: ```cs - if (item != null && item.category == 0) + if (item != null && item.category.Value == 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 +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. diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs index 51e0b059..8ca27847 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs @@ -92,7 +92,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests DiagnosticResult expected = new DiagnosticResult { Id = "SMAPI001", - Message = $"This implicitly converts '{expression}' from {fromType} to {toType}, but {fromType} has unintuitive implicit conversion rules. Consider comparing against the actual value instead to avoid bugs. See https://smapi.io/buildmsg/SMAPI001 for details.", + Message = $"This implicitly converts '{expression}' from {fromType} to {toType}, but {fromType} has unintuitive implicit conversion rules. Consider comparing against the actual value instead to avoid bugs. See https://smapi.io/buildmsg/smapi001 for details.", Severity = DiagnosticSeverity.Warning, Locations = new[] { new DiagnosticResultLocation("Test0.cs", UnitTests.SampleCodeLine, UnitTests.SampleCodeColumn + column) } }; @@ -118,7 +118,7 @@ namespace SMAPI.ModBuildConfig.Analyzer.Tests 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.", + 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) } }; diff --git a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs index ae646fd8..915a50e8 100644 --- a/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs +++ b/src/SMAPI.ModBuildConfig.Analyzer/NetFieldAnalyzer.cs @@ -130,22 +130,20 @@ namespace StardewModdingAPI.ModBuildConfig.Analyzer ["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.", + 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" + 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.", + 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" + helpLinkUri: "https://smapi.io/buildmsg/smapi001" ) }; -- cgit From 13f31e8b725e46ca8442a943a5675723d22b4fdc Mon Sep 17 00:00:00 2001 From: Jesse Plamondon-Willard Date: Tue, 10 Apr 2018 18:23:57 -0400 Subject: warn for fields which no longer work (#471) --- docs/mod-build-config.md | 6 + .../Mock/StardewValley/Farmer.cs | 11 ++ .../NetFieldAnalyzerTests.cs | 140 +++++++++++++++++++++ .../ObsoleteFieldAnalyzerTests.cs | 88 +++++++++++++ .../UnitTests.cs | 140 --------------------- .../ObsoleteFieldAnalyzer.cs | 98 +++++++++++++++ 6 files changed, 343 insertions(+), 140 deletions(-) create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/ObsoleteFieldAnalyzerTests.cs delete mode 100644 src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs create mode 100644 src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs (limited to 'src') diff --git a/docs/mod-build-config.md b/docs/mod-build-config.md index 00f9a356..99a567f2 100644 --- a/docs/mod-build-config.md +++ b/docs/mod-build-config.md @@ -194,6 +194,12 @@ field has an equivalent non-net property that avoids those issues. Suggested fix: access the suggested property name instead. +### SMAPI003 +**Avoid obsolete fields:** +> The '{{old field}}' field is obsolete and should be replaced with '{{new field}}'. + +Your code accesses a field which is obsolete or no longer works. Use the suggested field 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/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs new file mode 100644 index 00000000..e0f0e30c --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/Mock/StardewValley/Farmer.cs @@ -0,0 +1,11 @@ +// ReSharper disable CheckNamespace, InconsistentNaming -- matches Stardew Valley's code +using System.Collections.Generic; + +namespace StardewValley +{ + /// A simplified version of Stardew Valley's StardewValley.Farmer class for unit testing. + internal class Farmer + { + public IDictionary friendships; + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs new file mode 100644 index 00000000..101f4c21 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/NetFieldAnalyzerTests.cs @@ -0,0 +1,140 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Framework; +using SMAPI.ModBuildConfig.Analyzer.Tests.Framework; +using StardewModdingAPI.ModBuildConfig.Analyzer; + +namespace SMAPI.ModBuildConfig.Analyzer.Tests +{ + /// Unit tests for . + [TestFixture] + public class NetFieldAnalyzerTests : DiagnosticVerifier + { + /********* + ** Properties + *********/ + /// Sample C# mod code, with a {{test-code}} placeholder for the code in the Entry method to test. + const string SampleProgram = @" + using System; + using StardewValley; + using Netcode; + using SObject = StardewValley.Object; + + namespace SampleMod + { + class ModEntry + { + public void Entry() + { + {{test-code}} + } + } + } + "; + + /// The line number where the unit tested code is injected into . + private const int SampleCodeLine = 13; + + /// The column number where the unit tested code is injected into . + private const int SampleCodeColumn = 25; + + + /********* + ** Unit tests + *********/ + /// Test that no diagnostics are raised for an empty code block. + [TestCase] + public void EmptyCode_HasNoDiagnostics() + { + // arrange + string test = @""; + + // assert + this.VerifyCSharpDiagnostic(test); + } + + /// 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. + /// 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")] + [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")] + [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 != null);", 22, "item?.netIntField", "NetInt", "object")] + [TestCase("Item item = null; if (item.netIntProperty < 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty <= 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty > 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty >= 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty == 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item.netIntProperty != 42);", 22, "item.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item?.netIntProperty != 42);", 22, "item?.netIntProperty", "NetInt", "int")] + [TestCase("Item item = null; if (item?.netIntProperty != null);", 22, "item?.netIntProperty", "NetInt", "object")] + [TestCase("Item item = null; if (item.netRefField == null);", 22, "item.netRefField", "NetRef", "object")] + [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.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")] + [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")] + public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType) + { + // arrange + string code = NetFieldAnalyzerTests.SampleProgram.Replace("{{test-code}}", codeText); + DiagnosticResult expected = new DiagnosticResult + { + Id = "SMAPI001", + Message = $"This implicitly converts '{expression}' from {fromType} to {toType}, but {fromType} has unintuitive implicit conversion rules. Consider comparing against the actual value instead to avoid bugs. See https://smapi.io/buildmsg/smapi001 for details.", + Severity = DiagnosticSeverity.Warning, + Locations = new[] { new DiagnosticResultLocation("Test0.cs", NetFieldAnalyzerTests.SampleCodeLine, NetFieldAnalyzerTests.SampleCodeColumn + column) } + }; + + // assert + 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("Item item = null; int category = item.category;", 33, "item.category", "NetInt", "Category")] + [TestCase("Item item = null; int category = (item).category;", 33, "(item).category", "NetInt", "Category")] + [TestCase("Item item = null; int category = ((Item)item).category;", 33, "((Item)item).category", "NetInt", "Category")] + [TestCase("SObject obj = null; int category = obj.category;", 35, "obj.category", "NetInt", "Category")] + public void AvoidNetFields_RaisesDiagnostic(string codeText, int column, string expression, string netType, string suggestedProperty) + { + // arrange + string code = NetFieldAnalyzerTests.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", NetFieldAnalyzerTests.SampleCodeLine, NetFieldAnalyzerTests.SampleCodeColumn + column) } + }; + + // assert + this.VerifyCSharpDiagnostic(code, expected); + } + + + /********* + ** Helpers + *********/ + /// Get the analyzer being tested. + protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() + { + return new NetFieldAnalyzer(); + } + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/ObsoleteFieldAnalyzerTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/ObsoleteFieldAnalyzerTests.cs new file mode 100644 index 00000000..dc7476ef --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer.Tests/ObsoleteFieldAnalyzerTests.cs @@ -0,0 +1,88 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Framework; +using SMAPI.ModBuildConfig.Analyzer.Tests.Framework; +using StardewModdingAPI.ModBuildConfig.Analyzer; + +namespace SMAPI.ModBuildConfig.Analyzer.Tests +{ + /// Unit tests for . + [TestFixture] + public class ObsoleteFieldAnalyzerTests : DiagnosticVerifier + { + /********* + ** Properties + *********/ + /// Sample C# mod code, with a {{test-code}} placeholder for the code in the Entry method to test. + const string SampleProgram = @" + using System; + using StardewValley; + using Netcode; + using SObject = StardewValley.Object; + + namespace SampleMod + { + class ModEntry + { + public void Entry() + { + {{test-code}} + } + } + } + "; + + /// The line number where the unit tested code is injected into . + private const int SampleCodeLine = 13; + + /// The column number where the unit tested code is injected into . + private const int SampleCodeColumn = 25; + + + /********* + ** Unit tests + *********/ + /// Test that no diagnostics are raised for an empty code block. + [TestCase] + public void EmptyCode_HasNoDiagnostics() + { + // arrange + string test = @""; + + // assert + this.VerifyCSharpDiagnostic(test); + } + + /// Test that the expected diagnostic message is raised for an obsolete field reference. + /// The code line to test. + /// The column within the code line where the diagnostic message should be reported. + /// The old field name which should be reported. + /// The new field name which should be reported. + [TestCase("var x = new Farmer().friendships;", 8, "StardewValley.Farmer.friendships", "friendshipData")] + public void AvoidObsoleteField_RaisesDiagnostic(string codeText, int column, string oldName, string newName) + { + // arrange + string code = ObsoleteFieldAnalyzerTests.SampleProgram.Replace("{{test-code}}", codeText); + DiagnosticResult expected = new DiagnosticResult + { + Id = "SMAPI003", + Message = $"The '{oldName}' field is obsolete and should be replaced with '{newName}'. See https://smapi.io/buildmsg/smapi003 for details.", + Severity = DiagnosticSeverity.Warning, + Locations = new[] { new DiagnosticResultLocation("Test0.cs", ObsoleteFieldAnalyzerTests.SampleCodeLine, ObsoleteFieldAnalyzerTests.SampleCodeColumn + column) } + }; + + // assert + this.VerifyCSharpDiagnostic(code, expected); + } + + + /********* + ** Helpers + *********/ + /// Get the analyzer being tested. + protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() + { + return new ObsoleteFieldAnalyzer(); + } + } +} diff --git a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs b/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs deleted file mode 100644 index 8ca27847..00000000 --- a/src/SMAPI.ModBuildConfig.Analyzer.Tests/UnitTests.cs +++ /dev/null @@ -1,140 +0,0 @@ -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.Diagnostics; -using NUnit.Framework; -using SMAPI.ModBuildConfig.Analyzer.Tests.Framework; -using StardewModdingAPI.ModBuildConfig.Analyzer; - -namespace SMAPI.ModBuildConfig.Analyzer.Tests -{ - /// Unit tests for the C# analyzers. - [TestFixture] - public class UnitTests : DiagnosticVerifier - { - /********* - ** Properties - *********/ - /// 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; - using SObject = StardewValley.Object; - - namespace SampleMod - { - class ModEntry - { - public void Entry() - { - {{test-code}} - } - } - } - "; - - /// The line number where the unit tested code is injected into . - private const int SampleCodeLine = 13; - - /// The column number where the unit tested code is injected into . - private const int SampleCodeColumn = 25; - - - /********* - ** Unit tests - *********/ - /// Test that no diagnostics are raised for an empty code block. - [TestCase] - public void EmptyCode_HasNoDiagnostics() - { - // arrange - string test = @""; - - // assert - this.VerifyCSharpDiagnostic(test); - } - - /// 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. - /// 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")] - [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")] - [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 != null);", 22, "item?.netIntField", "NetInt", "object")] - [TestCase("Item item = null; if (item.netIntProperty < 42);", 22, "item.netIntProperty", "NetInt", "int")] - [TestCase("Item item = null; if (item.netIntProperty <= 42);", 22, "item.netIntProperty", "NetInt", "int")] - [TestCase("Item item = null; if (item.netIntProperty > 42);", 22, "item.netIntProperty", "NetInt", "int")] - [TestCase("Item item = null; if (item.netIntProperty >= 42);", 22, "item.netIntProperty", "NetInt", "int")] - [TestCase("Item item = null; if (item.netIntProperty == 42);", 22, "item.netIntProperty", "NetInt", "int")] - [TestCase("Item item = null; if (item.netIntProperty != 42);", 22, "item.netIntProperty", "NetInt", "int")] - [TestCase("Item item = null; if (item?.netIntProperty != 42);", 22, "item?.netIntProperty", "NetInt", "int")] - [TestCase("Item item = null; if (item?.netIntProperty != null);", 22, "item?.netIntProperty", "NetInt", "object")] - [TestCase("Item item = null; if (item.netRefField == null);", 22, "item.netRefField", "NetRef", "object")] - [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.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")] - [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")] - public void AvoidImplicitNetFieldComparisons_RaisesDiagnostic(string codeText, int column, string expression, string fromType, string toType) - { - // arrange - string code = UnitTests.SampleProgram.Replace("{{test-code}}", codeText); - DiagnosticResult expected = new DiagnosticResult - { - Id = "SMAPI001", - Message = $"This implicitly converts '{expression}' from {fromType} to {toType}, but {fromType} has unintuitive implicit conversion rules. Consider comparing against the actual value instead to avoid bugs. See https://smapi.io/buildmsg/smapi001 for details.", - Severity = DiagnosticSeverity.Warning, - Locations = new[] { new DiagnosticResultLocation("Test0.cs", UnitTests.SampleCodeLine, UnitTests.SampleCodeColumn + column) } - }; - - // assert - 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("Item item = null; int category = item.category;", 33, "item.category", "NetInt", "Category")] - [TestCase("Item item = null; int category = (item).category;", 33, "(item).category", "NetInt", "Category")] - [TestCase("Item item = null; int category = ((Item)item).category;", 33, "((Item)item).category", "NetInt", "Category")] - [TestCase("SObject obj = null; int category = obj.category;", 35, "obj.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 - *********/ - /// Get the analyzer being tested. - protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() - { - return new NetFieldAnalyzer(); - } - } -} diff --git a/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs new file mode 100644 index 00000000..00565329 --- /dev/null +++ b/src/SMAPI.ModBuildConfig.Analyzer/ObsoleteFieldAnalyzer.cs @@ -0,0 +1,98 @@ +using System; +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 +{ + /// Detects references to a field which has been replaced. + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class ObsoleteFieldAnalyzer : DiagnosticAnalyzer + { + /********* + ** Properties + *********/ + /// Maps obsolete fields/properties to their non-obsolete equivalent. + private readonly IDictionary ReplacedFields = new Dictionary + { + // Farmer + ["StardewValley.Farmer::friendships"] = "friendshipData" + }; + + /// Describes the diagnostic rule covered by the analyzer. + private readonly IDictionary Rules = new Dictionary + { + ["SMAPI003"] = new DiagnosticDescriptor( + id: "SMAPI003", + 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.", + category: "SMAPI.CommonErrors", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: "https://smapi.io/buildmsg/smapi003" + ) + }; + + + /********* + ** Accessors + *********/ + /// The descriptors for the diagnostics that this analyzer is capable of producing. + public override ImmutableArray SupportedDiagnostics { get; } + + + /********* + ** Public methods + *********/ + /// Construct an instance. + public ObsoleteFieldAnalyzer() + { + 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) + { + // SMAPI003: avoid obsolete fields + context.RegisterSyntaxNodeAction( + this.AnalyzeObsoleteFields, + SyntaxKind.SimpleMemberAccessExpression + ); + } + + + /********* + ** Private methods + *********/ + /// Analyse a syntax node and add a diagnostic message if it references an obsolete field. + /// The analysis context. + private void AnalyzeObsoleteFields(SyntaxNodeAnalysisContext context) + { + try + { + // get reference info + MemberAccessExpressionSyntax node = (MemberAccessExpressionSyntax)context.Node; + ITypeSymbol declaringType = context.SemanticModel.GetTypeInfo(node.Expression).Type; + string propertyName = node.Name.Identifier.Text; + + // suggest replacement + for (ITypeSymbol type = declaringType; type != null; type = type.BaseType) + { + if (this.ReplacedFields.TryGetValue($"{type}::{propertyName}", out string replacement)) + { + context.ReportDiagnostic(Diagnostic.Create(this.Rules["SMAPI003"], context.Node.GetLocation(), $"{type}.{propertyName}", replacement)); + break; + } + } + } + catch (Exception ex) + { + throw new InvalidOperationException($"Failed processing expression: '{context.Node}'. Exception details: {ex.ToString().Replace('\r', ' ').Replace('\n', ' ')}"); + } + } + } +} -- cgit