Roslyn-analyzers: CA2235 warning on primitive data types

[SWarnberg]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

Example: v2.6.2-beta2

Diagnostic ID

CA2235

Repro steps

1. Create a class decorated with the Serializable attribute.
2. Add an auto-property of type string.

Expected behavior

No warnings shown since all .NET primitive types are serializable by defualt.

Actual behavior

A CA2235 warning is shown:
Warning CA2235 Field Name is a member of type MyClass which is serializable but is of type string which is not serializable

[mavasani]

This should get fixed with https://github.com/dotnet/roslyn-analyzers/pull/1758

[mavasani]

Fixed with #1758
Note that the fix will be in 2.9.XXX packages.

[Yanal-Yves]

Hello,

Do you know when 2.9.XXX packages will be available on NuGet? The latest version I can see is 2.6.2.

[carlreinke]

Using version 2.9.3, I am still able to reproduce this issue.

using System;

[Serializable]
public class C
{
    public string F;
}

Field F is a member of type C which is serializable but is of type string which is not serializable

[StefanOssendorf]

We are experiencing the same problem with .Net Core 3 (Preview) and FxCop 2.9.3.
Here's a minimal working example with more information (installed dotnet core version, used visual studio version): https://github.com/StefanOssendorf/CA2235-MWE-Core3

[StefanOssendorf]

Ping @mavasani
Should we open a new issue for this?

[mavasani]

Seems that the fix still doesn't work for netcore projects.

[StefanOssendorf]

Will this fix be made available with the 2.9.4 package release or in a later one?

[mavasani]

Yes, 2.9.4 release should have this fix. You can also try out https://dotnet.myget.org/feed/roslyn-analyzers/package/nuget/Microsoft.CodeAnalysis.FxCopAnalyzers/2.9.4-beta2.19352.2+2cf856f3 meanwhile.

[sbeaudoin]

Still occur in official version 2.9.4.

[Serializable()]
[DesignerCategory("code")]
[XmlType(AnonymousType = true)]
[XmlRoot("MediaContainer", Namespace = "", IsNullable = false)]
public sealed class LibraryArtistsHeader
{
...
        [XmlAttribute("thumb")]
        public string Thumb { get; set; }

        [XmlAttribute("title1")]
        public string LibraryName { get; set; }

        [XmlAttribute("title2")]
        public string SelectionName { get; set; }

        [XmlAttribute("viewGroup")]
        public string ViewGroup { get; set; }
...
}

CA2235 Field Thumb is a member of type LibraryArtistsHeader which is serializable but is of type string which is not serializable

CA2235 Field LibraryName is a member of type LibraryArtistsHeader which is serializable but is of type string which is not serializable

Same thing for the two other properties.

[sbeaudoin]

Occur also for DateTime :

[Serializable]
public sealed class DeviceProfile
{
...
        [XmlAttribute("lastSyncedAt")]
        public DateTime IntermediaryLastSyncedAt { get; set; }
...
}

[sbeaudoin]

...and TimeSpan.

[mavasani]

The core issue seems to be that the analyzer is using the compiler API INamedTypeSymbol.IsSerializable that @jcouv added to determine if a named type in metadata has Serializable bit set. This seems to work perfectly fine for NetFramework projects as these primitive types (string, int, DateTime, TimeSpan, etc.) in msorlib have this bit set. However, it seems these same primitive types in netstandard.dll don't have this bit set.

I am wondering if this is related to https://github.com/dotnet/corefx/issues/19119 - @stephentoub to shed some light on whether the serializable bit not being set in metadata for primitive types in netstandard.dll is by design or just pending port work.

[stephentoub]

We have by-design not included [Serializable] in reference assemblies for .NET Core or .NET Standard, under the premise that they're only meaningful at run-time and not at design-time, and because we've been trying effectively to deprecate the feature, only keeping around runtime serialization for compat/porting purposes. We've generally also not included attributes (pseudo or otherwise) in the ref assemblies when they're meant to only have meaning at run-time. I do see, however, that the reference assemblies for .NET Framework include such attribution. I'm inclined to stick with our decision and not add such attribution to the refs, but @jkotas, @terrajobst, @ericstj, any conflicting opinions?

[mavasani]

@stephentoub Given that design choice, any suggestions on how to implement CA2235 for netcore/netstandard projects? Seems we cannot rely on the compiler API INamedTypeSymbol.IsSerializable for named types in metadata...

[stephentoub]

any suggestions on how to implement CA2235 for netcore/netstandard projects?

If the goal is to solve this just for primitives, the rule can special-case primitives, which are all special-cased anyway in BinaryFormatter. For types beyond that, I don't have a great suggestion; you could hardcode knowledge of additional types, turn false positives into false negatives by just not warning about any framework types, etc.
cc: @ViktorHofer

[mavasani]

We already special case the primitive types as serializable, but then got reports on DateTime and TimeSpan not being handled. I can special those as well, but I am afraid it will not be a complete solution.

[terrajobst]

I'm inclined to stick with our decision and not add such attribution to the refs, but @jkotas, @terrajobst, @ericstj, any conflicting opinions?

I'd say it depends. If there is compelling reason to expose this information to tooling (analyzers, compilers) then I think exposing this in the reference assemblies makes sense. I'm undecided whether supporting CA2235 is compelling enough.

Given that design choice, any suggestions on how to implement CA2235 for netcore/netstandard projects?

I'd say in today's state you can't.

[mavasani]

I'm undecided whether supporting CA2235 is compelling enough.

For now, I'll add logic to CA2235 analyzer to bail out for netcore/netstandard projects.

[terrajobst]

That seems reasonable, especially because we try to steer people away from BinaryFormatter.

[StefanOssendorf]

@mavasani Any ETA for a fix for this?

[mavasani]

@StefanOssendorf I am going to work on a fix to bail out completely from the analyzer on netcore/netstandard projects. I would recommend that meanwhile you disable this rule ID for your project with /nowarn or a ruleset entry.