Precisely track reflected on fields

[MichalStrehovsky]

The AOT compiler used simple logic to decide what fields should be kept/generated: if a type is considered constructed, generate all fields. This works, but it's not very efficient.

With this change I'm introducing precise tracking of each field that should be accessible from reflection at runtime. The fields are represented by new nodes within the dependency graph.

We track fields on individual generic instantiations and ensure we end up in a consistent state where if e.g. we decided Class<int>.Foo should be reflection accessible and Class<double> is used elsewhere in the program, Class<double>.Foo is also reflection accessible. This matches how IL Linker thinks about reflectability where genericness doesn't matter. We could be more optimal here, but various suppressions in framework rely on this logic. Additional reflectable fields only cost tens of bytes.

I had to update various places within the compiler that didn't bother specifying field dependencies because they didn't matter in the past.

Added a couple new tests that test the various invariants.

This saves 2% in size on a dotnet new webapi template project with IlcTrimMetadata on. It is a small regression with IlcTrimMetadata off because we actually now track more things for the reflectable field: previously we would not make sure the field is reflection-accessible at runtime. We need a TypeHandle for the field type for it to be usable. As a potential future optimization, we could look into allowing reflection to see "unconstructed" TypeHandles (and use that one). Reflection is currently not allowed to see unconstructed TypeHandles to prevent people falling into a RuntimeHelpers.AllocateUninitializedObject(someField.FieldType.TypeHandle) trap that has a bad failure mode right now. Once we fix the failure mode, we could potentially allow it.

Cc @dotnet/ilc-contrib

[vitek-karas]

Looks good - but I don't know enough of the runtime side to tell if this will keep enough info around for the runtime to work correctly (I mean the tests work, but for a review).

[vitek-karas]

Why not just remove this branch and let the if immediately below do this? It seems to be doing the same thing.

[MichalStrehovsky]

It's part of a if/else cascade - we only need to add the TypeNonGCStaticsSymbol if there's a class constructor, or the field is non-GC static. There's no quick way to check if a field is non-GC static - one has to ask the "is it RVA/ThreadStatic/GCStatic" questions first.

The best we could do here is change this else block to needsNonGcStaticBase = true; and delete the dependencies.Add line but then the reason string will be wrong.

[vitek-karas]

