`CA2229` conflicts with the new `SYSLIB0051`

[RenderMichael]

Analyzer

Diagnostic ID: CA2229: Implement serialization constructors

Analyzer source

Version: 8.0 preview 6

Describe the bug

In .NET 8, obsoletion warning SYSLIB0051 was introduced which added a warning to any type using Exception's .ctor(SerializationInfo, StreamingContext)

However, not implementing this .ctor lights up CA2229.

Steps To Reproduce

public class M : Exception // CA2229: Add a constructor to M with the following signature: 'protected M(SerializationInfo info, StreamingContext context)'.
{
   public M(string? message) : base(message) { }

   public M(string? message, Exception innerException) : base(message, innerException) { }

//   protected M(SerializationInfo info, StreamingContext context) : base(info, context) { }

   public M() { }
}

Expected behavior

Given the long term obsoletion strategy of binary serialization, this analyzer should probably be sunset, or perhaps only apply to .NET <8.

Actual behavior

Either CA2229 or SYSLIB0051 are triggered, depending on whether the commented code above is un-commented.

[mavasani]

Given the long term obsoletion strategy of binary serialization, this analyzer should probably be sunset, or perhaps only apply to .NET <8.

Moving to runtime repo to get this triaged.

[mavasani]

FYI @buyaa-n

[danmoseley]

Cc @GrabYourPitchforks

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Analyzer

Diagnostic ID: CA2013: Implement serialization constructors

Analyzer source

Version: 8.0 preview 6

Describe the bug

In .NET 8, obsoletion warning SYSLIB0051 was introduced which added a warning to any type implementing Exception's .ctor(SerializationInfo, StreamingContext)

However, not implementing this .ctor lights up CA2229.

Steps To Reproduce

public class M : Exception // CA2229: Add a constructor to M with the following signature: 'protected M(SerializationInfo info, StreamingContext context)'.
{
   public M(string? message) : base(message) { }

   public M(string? message, Exception innerException) : base(message, innerException) { }

//   protected M(SerializationInfo info, StreamingContext context) : base(info, context) { }

   public M() { }
}

Expected behavior

Given the long term obsoletion strategy of binary serialization, this analyzer should probably be sunset, or perhaps only apply to .NET <8.

Actual behavior

Either CA2229 or SYSLIB0051 are triggered, depending on whether the commented code above is un-commented.

Author:	RenderMichael
Assignees:	-
Labels:	area-Meta, untriaged, needs-area-label
Milestone:	-

[buyaa-n]

Given the long term obsoletion strategy of binary serialization, this analyzer should probably be sunset, or perhaps only apply to .NET <8.

Removing the analyzer sounds good to me, what you think @GrabYourPitchforks, and probably need to ported to 8.0 CC @jeffhandley

[jeffhandley]

Since CA2229 indicates the following, I'm uncertain we can just delete the rule without potentially causing harm to folks.

Do not suppress a violation of the rule. The type will not be deserializable, and will not function in many scenarios.

I presume we'd also need to reconsider CA2237: Mark ISerializable types with SerializableAttribute and CA2235: Mark all non-serializable fields as well.

What do you recommend, @GrabYourPitchforks?

Thanks a bunch for reporting this, @RenderMichael!!

[RenderMichael]

Thanks a bunch for reporting this, @RenderMichael!!

Always glad! This issue would come up for every user with CA2229 enabled and a user-defined exception, so hopefully this can be resolved before .NET 8 ships

[GrabYourPitchforks]

CA2235 strikes me as still useful. If you've slapped [Serializable] on a type but introduced members which are not serializable, then you've probably made a mistake. The ways to correct this are: (a) remove [Serializable]; or (b) change each of the fields to be a serializable type; or (c) customize the serialization logic via implementing ISerializable.

CA2237 should probably be changed to say that if you have implemented the ISerializable interface directly, you should also annotate yourself as [Serializable]. That is, the rule should not fire if the interface is introduced not at your layer, but rather at a layer somewhere else in your type hierarchy. That way you could still subclass ISerializable types and not have to worry about implementing serialization logic yourself.