RuntimeModule::ResolveLiteralField has unnecessary call to Type.GetFields()

[eerhardt]

We have the following code:

runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs

Lines 159 to 168 in 53c1619

	Type declaringType = ResolveType(tkDeclaringType, genericTypeArguments, genericMethodArguments);
	declaringType.GetFields();
	try
	{
	return declaringType.GetField(fieldName,
	BindingFlags.Static | BindingFlags.Instance |
	BindingFlags.Public | BindingFlags.NonPublic |
	BindingFlags.DeclaredOnly);

The call to declaringType.GetFields(); doesn't do anything with the resulting array of FieldInfos, and looks like it can be removed. If we can, we should remove it. If we can't remove it, we should add a comment why that line is necessary.

One thought I had was maybe the call to GetFields() "warmed up" the Type's cache of fields. But I would hope that the call to GetField(fieldName) would be valid even without the previous call to GetFields().

I searched the internal source code history back to December 2003, and it appears this line has always existed since this method was first written. So my initial suspicion is that it is a left-over line of code when the feature we first developed.

---

Another related issue is that I can't seem to find out how RuntimeModule::ResolveLiteralField would ever actually be called. The only call-site is in the public ResolveField method, but only when a MissingMethodException is thrown. The problem is - I didn't find a place inside the try-catch that throws a MissingMethodException. Maybe it is coming from the clr runtime?

If we are going to remove the above line, we should try to find a way to unit test this code to get it executed to ensure it still works as necessary. If we can't find a way to exercise this code, maybe the whole method should be removed?

[joperezr]

cc: @steveharter

[steveharter]

It is unfortunate the line is not commented. The only case I can think of is that it is an attempt on making field access predicable later when calling GetFields(). When calling the singular GetField() only that field is added to the cache, and when the plural GetFields() is called, it appends to that cache meaning the order of the various GetField() calls impact the ordering of GetFields().

However, if this is the reason for that line of code, it may not have the desired semantics of somewhere else called GetField() previously.

See also #46272 for the issue of having consistent ordering of properties.

A sample:

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

            GetMembers();
        }

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

            foreach (FieldInfo f in typeof(Foo).GetFields())
            {
                Console.WriteLine(f.Name);
            }
        }
    }

    public class Foo
    {
        public int _a;
        public int _b;
        public int A { get; set; }
        public int B { get; set; }
    }

[buyaa-n]

We can remove that row after #46272 implemented, keeping this issue as a reminder for that

[buyaa-n]

Fixed with #77497