-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Avoid TypeSpec resolution when checking for IsValueType #80842
Conversation
This creates a much narrower path to get base type of a type that doesn't bring in complete support for token resolution. IsValueType and IsEnum that call into this are very common.
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsContributes to #80165. This creates a much narrower path to get base type of a type that doesn't bring in complete support for token resolution. Also includes a test because the scenario where we wouldn't have a type handle is obscure and we probably didn't have any coverage (even for the previous implementation). (I have one more reflection fix for #80165 and then I'm done touching the reflection stack.) Cc @dotnet/ilc-contrib
|
return Type.GetTypeFromHandle(baseTypeHandle); | ||
} | ||
|
||
// Metadata fallback. We only care about very limited subset of all possibilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this handle struct
constrains correctly?
Console.WriteLine(typeof(G<>).GetGenericArguments()[0].IsValueType); // Prints true
class G<T> where T:struct { }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes because generic parameters implement TypeRefDefOrSpecForBaseType
as below.
I'll add a dedicated test. We probably have one somewhere, but can't hurt.
Lines 143 to 159 in 12f65cd
internal sealed override QTypeDefRefOrSpec TypeRefDefOrSpecForBaseType | |
{ | |
get | |
{ | |
QTypeDefRefOrSpec[] constraints = Constraints; | |
TypeInfo[] constraintInfos = ConstraintInfos; | |
for (int i = 0; i < constraints.Length; i++) | |
{ | |
TypeInfo constraintInfo = constraintInfos[i]; | |
if (constraintInfo.IsInterface) | |
continue; | |
return constraints[i]; | |
} | |
RuntimeNamedTypeInfo objectTypeInfo = typeof(object).CastToRuntimeNamedTypeInfo(); | |
return objectTypeInfo.TypeDefinitionQHandle; | |
} |
// generic types. | ||
static Type GetLimitedBaseType(RuntimeTypeInfo thisType) | ||
{ | ||
// If we have a type handle, just use that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that we got rid of the metadata-only types. Do we still need the metadata-only types? How much benefit do they provide? They feel like an obscure case bug-farm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We got rid of the ones that have type handle, but no metadata. We still have those that have metadata but no type handle. The test has one example where we can come up with such thing. There might be more, I'm not sure I know all situations where this can happen. I think it also might happen if we have a reflected on nested type, but the owning type wasn't touched. We need the metadata. We certainly don't need the vtable.
This creates a much narrower path to get base type of a type that doesn't bring in complete support for token resolution. IsValueType and IsEnum that call into this are very common.
Contributes to #80165.
This creates a much narrower path to get base type of a type that doesn't bring in complete support for token resolution.
IsValueType
andIsEnum
that call into this are very common.Also includes a test because the scenario where we wouldn't have a type handle is obscure and we probably didn't have any coverage (even for the previous implementation).
(I have one more reflection fix for #80165 and then I'm done touching the reflection stack.)
Cc @dotnet/ilc-contrib