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

Allow GetProperties to order results by declaration order #46272

Closed
Tracked by #44655
techfan101 opened this issue Dec 20, 2020 · 23 comments · Fixed by #69506
Closed
Tracked by #44655

Allow GetProperties to order results by declaration order #46272

techfan101 opened this issue Dec 20, 2020 · 23 comments · Fixed by #69506
Labels
area-System.Reflection enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@techfan101
Copy link

techfan101 commented Dec 20, 2020

Background and Motivation

Currently, Type.GetProperties returns the properties in an indeterminate order. While this works fine for the majority of use-cases, it can complicate serialization.

When serializing a simple data model, it is often desirable to serialize the properties in the order in which they are declared within the class. For many serialized formats (e.g. JSON), ordering is not particularly important. For others (e.g. CSV), ordering can be critical.

Currently, there is no reliable mechanism available to support serializing by declaration order. Judging by the amount of discussion on this topic available on the web, it seems to be a somewhat common issue.

Searching the web for a solution, yields two common suggestions: 1) decorating each property with an "order" attribute or 2) ordering the properties by MemberInfo.MetadataToken. Each of these suggestions has some drawbacks.

Order Attribute Drawbacks

Requiring the consumer of the serialization to decorate each property with an attribute creates additional work for the consumer and adds clutter to simple data models. It also introduces design considerations. What should the behavior be if the attribute is missing? What should the behavior be if two attributes specify the same order? What should the user do if the data model comes from a third-party? Obviously, there are relatively simple workarounds for each of these design considerations as well.

MetadataToken Drawbacks

Currently, ordering by the metadata token seems to closely mirror declaration order. However, there is no guarantee that this will continue to be the case.

Proposed API

There are a few possible changes to the API that might address these concerns:

  1. exposing a simple MemberInfo.DeclarationOrder field,
  2. adding a Type.GetOrderedProperties() method, or
  3. documenting that MemberInfo.MetadataToken can be relied upon for this purpose.

Personally, I prefer option number 1. However, I'd be happy with any reliable mechanism that allows a developer to visit properties in declaration order. Please seriously consider adding such a mechanism. Thank you for your time.

@techfan101 techfan101 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 20, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 20, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas
Copy link
Member

jkotas commented Dec 21, 2020

There are a few possible changes to the API that might address these concerns

This can be fixed without any API changes. We can simply fix the existing API to always return the declaration order.

@jkotas jkotas added enhancement Product code improvement that does NOT require public API changes/additions and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Dec 21, 2020
@GrabYourPitchforks
Copy link
Member

I don't think changing the behavior of the existing API is going to help the ecosystem at large. It might specifically help people targeting .NET 6, but it won't help library authors targeting netstandard or cross-compiling. And I suspect it'll lead to issues where data structures serialized using .NET Framework or .NET 5 or .NET 6 produce different output, possibly breaking applications which are sensitive to the order of properties.

If the runtime is able to figure out the order of declaration, then this means that the C# compiler must have emitted something into the metadata that allows us to determine this. I think we should instead document what this metadata is, work with the C# team to guarantee that it reflects the member ordering in the original source file (with some type of accounting for partial classes), and tell library authors exactly how they can consume this metadata in their own code.

@jkotas
Copy link
Member

jkotas commented Dec 21, 2020

It might specifically help people targeting .NET 6,

Yep, anything we do in the runtime for this will only help people targeting future versions. I doubt that we would be able to justify producing partial OOB for this.

data structures serialized using .NET Framework or .NET 5 or .NET 6 produce different output

This is the status quo. We do not guarantee the order returned today, and it varies based on number of factors. BTW: We had several compat issues in .NET Framework due to apps taking dependency on the specific order, and the order changing in a servicing update.

C# compiler must have emitted something into the metadata that allows us to determine this.

The metadata order produced by C# generally matches the declaration order today. I am not sure whether it is written down somewhere, but features like COM interop depend on this. It is also what makes the workaround using Metadata token work.

@techfan101
Copy link
Author

@GrabYourPitchforks and @jkotas, thank you for your time. I agree with all of your points.

Personally, I'm a "have your cake and eat it too" kind of guy :) Perhaps, we need both a short-term solution to help with current/past versions and a long-term solution that's easier to describe/use?

