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

Change in Source Generator generation method #1691

Closed
neuecc opened this issue Oct 16, 2023 · 8 comments
Closed

Change in Source Generator generation method #1691

neuecc opened this issue Oct 16, 2023 · 8 comments
Assignees
Milestone

Comments

@neuecc
Copy link
Member

neuecc commented Oct 16, 2023

@AArnott
Currently, the behavior of MessagePack.SourceGenerator is more like the mpc implementation that existed before.
However, there are a few areas where it is unnatural and unreasonable to consider it as a Source Generator.

As a solution, how about adding attributes to the partial class in the same way as the Source Generator of System.Text.Json?

namespace MyApi; // use this namespace

[MessagePackResolver(/* new attribute */ usesMapMode: true/* options here */)]
public /* use this accessibility */ partial class FooResolver/* use this name */;

Additionally, the generation of formatters will only be executed when the above Resolver exists.
When referencing the Source Generator, reacting to attributes other than just the Source Generator and initiating generation by default seemed a bit aggressive in behavior.

The AdditionalText in the config was quite inconvenient to use.
It would be better to accomplish as much as possible using attributes.

The public, name, and namespaces for the resolver are unnecessary.
usesMapMode and customFormattedTypes (which I think should be renamed) seem appropriate as parameters for the MessagePackResolverAttribute.
I wonder if formatters can also be parameterized.

This approach should be more user-friendly.
In other words, loading configs through AdditionalFiles will be completely deprecated.

Since there won't be any impact as long as it's not generated forcibly,
it might be okay to include references to the Source Generator (and maybe Analyzer?) in the Annotations,
similar to how Microsoft.Extensions.Logging.Abstractions does.
https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj#L44-L54"

Off-topic:
Would you consider adding a FUNDING.yml to GitHub?

@AArnott
Copy link
Collaborator

AArnott commented Oct 19, 2023

Thanks for the feedback. I'm not entirely opposed to AdditionalFiles, but I think your suggestions sound good. And waiting for this type of feedback is why I've left source generation in an unstable branch. I figure more feedback would come. :) Thanks for trying it out.
I'm not sure how quickly I'd get to this. It may be a few weeks or so.

Regarding funding, I have no concerns. I filed #1693.

@AArnott AArnott added this to the v2.6 milestone Oct 19, 2023
@AArnott AArnott self-assigned this Oct 19, 2023
@AArnott
Copy link
Collaborator

AArnott commented Jan 11, 2024

I'm working on this now.

@AArnott
Copy link
Collaborator

AArnott commented Jan 11, 2024

One problem I hit immediately is that the analyzer only use case doesn't require that the user declare a resolver type at all, yet the analyzer use case does need a way to specify options.
Still, all the generator-only options can still be expressed via the attribute idea, leaving the custom formatted types as the only configuration that must be specified another way.

@AArnott
Copy link
Collaborator

AArnott commented Jan 11, 2024

We could perhaps change the way custom formatted types are listed. Instead of AdditionalFiles, through assembly-level attributes. And instead of pointing to the custom formatted types, we could perhaps point to the custom formatters:

[assembly: CustomFormatter(typeof(MyCustomFormatter))]

@AArnott
Copy link
Collaborator

AArnott commented Jan 11, 2024

BTW, is there any particular reason the generated formatters must be accessible to the rest of the compilation? One of the settings is the namespace to use for the formatters, but if the formatters were nested types within the generated resolver, we could avoid that setting. We could even make the formatters private types. That would require that the program always get them via the resolver.

@neuecc
Copy link
Member Author

neuecc commented Jan 12, 2024

is there any particular reason the generated formatters must be accessible to the rest of the compilation?

I think there is no problem with private.

@AArnott
Copy link
Collaborator

AArnott commented Jan 14, 2024

We currently support:

  1. analyzer-only
  2. 4.3 source generator (for Unity)
  3. 4.4 source generator (leverages incremental source generation APIs)

There's a lot of code sharing, but each of these carries unique code too. And now with the removal of AdditionalFiles as the source for types that are allowed to not have attributes but may be referenced by MessagePackObject types, the amount of unique code for each of these skyrockets.

To avoid having to write and test a bunch of redundant code, I'm going to drop the 'analyzer-only' one. This shouldn't be a problem because source generators can report diagnostics too, even if not emitting any source code. So essentially an analyzer is redundant when you have a source generator. This should speed up user compilations that consume MessagePack too, since the analyzer and the source generator were doing identical work to understand the types involved, up to the point where source is generated.

@AArnott
Copy link
Collaborator

AArnott commented Jan 14, 2024

That actually may be too over-simplified. Some specific diagnostic codes like MsgPack001 and MsgPack002 may still be best expressed as a traditional analyzer. It's the codes that are emitted based on analysis of the messagepack attributes that seem best to combine into the source generator.

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

No branches or pull requests

2 participants