diff --git a/docfx/analyzers/NBMsgPack020.md b/docfx/analyzers/NBMsgPack020.md new file mode 100644 index 00000000..4364d1c1 --- /dev/null +++ b/docfx/analyzers/NBMsgPack020.md @@ -0,0 +1,37 @@ +# NBMsgPack020: `[MessagePackConverter]` type must be compatible converter + +@Nerdbank.MessagePack.MessagePackConverterAttribute should specify a type that derives from @Nerdbank.MessagePack.MessagePackConverter`1 where the type argument is the type the attribute is applied to. + +Learn more about [custom converters](../docs/custom-converters.md). + +## Example violations + +```cs +[MessagePackConverter(typeof(MyTypeConverter))] // MyTypeConverter does not derive from the correct base type +public class MyType +{ +} + +public class MyTypeConverter +{ + public MyType Deserialize(ref MessagePackReader reader, SerializationContext context) => throw new System.NotImplementedException(); + public void Serialize(ref MessagePackWriter writer, ref MyType value, SerializationContext context) => throw new System.NotImplementedException(); +} +``` + +## Resolution + +Fix the converter type to derive from @Nerdbank.MessagePack.MessagePackConverter`1. + +```cs +[MessagePackConverter(typeof(MyTypeConverter)] +class MyType +{ +} + +public class MyTypeConverter : MessagePackConverter +{ + public override MyType Deserialize(ref MessagePackReader reader, SerializationContext context) => throw new System.NotImplementedException(); + public override void Serialize(ref MessagePackWriter writer, ref MyType value, SerializationContext context) => throw new System.NotImplementedException(); +} +``` diff --git a/docfx/analyzers/NBMsgPack021.md b/docfx/analyzers/NBMsgPack021.md new file mode 100644 index 00000000..8e119286 --- /dev/null +++ b/docfx/analyzers/NBMsgPack021.md @@ -0,0 +1,40 @@ +# NBMsgPack021: `[MessagePackConverter]` type missing default constructor + +@Nerdbank.MessagePack.MessagePackConverterAttribute should specify a type that declares a public default constructor. + +Learn more about [custom converters](../docs/custom-converters.md). + +## Example violations + +In the below example, `MyTypeConverter` declares an explicit, *non-public* constructor. + +```cs +[MessagePackConverter(typeof(MyTypeConverter))] +public class MyType +{ +} + +public class MyTypeConverter : MessagePackConverter +{ + private MyTypeConverter() { } + public override MyType Deserialize(ref MessagePackReader reader, SerializationContext context) => throw new System.NotImplementedException(); + public override void Serialize(ref MessagePackWriter writer, ref MyType value, SerializationContext context) => throw new System.NotImplementedException(); +} +``` + +## Resolution + +Fix the converter type to declare a public default constructor, or remove the explicit constructor so the C# language provides a default constructor. + +```cs +[MessagePackConverter(typeof(MyTypeConverter))] +public class MyType +{ +} + +public class MyTypeConverter : MessagePackConverter +{ + public override MyType Deserialize(ref MessagePackReader reader, SerializationContext context) => throw new System.NotImplementedException(); + public override void Serialize(ref MessagePackWriter writer, ref MyType value, SerializationContext context) => throw new System.NotImplementedException(); +} +``` diff --git a/docfx/analyzers/index.md b/docfx/analyzers/index.md index 354819f8..24c1293b 100644 --- a/docfx/analyzers/index.md +++ b/docfx/analyzers/index.md @@ -13,4 +13,6 @@ Rule ID | Category | Severity | Notes [NBMsgPack010](NBMsgPack010.md) | Usage | Error | `[KnownSubType]` should specify an assignable type [NBMsgPack011](NBMsgPack011.md) | Usage | Error | `[KnownSubType]` alias must be unique [NBMsgPack012](NBMsgPack012.md) | Usage | Error | `[KnownSubType]` type must be unique -[NBMsgPack012](NBMsgPack013.md) | Usage | Error | `[KnownSubType]` type must not be an open generic +[NBMsgPack013](NBMsgPack013.md) | Usage | Error | `[KnownSubType]` type must not be an open generic +[NBMsgPack020](NBMsgPack020.md) | Usage | Error | `[MessagePackConverter]` type must be compatible converter +[NBMsgPack021](NBMsgPack021.md) | Usage | Error | `[MessagePackConverter]` type missing default constructor diff --git a/docfx/analyzers/toc.yml b/docfx/analyzers/toc.yml index 9f1fb94b..8166ff34 100644 --- a/docfx/analyzers/toc.yml +++ b/docfx/analyzers/toc.yml @@ -14,3 +14,7 @@ href: NBMsgPack012.md - name: NBMsgPack013 href: NBMsgPack013.md +- name: NBMsgPack020 + href: NBMsgPack020.md +- name: NBMsgPack021 + href: NBMsgPack021.md diff --git a/src/Nerdbank.MessagePack.Analyzers/AnalyzerReleases.Unshipped.md b/src/Nerdbank.MessagePack.Analyzers/AnalyzerReleases.Unshipped.md index d155a735..35b6b9a6 100644 --- a/src/Nerdbank.MessagePack.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/Nerdbank.MessagePack.Analyzers/AnalyzerReleases.Unshipped.md @@ -12,3 +12,5 @@ NBMsgPack010 | Usage | Error | `[KnownSubType]` should specify an assignable typ NBMsgPack011 | Usage | Error | `[KnownSubType]` alias must be unique NBMsgPack012 | Usage | Error | `[KnownSubType]` type must be unique NBMsgPack013 | Usage | Error | `[KnownSubType]` type must not be an open generic +NBMsgPack020 | Usage | Error | `[MessagePackConverter]` type must be compatible converter +NBMsgPack021 | Usage | Error | `[MessagePackConverter]` type missing default constructor diff --git a/src/Nerdbank.MessagePack.Analyzers/AnalyzerUtilities.cs b/src/Nerdbank.MessagePack.Analyzers/AnalyzerUtilities.cs index c29bbeca..6c3cf151 100644 --- a/src/Nerdbank.MessagePack.Analyzers/AnalyzerUtilities.cs +++ b/src/Nerdbank.MessagePack.Analyzers/AnalyzerUtilities.cs @@ -1,6 +1,8 @@ // Copyright (c) Andrew Arnott. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Microsoft.CodeAnalysis.CSharp.Syntax; + namespace Nerdbank.MessagePack.Analyzers; internal static class AnalyzerUtilities @@ -91,4 +93,14 @@ internal static bool IsInNamespace(ISymbol? symbol, ReadOnlySpan namespa return targetSymbol.ContainingNamespace.IsGlobalNamespace; } + + internal static Location? GetArgumentLocation(AttributeData att, int argumentIndex, CancellationToken cancellationToken) + { + if (att.ApplicationSyntaxReference?.GetSyntax(cancellationToken) is AttributeSyntax a && a.ArgumentList?.Arguments.Count >= argumentIndex) + { + return a.ArgumentList.Arguments[argumentIndex].GetLocation(); + } + + return null; + } } diff --git a/src/Nerdbank.MessagePack.Analyzers/KnownSubTypeAnalyzers.cs b/src/Nerdbank.MessagePack.Analyzers/KnownSubTypeAnalyzers.cs index 4aa95e92..dc71a947 100644 --- a/src/Nerdbank.MessagePack.Analyzers/KnownSubTypeAnalyzers.cs +++ b/src/Nerdbank.MessagePack.Analyzers/KnownSubTypeAnalyzers.cs @@ -138,13 +138,8 @@ public override void Initialize(AnalysisContext context) Location? GetArgumentLocation(int argumentIndex) { - Location? location = appliedSymbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(context.CancellationToken).GetLocation(); - if (att.ApplicationSyntaxReference?.GetSyntax(context.CancellationToken) is AttributeSyntax a && a.ArgumentList?.Arguments.Count >= argumentIndex) - { - location = a.ArgumentList.Arguments[argumentIndex].GetLocation(); - } - - return location; + return AnalyzerUtilities.GetArgumentLocation(att, argumentIndex, context.CancellationToken) + ?? appliedSymbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(context.CancellationToken).GetLocation(); } } }, diff --git a/src/Nerdbank.MessagePack.Analyzers/MessagePackConverterAttributeAnalyzer.cs b/src/Nerdbank.MessagePack.Analyzers/MessagePackConverterAttributeAnalyzer.cs new file mode 100644 index 00000000..81232f6b --- /dev/null +++ b/src/Nerdbank.MessagePack.Analyzers/MessagePackConverterAttributeAnalyzer.cs @@ -0,0 +1,79 @@ +// Copyright (c) Andrew Arnott. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Nerdbank.MessagePack.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class MessagePackConverterAttributeAnalyzer : DiagnosticAnalyzer +{ + public const string InvalidConverterTypeDiagnosticId = "NBMsgPack020"; + public const string ConverterMissingDefaultCtorDiagnosticId = "NBMsgPack021"; + + public static readonly DiagnosticDescriptor InvalidConverterTypeDescriptor = new( + id: InvalidConverterTypeDiagnosticId, + title: Strings.NBMsgPack020_Title, + messageFormat: Strings.NBMsgPack020_MessageFormat, + category: "Usage", + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + helpLinkUri: AnalyzerUtilities.GetHelpLink(InvalidConverterTypeDiagnosticId)); + + public static readonly DiagnosticDescriptor ConverterMissingDefaultCtorDescriptor = new( + id: ConverterMissingDefaultCtorDiagnosticId, + title: Strings.NBMsgPack021_Title, + messageFormat: Strings.NBMsgPack021_MessageFormat, + category: "Usage", + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + helpLinkUri: AnalyzerUtilities.GetHelpLink(ConverterMissingDefaultCtorDiagnosticId)); + + public override ImmutableArray SupportedDiagnostics => [ + InvalidConverterTypeDescriptor, + ConverterMissingDefaultCtorDescriptor, + ]; + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); + + context.RegisterCompilationStartAction(context => + { + if (!ReferenceSymbols.TryCreate(context.Compilation, out ReferenceSymbols? referenceSymbols)) + { + return; + } + + context.RegisterSymbolAction( + context => + { + var appliedType = (INamedTypeSymbol)context.Symbol; + if (appliedType.FindAttributes(referenceSymbols.MessagePackConverterAttribute).FirstOrDefault() is not { } att) + { + return; + } + + if (att.ConstructorArguments is not [{ Value: INamedTypeSymbol converterType }]) + { + return; + } + + if (!converterType.IsDerivedFrom(referenceSymbols.MessagePackConverter.Construct(appliedType))) + { + context.ReportDiagnostic(Diagnostic.Create(InvalidConverterTypeDescriptor, GetArgumentLocation(0), appliedType.Name)); + } + else if (!converterType.InstanceConstructors.Any(c => c.Parameters.IsEmpty && c.DeclaredAccessibility == Accessibility.Public)) + { + context.ReportDiagnostic(Diagnostic.Create(ConverterMissingDefaultCtorDescriptor, GetArgumentLocation(0), appliedType.Name)); + } + + Location? GetArgumentLocation(int argumentIndex) + { + return AnalyzerUtilities.GetArgumentLocation(att, argumentIndex, context.CancellationToken) + ?? appliedType.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(context.CancellationToken).GetLocation(); + } + }, + SymbolKind.NamedType); + }); + } +} diff --git a/src/Nerdbank.MessagePack.Analyzers/ReferenceSymbols.cs b/src/Nerdbank.MessagePack.Analyzers/ReferenceSymbols.cs index 142a9ae5..e3a16a83 100644 --- a/src/Nerdbank.MessagePack.Analyzers/ReferenceSymbols.cs +++ b/src/Nerdbank.MessagePack.Analyzers/ReferenceSymbols.cs @@ -7,6 +7,7 @@ namespace Nerdbank.MessagePack.Analyzers; internal record ReferenceSymbols( INamedTypeSymbol MessagePackConverter, + INamedTypeSymbol MessagePackConverterAttribute, INamedTypeSymbol KeyAttribute, INamedTypeSymbol KnownSubTypeAttribute, INamedTypeSymbol PropertyShapeAttribute) @@ -26,6 +27,13 @@ internal static bool TryCreate(Compilation compilation, [NotNullWhen(true)] out return false; } + INamedTypeSymbol? messagePackConverterAttribute = compilation.GetTypeByMetadataName("Nerdbank.MessagePack.MessagePackConverterAttribute"); + if (messagePackConverterAttribute is null) + { + referenceSymbols = null; + return false; + } + INamedTypeSymbol? keyAttribute = compilation.GetTypeByMetadataName("Nerdbank.MessagePack.KeyAttribute"); if (keyAttribute is null) { @@ -47,7 +55,7 @@ internal static bool TryCreate(Compilation compilation, [NotNullWhen(true)] out return false; } - referenceSymbols = new ReferenceSymbols(messagePackConverter, keyAttribute, knownSubTypeAttribute, propertyShapeAttribute); + referenceSymbols = new ReferenceSymbols(messagePackConverter, messagePackConverterAttribute, keyAttribute, knownSubTypeAttribute, propertyShapeAttribute); return true; } } diff --git a/src/Nerdbank.MessagePack.Analyzers/Strings.resx b/src/Nerdbank.MessagePack.Analyzers/Strings.resx index c5224947..5b9f3629 100644 --- a/src/Nerdbank.MessagePack.Analyzers/Strings.resx +++ b/src/Nerdbank.MessagePack.Analyzers/Strings.resx @@ -165,4 +165,16 @@ [KnownSubType] type must have a shape + + [MessagePackConverter] type must derive from MessagePackConverter<T> where T is the type the attribute is applied to + + + [MessagePackConverter] type must be compatible converter + + + [MessagePackConverter] type must declare a public constructor with no parameters + + + [MessagePackConverter] type missing default constructor + \ No newline at end of file diff --git a/src/Nerdbank.MessagePack/MessagePackConverterAttribute.cs b/src/Nerdbank.MessagePack/MessagePackConverterAttribute.cs new file mode 100644 index 00000000..d93d4446 --- /dev/null +++ b/src/Nerdbank.MessagePack/MessagePackConverterAttribute.cs @@ -0,0 +1,32 @@ +// Copyright (c) Andrew Arnott. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Diagnostics.CodeAnalysis; + +namespace Nerdbank.MessagePack; + +/// +/// A class applied to a custom data type to prescribe a custom +/// implementation to use for serialization. +/// +[AttributeUsage(AttributeTargets.Interface | AttributeTargets.Class | AttributeTargets.Struct, AllowMultiple = false, Inherited = false)] +public class MessagePackConverterAttribute : Attribute +{ + /// + /// Initializes a new instance of the class. + /// + /// + /// A type that implements + /// where T is a type argument matching the type to which this attribute is applied. + /// + public MessagePackConverterAttribute(Type converterType) + { + this.ConverterType = converterType; + } + + /// + /// Gets the type that implements . + /// + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] + public Type ConverterType { get; } +} diff --git a/src/Nerdbank.MessagePack/StandardVisitor.cs b/src/Nerdbank.MessagePack/StandardVisitor.cs index 7122a05a..1f827f28 100644 --- a/src/Nerdbank.MessagePack/StandardVisitor.cs +++ b/src/Nerdbank.MessagePack/StandardVisitor.cs @@ -4,6 +4,7 @@ #pragma warning disable NBMsgPackAsync using System.Collections.Frozen; +using System.Reflection; using System.Text; using Microsoft; @@ -28,6 +29,11 @@ internal class StandardVisitor(MessagePackSerializer owner) : TypeShapeVisitor, /// public override object? VisitObject(IObjectTypeShape objectShape, object? state = null) { + if (this.GetCustomConverter(objectShape) is MessagePackConverter customConverter) + { + return customConverter; + } + SubTypes? unionTypes = this.DiscoverUnionTypes(objectShape); IConstructorShape? ctorShape = objectShape.GetConstructor(); @@ -380,4 +386,19 @@ protected IMessagePackConverter GetConverter(ITypeShape shape, object? state = n Serializers = serializerData.ToFrozenDictionary(), }; } + + private MessagePackConverter? GetCustomConverter(ITypeShape typeShape) + { + if (typeShape.AttributeProvider?.GetCustomAttributes(typeof(MessagePackConverterAttribute), false).FirstOrDefault() is not MessagePackConverterAttribute customConverterAttribute) + { + return null; + } + + if (customConverterAttribute.ConverterType.GetConstructor(Type.EmptyTypes) is not ConstructorInfo ctor) + { + throw new MessagePackSerializationException($"{typeof(T).FullName} has {typeof(MessagePackConverterAttribute)} that refers to {customConverterAttribute.ConverterType.FullName} but that converter has no default constructor."); + } + + return (MessagePackConverter)ctor.Invoke(Array.Empty()); + } } diff --git a/test/Nerdbank.MessagePack.Analyzers.Tests/MessagePackConverterAttributeAnalyzerTests.cs b/test/Nerdbank.MessagePack.Analyzers.Tests/MessagePackConverterAttributeAnalyzerTests.cs new file mode 100644 index 00000000..3b78bf7f --- /dev/null +++ b/test/Nerdbank.MessagePack.Analyzers.Tests/MessagePackConverterAttributeAnalyzerTests.cs @@ -0,0 +1,96 @@ +// Copyright (c) Andrew Arnott. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using VerifyCS = CodeFixVerifier; + +public class MessagePackConverterAttributeAnalyzerTests +{ + [Fact] + public async Task NoIssues() + { + string source = /* lang=c#-test */ """ + using TypeShape; + using Nerdbank.MessagePack; + + [MessagePackConverter(typeof(MyTypeConverter))] + public class MyType + { + } + + public class MyTypeConverter : MessagePackConverter + { + public override MyType Deserialize(ref MessagePackReader reader, SerializationContext context) => throw new System.NotImplementedException(); + public override void Serialize(ref MessagePackWriter writer, ref MyType value, SerializationContext context) => throw new System.NotImplementedException(); + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task MissingPublicDefaultCtor() + { + string source = /* lang=c#-test */ """ + using TypeShape; + using Nerdbank.MessagePack; + + [MessagePackConverter({|NBMsgPack021:typeof(MyTypeConverter)|})] + public class MyType + { + } + + public class MyTypeConverter : MessagePackConverter + { + private MyTypeConverter() { } + public override MyType Deserialize(ref MessagePackReader reader, SerializationContext context) => throw new System.NotImplementedException(); + public override void Serialize(ref MessagePackWriter writer, ref MyType value, SerializationContext context) => throw new System.NotImplementedException(); + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task TypeDoesNotDeriveFromConverter() + { + string source = /* lang=c#-test */ """ + using TypeShape; + using Nerdbank.MessagePack; + + [MessagePackConverter({|NBMsgPack020:typeof(MyTypeConverter)|})] + public class MyType + { + } + + public class MyTypeConverter + { + public MyType Deserialize(ref MessagePackReader reader, SerializationContext context) => throw new System.NotImplementedException(); + public void Serialize(ref MessagePackWriter writer, ref MyType value, SerializationContext context) => throw new System.NotImplementedException(); + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task TypeDoesNotDeriveFromConverterOfMatchingType() + { + string source = /* lang=c#-test */ """ + using TypeShape; + using Nerdbank.MessagePack; + + [MessagePackConverter({|NBMsgPack020:typeof(IntConverter)|})] + public class MyType + { + } + + public class IntConverter : MessagePackConverter + { + public override int Deserialize(ref MessagePackReader reader, SerializationContext context) => throw new System.NotImplementedException(); + public override void Serialize(ref MessagePackWriter writer, ref int value, SerializationContext context) => throw new System.NotImplementedException(); + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source); + } +} diff --git a/test/Nerdbank.MessagePack.Tests/MessagePackConverterAttributeTests.cs b/test/Nerdbank.MessagePack.Tests/MessagePackConverterAttributeTests.cs new file mode 100644 index 00000000..733766bd --- /dev/null +++ b/test/Nerdbank.MessagePack.Tests/MessagePackConverterAttributeTests.cs @@ -0,0 +1,35 @@ +// Copyright (c) Andrew Arnott. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +public partial class MessagePackConverterAttributeTests(ITestOutputHelper logger) : MessagePackSerializerTestBase(logger) +{ + [Fact] + public void CustomTypeWithConverter() + { + this.AssertRoundtrip(new CustomType() { InternalProperty = "some value" }); + } + + [GenerateShape] // to allow use as a direct serialization argument + [MessagePackConverter(typeof(CustomTypeConverter))] + public partial record CustomType + { + // This property is internal so that if the auto-generated serializer were used instead of the custom one, + // the value would be dropped and the test would fail. + internal string? InternalProperty { get; set; } + + public override string ToString() => this.InternalProperty ?? "(null)"; + } + + public class CustomTypeConverter : MessagePackConverter + { + public override CustomType? Deserialize(ref MessagePackReader reader, SerializationContext context) + { + return new() { InternalProperty = reader.ReadString() }; + } + + public override void Serialize(ref MessagePackWriter writer, ref CustomType? value, SerializationContext context) + { + writer.Write(value?.InternalProperty); + } + } +}