In the short-term, clarity on the limitations of "metadata token" ordering would help with my project. Currently, I provide an "order" attribute and encourage consumers to use it. When the attribute is missing or there is a tie, I use the metadata token as the tie-breaker, in the hopes that (in most cases) this will mirror declaration order.

While I document this approach, uncertainties about its limitations and exact behavior, lead to a less than ideal description coupled with a "no guarantees" warning.

As mentioned, I'm completely OK with any approach/workaround as long as it's supported. I don't mind jumping through a few hoops. Though, longer term, for the sake of the framework, a nice "clean" solution would probably be best :)

Thanks again for anything you can do to help short and/or long term.

@GrabYourPitchforks
Copy link
Member

It is also what makes the workaround using Metadata token work.

Then can we document that using the metadata token in this manner is guaranteed to reflect the declaration order?

@GrabYourPitchforks
Copy link
Member

I should clarify - IMO there's nothing inherently wrong with making GetProperties return a different order (assuming we can do it quickly). But given how long it'll take for libraries and apps to target .NET 6+, I'd prefer for us to have a supported, documented downlevel solution as well.

@jkotas
Copy link
Member

jkotas commented Dec 22, 2020

using the metadata token in this manner is guaranteed to reflect the declaration order?

I think it would ok to say that is acceptable workaround that works most of the time. Guaranteeing it both retroactively and for future may need more homework:

@techfan101
Copy link
Author

@jkotas thanks again for your time. It seems like those caveats would exclude using the metadata token workaround as something reliable for the general purpose of this issue (getting properties in declaration order). I hope a supported solution becomes available.

For a moment, I'm going to be selfish and focus on my very specific requirements. I'm mostly looking for predictability. So, let's say a consumer of my class library creates the class MyClass (without "order" attributes). They build their project and are happy with metadata token order (which may not be declaration order).

Can I safely assume that when they later rebuild their project the ordering will remain the same, providing that they don't change the code for MyClass?

For the moment, we'll forget future versions of the .NET framework. I'll cross that bridge when I come to it...hoping that a future version has a better-supported mechanism and that I am smart enough to use it :)

@steveharter
Copy link
Member

Working on the System.Text.Json serializer, yes this in an issue. I have noticed differences in tests between Windows and Linux which I have not researched. Due to this, our cross-platform tests cannot assume a specific ordering.

Also, prior reflection can affect ordering in .NET Core:

    class Program
    {
        static void Main(string[] args)
        {
            // Uncommenting this makes B come first:
            // typeof(Foo).GetProperty("B");

            foreach (PropertyInfo p in typeof(Foo).GetProperties())
            {
                Console.WriteLine(p.Name);
            }
        }
    }

    public class Foo
    {
        public virtual int A { get; set; }
        public virtual int B { get; set; }
    }

@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Feb 22, 2021
@joperezr
Copy link
Member

If/When we do this, @steveharter mentioned that we should test it across different platforms as he has seen that there are sometimes order inconsistencies across different platforms.

@steveharter
Copy link
Member

For brainstorming purposes, these could be considered possible implementations pending additional research\testing:

  • Call GetProperties() when calling GetProperty() to fully populate the cache to ensure ordering later. Likely will have an impact on perf for the single-use cases.
  • In GetProperties() mix in the previously cached properties in the appropriate location instead of just calling .Add() for the new ones. I think we want to preserve the original cached single instances, thus "mix in" and not "replace".

See also #45986 for fields.

@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 23, 2021
@buyaa-n buyaa-n removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Mar 2, 2021
@Phylum123
Copy link

Any word on this?

@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Dec 27, 2021
@buyaa-n buyaa-n modified the milestones: Future, 7.0.0 Jan 5, 2022
@buyaa-n buyaa-n removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Jan 5, 2022
@steveharter
Copy link
Member

steveharter commented Jan 26, 2022

For additional brainstorming purposes, consider the reference implementation of System.Text.Json deterministic property ordering. It works via an integer-based attribute applied to a property or field. This approach, however, requires the user to own the Type (in order to add the attribute) and requires it to be done at design-time so it is not generalized for all cases.

If we wanted true ordering semantics that is independent of compiler and runtime whims, additional metadata is required that defines the order and\or a strategy to use (e.g. sort alphabetically).

