-
Notifications
You must be signed in to change notification settings - Fork 334
Description
In our mgmt generator practice, it is very likely that when building our own TypeProviders, we need some information from those providers built by the dependencies (ie MTG or Azure Generator).
For instance, when we build the ResourceClientProvider which corresponds to a class like VirtualMachineResource, we need to access the ClientProvider to get those "create request" methods.
In the meantime, we have visitors for those providers which runs to modify something on them.
It would be a huge problem if those visitors are not applied when our new providers are trying to fetch information in them. One of these issues happened recently is around the MatchConditions visitor (see this)
Our current visitor behavior will never apply the visitors when we are building a type.
I think this is when the visitor process is triggered:
typespec/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CSharpGen.cs
Lines 56 to 68 in 1e3aa03
| // Build all TypeProviders | |
| foreach (var type in output.TypeProviders) | |
| { | |
| type.EnsureBuilt(); | |
| } | |
| LoggingHelpers.LogElapsedTime("All generated type providers built"); | |
| // visit the entire library before generating files | |
| foreach (var visitor in CodeModelGenerator.Instance.Visitors) | |
| { | |
| visitor.VisitLibrary(output); | |
| } |
In the code, we first iterate those TypeProviders from output library, and call
EnsureBuilt on them.During this process,
EnsureBuilt will call those properties on a typeprovider like Methods, Properties, etc.Then, we iterate the type providers, and apply visitors on them.
In this process, when we EnsureBuilt on one of our mgmt providers, and it fetched a method from a ClientProvider, we will ALWAYS get one without any visitor applied to them.
I am not 100% sure how we could resolve this - even if we combine the two iterations, the problem might still exist because of some order issues, which makes the system fragile and implicit.
Can we overhaul how visitor works so that it is called on each TypeProvider? For instance, when we have the final Methods on a typeprovider, visitors can be called using the MethodProvider.Accept method on them, like this:
// public IReadOnlyList<MethodProvider> Methods => _methods ??= FilterCustomizedMembers ? FilterCustomizedMethods(BuildMethods()) : BuildMethods(); // this is current code
public IReadOnlyList<MethodProvider> Methods => _methods ??= FilterCustomizedMembers ? ApplyMethodVisitors(FilterCustomizedMethods(BuildMethods())) : ApplyMethodVisitors(BuildMethods());
private static IReadOnlyList<MethodProvider> ApplyMethodVisitors(IReadOnlyList<MethodProvider> methods)
{
foreach (var method in methods)
{
foreach (var visitor in CodeModelGenerator.Instance.Visitors)
{
method.Accept(visitor);
}
}
return methods;
}
I am not 100% sure if this would cause "stack overflow" frequently - but visitors should not be something that when mutating one method, it requires a whole picture of everything. If this is required, it is not suitable to be done via visitors.