dotnet / codeformatter

Tool that uses Roslyn to automatically rewrite the source to follow our coding styles
MIT License
1.24k stars 245 forks source link

Running CodeFormatter renames private fields that have [DataMember] which breaks serialization #191

Open jaredmoo opened 8 years ago

jaredmoo commented 8 years ago

We were bit by a nasty issue that running CodeFormatter broke serialization of a type that had [DataMember] on a private field. CodeFormatter renamed the field, which caused the serialization format to change, so our pre-CodeFormatter and post-CodeFormatter serialization was incompatible.

Below program demonstrates the issue:

using System;
using System.Runtime.Serialization;
using System.Xml;

namespace DataMemberSample
{
    class Program
    {
        static void Main(string[] args)
        {
            var myData = new MyData(5);

            var dataContractSerializer = new DataContractSerializer(typeof(MyData));
            using (var xw = XmlWriter.Create(Console.Out, new XmlWriterSettings { Indent = true }))
            {
                dataContractSerializer.WriteObject(xw, myData);
            }
        }
    }

    [DataContract]
    class MyData
    {
        [DataMember]
        private int MyInteger;

        public MyData(int myInteger)
        {
            MyInteger = myInteger;
        }
    }
}

Before CodeFormatter, produces the below output:

<?xml version="1.0" encoding="IBM437"?>
<MyData xmlns:i="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://schemas.datacontract.org/2004/07/DataMemberSample">
<MyInteger>5</MyInteger>
</MyData>

Running CodeFormatter renames MyInteger to _myInteger, so now it produces this output:

<?xml version="1.0" encoding="IBM437"?>
<MyData xmlns:i="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://schemas.datacontract.org/2004/07/DataMemberSample">
<_myInteger>5</_myInteger>
</MyData>

Of course we should have had tests that detected this issue, but I should feel confident that CodeFormatter will not affect code functionality.

AndyGerlicher commented 8 years ago

+1 on this. In MSBuild we use binary serialization all over (uhg) and changing private fields on [Serializable] classes changes the contract for serialization. It would be good to detect both of these and be able to disable renaming in those cases (even if it's an extra option).

ghost commented 8 years ago

Another irritant: renaming fields that are part of an interop struct and are intentionally named to match their C++ counterparts. Opened a separate issue for this:

https://github.com/dotnet/codeformatter/issues/230