This could be implemented as an additional layer or option on top of the existing reflection code to not affect perf for those that don't need it.

@ericstj
Copy link
Member

ericstj commented Apr 20, 2022

I'm not sure if this is particularly specific to Properties or not, but it seems like predictable deterministic ordering is something we'd want to have consistently in refelection for all APIs. So if we have problems like this elsewhere (GetTypes, GetMethods, GetCustomAttributes, etc) we'd want to at least consider them when looking at this.

@buyaa-n
Copy link
Contributor

buyaa-n commented Apr 20, 2022

Yes we were talking to apply a deterministic order for all members of Type like GetMethods, GetFields, GetProperties etc. Not sure if GetMembers() should also be covered, seems there is no any ordering info kept for those as each member cached in different tables.

@jkotas
Copy link
Member

jkotas commented Apr 21, 2022

Not sure if GetMembers() should also be covered

GetMembers concatenates all other lists, so it should just work.

@svick
Copy link
Contributor

svick commented Apr 22, 2022

@jkotas

Not sure if GetMembers() should also be covered

GetMembers concatenates all other lists, so it should just work.

I think that in an ideal world, GetMembers() should return members in the order in which they were declared. I.e. for class C { int f; void M() {} } it returns "f, M", but for class C { void M() {} int f; }, it returns "M, f".

Concatenating lists does not give you that. But, as far as I know, the runtime has no information about this kind of order, since compilers don't encode it in the assembly in any way. And inventing some scheme for encoding this information is probably not worth it.

@steveharter
Copy link
Member

Note this issue also affects binary serialization which is based on fields. During a draft PR of mine I encountered test issues that are failing due to field ordering being different. That binary serialization test is System.Runtime.Serialization.Formatters.Tests.BinaryFormatterTests.ValidateAgainstBlobs

If the test I provided earlier above for properties is modified for fields, the issue repros. However, here's a repro for binary serialization:

using System.Runtime.Serialization.Formatters.Binary;

// Uncommenting this makes order change:
//FieldInfo f = typeof(System.Net.Cookie).GetField("m_name", BindingFlags.NonPublic | BindingFlags.Instance);

using (MemoryStream ms = new())
{
    BinaryFormatter formatter = new BinaryFormatter();
    formatter.Serialize(ms, new System.Net.Cookie());
    string encodedObject = Convert.ToBase64String(ms.ToArray());
    Console.WriteLine(encodedObject);
}

// default: AAEAAAD/////AQAAAAAAAAAMAgAAAElTeXN0ZW0sIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5BQEAAAARU3lzdGVtLk5ldC5Db29raWUVAAAACW1fY29tbWVudAxtX2NvbW1lbnRVcmkPbV9jb29raWVWYXJpYW50CW1fZGlzY2FyZAhtX2RvbWFpbhFtX2RvbWFpbl9pbXBsaWNpdAltX2V4cGlyZXMGbV9uYW1lBm1fcGF0aA9tX3BhdGhfaW1wbGljaXQGbV9wb3J0D21fcG9ydF9pbXBsaWNpdAttX3BvcnRfbGlzdAhtX3NlY3VyZQptX2h0dHBPbmx5C21fdGltZVN0YW1wB21fdmFsdWUJbV92ZXJzaW9uC21fZG9tYWluS2V5D0lzUXVvdGVkVmVyc2lvbg5Jc1F1b3RlZERvbWFpbgEEBAABAAABAQABAAcAAAABAAEAAApTeXN0ZW0uVXJpAgAAABhTeXN0ZW0uTmV0LkNvb2tpZVZhcmlhbnQCAAAAAQENAQEIAQENCAEBAgAAAAYDAAAAAAoF/P///xhTeXN0ZW0uTmV0LkNvb2tpZVZhcmlhbnQBAAAAB3ZhbHVlX18ACAIAAAABAAAAAAkDAAAAAQAAAAAAAAAACQMAAAAJAwAAAAEJAwAAAAEKAAAXlW650CbaiAkDAAAAAAAAAAkDAAAAAAAL
// GetField:AAEAAAD/////AQAAAAAAAAAMAgAAAElTeXN0ZW0sIFZlcnNpb249NC4wLjAuMCwgQ3VsdHVyZT1uZXV0cmFsLCBQdWJsaWNLZXlUb2tlbj1iNzdhNWM1NjE5MzRlMDg5BQEAAAARU3lzdGVtLk5ldC5Db29raWUVAAAABm1fbmFtZQltX2NvbW1lbnQMbV9jb21tZW50VXJpD21fY29va2llVmFyaWFudAltX2Rpc2NhcmQIbV9kb21haW4RbV9kb21haW5faW1wbGljaXQJbV9leHBpcmVzBm1fcGF0aA9tX3BhdGhfaW1wbGljaXQGbV9wb3J0D21fcG9ydF9pbXBsaWNpdAttX3BvcnRfbGlzdAhtX3NlY3VyZQptX2h0dHBPbmx5C21fdGltZVN0YW1wB21fdmFsdWUJbV92ZXJzaW9uC21fZG9tYWluS2V5D0lzUXVvdGVkVmVyc2lvbg5Jc1F1b3RlZERvbWFpbgEBBAQAAQAAAQABAAcAAAABAAEAAApTeXN0ZW0uVXJpAgAAABhTeXN0ZW0uTmV0LkNvb2tpZVZhcmlhbnQCAAAAAQENAQEIAQENCAEBAgAAAAYDAAAAAAkDAAAACgX8////GFN5c3RlbS5OZXQuQ29va2llVmFyaWFudAEAAAAHdmFsdWVfXwAIAgAAAAEAAAAACQMAAAABAAAAAAAAAAAJAwAAAAEJAwAAAAEKAACVuHPD0CbaiAkDAAAAAAAAAAkDAAAAAAAL

