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

[ghost]

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.

[jeffhandley]

Regarding how to resolve this conflict, we will need to consider folks who have explicitly turned CA2229 into an error. If we keep the analyzer but change its default severity, they'll still be stuck. We could augment the SYSLIB0051 message to say CA2229 can be disabled. Or, we could change the analyzer to not apply to them anymore regardless of their config. I defer to @buyaa-n and/or @stephentoub on the best fix.

[RenderMichael]

The analyzer severity could be tied to the feature switch for binary serialization

Ultimately, this does not need a detailed analysis because types like ISerializable and [Serializable] are being obsoleted in .NET 9.

[buyaa-n]

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.

That warning probably only matter for to .Net framework users + .Net core users that turned on the switch for binary serialization, it seems Net framework users could use the NetAnalyzers from nuget, in that case I guess they would still want to the diagnostics, correct me if I am wrong @mavasani @stephentoub.

The analyzer severity could be tied to the feature switch for binary serialization

Interesting, but not sure if we could check within analyzer if a user turned on a feature switch at compile time CC @mavasani

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

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].

Looks CA2235 doesn't work properly on .NET Core/netstandard (because the [Serializable] attribute not included in reference assemblies), therefore CA2235 is bail out on .NET Core/netstandard. Both analyzers CA2235 and CA2237 are disabled by default and marked as candidate for removal.

Is that would change your suggestion @GrabYourPitchforks?

[buyaa-n]

Solution proposal:

• For CA2229, we are going to remove it as suggested from our experts, as We have been fine to delete other security and serialization analyzers that are no longer valid on modern .net versions. We can always ask users to stay on older versions of NuGet package if they really care so much about such deleted rules

• For CA2235 and CA2237 as they have already disabled by default and marked as candidate for removal leaving them as is, they could be reviewed/handled with Review CandidateForRemovals rules and remove in .NET 9 later

Please let me know if you have other thoughts/comments @GrabYourPitchforks @jeffhandley @mavasani @RenderMichael @stephentoub.