TypeRegistrar and custom HelpProvider cannot be used together

[nothingalike]

Information

• OS: Windows
• Version: 0.47.1-preview.0.23
• Terminal: Windows Terminal / PowerShell

Describe the bug
When passing a TypeRegistrar to the CommandApp, it will prevent any custom HelpProviders from being used.

To Reproduce

1. Create a Custom HelpProvider and add it the app config
2. Create a TypeRegistrar and pass it to the CommandApp
3. Notice your Custom HelpProvider implementation is completely ignored.

Expected behavior
I can use a TypeRegistrar and Custom Help Providers

[FrankRay78]

Hello @nothingalike, I implemented the recent implementation of a pluggable HelpProvider and I'm happy to investigate your issue further.

There are currently two unit tests covering an injected TypeRegistrar and a custom HelpProvider, see:

spectre.console/test/Spectre.Console.Cli.Tests/Unit/CommandAppTests.Help.cs

Line 274 in 943c045

public Task Should_Output_Custom_Help_When_Registered_By_Instance()

            var registrar = new DefaultTypeRegistrar();

            // Given
            var fixture = new CommandAppTester(registrar);
            fixture.Configure(configurator =>
            {
                // Create the custom help provider
                var helpProvider = new CustomHelpProvider(configurator.Settings, "1.0");

                // Register the custom help provider instance
                registrar.RegisterInstance(typeof(IHelpProvider), helpProvider);

                configurator.SetApplicationName("myapp");
                configurator.AddCommand<DogCommand>("dog");
            });

And

spectre.console/test/Spectre.Console.Cli.Tests/Unit/CommandAppTests.Help.cs

Line 301 in 943c045

public Task Should_Output_Custom_Help_When_Registered_By_Type()

            var registrar = new DefaultTypeRegistrar();

            // Given
            var fixture = new CommandAppTester(registrar);
            fixture.Configure(configurator =>
            {
                // Register the custom help provider type
                registrar.Register(typeof(IHelpProvider), typeof(RedirectHelpProvider));

                configurator.SetApplicationName("myapp");
                configurator.AddCommand<DogCommand>("dog");
            });

I'm not sure if you aware of these, and if not, whether they help?

If not, are you able to put an example on a branch/into a gist for me so I can reproduce it locally?

[nothingalike]

Ah ok. Let me try registering the help provider.

[FrankRay78]

@nothingalike I also wrote some documentation here: https://spectreconsole.net/cli/command-help

It would be good to know if it's fit for purpose, or if you have any suggestions / feedback etc as you go about this.

[nothingalike]

@FrankRay78 ok so i had some time to test this morning. So far main difference is that you are able to use the DefaultTypeRegistrar since your tests are running within the same namespace. Nuget consumers do not have that luxury due to the protection level. I made my own TypeRegistrar using the following links:

• https://spectreconsole.net/cli/commandapp#dependency-injection
• https://github.com/spectreconsole/spectre.console/tree/main/examples/Cli/Injection

thoughts?

[FrankRay78]

Hmmm....

There is an example here, where the protection level isn't a problem: https://github.com/spectreconsole/spectre.console/tree/main/examples/Cli/Help
But that doesn't use DI.

There is a DI example here https://github.com/spectreconsole/spectre.console/tree/main/examples/Cli/Injection,
with a custom TypeRegistrar/Resolver.

But we don't seem to have an example combining both scenarios. I wonder if that's what's happening in your case?

[nils-a]

@FrankRay78 I had a quick glance and I see two "problems" here:

1. The default IHelpProvider is always registered, even if a custom IHelpProvider is already registered.
   

   spectre.console/src/Spectre.Console.Cli/Internal/CommandExecutor.cs

   Lines 26 to 27 in 5296e56

   	var defaultHelpProvider = new HelpProvider(configuration.Settings);
   	_registrar.RegisterInstance(typeof(IHelpProvider), defaultHelpProvider);

   
   Which probably means that in

   spectre.console/src/Spectre.Console.Cli/Internal/CommandExecutor.cs

   Line 55 in 5296e56

   var helpProvider = resolver.Resolve(typeof(IHelpProvider)) as IHelpProvider ?? defaultHelpProvider;

   
   the ?? defaultHelpProvider is never used, since there's always at least the one IHelpProvider that was registered in line 27.

