Skip to content

[http-client-csharp] Visitor issue #9414

@ArcturusZhang

Description

@ArcturusZhang

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:

// 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.

Metadata

Metadata

Assignees

Labels

emitter:client:csharpIssue for the C# client emitter: @typespec/http-client-csharp

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions