diff --git a/ReadMe.md b/ReadMe.md index a521125..56daeda 100644 --- a/ReadMe.md +++ b/ReadMe.md @@ -57,6 +57,7 @@ Add the following nuget package to you project: https://www.nuget.org/packages/E | [EPC34](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC34.md) | Method return value marked with MustUseResultAttribute must be used | | [ERP021](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/ERP021.md) | Incorrect exception propagation | | [ERP022](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/ERP022.md) | Unobserved exception in a generic exception handler | +| [EPC42](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC42.md) | A member of a data contract is not serializable | ### Performance diff --git a/docs/Rules/EPC42.md b/docs/Rules/EPC42.md new file mode 100644 index 0000000..fd84f9e --- /dev/null +++ b/docs/Rules/EPC42.md @@ -0,0 +1,151 @@ +# EPC42 - A member of a data contract is not serializable + +Warns when a `[DataMember]` of a `[DataContract]` type has a type that `DataContractSerializer` +cannot serialize. + +## Description + +`DataContractSerializer` validates the object graph **lazily**: the constructor succeeds and the +failure only happens on the first serialization attempt, with an `InvalidDataContractException`. +This makes the bug very easy to miss — the code compiles, the serializer is created successfully, +and the application blows up in production the first time the payload is actually written. + +```csharp +[DataContract] +public class Config +{ + [DataMember] + public IPAddress Address { get; set; } // EPC42 +} + +var serializer = new DataContractSerializer(typeof(Config)); // works! +serializer.WriteObject(stream, config); // InvalidDataContractException +``` + +`System.Net.IPAddress` is the canonical example: it is marked with `[Serializable]` on the .NET +Framework, but **not** on .NET Core / .NET 5+. Since it also has no parameterless constructor, it +stopped being data-contract-serializable when the code was ported. `System.Net.IPEndPoint` has the +same problem. + +> Note: even a "fixed" `IPAddress` would be problematic — `IPAddress.Loopback`, `IPAddress.Any` and +> `IPAddress.None` return an instance of the private nested type `IPAddress+ReadOnlyIPAddress`, +> which is not serializable either. + +## When a type is data-contract-serializable + +A type is serializable by `DataContractSerializer` when at least one of the following holds: + +- it is marked with `[DataContract]` or `[CollectionDataContract]`; +- it is marked with `[Serializable]`, or implements `ISerializable` / `IXmlSerializable`; +- it is a primitive, a `string`, an enum, a `Guid`, a `DateTime`, a `TimeSpan` etc.; +- it is a **public** type with a **parameterless constructor**. + +The last rule is the one that is easy to violate. Note that: + +- a **non-public** (`internal`, `private` nested) POCO type is *not* serializable, even though it + compiles fine and looks perfectly reasonable; +- a **non-public parameterless constructor** *is* good enough — `private Foo() { }` works; +- collection types are not exempt: a custom collection without a default constructor fails with + `"... is an invalid collection type since it does not have a default constructor"`. + +## Records + +`record` types follow exactly the same rules, and the positional syntax is a common trap: a +positional record has no parameterless constructor, so it is **not** serializable. + +```csharp +public record PositionalRecord(int X); // no parameterless ctor +public record struct PositionalStruct(int X); // fine: a struct always has one +public record RecordWithBody { public int X { get; set; } } // fine + +[DataContract] +public class Config +{ + [DataMember] public PositionalRecord Bad { get; set; } // EPC42 + [DataMember] public PositionalStruct Ok { get; set; } + [DataMember] public RecordWithBody AlsoOk { get; set; } +} +``` + +Positional members of a `[DataContract] record` are only serialized when they are annotated with +`[property: DataMember]`, and the analyzer follows that rule: + +```csharp +[DataContract] +public record Config([property: DataMember] IPAddress Address); // EPC42 + +[DataContract] +public record Ignored(IPAddress Address); // not reported: the member is not a data member +``` + +## What is reported +For every member of a `[DataContract]` type that is marked with `[DataMember]`, the analyzer checks +the member's type, looking through arrays, `Nullable` and generic type arguments. So all of these +are reported: + +```csharp +[DataContract] +public class Config +{ + [DataMember] public IPAddress Address { get; set; } // EPC42 + [DataMember] public IPAddress[] Addresses { get; set; } // EPC42 + [DataMember] public List More { get; set; } // EPC42 + [DataMember] public Dictionary Map { get; set; } // EPC42 + [DataMember] public InternalPoco Poco { get; set; } // EPC42: the type is not public + [DataMember] public NoParameterlessCtor Value { get; set; } // EPC42: no parameterless ctor +} +``` + +## What is NOT reported + +- Members that are not marked with `[DataMember]` (including the ones marked with + `[IgnoreDataMember]`) — `DataContractSerializer` ignores them. +- Types that are not marked with `[DataContract]`. +- Members typed as an interface, an abstract class or `object`. The runtime type is unknown at + compile time, and such cases fail with a different exception (`SerializationException`) that is + solved by adding `[KnownType]`. +- Members typed as a generic type parameter. +- Delegate-typed members. +- Union-typed members. A `union` compiles to a struct whose state is invisible to + `DataContractSerializer`, so such a member is silently serialized as empty and the payload is + lost. This is a known gap: it is tracked separately and requires a Roslyn update first. +- Structs, `record struct`s and records with a parameterless constructor. +- Nested problems: if a `[DataMember]` has a POCO type and *that* type has a bad member, the + diagnostic is reported on the nested type's own declaration when it is a `[DataContract]`, but the + analyzer does not walk arbitrary object graphs. + +## How to fix + +There is no single mechanical fix, pick the one that fits: + +1. **Use a serializable surrogate.** Most common for BCL types: + + ```csharp + [DataContract] + public class Config + { + [DataMember(Name = "Address")] + private string AddressString { get; set; } + + [IgnoreDataMember] + public IPAddress Address + { + get => IPAddress.Parse(AddressString); + set => AddressString = value.ToString(); + } + } + ``` + +2. **Exclude the member** — remove `[DataMember]` (or replace it with `[IgnoreDataMember]`) if the + data does not need to travel over the wire. + +3. **Make your own type serializable** — add a (possibly private) parameterless constructor, make + the type public, or annotate it with `[DataContract]` / `[Serializable]`. + +4. **Register a surrogate** via `DataContractSerializer`'s `IDataContractSurrogate` / + `ISerializationSurrogateProvider` if you cannot change either side. In that case suppress the + diagnostic: + + ```csharp + #pragma warning disable EPC42 // A surrogate is registered for IPAddress + ``` diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/CoreAnalyzers/DataContractSerializableMemberAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/CoreAnalyzers/DataContractSerializableMemberAnalyzerTests.cs new file mode 100644 index 0000000..1b30fc7 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/CoreAnalyzers/DataContractSerializableMemberAnalyzerTests.cs @@ -0,0 +1,518 @@ +using Microsoft.CodeAnalysis.CSharp; +using NUnit.Framework; +using System.Threading.Tasks; +using VerifyCS = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< + ErrorProne.NET.CoreAnalyzers.DataContractSerializableMemberAnalyzer, + Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.CoreAnalyzers +{ + [TestFixture] + public class DataContractSerializableMemberAnalyzerTests + { + [Test] + public async Task Warn_On_IPAddress_Property() + { + string code = @" +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public class MyContract +{ + [DataMember] + public IPAddress [|Address|] { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task Warn_On_IPAddress_Field() + { + string code = @" +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public class MyContract +{ + [DataMember] + public IPAddress [|Address|]; +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task Warn_On_IPEndPoint() + { + string code = @" +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public class MyContract +{ + [DataMember] + public IPEndPoint [|EndPoint|] { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task Warn_On_Collection_Of_IPAddress() + { + string code = @" +using System.Collections.Generic; +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public class MyContract +{ + [DataMember] + public List [|Addresses|] { get; set; } + + [DataMember] + public IPAddress[] [|MoreAddresses|] { get; set; } + + [DataMember] + public Dictionary [|Map|] { get; set; } + + [DataMember] + public IEnumerable [|Sequence|] { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task Warn_On_User_Type_Without_Parameterless_Constructor() + { + string code = @" +using System.Runtime.Serialization; + +public class NoParameterlessCtor +{ + public NoParameterlessCtor(int x) { X = x; } + public int X { get; set; } +} + +[DataContract] +public class MyContract +{ + [DataMember] + public NoParameterlessCtor [|Value|] { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task Warn_On_Non_Public_Type() + { + string code = @" +using System.Runtime.Serialization; + +internal class InternalPoco +{ + public int X { get; set; } +} + +[DataContract] +internal class MyContract +{ + [DataMember] + public InternalPoco [|Value|] { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task Warn_On_Struct_In_A_Data_Contract_Struct() + { + string code = @" +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public struct MyContract +{ + [DataMember] + public IPAddress [|Address|] { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task Warn_On_Nullable_And_Nested_Generics() + { + string code = @" +using System.Collections.Generic; +using System.Runtime.Serialization; + +public struct BadStruct +{ + // The struct itself is fine, but it contains a bad member. + public int X { get; set; } +} + +public class NoCtor { public NoCtor(int x) { } } + +[DataContract] +public class MyContract +{ + [DataMember] + public List> [|Nested|] { get; set; } + + [DataMember] + public BadStruct? Fine { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarn_On_Non_DataMember_Members() + { + string code = @" +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public class MyContract +{ + [DataMember] + public string Name { get; set; } + + // Not marked with [DataMember], so it is not serialized. + public IPAddress Address { get; set; } + + [IgnoreDataMember] + public IPAddress AnotherAddress { get; set; } + + public static IPAddress StaticAddress { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarn_When_Type_Is_Not_A_DataContract() + { + string code = @" +using System.Net; + +public class NotAContract +{ + public IPAddress Address { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarn_On_Serializable_And_DataContract_Types() + { + string code = @" +using System; +using System.Collections.Generic; +using System.Runtime.Serialization; + +[Serializable] +public class SerializableNoCtor +{ + public SerializableNoCtor(int x) { } +} + +[DataContract] +public class NestedContract +{ + [DataMember] + public int X { get; set; } +} + +[DataContract] +public class MyContract +{ + [DataMember] + public SerializableNoCtor Serializable { get; set; } + + [DataMember] + public NestedContract Nested { get; set; } + + [DataMember] + public Uri Url { get; set; } + + [DataMember] + public Version Version { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarn_On_Primitives_Enums_And_Collections() + { + string code = @" +using System; +using System.Collections.Generic; +using System.Runtime.Serialization; + +public enum MyEnum { One } + +[DataContract] +public class MyContract +{ + [DataMember] + public int Number { get; set; } + + [DataMember] + public string Name { get; set; } + + [DataMember] + public int? NullableNumber { get; set; } + + [DataMember] + public MyEnum Enum { get; set; } + + [DataMember] + public Guid Id { get; set; } + + [DataMember] + public DateTime Date { get; set; } + + [DataMember] + public TimeSpan Duration { get; set; } + + [DataMember] + public byte[] Blob { get; set; } + + [DataMember] + public List Names { get; set; } + + [DataMember] + public Dictionary Map { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarn_On_Private_Parameterless_Constructor() + { + string code = @" +using System.Runtime.Serialization; + +public class PrivateCtor +{ + private PrivateCtor() { } + public static PrivateCtor Create() => new PrivateCtor(); + public int X { get; set; } +} + +[DataContract] +public class MyContract +{ + [DataMember] + public PrivateCtor Value { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarn_On_Abstract_Interface_And_Object_Members() + { + string code = @" +using System.Runtime.Serialization; + +public abstract class AbstractBase +{ + public AbstractBase(int x) { } +} + +public interface IThing { } + +[DataContract] +public class MyContract +{ + [DataMember] + public AbstractBase Base { get; set; } + + [DataMember] + public IThing Thing { get; set; } + + [DataMember] + public object Any { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task NoWarn_On_Generic_Type_Parameters() + { + string code = @" +using System.Runtime.Serialization; + +[DataContract] +public class MyContract +{ + [DataMember] + public T Value { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + + [Test] + public async Task Warn_On_Positional_Record_Member_Type() + { + // A positional record has no parameterless constructor and is not serializable. + string code = @" +using System.Runtime.Serialization; + +public record PositionalRecord(int X, string Name); + +[DataContract] +public class MyContract +{ + [DataMember] + public PositionalRecord [|Value|] { get; set; } +}"; + + await VerifyCS.VerifyAsync(code, LanguageVersion.CSharp10); + } + + [Test] + public async Task Warn_On_Bad_Member_Of_A_Record_Data_Contract() + { + string code = @" +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public record MyContract +{ + [DataMember] + public IPAddress [|Address|] { get; set; } +}"; + + await VerifyCS.VerifyAsync(code, LanguageVersion.CSharp10); + } + + [Test] + public async Task Warn_On_Bad_Positional_Member_Of_A_Record_Data_Contract() + { + string code = @" +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public record MyContract([property: DataMember] int X, [property: DataMember] IPAddress [|Address|]);"; + + await VerifyCS.VerifyAsync(code, LanguageVersion.CSharp10); + } + + [Test] + public async Task Warn_On_Bad_Member_Of_A_Record_Struct_Data_Contract() + { + string code = @" +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public record struct MyContract +{ + [DataMember] + public IPAddress [|Address|] { get; set; } +}"; + + await VerifyCS.VerifyAsync(code, LanguageVersion.CSharp10); + } + + [Test] + public async Task NoWarn_On_Record_Struct_And_Record_With_Parameterless_Constructor() + { + string code = @" +using System.Runtime.Serialization; + +public record struct PositionalRecordStruct(int X); + +public record RecordWithBody +{ + public int X { get; set; } +} + +[DataContract] +public class MyContract +{ + [DataMember] + public PositionalRecordStruct Value { get; set; } + + [DataMember] + public RecordWithBody Another { get; set; } +}"; + + await VerifyCS.VerifyAsync(code, LanguageVersion.CSharp10); + } + + [Test] + public async Task NoWarn_On_Synthesized_Record_Members() + { + // 'EqualityContract' and the other compiler-generated members must not be analyzed. + string code = @" +using System.Runtime.Serialization; + +[DataContract] +public record MyContract(int X) +{ + [DataMember] + public string Name { get; set; } +}"; + + await VerifyCS.VerifyAsync(code, LanguageVersion.CSharp10); + } + + [Test] + public async Task NoWarn_On_Positional_Record_Member_Without_DataMember() + { + // Without '[property: DataMember]' the positional member is not serialized at all. + string code = @" +using System.Net; +using System.Runtime.Serialization; + +[DataContract] +public record MyContract(IPAddress Address);"; + + await VerifyCS.VerifyAsync(code, LanguageVersion.CSharp10); + } + + [Test] + public async Task NoWarn_On_ISerializable_Implementation() + { + string code = @" +using System.Runtime.Serialization; + +public class CustomSerializable : ISerializable +{ + public CustomSerializable(int x) { } + public void GetObjectData(SerializationInfo info, StreamingContext context) { } +} + +[DataContract] +public class MyContract +{ + [DataMember] + public CustomSerializable Value { get; set; } +}"; + + await VerifyCS.VerifyAsync(code); + } + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md index 9d8592b..6826c2b 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md +++ b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md @@ -33,6 +33,7 @@ EPC38 | Async | Disabled | TaskEnumerableReEnumerationAnalyzer EPC39 | Performance | Disabled | QuadraticEnumerationAnalyzer EPC40 | Performance | Disabled | PrivateMethodMultipleEnumerationAnalyzer EPC41 | ErrorHandling | Warning | FormatMethodArgumentsAnalyzer +EPC42 | ErrorHandling | Warning | DataContractSerializableMemberAnalyzer ERP021 | ErrorHandling | Warning | ThrowExAnalyzer ERP022 | ErrorHandling | Warning | SwallowAllExceptionsAnalyzer ERP031 | Concurrency | Warning | ConcurrentCollectionAnalyzer diff --git a/src/ErrorProne.NET.CoreAnalyzers/CoreAnalyzers/DataContractSerializableMemberAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/CoreAnalyzers/DataContractSerializableMemberAnalyzer.cs new file mode 100644 index 0000000..193da62 --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/CoreAnalyzers/DataContractSerializableMemberAnalyzer.cs @@ -0,0 +1,253 @@ +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using ErrorProne.NET.Core; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace ErrorProne.NET.CoreAnalyzers +{ + /// + /// An analyzer that warns when a data member of a type marked with 'DataContractAttribute' + /// has a type that cannot be serialized by 'DataContractSerializer'. + /// + /// + /// 'DataContractSerializer' validates the object graph lazily, i.e. the constructor succeeds and + /// the failure happens on the first serialization attempt with 'InvalidDataContractException'. + /// A canonical example is 'System.Net.IPAddress' that is serializable on the .NET Framework but is + /// not serializable on .NET Core, because it is not marked with 'SerializableAttribute' and has no + /// parameterless constructor. + /// + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class DataContractSerializableMemberAnalyzer : DiagnosticAnalyzerBase + { + /// + public static DiagnosticDescriptor Rule => DiagnosticDescriptors.EPC42; + + /// + public DataContractSerializableMemberAnalyzer() + : base(Rule) + { + } + + /// + protected override void InitializeCore(AnalysisContext context) + { + context.RegisterCompilationStartAction(compilationContext => + { + var knownTypes = SerializationTypes.TryCreate(compilationContext.Compilation); + if (knownTypes is null) + { + // 'System.Runtime.Serialization' is not referenced, nothing to do. + return; + } + + compilationContext.RegisterSymbolAction(symbolContext => AnalyzeNamedType(symbolContext, knownTypes), SymbolKind.NamedType); + }); + } + + private static void AnalyzeNamedType(SymbolAnalysisContext context, SerializationTypes knownTypes) + { + var type = (INamedTypeSymbol)context.Symbol; + + // Note that this check covers more than plain classes and structs: + // a 'record' is a class ('TypeKind.Class') and a 'record struct' is a struct ('TypeKind.Struct'), + // so all the record flavors are analyzed here as well. + // The same is true for the union types: a union is compiled into a struct. + // Everything else (interfaces, enums, delegates) cannot be a data contract. + if (type.TypeKind != TypeKind.Class && type.TypeKind != TypeKind.Struct) + { + return; + } + + if (!type.HasAttribute(knownTypes.DataContractAttribute)) + { + return; + } + + foreach (var member in type.GetMembers()) + { + if (member.IsStatic || member.IsImplicitlyDeclared) + { + continue; + } + + ITypeSymbol memberType; + switch (member) + { + case IPropertySymbol property when !property.IsIndexer: + memberType = property.Type; + break; + case IFieldSymbol field when !field.IsConst: + memberType = field.Type; + break; + default: + continue; + } + + if (!member.HasAttribute(knownTypes.DataMemberAttribute)) + { + // Only the members marked with 'DataMemberAttribute' are serialized + // when the enclosing type is marked with 'DataContractAttribute'. + continue; + } + + foreach (var candidate in EnumerateTypesToCheck(memberType)) + { + if (!IsDataContractSerializable(candidate, knownTypes, out var reason)) + { + var location = member.Locations.FirstOrDefault() ?? Location.None; + context.ReportDiagnostic( + Diagnostic.Create( + Rule, + location, + $"{type.Name}.{member.Name}", + reason)); + + // Reporting a single diagnostic per member. + break; + } + } + } + } + + /// + /// Returns all the types that have to be serializable in order for to be serializable. + /// + /// + /// Arrays and 'Nullable{T}' are unwrapped and the generic arguments are checked as well, + /// because 'List{IPAddress}' or 'IPAddress[]' fail exactly the same way a plain 'IPAddress' member does. + /// + private static IEnumerable EnumerateTypesToCheck(ITypeSymbol type) + { + // The recursion always terminates, because every step strips one layer off the type. + switch (type) + { + case IArrayTypeSymbol arrayType: + return EnumerateTypesToCheck(arrayType.ElementType); + case INamedTypeSymbol nullableType when nullableType.IsNullableType() && nullableType.TypeArguments.Length == 1: + return EnumerateTypesToCheck(nullableType.TypeArguments[0]); + case INamedTypeSymbol genericType when genericType.IsGenericType: + return new[] { (ITypeSymbol)genericType }.Concat(genericType.TypeArguments.SelectMany(EnumerateTypesToCheck)); + default: + return new[] { type }; + } + } + + private static bool IsDataContractSerializable(ITypeSymbol type, SerializationTypes knownTypes, out string reason) + { + reason = string.Empty; + + switch (type.TypeKind) + { + // Note that the type parameters, pointers, function pointers and 'dynamic' are not + // 'INamedTypeSymbol' and are filtered out by the check below. + case TypeKind.Error: + case TypeKind.Enum: + // Delegates are not supported by the serializer, but they're typically used + // in a combination with 'IgnoreDataMemberAttribute' and are out of scope of this rule. + case TypeKind.Delegate: + // The runtime type of an interface-typed member is unknown at compile time. + // Such cases fail with a different exception and are solved with 'KnownTypeAttribute'. + case TypeKind.Interface: + return true; + } + + if (type.SpecialType != SpecialType.None) + { + // Primitives, 'string', 'object', 'decimal', 'DateTime' etc. + return true; + } + + if (type is not INamedTypeSymbol namedType) + { + return true; + } + + if (namedType.IsAbstract) + { + // The runtime type is unknown at compile time. See the comment for the interfaces. + return true; + } + + if (namedType.HasAttribute(knownTypes.DataContractAttribute) || + namedType.HasAttribute(knownTypes.CollectionDataContractAttribute) || + // 'SerializableAttribute' is a metadata flag and is not exposed via 'GetAttributes' + // for the types coming from metadata. + namedType.IsSerializable) + { + return true; + } + + if (namedType.ImplementsAny(knownTypes.ISerializable, knownTypes.IXmlSerializable)) + { + return true; + } + + // Falling back to the 'POCO' data contract: the type must be public + // and must have a parameterless constructor (a non-public one is fine). + var isPublic = IsPubliclyVisible(namedType); + + // Value types always have a parameterless constructor. + var hasParameterlessConstructor = + namedType.IsValueType || namedType.InstanceConstructors.Any(c => c.Parameters.Length == 0); + + if (isPublic && hasParameterlessConstructor) + { + return true; + } + + var typeName = namedType.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat); + var problem = (isPublic, hasParameterlessConstructor) switch + { + (false, false) => "is not public and has no parameterless constructor", + (false, true) => "is not public", + _ => "has no parameterless constructor", + }; + + reason = $"type '{typeName}' {problem} and is not marked with [DataContract], [CollectionDataContract] or [Serializable]"; + return false; + } + + private static bool IsPubliclyVisible(INamedTypeSymbol type) + { + for (var current = type; current is not null; current = current.ContainingType) + { + if (current.DeclaredAccessibility != Accessibility.Public && + current.DeclaredAccessibility != Accessibility.NotApplicable) + { + return false; + } + } + + return true; + } + + private sealed record SerializationTypes( + INamedTypeSymbol DataContractAttribute, + INamedTypeSymbol DataMemberAttribute, + INamedTypeSymbol? CollectionDataContractAttribute, + INamedTypeSymbol? ISerializable, + INamedTypeSymbol? IXmlSerializable) + { + public static SerializationTypes? TryCreate(Compilation compilation) + { + var provider = WellKnownTypeProvider.GetOrCreate(compilation); + var dataContractAttribute = provider.GetTypeByFullName("System.Runtime.Serialization.DataContractAttribute"); + var dataMemberAttribute = provider.GetTypeByFullName("System.Runtime.Serialization.DataMemberAttribute"); + + if (dataContractAttribute is null || dataMemberAttribute is null) + { + return null; + } + + return new SerializationTypes( + dataContractAttribute, + dataMemberAttribute, + provider.GetTypeByFullName("System.Runtime.Serialization.CollectionDataContractAttribute"), + provider.GetTypeByFullName("System.Runtime.Serialization.ISerializable"), + provider.GetTypeByFullName("System.Xml.Serialization.IXmlSerializable")); + } + } + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs index 5e95de7..cf56702 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs @@ -356,6 +356,17 @@ internal static class DiagnosticDescriptors description: "Format-string placeholders (e.g. '{0}', '{1}') do not match the arguments supplied to a user-annotated formatting method; the call would throw FormatException at runtime. Annotate formatting methods via 'dotnet_diagnostic.EPC41.format_methods' in .editorconfig (e.g. MyCorp.Logger.Log:0).", helpLinkUri: GetHelpUri(nameof(EPC41))); + /// + public static readonly DiagnosticDescriptor EPC42 = new DiagnosticDescriptor( + nameof(EPC42), + title: "A member of a data contract is not serializable", + messageFormat: "Member '{0}' cannot be serialized by DataContractSerializer because {1}", + category: ErrorHandlingCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "DataContractSerializer fails lazily (on the first serialization attempt) with 'InvalidDataContractException' when a type of a data member is not serializable. A type is serializable when it is marked with 'DataContractAttribute', 'CollectionDataContractAttribute' or 'SerializableAttribute', implements 'ISerializable'/'IXmlSerializable', or is a public type with a parameterless constructor. Types like 'System.Net.IPAddress' or 'System.Net.IPEndPoint' fail this check on .NET Core, and so do positional records.", + helpLinkUri: GetHelpUri(nameof(EPC42))); + public static string GetHelpUri(string ruleId) { return $"https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/{ruleId}.md"; diff --git a/src/ErrorProne.NET.CoreAnalyzers/SymbolExtensions.cs b/src/ErrorProne.NET.CoreAnalyzers/SymbolExtensions.cs index 31e65f9..bab4c3b 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/SymbolExtensions.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/SymbolExtensions.cs @@ -33,6 +33,32 @@ public static bool IsConstructor(this ISymbol symbol) return (symbol is IMethodSymbol methodSymbol && methodSymbol.MethodKind == MethodKind.Constructor); } + /// + /// Returns true if a given is marked with a given . + /// + /// + /// is nullable for the callers' convenience: the attribute may be + /// missing from the current compilation, and in this case no symbol can be marked with it. + /// + public static bool HasAttribute(this ISymbol symbol, INamedTypeSymbol? attributeType) + { + return attributeType is not null && + symbol.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, attributeType)); + } + + /// + /// Returns true if a given implements any of the given . + /// + /// + /// The interfaces are nullable for the callers' convenience: an interface may be missing from + /// the current compilation, and in this case no type can implement it. + /// + public static bool ImplementsAny(this ITypeSymbol type, params INamedTypeSymbol?[] interfaceTypes) + { + return type.AllInterfaces.Any( + i => interfaceTypes.Any(t => t is not null && SymbolEqualityComparer.Default.Equals(i.OriginalDefinition, t))); + } + public static bool IsDisposeMethod(this ISymbol symbol) { if (symbol is IMethodSymbol method