Merge pull request #41 from AArnott/nbmsgpack003

Add NBMsgPack003 analyzer: `[Key]` index must be unique

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.

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

docfx/analyzers/toc.yml

@@ -4,3 +4,5 @@
   href: NBMsgPack001.md
 - name: NBMsgPack002
   href: NBMsgPack002.md
+- name: NBMsgPack003
+  href: NBMsgPack003.md

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

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<DiagnosticDescriptor> 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<int, ISymbol>? 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;
 			}
 		}

src/Nerdbank.MessagePack.Analyzers/Strings.resx

@@ -129,4 +129,10 @@
   <data name="NBMsgPack002_Title" xml:space="preserve">
     <value>Avoid [Key] on non-serialized members</value>
   </data>
+  <data name="NBMsgPack003_MessageFormat" xml:space="preserve">
+    <value>Each index specified in [Key(index)] must be unique, but this member's index collides with that of {0}</value>
+  </data>
+  <data name="NBMsgPack003_Title" xml:space="preserve">
+    <value>[Key] index must be unique</value>
+  </data>
 </root>

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()
 	{