2. "Obviously", our ITypeResolver uses the first registered instance of a type when multiple registrations are present for a given type. Hitherto, this is a not documented requirement for custom implementations.

Point 1. should be fixable easily enough. Point 2 should be added to TypeRegistrarBaseTests (and will then most probably break at least all my implementations of an ITypeResolver 😬)

@nothingalike as a quick workaround it should be possible to modify your ITypeResolver to return the fist registered instance of a given type instead of the last.

[nothingalike]

@nils-a ok cool, ill give that a shot. thank you

[FrankRay78]

Re: 1 @nils-a, that was a cludge when I found out that our type resolver always returned the first instance. So if you add your own help provider in the configuration, then the lines in Execute is basically pointless. I always suspected some of the DI behaviour would need to be revisited, now is the time.

Ps any reason why the internal spectre.console type registrar shouldn't be publicly available? Then users who use it, should get the same behaviour as our unit tests. Rather than being forced to implement their own to only find undocumented assumptions. I've been thinking about this one for a while.

(Nb. Writing this on iPhone in my STATIONARY car)

[nils-a]

You really shouldn't answer GitHub issues while driving 🧐

The point of the DefaultResolver is that it is tied to the DefaultRegistrar.
Making those two public would be equivalent to publishing a DI container, the scope of which would be much broader than our own "little" scope.

For everyone unwilling to implement a custom implementations, 3rd party libraries are available. From the top of my head, there's one for Microsoft.Extensions.DependecyInjection and one for SimpleInjector but I guess those are not the only ones.

[FrankRay78]

Ok @nils-a, I've thought about it and I think this issue has fundamentally arisen from the current methods on the DefaultRegistrar, in regards to what happens when a duplicate interface/instance is added.

I had wondered about the following:

1. What should be the behaviour when adding a duplicate type - replace, silently ignore, throw exception
2. Should the add methods actually be called something like AddReplaceInstance() and AddReplaceType() so it's clear the add will always replace/supersede if necessary
3. Should a method called Contains be added, so users can check if a type already exists, prior to adding one
4. Should a method be added to remove a previously registered instance/type, avoiding the need for the badly named AddReplaceInstance/Type() monikers

It was the absence of clarifying above, that led me to add this cludge (on an already massive PR that needed to be drawn to a close, quickly):

var defaultHelpProvider = new HelpProvider(configuration.Settings); 
 _registrar.RegisterInstance(typeof(IHelpProvider), defaultHelpProvider); 

But always blindly registering the internal HelpProvider, without first being able to check if one exists, or understand what should happen when calling register when one already does exist, is how this issue will be ultimately fixed.

Any and all guiding thoughts welcome, and I'm happy to do the coding donkey work.

[nils-a]

@FrankRay78 The answer to 1. is: "add". The answers for 2-4 are probably material for a dedicated discussion on the TypeRegistrar/-Resolver system.

For this issue, I would most likely:

• not register the default IHelpProvider in the ITypeRegistrar.
• construct a default IHelpProvider, only if none could be resolved from the ITypeResolver.
• add 3 tests to the TypeRegistrarBaseTests:
  • one for asserting multiple registrations of the same interface are possible
  • one for asserting that, given multiple registrations of ISomeInterface, only the last registration is resolved for Resolve(typeof(ISomeInterface))
  • one for asserting that, given multiple registrations of ISomeInterface, all registrations are resolved for Resolve(typeof(IEnumerable<ISomeInterface>))

[andreasciamanna]

I am a bit lost. What is the solution (or workaround) to the issue?

That's my code (commented with the errors):

public static class Program
{
    public static int Main(string[] args)
    {
        var services = new ServiceCollection();
        services.AddSingleton<IConfigManager>(serviceProvider =>
        {
            // ...
            return new ConfigManager(configLocations);
        });

        // Error CS0122 : 'TypeRegistrar' is inaccessible due to its protection level
        var registrar = new TypeRegistrar();

        // Error CS0122 : 'TypeRegistrar.Register<TService, TImplementation>()' is inaccessible due to its protection level
        registrar.Register(typeof(IConfigManager), typeof(ConfigManager));
        
        var app = new CommandApp(registrar);

//...

I'm currently using 0.47.1-preview.0.37 (I was hoping it would contain a fix).

[andreasciamanna]

Sorry for the noise: I figured it out.

For the future me, see #1285 (comment)

[FrankRay78]

No worries @andreasciamanna, thanks for using our library.