From d14fb25cd8840be82735196698cf2ecab6c8e532 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Tue, 5 Nov 2024 23:18:41 -0700 Subject: [PATCH] Add NBMsgPack003 analyzer: `[Key]` index must be unique --- docfx/analyzers/NBMsgPack003.md | 41 +++++++++++ docfx/analyzers/index.md | 1 + docfx/analyzers/toc.yml | 2 + .../AnalyzerReleases.Unshipped.md | 1 + .../KeyAttributeUseAnalyzer.cs | 43 +++++++++++- .../Strings.resx | 6 ++ .../KeyAttributeUseAnalyzerTests.cs | 70 +++++++++++++++++++ 7 files changed, 162 insertions(+), 2 deletions(-) create mode 100644 docfx/analyzers/NBMsgPack003.md diff --git a/docfx/analyzers/NBMsgPack003.md b/docfx/analyzers/NBMsgPack003.md new file mode 100644 index 00000000..b67c711c --- /dev/null +++ b/docfx/analyzers/NBMsgPack003.md @@ -0,0 +1,41 @@ +# NBMsgPack003: `[Key]` index must be unique + +@Nerdbank.MessagePack.KeyAttribute must have unique indexes provided across all members of a type, including base types. + +This is because for a given object, the indexes determine the array index that will contain the value for a property. + +## Example violations + +Non-unique within a class: + +```cs +class MyType +{ + [Key(0)] + public int Property1 { get; set; } + + [Key(0)] + public int Property2 { get; set; } +} +``` + +Non-unique across a type hierarchy: + +```cs +class BaseType +{ + [Key(0)] + public int BaseProperty { get; set; } +} + +class DerivedType : BaseType +{ + [Key(0)] // This must be a different index than BaseType.BaseProperty + public int DerivedProperty { get; set; } +} +``` + +## Resolution + +Reassign indexes so that each member has a unique value assigned. +When `[Key]` is used across a type hierarchy, lower indexes are typically assigned to base types and higher indexes to derived types. diff --git a/docfx/analyzers/index.md b/docfx/analyzers/index.md index 84430309..60757915 100644 --- a/docfx/analyzers/index.md +++ b/docfx/analyzers/index.md @@ -9,3 +9,4 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- [NBMsgPack001](NBMsgPack001.md) | Usage | Error | Apply `[Key]` consistently across members [NBMsgPack002](NBMsgPack002.md) | Usage | Warning | Avoid `[Key]` on non-serialized members +[NBMsgPack003](NBMsgPack003.md) | Usage | Error | `[Key]` index must be unique diff --git a/docfx/analyzers/toc.yml b/docfx/analyzers/toc.yml index d453298e..dd53db44 100644 --- a/docfx/analyzers/toc.yml +++ b/docfx/analyzers/toc.yml @@ -4,3 +4,5 @@ href: NBMsgPack001.md - name: NBMsgPack002 href: NBMsgPack002.md +- name: NBMsgPack003 + href: NBMsgPack003.md diff --git a/src/Nerdbank.MessagePack.Analyzers/AnalyzerReleases.Unshipped.md b/src/Nerdbank.MessagePack.Analyzers/AnalyzerReleases.Unshipped.md index aff1c730..c87458c4 100644 --- a/src/Nerdbank.MessagePack.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/Nerdbank.MessagePack.Analyzers/AnalyzerReleases.Unshipped.md @@ -7,3 +7,4 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- NBMsgPack001 | Usage | Error | Apply `[Key]` consistently across members NBMsgPack002 | Usage | Warning | Avoid `[Key]` on non-serialized members +NBMsgPack003 | Usage | Error | `[Key]` index must be unique diff --git a/src/Nerdbank.MessagePack.Analyzers/KeyAttributeUseAnalyzer.cs b/src/Nerdbank.MessagePack.Analyzers/KeyAttributeUseAnalyzer.cs index 3887190f..ed1a6ca0 100644 --- a/src/Nerdbank.MessagePack.Analyzers/KeyAttributeUseAnalyzer.cs +++ b/src/Nerdbank.MessagePack.Analyzers/KeyAttributeUseAnalyzer.cs @@ -8,6 +8,7 @@ public class KeyAttributeUseAnalyzer : DiagnosticAnalyzer { public const string InconsistentUseDiagnosticId = "NBMsgPack001"; public const string KeyOnNonSerializedMemberDiagnosticId = "NBMsgPack002"; + public const string NonUniqueKeysDiagnosticId = "NBMsgPack003"; public static readonly DiagnosticDescriptor InconsistentUseDescriptor = new( id: InconsistentUseDiagnosticId, @@ -27,9 +28,19 @@ public class KeyAttributeUseAnalyzer : DiagnosticAnalyzer isEnabledByDefault: true, helpLinkUri: AnalyzerUtilities.GetHelpLink(KeyOnNonSerializedMemberDiagnosticId)); + public static readonly DiagnosticDescriptor NonUniqueKeysDescriptor = new( + id: NonUniqueKeysDiagnosticId, + title: Strings.NBMsgPack003_Title, + messageFormat: Strings.NBMsgPack003_MessageFormat, + category: "Usage", + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + helpLinkUri: AnalyzerUtilities.GetHelpLink(NonUniqueKeysDiagnosticId)); + public override ImmutableArray SupportedDiagnostics => [ InconsistentUseDescriptor, KeyOnNonSerializedMemberDescriptor, + NonUniqueKeysDescriptor, ]; public override void Initialize(AnalysisContext context) @@ -57,12 +68,13 @@ private void SearchForInconsistentKeyUsage(SymbolAnalysisContext context, Refere { ITypeSymbol typeSymbol = (ITypeSymbol)context.Symbol; bool? keyAttributeApplied = null; + Dictionary? keysAssigned = null; foreach (ISymbol memberSymbol in typeSymbol.GetMembers()) { switch (memberSymbol) { - case IPropertySymbol propertySymbol: - case IFieldSymbol fieldSymbol: + case IPropertySymbol: + case IFieldSymbol: AttributeData? keyAttribute = memberSymbol.FindAttributes(referenceSymbols.KeyAttribute).FirstOrDefault(); if (!this.IsMemberSerialized(memberSymbol, referenceSymbols)) { @@ -85,6 +97,33 @@ private void SearchForInconsistentKeyUsage(SymbolAnalysisContext context, Refere context.ReportDiagnostic(Diagnostic.Create(InconsistentUseDescriptor, memberSymbol.Locations.First())); } + if (keyAttribute is not null) + { + keysAssigned ??= new(); + if (keyAttribute.ConstructorArguments is [{ Value: int index }]) + { + if (keysAssigned.TryGetValue(index, out ISymbol? priorUser)) + { + Location? location = keyAttribute.ApplicationSyntaxReference?.GetSyntax(context.CancellationToken).GetLocation() ?? memberSymbol.Locations.First(); + Location[]? addlLocations = null; + if (priorUser.DeclaringSyntaxReferences is [SyntaxReference priorLocation, ..]) + { + addlLocations = [priorLocation.GetSyntax(context.CancellationToken).GetLocation()]; + } + + context.ReportDiagnostic(Diagnostic.Create( + NonUniqueKeysDescriptor, + location, + addlLocations, + priorUser.Name)); + } + else + { + keysAssigned.Add(index, memberSymbol); + } + } + } + break; } } diff --git a/src/Nerdbank.MessagePack.Analyzers/Strings.resx b/src/Nerdbank.MessagePack.Analyzers/Strings.resx index f327c08d..6ee03985 100644 --- a/src/Nerdbank.MessagePack.Analyzers/Strings.resx +++ b/src/Nerdbank.MessagePack.Analyzers/Strings.resx @@ -129,4 +129,10 @@ Avoid [Key] on non-serialized members + + Each index specified in [Key(index)] must be unique, but this member's index collides with that of {0} + + + [Key] index must be unique + \ No newline at end of file diff --git a/test/Nerdbank.MessagePack.Analyzers.Tests/KeyAttributeUseAnalyzerTests.cs b/test/Nerdbank.MessagePack.Analyzers.Tests/KeyAttributeUseAnalyzerTests.cs index 4dd61d7d..5d11f2e1 100644 --- a/test/Nerdbank.MessagePack.Analyzers.Tests/KeyAttributeUseAnalyzerTests.cs +++ b/test/Nerdbank.MessagePack.Analyzers.Tests/KeyAttributeUseAnalyzerTests.cs @@ -29,6 +29,52 @@ public class MyType await VerifyCS.VerifyAnalyzerAsync(source); } + [Fact] + public async Task KeyReuseInOneClass() + { + string source = /* lang=c#-test */ """ + using TypeShape; + using Nerdbank.MessagePack; + + [GenerateShape] + public class MyType + { + [Key(0)] + public int MyProperty1 { get; set; } + + [{|NBMsgPack003:Key(0)|}] + public int MyProperty2 { get; set; } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact(Skip = "Not yet passing.")] + public async Task KeyReuseAcrossClassHierarchy() + { + string source = /* lang=c#-test */ """ + using TypeShape; + using Nerdbank.MessagePack; + + [GenerateShape] + public class MyBaseType + { + [Key(0)] + public int MyProperty1 { get; set; } + } + + [GenerateShape] + public class MyType : MyBaseType + { + [{|NBMsgPack003:Key(0)|}] + public int MyProperty2 { get; set; } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source); + } + [Fact] public async Task MissingKey() { @@ -49,6 +95,30 @@ public class MyType await VerifyCS.VerifyAnalyzerAsync(source); } + [Fact(Skip = "Not yet passing.")] + public async Task MissingKeyOnBaseType() + { + string source = /* lang=c#-test */ """ + using TypeShape; + using Nerdbank.MessagePack; + + [GenerateShape] + public class MyBaseType + { + public int MyProperty1 { get; set; } + } + + [GenerateShape] + public class MyType : {|NBMsgPack001:MyBaseType|} + { + [Key(1)] + public int MyProperty2 { get; set; } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source); + } + [Fact] public async Task KeyOnNonSerializedInternalProperty() {