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

Support open generic metatypes #857

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Turnerj
Copy link

@Turnerj Turnerj commented Nov 4, 2021

Fixes #802

Currently you can't write something like this:

RuntimeTypeModel.Default.Add(typeof(CacheEntry<>))
	.Add(1, nameof(CacheEntry<object>.Expiry))
	.Add(2, nameof(CacheEntry<object>.Value));

The problem is that the "type" is typically matched exactly however by writing such code, I'm intending it match all types that use that generic definition.

The fix is accomplished by looking up by the generic definition if it has failed to find an exact match or a contract attribute specified. If found, it generates a new metatype entry, using the fields and surrogate from the generic definition's metatype entry.

Comment on lines 2488 to 2491
var metaType = new MetaType(model, type, factory);
metaType.SetSurrogate(surrogateType.GetGenericTypeDefinition());
metaType._fields = _fields;
return metaType;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this part will need further refinement. I'm assigning _fields versus duplicating the field data as it should be an exact match of data - I can't think of a way this wouldn't work. Happy to instead iterate over the data and manually call AddField etc.

Also there may be other important properties to set from one MetaType instance to the other here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the code to include additional properties on MetaType that seemed relevant but I'd need your expertise to confirm my approach and whether there are any other considerations I should be aware of.

Comment on lines +729 to +733
var genericMetaType = (MetaType)types[key];
if (genericMetaType.Pending)
{
WaitOnLock();
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda winging this part here based on existing code further up in the method.

Comment on lines +738 to +739
if (metaType is null)
{
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This additional check seemed like the best solution to avoid triggering the next block if a metatype was generated in the new code I wrote. In my head it was either this or a goto statement.

@Turnerj
Copy link
Author

Turnerj commented Nov 4, 2021

Not sure why the build failed - as far as I can tell, all the tests passed on AppVeyor 🤷‍♂️

@Turnerj
Copy link
Author

Turnerj commented Feb 15, 2022

Hey @mgravell, if you have the time, I'd love to get your feedback on the fix - in particular, my changes in MetaType.cs are potentially controversial with how I'm initializing the new MetaType instance.

If you can't look at it at the moment because your plate is full, I totally understand!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extending open generic types support
1 participant