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] Fix collecting closed type generics #891

Merged
merged 1 commit into from
Apr 27, 2020
Merged

[mpc] Fix collecting closed type generics #891

merged 1 commit into from
Apr 27, 2020

Conversation

waitxd
Copy link
Contributor

@waitxd waitxd commented Apr 19, 2020

Most of work was done in #830.
However, current implementation fails on this code:

[MessagePackObject]
public class CustomGenericClass<T>
{
    [Key(1)] public List<T> ListOfT;
}

[MessagePackObject]
public class ClassToSerialize
{
    [Key(0)] public CustomGenericClass<string> GenericField;
}
Generated code
        static GeneratedResolverGetFormatterHelper()
        {
            lookup = new global::System.Collections.Generic.Dictionary<Type, int>(3)
            {
                { typeof(global::CustomGenericClass<string>), 0 },
                { typeof(global::System.Collections.Generic.List<T>), 1 },
                { typeof(global::ClassToSerialize), 2 },
            };
        }

and

            switch (key)
            {
                case 0: return new MessagePack.Formatters.CustomGenericClassFormatter<string>();
                case 1: return new global::MessagePack.Formatters.ListFormatter<T>();
                case 2: return new MessagePack.Formatters.ClassToSerializeFormatter();
                default: return null;
            }

MPC is trying to collect type argument T from open generic type CustomGenericClass<T> here:
https://github.com/neuecc/MessagePack-CSharp/blob/275eb96cdcce5d10c3326a2b940af5fcdeeda066/src/MessagePack.GeneratorCore/CodeAnalysis/TypeCollector.cs#L544-L548
But because type argument has no declared accessibility (Accessibility.NotApplicable) it fails to pass IsAllowAccessibility check and collect stops there.

We can skip all generics definitions (e.g. Foo<T>) and collect only closed type references (e.g. Foo<string>). That's a main purpose of this commit.

Generated code
        static GeneratedResolverGetFormatterHelper()
        {
            lookup = new global::System.Collections.Generic.Dictionary<Type, int>(3)
            {
                { typeof(global::CustomGenericClass<string>), 0 },
                { typeof(global::System.Collections.Generic.List<string>), 1 },
                { typeof(global::ClassToSerialize), 2 },
            };
        }

and

            switch (key)
            {
                case 0: return new MessagePack.Formatters.CustomGenericClassFormatter<string>();
                case 1: return new global::MessagePack.Formatters.ListFormatter<string>();
                case 2: return new MessagePack.Formatters.ClassToSerializeFormatter();
                default: return null;
            }

… that have [MessagePackObject] attribute collected by MPC.
@neuecc
Copy link
Member

neuecc commented Apr 23, 2020

Thanks, seems very good!

@AArnott
Copy link
Collaborator

AArnott commented Apr 23, 2020

I see no problems either. I'll leave it to @neuecc to complete the PR since mpc is really his area.

@AArnott
Copy link
Collaborator

AArnott commented Apr 23, 2020

Can we close #448 when this is merged?

@waitxd
Copy link
Contributor Author

waitxd commented Apr 23, 2020

Can we close #448 when this is merged?

I think to finalize generics support in mpc we should decide do we need to support such cases:

[MessagePackObject]
public class Foo<T>
{
}

var resolver = MessagePack.Resolvers.CompositeResolver.Create(
    MessagePack.Resolvers.GeneratedResolver.Instance,
    MessagePack.Resolvers.BuiltinResolver.Instance
);
var options = MessagePackSerializerOptions.Standard.WithResolver(resolver);

var foo = new Foo<string>();
var serializedValue = MessagePackSerializer.Serialize(foo, options);

Right now mpc is unable to resolve type attributes (T) for types which are not referenced somewhere in types with MessagePackObject attribute.

So we should update docs and reflect this as known issue or extend syntax analysis (either via custom attribute or code scanning) to support such cases.

@neuecc
Copy link
Member

neuecc commented Apr 27, 2020

I'm against spreading the analysis across the board, so it would be a document.
Anyway, I'll merge this one, thanks!

@neuecc neuecc merged commit b0aec25 into MessagePack-CSharp:master Apr 27, 2020
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.

None yet

3 participants