@buyaa-n
Copy link
Contributor

buyaa-n commented May 27, 2022

I'm not sure if this is particularly specific to Properties or not, but it seems like predictable deterministic ordering is something we'd want to have consistently in refelection for all APIs. So if we have problems like this elsewhere (GetTypes, GetMethods, GetCustomAttributes, etc) we'd want to at least consider them when looking at this.

The fix we are doing here guarantees individual Type members call (GetMethods, GetProperties, GetNestedTypes etc) returned in declared (native) order, but it will not guarantee the order for hot reload and any other scenarios that would modify Type structure at runtime.

Also as mentioned above Type.GetMembers() will return members in order grouped by member type, there is no way to get mixed members in exact declared order.

public override MemberInfo[] GetMembers(BindingFlags bindingAttr)
{
ListBuilder<MethodInfo> methods = GetMethodCandidates(null, GenericParameterCountAny, bindingAttr, CallingConventions.Any, null, false);
ListBuilder<ConstructorInfo> constructors = GetConstructorCandidates(null, bindingAttr, CallingConventions.Any, null, false);
ListBuilder<PropertyInfo> properties = GetPropertyCandidates(null, bindingAttr, null, false);
ListBuilder<EventInfo> events = GetEventCandidates(null, bindingAttr, false);
ListBuilder<FieldInfo> fields = GetFieldCandidates(null, bindingAttr, false);
ListBuilder<Type> nestedTypes = GetNestedTypeCandidates(null, bindingAttr, false);

Other calls that are not cached internally like GetCustomAttributes , GetCustomAttributesData looks already producing the list in order. It seems to me that the declared order doesn't make much sense for members of Assembly/Module, except Assembly.GetTypes() which is already retuning the types in a file in declared order.

Repository owner moved this from Future to Done in Triage POD for Reflection, META, etc. May 31, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 31, 2022
@KalleOlaviNiemitalo
Copy link

IIRC, the C++/CLI compiler did not preserve the declaration order of enum constants from source to metadata. If this is still the case, please be careful when you update the documentation of the reflection API, to avoid giving the impression that the order is preserved from source in all languages.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jun 2, 2022

The fix did not cover Type.GetEnumNames nor Type.GetEnumValues path, so in case we update docs those APIs docs will not updated.

FWIW: I see the Enum constants are coming in declared order unless they have assigned values in different order, in that case they are ordered by the value but I don't think that is an issue or it should keep the declared order.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 3, 2022
@steveharter
Copy link
Member

@techfan101 can you try this out and report back? Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection enhancement Product code improvement that does NOT require public API changes/additions
Projects
No open projects
Development

Successfully merging a pull request may close this issue.