ISerializable and inheritance, proper usage, CA2236

Question

I have a question regarding the correct implementation of ISerializable in the inherited class.

I had two classes, AbstractBaseClass and its implementation BaseClass. After the implementation of the FinalClass which derives from the BaseClass I'm getting the CA warning:

CA2236 Call base class methods on ISerializable types Method 'FinalClass.FinalClass(SerializationInfo, StreamingContext)' should be modified to call its base class implementation.

What I do not understand is why should I call the base class if the extracted info.GetString("FileName") and info.GetString("Password") are enough to create the instance of the FinalClass?

Questions:

1) Could someone please explain it to me?

2) Do I have any design problem if I leave it as is and just supress the warning?

Thanks

The simplified code sample is:

public abstract class AbstractBaseClass : ISerializable
{
    [SecurityPermission(SecurityAction.Demand, SerializationFormatter = true)]
    public virtual void GetObjectData(SerializationInfo info, StreamingContext context)
    {
        info.AddValue ...
        //...
    }

    protected AbstractBaseClass() {  }
    protected AbstractBaseClass(SerializationInfo info, StreamingContext context)
    {
        if (info.GetInt32("Version") >= 1)
        {
            //...
        }
    }
}

public class BaseClass : AbstractBaseClass, ISerializable
{
    public BaseClass() : base() { }

    public override void GetObjectData(SerializationInfo info, StreamingContext context)
    {
        base.GetObjectData(info, context);
        info.AddValue ...
    }

    protected BaseClass(SerializationInfo info, StreamingContext context)
        : base(info, context)
    {
        if (info.GetInt32("Version") >= 1)
        {
            Connection = new MyConnection();
        }
    }
}

public class FinalClass : BaseClass
{
    public string FileName { get; private set; }
    public string Password { get; private set; }

    public FinalClass(string fileName, string password)
        : base()
    {
        FileName = fileName;
        Password = password;
    }

    protected FinalClass(SerializationInfo info, StreamingContext context)
        : this(info.GetString("FileName"), info.GetString("Password")) { }

    public override void GetObjectData(SerializationInfo info, StreamingContext context)
    {
        base.GetObjectData(info, context);
        info.AddValue("FileName", FileName);
        info.AddValue("Password", Password);
    }
}

Answer 1

Because, in general, that would be sound advice. Should you ever change something in the base implementation, calling base would ensure that everything will keep working.

The compiler doesn't have the same knowledge as you. You know it's not necessary to call base, the compiler doesn't. The compiler does know that it's not illegal what you're doing, so it just emits a warning, telling you that there's some code that might require your attention (and warnings always should get your attention).

If this is really what you want, suppress the warning, but I personally would call the base constructor, just like the compiler suggests.

What you are doing now is not wrong perse. It's perfectly valid C# code, but the compiler decided to issue a warning because it "knows" (for simplicity's sake) about how ISerializable in combination with a serialization constructor is usually implemented.

Let's say in a next iteration BaseClass adds some property of its own, the Foo property, and assume that we want that property (de-)serialized. You would probably implement the necessary code for deserialization in the BaseClass(SerializationInfo, StreamingContext) constructor

public class BaseClass : AbstractBaseClass, ISerializable
{
    public int Foo { get; set; }

    protected BaseClass(SerializationInfo info, StreamingContext context)
        : base(info, context)
    {
        // ...
        Foo = info.GetInt32("Foo");
        // ...
  }
}

In your current setup, when deserializing FinalClass, the Foo property of BaseClass would not be deserialized, since you decided to call the this(string fileName, string password) constructor, instead of base(SerializationInfo, StreamingContext) constructor. And that's the scenario that this warning is all about. The setup is not future proof, and any addition to either AbstractBaseClass or BaseClass in the future that should also be deserialized in the constructor will in your current implementation not be deserialized.

So yes, you could say that the current implementation is a design mistake, although I would probably more likely call it a wrong implementation of the design.