Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
AArnott merged 10 commits intodevelopfromfix1739
Apr 11, 2024

Conversation

@AArnott
Copy link
Collaborator

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.
@AArnottAArnott added this to thev3.0 milestoneApr 7, 2024
@AArnottAArnott self-assigned thisApr 7, 2024
@AArnottAArnott changed the base branch frommaster todevelopApril 7, 2024 17:38
@AArnottAArnott marked this pull request as ready for reviewApril 7, 2024 18:37
@AArnottAArnott requested a review fromneueccApril 7, 2024 18:39
@neuecc
Copy link
Member

Thanks, I'll check soon.

AArnott reacted with thumbs up emoji

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

@neueccneuecc 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>))]// ...publicclassMyCustomFormatter<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>))]// ...publicclassMyCustomFormatter<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
CollaboratorAuthor

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 adata 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 whereMessagePackFormatterAttribute is required after this change would be to bring in a formatter that is declared in someother assembly than the data type itself.

neuecc reacted with thumbs up emoji

@AArnottAArnott merged commitb7be200 intodevelopApr 11, 2024
@AArnottAArnott deleted the fix1739 branchApril 11, 2024 15:14
@neuecc
Copy link
Member

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]publicclassMyClass{[Key(0)][MessagePackFormatter(typeof(MyCustomFormatter<int>))]publicintMyProperty{get;set;}}publicclassMyCustomFormatter<T>:IMessagePackFormatter<T>{//...snip}

@AArnott
Copy link
CollaboratorAuthor

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 avoidingActivator.CreateInstance and allowing AOT compilers to know which formatter closed generic types to compile native code for.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@neueccneueccneuecc approved these changes

Assignees

@AArnottAArnott

Projects

None yet

Milestone

v3.0

Development

Successfully merging this pull request may close these issues.

Source generated resolver should include hand-written formatters

3 participants

@AArnott@neuecc

[8]ページ先頭

©2009-2025 Movatter.jp