Out<T> and Print<T> problematic as generic methods

[scottbilas]

I've run into problems with the Print<T> and Out<T> generics. These do a ToString() on whatever is passed in, which is generally nice for usability, but I have more than once accidentally passed in something that gets auto-stringized, rather than the compile failing and catching my mistake.

Furthermore, the generic blocks my ability to add extension methods for custom type handling. A great example of this is when I wanted to add an Out(Spectre.Console.Rendering.IRenderable) extension. Unfortunately, the Out<T> always wins over an extension, and for any parameter type. And if I simply forget to call AnsiConsole.Console.ToAnsi first, then I get the default/bad Object.ToString behavior. IMO these generic functions cause more problems than they solve.

I can think of a few potential improvements:

• Remove Out<T> entirely
• Rename to OutValue<T>
• Move Out<T> to an extension method
• Change it to Out(object) (as with System.IO.TextWriter, adding in a bunch of overloads for primitives to reduce boxing)

My favorite is the first option. Cathode feels to me much more close-to-the-metal API vs the console stuff .NET ships with. Lots of work has gone into minimizing allocs and avoiding copying. So these stringizing helper methods feel out of place to me.

(Related: #192 (comment))

[alexrp]

Removing it outright would regress stuff like this:

cathode/src/samples/scrolling/Program.cs

Lines 11 to 16 in 095df82

	await OutAsync(
	new ControlBuilder()
	.SetDecorations(intense: true)
	.PrintLine("The last string entered will be displayed here.")
	.ResetAttributes()
	.PrintLine(new string('-', Terminal.Size.Width)));

Obviously you shouldn't be writing code in that fashion when performance matters, but it is quite convenient when it doesn't. Not having some sort of "just output the stringified form of whatever I pass in" method will also likely be a significant paper cut for someone coming from System.Console.

The overloading problems do need to be solved though. I'm leaning towards the extension method approach since I don't think any of these methods need access to TerminalWriter internals anyway. Will think a bit more on it.

In regards to these methods being a performance trap: If it happens often enough, maybe you could make use of BannedApiAnalyzers? That's what Cathode itself does to ban System.Console. Since Cathode already pulls the analyzer in, I believe all you need to do is add this to your project file:

    <ItemGroup>
        <AdditionalFiles Include="BannedSymbols.txt" />
    </ItemGroup>

And then add the problematic methods to BannedSymbols.txt.

[scottbilas]

Cool, I didn't think to make it a locally banned API for myself. This will work 👍🏽

[scottbilas]

I'm using this banned list to start out:

M:Vezel.Cathode.Terminal.Error`1(``0)
M:Vezel.Cathode.Terminal.ErrorLine`1(``0)
M:Vezel.Cathode.Terminal.Out`1(``0)
M:Vezel.Cathode.Terminal.OutLine`1(``0)
M:Vezel.Cathode.IO.TerminalWriter.Write`1(``0)
M:Vezel.Cathode.IO.TerminalWriter.WriteLine`1(``0)
M:Vezel.Cathode.Text.Control.ControlBuilder.Print`1(``0)
M:Vezel.Cathode.Text.Control.ControlBuilder.PrintLine`1(``0)
M:Vezel.Cathode.VirtualTerminal.Error`1(``0)
M:Vezel.Cathode.VirtualTerminal.ErrorLine`1(``0)
M:Vezel.Cathode.VirtualTerminal.Out`1(``0)
M:Vezel.Cathode.VirtualTerminal.OutLine`1(``0)

...and immediately ran into a problem: I don't want to ban Out<string>(), because its ToString does not alloc, and most of the time I do have a string to pass in.

The banned symbols format doesn't support un-banning/exclusions, or selecting specific generic instantiations either. I can work around this by tacking on an AsSpan() everywhere, which is not nice.

I wouldn't mind adding my own Out(string) etc. overloads as extension methods, but that requires resolving the "cannot extend when there is a generic" issue.

For the moment, I'm locally modifying Cathode to add Out(string) type methods as needed.

[alexrp]

...and immediately ran into a problem: I don't want to ban Out<string>(), because its ToString does not alloc, and most of the time I do have a string to pass in.

I think it'd be reasonable to add a string overload to resolve this.

[scottbilas]

Cool, I'll do a PR this weekend

[alexrp]

I'm already putting together a patch to extract most of the methods out as extensions, so I'll just do it as part of that. I just posted the above to acknowledge that point. 🙂

[scottbilas]

Even better. :) I look forward to reverting my local changes.

[alexrp]

@scottbilas for Print<T>(), are you referring to the method on ControlBuilder?

[scottbilas]

Yes and PrintLine. My prev comment with the banned api list has the full set I discovered.

[alexrp]

Ok, I pushed some initial work on this. Let me know how that looks to you.

Still need to do the WriteLine() family of methods. I never added all the same overloads to that as I did for Write(). I think it's because the implementation looks like this:

cathode/src/core/IO/TerminalIOExtensions.cs

Lines 236 to 239 in 2d3ed1d

	public static void WriteLine<T>(this TerminalWriter writer, T value)
	{
	writer.Write(value?.ToString() + Environment.NewLine);
	}

That is, it looks like it's atomically writing the value and the new-line character... of course, in reality, that's not how that plays out at all:

cathode/src/core/IO/TerminalIOExtensions.cs

Lines 74 to 81 in 2d3ed1d

	public static void Write(this TerminalWriter writer, scoped ReadOnlySpan<byte> value)
	{
	Check.Null(writer);
	for (var count = 0; count < value.Length; count += writer.WritePartial(value[count..]))
	{
	}
	}

So I think it should just get the same treatment as Write().

[scottbilas]

Looks great! The increasing list of overloads across types would make me reach for some T4 codegen, personally. :D

One suggestion - given the nature of ControlBuilder, I also find it useful to have a Print(char) overload. This would also let you change a couple slightly awkward new(in ch) to just ch in that same file.

Regarding the *Line variants, I didn't realize the WritePartial chunking was going on. But it's also good to avoid the alloc+copying - overly expensive to do that, just to add a char or two at the end.

[alexrp]

I think this is mostly done now.

One question that remains is: Should VirtualTerminal get the same treatment? It seems like someone trying to add extension methods to that would run into the same issues.

One suggestion - given the nature of ControlBuilder, I also find it useful to have a Print(char) overload. This would also let you change a couple slightly awkward new(in ch) to just ch in that same file.

Those should just be Print([ch]) (which I've just changed them to). Once C# allows params ReadOnlySpan<char>, it can even become Print(ch) without adding another overload.