-
-
Notifications
You must be signed in to change notification settings - Fork 700
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
Source Generator configuration via attributes instead of .json file #1736
Conversation
The goal is to eliminate all dependencies on MSBuild properties (which Unity cannot provide) or AdditionalFiles (which MSBuild and Unity support but are a pain to use). Instead, we'll rely on a partial class with an attribute to trigger source generation. "custom types" that used to be specified via the AdditionalFiles will now be specified via assembly-targeted attributes.
These support packages.config projects, which are very obsolete. Also given where we're going with multiple roslyn target assemblies, they wouldn't work properly anyway.
All tests now pass
thanks, I'll response soon(now reviewing). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, very good!
- All formatters are now inner class of Resolver, so when formatters to same name types of different namespaces exists, will be error.
namespace Namespace1
{
[MessagePackObject]
public class MogeMoge
{
[Key(0)]
public int MyProperty { get; set; }
}
}
namespace Namespace2
{
[MessagePackObject]
public class MogeMoge
{
[Key(0)]
public int MyProperty { get; set; }
}
}
- InstanceWithStandardAotResolver should generate
FormatterCache<T>
This is also true for StandardAotResolver.Instance
.
I don't think CompositeResolver.Create
should be used in the library.
- Avoid filescoped namespaces
Since Unity is up to C# 9.0, filescoped namespace (C# 10.0) cannot be used.
Since we do not dare to branch, it is better to assume C# 9.0 for the generated code.
- Package name
I am not sure if the name MessagePackAnalyzer is a good name ...... when the main use is Source Generator.
I think it's OK to use another package name (MessagePackGenerator
, MessagePackSourceGenerator
, MessagePack.SourceGenerator
).
We don't have to worry about the package name if bundle them, though.
Unity
Once again, let's get the Unity compiler version straight.
- Unity 2022.3.12f or newer: 4.3.0
- Unity 2022.2 or newer: 4.1.0
- Unity 2021.2 or newer: 3.8.0
We believe that our target should be "Unity 2022.3 (LTS)".
But the problem is that minor versions have different compilers.
As of 2022.3.0(released at 2023-5-30), it is 4.1.0
In 2022.3.12 (released at 2023-10-26) it was updated to 4.3.0.
Ideally, though, it would be good to support all of them.
However, in order to keep the code base simple, it is okay to support only 4.3.0.
However, since the 3.8.0 version already exists, 3.8.0 + 4.3.0 would be best.
In that case, the minimum supported version of Unity would be 2021.2.
In this PR, the Unity version will be released as a standalone package.
I think it would be better to bundle it with MessagePackAnalyzer after #1734.
In other words, no .Unity
suffix,
analyzers/dotnet/roslyn3.8/cs/MessagePack.SourceGenerator.dll
analyzers/dotnet/roslyn4.1/cs/MessagePack.SourceGenerator.dll
is a good packing project name.
Thanks for the review. I know it was a lot of code churn. I'll get to work on the updates. In same cases, I added points you made to the PR description so I don't lose track.
Ah yes, I anticipated that and meant to fix it, but forgot.
Elsewhere it is said that "Unity 2021.3 is using Roslyn v3 and it dies in 2024 April". Does that mean in April a later C# version is available? If so, can we adopt it now in our prerelease?
Oh, I'd love to eject the 3.8.0 version. A lot of complexity comes from that.
I can't say for unity for sure, but LTS typically means that version will be supported for a long time, provided you update to the latest servicing release. Given that, 2022.3.0 may not be supported by unity any more, and if so no one should be using it and we wouldn't need to support it either.
I'm not settled with the name either. But
Oh, it's so convenient though. :) How do you measure the perf difference between that and a direct
Not so. This PR only builds analyzers into one package, that includes the roslyn 3.8 and 4.3 analyzers together. The .Unity suffixes you see are only in the project names and the names of the files within the package. |
For regular Unity users, |
Unity explicitly fixes the langversion to 9.0 at compile time. Deciding the minimum version for Unity is quite troubling. I've been able to make this decision only since three months ago when the compiler was updated!
If you take MicroBenchmark, you will see a difference because of the lookup on every GetFormatter.
Oh, sorry, I see, I read csproj carefully, I understand, thank you. Regarding the package name, it might be good to keep it as MessagePackAnalyzer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been giving some thought to how we could make AOT generation more "on by default". With source generation being so much easier than the old mpc flow, it seems we may be within striking distance. So here's a thought I had tonight:
Suppose the source generator always generates the formatters, even if the user doesn't define a partial class with the [GeneratedMessagePackResolver]
attribute. Suppose further that the DynamicObjectResolver
looks for these formatters in the same assembly as the type to be formatted before it generates one dynamically and uses the pre-compiled one if it's found.
Now, we could 'discover' the formatter for a given type a variety of ways. But the most performant way is probably to... generate the resolver too! Then we have just one type to find via reflection for the whole assembly, and if we find it, we activate it (from the DynamicObjectResolver
) and use it to search for pre-created formatters before dynamically creating them. How do we find the resolver? Well, we could emit one assembly-level attribute that we search for at runtime that points directly at the resolver. This could work whether the resolver is declared partially by the user code (and thus in a user-determined namespace) or whether we just generated it fully automatically.
Now what about strictly AOT environments where reflection to find the formatters or resolver doesn't work? Well, that can work the same way today (in this PR): the user declares the partial class for the resolver to effectively control the namespace and name of the resolver so the user can write code that finds it up-front, thereby avoiding all reflection.
The user can also opt into declaring the partial class explicitly in order to specify non-default options for code generation in the attribute on the resolver class.
This proposal means that anyone compiling against MessagePack v3 would effectively get AOT performance 'for free'. It also means these AOT code generators had better be good, because they'll be promoted from being used for a (small?) subset of projects to all projects. We tend to get bug reports fairly regularly that are unique to AOT formatters, so since the new source generator is based on the same T4 templates, we'll inherit those and need to be prepared to respond quickly to incoming bug reports.
In MemoryPack, I have discontinued IL Generation and switched to using only Source Generators. Having multiple code generation processes makes it difficult to maintain quality, so it has always been desirable to consolidate them into a single process. However, it would be challenging to discontinue IL Generation in MessagePack... For your reference, MemoryPack has two Formatter registration processes. partial class Foo
{
static Foo()
{
MemoryPackFormatterProvider.Register<Foo>();
}
static void IMemoryPackFormatterRegister.RegisterFormatter()
{
if (!MemoryPackFormatterProvider.IsRegistered<Foo>())
{
MemoryPackFormatterProvider.Register(new MemoryPackableFormatter<Foo>());
}
if (!MemoryPackFormatterProvider.IsRegistered<Foo[]>())
{
MemoryPackFormatterProvider.Register(new ArrayFormatter<Foo>());
}
}
} The static constructor does not trigger unless the type is accessed, so as a fallback, it was also triggered during static bool TryInvokeRegisterFormatter(Type type)
{
if (typeof(IMemoryPackFormatterRegister).IsAssignableFrom(type))
{
// currently C# can not call like `if (T is IMemoryPackFormatterRegister) T.RegisterFormatter()`, so use reflection instead.
var m = type.GetMethod("MemoryPack.IMemoryPackFormatterRegister.RegisterFormatter", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static);
if (m == null)
{
throw new InvalidOperationException("Type implements IMemoryPackFormatterRegister but can not found RegisterFormatter. Type: " + type.FullName);
}
m!.Invoke(null, null); // Cache<T>.formatter will set from method
return true;
}
return false;
} This is possible because in MemoryPack, the Formatter is attached to the target type, making it easy to find. However, this is not something that can be directly applied to the MessagePack system, but it's worth mentioning for reference. I've also experimented with applying |
Thanks for the ideas from MemoryPack. Cool stuff you can do when you have a clean slate. However |
The roslyn team strongly urges diagnostics to come from analyzers. Source generators should only report diagnostics that are specific to code generation. This, even if it means we have to perform analysis twice: once in the source generator (which comes first) and again in the analyzer.
The analyzers appear to support other languages, but not really, because in formulating diagnostics, they 'downcast' to C# syntax nodes to get good locations. We can 'fix' this by making language-specific analyzer projects that share a lot of code, but unless we know there are VB/F# users out there, it's not worth it.
I've made all the changes originally scoped (this excludes the "always AOT" ideas, which I think I'll save for a follow-up PR), except for the proposal to drop roslyn 3.8 support. Since the support is already there (which took a lot of work in this PR), and it's a little work to remove, but that's work, and maybe we'll want to retain the special build authoring that supports two roslyn versions (whatever they are), in case we want to drop 3.8 support but then retain 4.3 support while also doing something special for 4.5... I think keeping the 3.8 support for now may make our lives a bit easier later on. So... I'm ready for a final review. |
@AArnott |
MessagePackAnalyzer.json
file.[GeneratedMessagePackResolverAttribute]
to activate source generation of the resolver and formatters for all discoverable serializable types. This new attribute has several properties that may be used to customize code generation.[assembly: MessagePackKnownFormatterAttribute(Type)]
points to implementations ofIMessagePackFormatter<T>
that should be included in the source generated resolver. This interface may be implemented more than once per class. All type arguments will be automatically considered to be serializable by the analyzer.[assembly: MessagePackAssumedFormattableAttribute(Type)]
points to types that should be assumed to have formatters defined (but may not be available to reference directly viaMessagePackKnownFormatterAttribute
). The formatters that justify this will have to be combined with the source generated resolver at runtime (possibly using theCompositeResolver
class).GeneratedMessagePackResolverAttribute
appearing anywhere in the compilation.MessagePackAnalyzer.json
file exists and emit a warning advising users to visit a migration document online.Remaining investigations
FormatterCache<T>
pattern.Potential directions
This PR does not do this, but a subsequent change could do it if there is interest:
This may be a bit complicated to pull off (though, far less than this PR was) because it means either the MessagePack project will have to depend on the analyzer building package project (adding several projects to its build dependency tree) or we'll have to play tricks to add it as a nuget dependency without being a build dependency.
Closes #1691