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

Include all hand-written formatters in the source generated resolver #1796

Merged
merged 10 commits into from
Apr 11, 2024

Conversation

AArnott
Copy link
Collaborator

@AArnott AArnott commented Apr 7, 2024

This required a great deal of (worthwhile) refactoring and fixes.

Closes #1739

The open generic formatter is merely skipped at this point to avoid generating code with compilation errors. This will need to be revisited to include the formatter in the resolver in a better way.
@AArnott AArnott added this to the v3.0 milestone Apr 7, 2024
@AArnott AArnott self-assigned this Apr 7, 2024
@AArnott AArnott changed the base branch from master to develop April 7, 2024 17:38
@AArnott AArnott marked this pull request as ready for review April 7, 2024 18:37
@AArnott AArnott requested a review from neuecc April 7, 2024 18:39
@neuecc
Copy link
Member

neuecc commented Apr 8, 2024

Thanks, I'll check soon.

The problem was when more than one custom formatter is discovered for a given data type, a non-deterministic selection is made as to which formatter will be used by the generated resolver.
We fix this by producing an error in this condition, and suppressing generation of code that uses either formatter for the colliding data types.
They spew NREs all over the place.
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.

The changes are so extensive that I cannot carefully review the source code, but the results of running it seem to be very good!

By the way, what happened to the policy for supporting Generics that you commented on here?
#1739 (comment)

[MessagePackFormatter(typeof(MyCustomFormatter<string>))]
// ...
public class MyCustomFormatter<T> : IMessagePackFormatter<int>

Currently, it generates invalid source code that cannot be compiled.
If possible, should we make it reject such cases with a Source Generator error?

[MessagePackFormatter(typeof(MyCustomFormatter<int>))]
// ...
public class MyCustomFormatter<T> : IMessagePackFormatter<T>

This case seems to be supportable, but currently the formatter generation completely disappears.
The current changes give users the impression that custom formatters are automatically included.
Unless we either issue a warning or provide support, there is a possibility of confusing users.

As an interim policy, it may be fine to say that we don't care about Generics-related cases, but...

@AArnott
Copy link
Collaborator Author

AArnott commented Apr 11, 2024

Thanks for reviewing. And ya, sorry about the size of the PR. I too am relying on tests overall to know that we're moving forward. And yes, we surely need to add more tests for better coverage as we go.

Regarding your generics question, I still like my proposal that you linked to. I don't think it's fully implemented yet. Generics is a huge space for this and I think it'll have to come incrementally.
But regarding the particular code snippets you shared, I don't understand why you would apply [MessagePackFormatter] attributes to a formatter type. Isn't that attribute meant to be applied to a data type in order to help a resolver find the formatter?

The current changes give users the impression that custom formatters are automatically included.

That's exactly right. The only case where MessagePackFormatterAttribute is required after this change would be to bring in a formatter that is declared in some other assembly than the data type itself.

@AArnott AArnott merged commit b7be200 into develop Apr 11, 2024
5 checks passed
@AArnott AArnott deleted the fix1739 branch April 11, 2024 15:14
@neuecc
Copy link
Member

neuecc commented Apr 12, 2024

I understand. No worries.

I don't understand why you would apply [MessagePackFormatter] attributes to a formatter type.

Oh, sorry, I meant to provide the following example but omitted it.

[MessagePackObject]
public class MyClass
{
    [Key(0)]
    [MessagePackFormatter(typeof(MyCustomFormatter<int>))]
    public int MyProperty { get; set; }
}

public class MyCustomFormatter<T> : IMessagePackFormatter<T>
{
    //...snip
}

@AArnott
Copy link
Collaborator Author

AArnott commented Apr 12, 2024

I think that example will do the right thing. Specifically, the 'open generic' formatter will get into the resolver as an open generic. Then specific closed generic types that are statically discoverable also make it into the resolver specifically, thus avoiding Activator.CreateInstance and allowing AOT compilers to know which formatter closed generic types to compile native code for.

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.

Source generated resolver should include hand-written formatters
2 participants