I understand the comment, but my question is why do we need to explicitly preserve the metadata for the fields - I would expect that if the app uses them we would see their usage directly. Or is this because annotations/suppressions in framework assume this? (If so I think it's worth a comment about that).

[MichalStrehovsky]

The fields are never referenced from code. Basically, an enum looks like this:

classMyEnum:Enum{privateint_value;publicconstintValue1=1;publicconstintValue2=2;}

One cannot do ldsfld MyEnum.Value1 in IL. It's lowered into ldc.i4.1.

Added comment.

[vitek-karas]

I know this is based on the existing code, but it would look "nicer" like this:

if(objisReflectableFieldNodereflectableFieldNode)

Maybe even change the whole thing into a switch?

[MichalStrehovsky]

I tend to follow the existing style in the file and don't change surrounding style as part of unrelated changes. It makes history/git blame tidier. We could reformat in a single pass if it's a problem.

[vitek-karas]

So literal fields have special handling in reflection code? Meaning they don't need any runtime support, it's all done via metadata, right?
(and we assume that since it's literal its type is primitive/string and thus we don't need to explicitly preserve its type either).

[MichalStrehovsky]

Literal fields (const/enum values) only exist in metadata. They don't have a "value" that is accessible through IL. Reflection GetValue reads the value from the metadata instead of reading some location in memory (like we do for other fields). There's no location in memory to worry about.

[MichalStrehovsky]

The AOT compiler used simple logic to decide what fields should be kept/generated: if a type is considered constructed, generate all fields. This works, but it's not very efficient.

With this change I'm introducing precise tracking of each field that should be accessible from reflection at runtime. The fields are represented by new nodes within the dependency graph.

We track fields on individual generic instantiations and ensure we end up in a consistent state where if e.g. we decided Class<int>.Foo should be reflection accessible and Class<double> is used elsewhere in the program, Class<double>.Foo is also reflection accessible. This matches how IL Linker thinks about reflectability where genericness doesn't matter. We could be more optimal here, but various suppressions in framework rely on this logic. Additional reflectable fields only cost tens of bytes.

I had to update various places within the compiler that didn't bother specifying field dependencies because they didn't matter in the past.

Added a couple new tests that test the various invariants.

This saves 2% in size on a dotnet new webapi template project with IlcTrimMetadata on. It is a small regression with IlcTrimMetadata off because we actually now track more things for the reflectable field: previously we would not make sure the field is reflection-accessible at runtime. We need a TypeHandle for the field type for it to be usable. As a potential future optimization, we could look into allowing reflection to see "unconstructed" TypeHandles (and use that one). Reflection is currently not allowed to see unconstructed TypeHandles to prevent people falling into a RuntimeHelpers.AllocateUninitializedObject(someField.FieldType.TypeHandle) trap that has a bad failure mode right now. Once we fix the failure mode, we could potentially allow it.

Cc @dotnet/ilc-contrib

[vitek-karas]

Looks good - but I don't know enough of the runtime side to tell if this will keep enough info around for the runtime to work correctly (I mean the tests work, but for a review).

[vitek-karas]

Why not just remove this branch and let the if immediately below do this? It seems to be doing the same thing.

[MichalStrehovsky]

It's part of a if/else cascade - we only need to add the TypeNonGCStaticsSymbol if there's a class constructor, or the field is non-GC static. There's no quick way to check if a field is non-GC static - one has to ask the "is it RVA/ThreadStatic/GCStatic" questions first.

The best we could do here is change this else block to needsNonGcStaticBase = true; and delete the dependencies.Add line but then the reason string will be wrong.

[vitek-karas]

I understand the comment, but my question is why do we need to explicitly preserve the metadata for the fields - I would expect that if the app uses them we would see their usage directly. Or is this because annotations/suppressions in framework assume this? (If so I think it's worth a comment about that).

[MichalStrehovsky]

The fields are never referenced from code. Basically, an enum looks like this:

classMyEnum:Enum{privateint_value;publicconstintValue1=1;publicconstintValue2=2;}

One cannot do ldsfld MyEnum.Value1 in IL. It's lowered into ldc.i4.1.

Added comment.

[vitek-karas]

I know this is based on the existing code, but it would look "nicer" like this:

if(objisReflectableFieldNodereflectableFieldNode)

Maybe even change the whole thing into a switch?

[MichalStrehovsky]

I tend to follow the existing style in the file and don't change surrounding style as part of unrelated changes. It makes history/git blame tidier. We could reformat in a single pass if it's a problem.

[vitek-karas]

So literal fields have special handling in reflection code? Meaning they don't need any runtime support, it's all done via metadata, right?
(and we assume that since it's literal its type is primitive/string and thus we don't need to explicitly preserve its type either).

[MichalStrehovsky]

Literal fields (const/enum values) only exist in metadata. They don't have a "value" that is accessible through IL. Reflection GetValue reads the value from the metadata instead of reading some location in memory (like we do for other fields). There's no location in memory to worry about.

[MichalStrehovsky]

The AOT compiler used simple logic to decide what fields should be kept/generated: if a type is considered constructed, generate all fields. This works, but it's not very efficient.

With this change I'm introducing precise tracking of each field that should be accessible from reflection at runtime. The fields are represented by new nodes within the dependency graph.

We track fields on individual generic instantiations and ensure we end up in a consistent state where if e.g. we decided Class<int>.Foo should be reflection accessible and Class<double> is used elsewhere in the program, Class<double>.Foo is also reflection accessible. This matches how IL Linker thinks about reflectability where genericness doesn't matter. We could be more optimal here, but various suppressions in framework rely on this logic. Additional reflectable fields only cost tens of bytes.

I had to update various places within the compiler that didn't bother specifying field dependencies because they didn't matter in the past.

Added a couple new tests that test the various invariants.

This saves 2% in size on a dotnet new webapi template project with IlcTrimMetadata on. It is a small regression with IlcTrimMetadata off because we actually now track more things for the reflectable field: previously we would not make sure the field is reflection-accessible at runtime. We need a TypeHandle for the field type for it to be usable. As a potential future optimization, we could look into allowing reflection to see "unconstructed" TypeHandles (and use that one). Reflection is currently not allowed to see unconstructed TypeHandles to prevent people falling into a RuntimeHelpers.AllocateUninitializedObject(someField.FieldType.TypeHandle) trap that has a bad failure mode right now. Once we fix the failure mode, we could potentially allow it.

Cc @dotnet/ilc-contrib

[vitek-karas]

Looks good - but I don't know enough of the runtime side to tell if this will keep enough info around for the runtime to work correctly (I mean the tests work, but for a review).

[vitek-karas]

Why not just remove this branch and let the if immediately below do this? It seems to be doing the same thing.

[MichalStrehovsky]

It's part of a if/else cascade - we only need to add the TypeNonGCStaticsSymbol if there's a class constructor, or the field is non-GC static. There's no quick way to check if a field is non-GC static - one has to ask the "is it RVA/ThreadStatic/GCStatic" questions first.

The best we could do here is change this else block to needsNonGcStaticBase = true; and delete the dependencies.Add line but then the reason string will be wrong.

[vitek-karas]

I understand the comment, but my question is why do we need to explicitly preserve the metadata for the fields - I would expect that if the app uses them we would see their usage directly. Or is this because annotations/suppressions in framework assume this? (If so I think it's worth a comment about that).

[MichalStrehovsky]

The fields are never referenced from code. Basically, an enum looks like this:

classMyEnum:Enum{privateint_value;publicconstintValue1=1;publicconstintValue2=2;}

One cannot do ldsfld MyEnum.Value1 in IL. It's lowered into ldc.i4.1.

Added comment.

[vitek-karas]

I know this is based on the existing code, but it would look "nicer" like this:

if(objisReflectableFieldNodereflectableFieldNode)

Maybe even change the whole thing into a switch?

[MichalStrehovsky]

I tend to follow the existing style in the file and don't change surrounding style as part of unrelated changes. It makes history/git blame tidier. We could reformat in a single pass if it's a problem.

[vitek-karas]

So literal fields have special handling in reflection code? Meaning they don't need any runtime support, it's all done via metadata, right?
(and we assume that since it's literal its type is primitive/string and thus we don't need to explicitly preserve its type either).

[MichalStrehovsky]

Literal fields (const/enum values) only exist in metadata. They don't have a "value" that is accessible through IL. Reflection GetValue reads the value from the metadata instead of reading some location in memory (like we do for other fields). There's no location in memory to worry about.

[MichalStrehovsky]

The AOT compiler used simple logic to decide what fields should be kept/generated: if a type is considered constructed, generate all fields. This works, but it's not very efficient.

With this change I'm introducing precise tracking of each field that should be accessible from reflection at runtime. The fields are represented by new nodes within the dependency graph.

We track fields on individual generic instantiations and ensure we end up in a consistent state where if e.g. we decided Class<int>.Foo should be reflection accessible and Class<double> is used elsewhere in the program, Class<double>.Foo is also reflection accessible. This matches how IL Linker thinks about reflectability where genericness doesn't matter. We could be more optimal here, but various suppressions in framework rely on this logic. Additional reflectable fields only cost tens of bytes.

I had to update various places within the compiler that didn't bother specifying field dependencies because they didn't matter in the past.

Added a couple new tests that test the various invariants.

This saves 2% in size on a dotnet new webapi template project with IlcTrimMetadata on. It is a small regression with IlcTrimMetadata off because we actually now track more things for the reflectable field: previously we would not make sure the field is reflection-accessible at runtime. We need a TypeHandle for the field type for it to be usable. As a potential future optimization, we could look into allowing reflection to see "unconstructed" TypeHandles (and use that one). Reflection is currently not allowed to see unconstructed TypeHandles to prevent people falling into a RuntimeHelpers.AllocateUninitializedObject(someField.FieldType.TypeHandle) trap that has a bad failure mode right now. Once we fix the failure mode, we could potentially allow it.

Cc @dotnet/ilc-contrib

[vitek-karas]

Looks good - but I don't know enough of the runtime side to tell if this will keep enough info around for the runtime to work correctly (I mean the tests work, but for a review).

[vitek-karas]

Why not just remove this branch and let the if immediately below do this? It seems to be doing the same thing.

[MichalStrehovsky]

It's part of a if/else cascade - we only need to add the TypeNonGCStaticsSymbol if there's a class constructor, or the field is non-GC static. There's no quick way to check if a field is non-GC static - one has to ask the "is it RVA/ThreadStatic/GCStatic" questions first.

The best we could do here is change this else block to needsNonGcStaticBase = true; and delete the dependencies.Add line but then the reason string will be wrong.

[vitek-karas]

I understand the comment, but my question is why do we need to explicitly preserve the metadata for the fields - I would expect that if the app uses them we would see their usage directly. Or is this because annotations/suppressions in framework assume this? (If so I think it's worth a comment about that).

[MichalStrehovsky]

The fields are never referenced from code. Basically, an enum looks like this:

classMyEnum:Enum{privateint_value;publicconstintValue1=1;publicconstintValue2=2;}

One cannot do ldsfld MyEnum.Value1 in IL. It's lowered into ldc.i4.1.

Added comment.

[vitek-karas]

I know this is based on the existing code, but it would look "nicer" like this:

if(objisReflectableFieldNodereflectableFieldNode)

Maybe even change the whole thing into a switch?

[MichalStrehovsky]

I tend to follow the existing style in the file and don't change surrounding style as part of unrelated changes. It makes history/git blame tidier. We could reformat in a single pass if it's a problem.

[vitek-karas]

So literal fields have special handling in reflection code? Meaning they don't need any runtime support, it's all done via metadata, right?
(and we assume that since it's literal its type is primitive/string and thus we don't need to explicitly preserve its type either).

[MichalStrehovsky]

Literal fields (const/enum values) only exist in metadata. They don't have a "value" that is accessible through IL. Reflection GetValue reads the value from the metadata instead of reading some location in memory (like we do for other fields). There's no location in memory to worry about.

[MichalStrehovsky]

The AOT compiler used simple logic to decide what fields should be kept/generated: if a type is considered constructed, generate all fields. This works, but it's not very efficient.

With this change I'm introducing precise tracking of each field that should be accessible from reflection at runtime. The fields are represented by new nodes within the dependency graph.

We track fields on individual generic instantiations and ensure we end up in a consistent state where if e.g. we decided Class<int>.Foo should be reflection accessible and Class<double> is used elsewhere in the program, Class<double>.Foo is also reflection accessible. This matches how IL Linker thinks about reflectability where genericness doesn't matter. We could be more optimal here, but various suppressions in framework rely on this logic. Additional reflectable fields only cost tens of bytes.

I had to update various places within the compiler that didn't bother specifying field dependencies because they didn't matter in the past.

Added a couple new tests that test the various invariants.

This saves 2% in size on a dotnet new webapi template project with IlcTrimMetadata on. It is a small regression with IlcTrimMetadata off because we actually now track more things for the reflectable field: previously we would not make sure the field is reflection-accessible at runtime. We need a TypeHandle for the field type for it to be usable. As a potential future optimization, we could look into allowing reflection to see "unconstructed" TypeHandles (and use that one). Reflection is currently not allowed to see unconstructed TypeHandles to prevent people falling into a RuntimeHelpers.AllocateUninitializedObject(someField.FieldType.TypeHandle) trap that has a bad failure mode right now. Once we fix the failure mode, we could potentially allow it.

Cc @dotnet/ilc-contrib

[vitek-karas]

Looks good - but I don't know enough of the runtime side to tell if this will keep enough info around for the runtime to work correctly (I mean the tests work, but for a review).

[vitek-karas]

Why not just remove this branch and let the if immediately below do this? It seems to be doing the same thing.

[MichalStrehovsky]

It's part of a if/else cascade - we only need to add the TypeNonGCStaticsSymbol if there's a class constructor, or the field is non-GC static. There's no quick way to check if a field is non-GC static - one has to ask the "is it RVA/ThreadStatic/GCStatic" questions first.

The best we could do here is change this else block to needsNonGcStaticBase = true; and delete the dependencies.Add line but then the reason string will be wrong.

[vitek-karas]

I understand the comment, but my question is why do we need to explicitly preserve the metadata for the fields - I would expect that if the app uses them we would see their usage directly. Or is this because annotations/suppressions in framework assume this? (If so I think it's worth a comment about that).

[MichalStrehovsky]

The fields are never referenced from code. Basically, an enum looks like this:

classMyEnum:Enum{privateint_value;publicconstintValue1=1;publicconstintValue2=2;}

One cannot do ldsfld MyEnum.Value1 in IL. It's lowered into ldc.i4.1.

Added comment.

[vitek-karas]

I know this is based on the existing code, but it would look "nicer" like this:

if(objisReflectableFieldNodereflectableFieldNode)

Maybe even change the whole thing into a switch?

[MichalStrehovsky]

I tend to follow the existing style in the file and don't change surrounding style as part of unrelated changes. It makes history/git blame tidier. We could reformat in a single pass if it's a problem.

[vitek-karas]

So literal fields have special handling in reflection code? Meaning they don't need any runtime support, it's all done via metadata, right?
(and we assume that since it's literal its type is primitive/string and thus we don't need to explicitly preserve its type either).

[MichalStrehovsky]

Literal fields (const/enum values) only exist in metadata. They don't have a "value" that is accessible through IL. Reflection GetValue reads the value from the metadata instead of reading some location in memory (like we do for other fields). There's no location in memory to worry about.

[MichalStrehovsky]

The AOT compiler used simple logic to decide what fields should be kept/generated: if a type is considered constructed, generate all fields. This works, but it's not very efficient.

With this change I'm introducing precise tracking of each field that should be accessible from reflection at runtime. The fields are represented by new nodes within the dependency graph.

We track fields on individual generic instantiations and ensure we end up in a consistent state where if e.g. we decided Class<int>.Foo should be reflection accessible and Class<double> is used elsewhere in the program, Class<double>.Foo is also reflection accessible. This matches how IL Linker thinks about reflectability where genericness doesn't matter. We could be more optimal here, but various suppressions in framework rely on this logic. Additional reflectable fields only cost tens of bytes.

I had to update various places within the compiler that didn't bother specifying field dependencies because they didn't matter in the past.

Added a couple new tests that test the various invariants.

This saves 2% in size on a dotnet new webapi template project with IlcTrimMetadata on. It is a small regression with IlcTrimMetadata off because we actually now track more things for the reflectable field: previously we would not make sure the field is reflection-accessible at runtime. We need a TypeHandle for the field type for it to be usable. As a potential future optimization, we could look into allowing reflection to see "unconstructed" TypeHandles (and use that one). Reflection is currently not allowed to see unconstructed TypeHandles to prevent people falling into a RuntimeHelpers.AllocateUninitializedObject(someField.FieldType.TypeHandle) trap that has a bad failure mode right now. Once we fix the failure mode, we could potentially allow it.

Cc @dotnet/ilc-contrib

[vitek-karas]

Looks good - but I don't know enough of the runtime side to tell if this will keep enough info around for the runtime to work correctly (I mean the tests work, but for a review).

[vitek-karas]

Why not just remove this branch and let the if immediately below do this? It seems to be doing the same thing.

[MichalStrehovsky]

It's part of a if/else cascade - we only need to add the TypeNonGCStaticsSymbol if there's a class constructor, or the field is non-GC static. There's no quick way to check if a field is non-GC static - one has to ask the "is it RVA/ThreadStatic/GCStatic" questions first.

The best we could do here is change this else block to needsNonGcStaticBase = true; and delete the dependencies.Add line but then the reason string will be wrong.

[vitek-karas]

I understand the comment, but my question is why do we need to explicitly preserve the metadata for the fields - I would expect that if the app uses them we would see their usage directly. Or is this because annotations/suppressions in framework assume this? (If so I think it's worth a comment about that).

[MichalStrehovsky]

The fields are never referenced from code. Basically, an enum looks like this:

classMyEnum:Enum{privateint_value;publicconstintValue1=1;publicconstintValue2=2;}

One cannot do ldsfld MyEnum.Value1 in IL. It's lowered into ldc.i4.1.

Added comment.

[vitek-karas]

I know this is based on the existing code, but it would look "nicer" like this:

if(objisReflectableFieldNodereflectableFieldNode)

Maybe even change the whole thing into a switch?

[MichalStrehovsky]

I tend to follow the existing style in the file and don't change surrounding style as part of unrelated changes. It makes history/git blame tidier. We could reformat in a single pass if it's a problem.

[vitek-karas]

So literal fields have special handling in reflection code? Meaning they don't need any runtime support, it's all done via metadata, right?
(and we assume that since it's literal its type is primitive/string and thus we don't need to explicitly preserve its type either).

[MichalStrehovsky]

Literal fields (const/enum values) only exist in metadata. They don't have a "value" that is accessible through IL. Reflection GetValue reads the value from the metadata instead of reading some location in memory (like we do for other fields). There's no location in memory to worry about.

[MichalStrehovsky]

The AOT compiler used simple logic to decide what fields should be kept/generated: if a type is considered constructed, generate all fields. This works, but it's not very efficient.

With this change I'm introducing precise tracking of each field that should be accessible from reflection at runtime. The fields are represented by new nodes within the dependency graph.

We track fields on individual generic instantiations and ensure we end up in a consistent state where if e.g. we decided Class<int>.Foo should be reflection accessible and Class<double> is used elsewhere in the program, Class<double>.Foo is also reflection accessible. This matches how IL Linker thinks about reflectability where genericness doesn't matter. We could be more optimal here, but various suppressions in framework rely on this logic. Additional reflectable fields only cost tens of bytes.

I had to update various places within the compiler that didn't bother specifying field dependencies because they didn't matter in the past.

Added a couple new tests that test the various invariants.

This saves 2% in size on a dotnet new webapi template project with IlcTrimMetadata on. It is a small regression with IlcTrimMetadata off because we actually now track more things for the reflectable field: previously we would not make sure the field is reflection-accessible at runtime. We need a TypeHandle for the field type for it to be usable. As a potential future optimization, we could look into allowing reflection to see "unconstructed" TypeHandles (and use that one). Reflection is currently not allowed to see unconstructed TypeHandles to prevent people falling into a RuntimeHelpers.AllocateUninitializedObject(someField.FieldType.TypeHandle) trap that has a bad failure mode right now. Once we fix the failure mode, we could potentially allow it.

Cc @dotnet/ilc-contrib

[vitek-karas]

Looks good - but I don't know enough of the runtime side to tell if this will keep enough info around for the runtime to work correctly (I mean the tests work, but for a review).

[vitek-karas]

Why not just remove this branch and let the if immediately below do this? It seems to be doing the same thing.

[MichalStrehovsky]

It's part of a if/else cascade - we only need to add the TypeNonGCStaticsSymbol if there's a class constructor, or the field is non-GC static. There's no quick way to check if a field is non-GC static - one has to ask the "is it RVA/ThreadStatic/GCStatic" questions first.

The best we could do here is change this else block to needsNonGcStaticBase = true; and delete the dependencies.Add line but then the reason string will be wrong.

[vitek-karas]

I understand the comment, but my question is why do we need to explicitly preserve the metadata for the fields - I would expect that if the app uses them we would see their usage directly. Or is this because annotations/suppressions in framework assume this? (If so I think it's worth a comment about that).

[MichalStrehovsky]

The fields are never referenced from code. Basically, an enum looks like this:

classMyEnum:Enum{privateint_value;publicconstintValue1=1;publicconstintValue2=2;}

One cannot do ldsfld MyEnum.Value1 in IL. It's lowered into ldc.i4.1.

Added comment.

[vitek-karas]

I know this is based on the existing code, but it would look "nicer" like this:

if(objisReflectableFieldNodereflectableFieldNode)

Maybe even change the whole thing into a switch?

[MichalStrehovsky]

I tend to follow the existing style in the file and don't change surrounding style as part of unrelated changes. It makes history/git blame tidier. We could reformat in a single pass if it's a problem.

[vitek-karas]

So literal fields have special handling in reflection code? Meaning they don't need any runtime support, it's all done via metadata, right?
(and we assume that since it's literal its type is primitive/string and thus we don't need to explicitly preserve its type either).

[MichalStrehovsky]

Literal fields (const/enum values) only exist in metadata. They don't have a "value" that is accessible through IL. Reflection GetValue reads the value from the metadata instead of reading some location in memory (like we do for other fields). There's no location in memory to worry about.

[MichalStrehovsky]

The AOT compiler used simple logic to decide what fields should be kept/generated: if a type is considered constructed, generate all fields. This works, but it's not very efficient.

With this change I'm introducing precise tracking of each field that should be accessible from reflection at runtime. The fields are represented by new nodes within the dependency graph.

We track fields on individual generic instantiations and ensure we end up in a consistent state where if e.g. we decided Class<int>.Foo should be reflection accessible and Class<double> is used elsewhere in the program, Class<double>.Foo is also reflection accessible. This matches how IL Linker thinks about reflectability where genericness doesn't matter. We could be more optimal here, but various suppressions in framework rely on this logic. Additional reflectable fields only cost tens of bytes.

I had to update various places within the compiler that didn't bother specifying field dependencies because they didn't matter in the past.

Added a couple new tests that test the various invariants.

This saves 2% in size on a dotnet new webapi template project with IlcTrimMetadata on. It is a small regression with IlcTrimMetadata off because we actually now track more things for the reflectable field: previously we would not make sure the field is reflection-accessible at runtime. We need a TypeHandle for the field type for it to be usable. As a potential future optimization, we could look into allowing reflection to see "unconstructed" TypeHandles (and use that one). Reflection is currently not allowed to see unconstructed TypeHandles to prevent people falling into a RuntimeHelpers.AllocateUninitializedObject(someField.FieldType.TypeHandle) trap that has a bad failure mode right now. Once we fix the failure mode, we could potentially allow it.

Cc @dotnet/ilc-contrib

[vitek-karas]

Looks good - but I don't know enough of the runtime side to tell if this will keep enough info around for the runtime to work correctly (I mean the tests work, but for a review).

[vitek-karas]

Why not just remove this branch and let the if immediately below do this? It seems to be doing the same thing.

[MichalStrehovsky]

It's part of a if/else cascade - we only need to add the TypeNonGCStaticsSymbol if there's a class constructor, or the field is non-GC static. There's no quick way to check if a field is non-GC static - one has to ask the "is it RVA/ThreadStatic/GCStatic" questions first.

The best we could do here is change this else block to needsNonGcStaticBase = true; and delete the dependencies.Add line but then the reason string will be wrong.

[vitek-karas]

I understand the comment, but my question is why do we need to explicitly preserve the metadata for the fields - I would expect that if the app uses them we would see their usage directly. Or is this because annotations/suppressions in framework assume this? (If so I think it's worth a comment about that).

[MichalStrehovsky]

The fields are never referenced from code. Basically, an enum looks like this:

classMyEnum:Enum{privateint_value;publicconstintValue1=1;publicconstintValue2=2;}

One cannot do ldsfld MyEnum.Value1 in IL. It's lowered into ldc.i4.1.

Added comment.

[vitek-karas]

I know this is based on the existing code, but it would look "nicer" like this:

if(objisReflectableFieldNodereflectableFieldNode)

Maybe even change the whole thing into a switch?

[MichalStrehovsky]

I tend to follow the existing style in the file and don't change surrounding style as part of unrelated changes. It makes history/git blame tidier. We could reformat in a single pass if it's a problem.

[vitek-karas]

So literal fields have special handling in reflection code? Meaning they don't need any runtime support, it's all done via metadata, right?
(and we assume that since it's literal its type is primitive/string and thus we don't need to explicitly preserve its type either).

[MichalStrehovsky]

Literal fields (const/enum values) only exist in metadata. They don't have a "value" that is accessible through IL. Reflection GetValue reads the value from the metadata instead of reading some location in memory (like we do for other fields). There's no location in memory to worry about.