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

Mpc support for generic class unions #994

Merged
merged 4 commits into from
Aug 6, 2020

Conversation

thorgeirk11
Copy link
Contributor

@thorgeirk11 thorgeirk11 commented Aug 4, 2020

Mpc now supports generating formatters base on generic class unions.

  • Added CollectGenericUnion method, which reads the union attributes on generic class definitions.
  • Formatter names are now created by using the MinimallyQualifiedFormat to support generic classes.
[MessagePackObject]
[Union(0, typeof(Wrapper<string>))]
[Union(1, typeof(Wrapper<int[]>))]
[Union(2, typeof(Wrapper<IEnumerable<Guid>>))]
public class Wrapper<T>
{
    [Key(0)]
    public T Content { get; set; }
}

Mpc will now recognize these Unions and generate the formatters accordingly

...
    case 1: return new MessagePack.Formatters.Wrapper_string_Formatter();
    case 2: return new MessagePack.Formatters.Wrapper_intArray1_Formatter();
    case 3: return new MessagePack.Formatters.Wrapper_IEnumerable_Guid__Formatter();
...

Issue

This enables #964

- Added CollectGenericUnion method, which reads the union attributes on generic class definitions.
- Formatter names are now created by using the MinimallyQualifiedFormat to support generic classes. e.g. AWrapper<IEnumerable<string>> would be called AWrapper_IEnumerable_string__Formatter
This enables MessagePack-CSharp#964
@neuecc
Copy link
Member

neuecc commented Aug 5, 2020

Ok to add CollectGenericUnion
but around GetMiniallyQualifiedClassName line, it has some removed code.
I don't know removed code cause what sideeffect.
Can you explain more info?
I worry about the possibility of breaking the existing code.

@thorgeirk11
Copy link
Contributor Author

thorgeirk11 commented Aug 6, 2020

The template string was used to generate the formatter classes with a generic type definition.
I found a bug in that approach, e.g.

[MessagePackObject]
public class TreeNode<T> 
{
    [Key(0)]
    public TreeNode<T> Parent { get; set; }   
}

[MessagePackObject]
public class Leaf : TreeNode<Leaf> { }

[MessagePackObject]
public class Leaf2 : TreeNode<Leaf2> { }

When you use mpc on this code it will generate the following:

public sealed class TreeNodeFormatter<T> : IMessagePackFormatter<TreeNode<Leaf>>
{  ...  }
public sealed class TreeNodeFormatter<T> : IMessagePackFormatter<TreeNode<Leaf2>>
{  ...  }

There will be two formatters created with the name TreeNodeFormatter<T> causing a compile error. The new GetMiniallyQualifiedClassName fixes the issue by explicitly stating the type names in the name.

public sealed class TreeNode_Leaf_Formatter : IMessagePackFormatter<TreeNode<Leaf>>
{  ...  }
public sealed class TreeNode_Leaf2_Formatter : IMessagePackFormatter<TreeNode<Leaf2>>
{  ...  }

@neuecc I am not 100% that this will not break anything, what do you think?

Copy link
Member

@neuecc neuecc left a comment

Choose a reason for hiding this comment

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

thank you for the clarification.
okay to merge.

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

Successfully merging this pull request may close these issues.

None yet

3 participants