Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make serializer work nicely with autoproperties #864

Open
Barsonax opened this issue Jul 7, 2020 · 3 comments
Open

Make serializer work nicely with autoproperties #864

Barsonax opened this issue Jul 7, 2020 · 3 comments
Labels
Cleanup Improving form, keeping function Discussion Nothing to be done until decided otherwise Nice2Have Beneficial, but only very slightly so

Comments

@Barsonax
Copy link
Member

Barsonax commented Jul 7, 2020

Summary

Currently we try to avoid using auto properties because when serializing a instance the serializer will use the name of the backing fields which have a ugly name like <Foo>k__BackingField.

Consider changing the serializers to understand the relationship between the backing fields and the properties to allow them to handle these in a nicer way

Analysis

  • Allows the use of auto properties without downsides in both our code and end users code. Which can clean up some files alot.
  • 3th party types that use autoproperties will end up with the ugly backingfield name in serialized form.
  • In the future features like records might be added to C# which might also make use of properties?
  • We have to find a reliable way to find the relationship between backingfields and properties.

Simple example of getting the backingfield or the autoproperty. No idea if this is stable enough, needs more research.

        const string prefix = "<";
        const string suffix = ">k__BackingField";

        public static FieldInfo GetBackingField(PropertyInfo propertyInfo)
        {
            var backingFieldName = $"{prefix}{propertyInfo.Name}{suffix}";
            return propertyInfo.DeclaringType.GetFields(BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Static).FirstOrDefault(x => x.Name == backingFieldName && x.IsDefined(typeof(CompilerGeneratedAttribute)));
        }

        public static PropertyInfo GetAutoProperty(FieldInfo field)
        {
            if (field.Name.StartsWith(prefix) && field.Name.EndsWith(suffix) && field.IsDefined(typeof(CompilerGeneratedAttribute), true))
            {
                var propertyName = field.Name[prefix.Length..^suffix.Length];
                if (propertyName != null)
                {
                    return field.DeclaringType.GetProperties(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static)
                             .Where(x => x.SetMethod != null && x.SetMethod.IsDefined(typeof(CompilerGeneratedAttribute)))
                             .Where(x => x.GetMethod != null && x.GetMethod.IsDefined(typeof(CompilerGeneratedAttribute)))
                             .FirstOrDefault(x => x.Name == propertyName);
                }
            }
            return null;
        }
@Barsonax Barsonax added Nice2Have Beneficial, but only very slightly so Cleanup Improving form, keeping function Discussion Nothing to be done until decided otherwise labels Jul 7, 2020
@ilexp
Copy link
Member

ilexp commented Jul 10, 2020

Some notes:

  • Regardless of implementation, our serializers should still support reading and an ugly backing field name as-is, if it is present in the serialized data stream.
  • We'd also need to expose the name-translation logic via public API, because if a field is renamed or its type changed, a AssignFieldError will be invoked and user-handled. Usually they'll just check for a hardcoded name, but there may be cases where reflection will be involved and name translation required.
  • There appears to be a CompilerGenerated attribute on both the properties and fields, which we could potentially use to filter out candidates for any matching.
  • With Roslyn around, we probably shouldn't rely too much on specific compiler behavior. Alternate compilers might handle stuff differently, and even mainstream C# compiler development is pretty fast these days. If there's a recent spec somewhere on exact behavior, we might be safe though.
  • Maybe we could re-phrase the problem into "nice-ify" variable names for human-readable serialization formats? This would deal with auto-properties for now, but would conceptually not be limited to them and allow us to evolve the rules over time with slightly less of a headache to match exactly what a compiler is doing.
  • It's an open question whether this is a general "serializer" feature in Duality, or just a bit of utility code that is used by the XmlSerializer and potentially others that might write a human-readable format in the future. I'm currently leaning towards the former though, since it reduces the mental load for implementers of specific serialization formats.
  • I'd probably go for adding a change here in SerializeType, replacing FieldInfo with a struct that has a field info, but also a "serialized name". I'd not make this specific for auto-properties, or even mention them.
  • We could even go with a primary serialized name, and a list of fallback names in the future (separate issue) to enable easily usable FormerlySerializedAs user attributes without having to rely on implementing a SerializeErrorHandler there.

@Barsonax
Copy link
Member Author

Barsonax commented Jul 10, 2020

There appears to be a CompilerGenerated attribute on both the properties and fields, which we could potentially use to filter out candidates for any matching.

Oh also on the property? Thats nice. Can make it quite robust then by just checking if theres a attribute on both sides.

EDIT: updated the example on this.

@ilexp
Copy link
Member

ilexp commented Jul 11, 2020

Not the property itself, but its internal getter and setter methods get one. Still, could be useful enough for some sort of verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Improving form, keeping function Discussion Nothing to be done until decided otherwise Nice2Have Beneficial, but only very slightly so
Development

No branches or pull requests

2 participants