[Analyzer idea] warn against using `MarkupLine` with string interpolation

[sliekens]

I encountered the following bug in a real application:

void WarningLine(string message, params object[] args) => AnsiConsole.MarkupLine($"[yellow]{message}[/]", args);

WarningLine("Are you sure you want to continue? [y/n]");

Exception thrown: 'System.InvalidOperationException' in Spectre.Console.dll: 'Could not find color or style 'y/n'.'

So I think it would be good to have an analyzer to warn against using MarkupLine with interpolated strings, and suggest the following fixes:

-AnsiConsole.MarkupLine($"[yellow]{message}[/]");
+AnsiConsole.MarkupLineInterpolated($"[yellow]{message}[/]");

-AnsiConsole.MarkupLine($"[yellow]{message}[/]", args);
+AnsiConsole.MarkupLineInterpolated($"[yellow]{string.Format(message, args)}[/]")

[byte2pixel]

I made a PR for this, but it wouldn't address the 2nd example and I'm not sure I fully understand the 2nd example and suggestion provided.

But it does address the 1st example.

[sliekens]

Thanks @byte2pixel

The second example is a bit hard to explain but it's about passing a formattable string which itself contains [].

string formattable = "[{0:s}] {1}";
DateTime date = DateTime.Now;
string message = "Hello, World!";

// System.InvalidOperationException: Could not find color or style '2025-02-17T12:25:19'
AnsiConsole.MarkupLine($"[yellow]{formattable}[/]", date, message);

The solution is to first format the string and then pass that output to MarkupLineInterpolated.

string formattable = "[{0:s}] {1}";
DateTime date = DateTime.Now;
string message = "Hello, World!";

-AnsiConsole.MarkupLine($"[yellow]{formattable}[/]", date, message);
+string formatted = string.Format(formattable, date, message);
+AnsiConsole.MarkupLineInterpolated($"[yellow]{formatted}[/]");

[byte2pixel]

Ok honestly I think this would warn for your 2nd example too. I will test it when get home from work.

[phil-scott-78]

We'd not want to warn if any of the holes have EscapeMarkup calls. Those cars we gotta assume the user knows what they are doing. And we might want to make this warning opt in - the interpolated methods are relatively recent and most usage is gonna be with Markup methods with a variety of approaches to escape. Don't want to light up the code base with warnings that aren't needed

Of course, I suspect there is a huge number of apps with time bombs waiting for bad input that do need the warnings, so some sort of approach is needed. I toyed around with the .net 9 overload attributes to make the interpolated behavior the default one for new apps that opt into the feature. Might need to revisit that.

[sliekens]

@phil-scott-78 you can include a .globalconfig file in the package with something like:

Usage.Spectre1030.severity= suggestion

Then it only lights up as an informational item, unless overridden in a user .editorconfig or (.globalconfig):

[*.cs]
Usage.Spectre1030.severity= warning

[MelDommer]

@phil-scott-78 you can include a .globalconfig file in the package with something like:

Usage.Spectre1030.severity= suggestion

Then it only lights up as an informational item, unless overridden in a user .editorconfig or (.globalconfig):

[*.cs]
Usage.Spectre1030.severity= warning

And is only needed if updating the analyzer to a newer version. But either way I was happy to mess around with this so like I said no worry that it isn't wanted.

Myself as a developer would welcome an analyzer update that warned me if I was potentially using something incorrectly even if I decide to ignore it. I would venture to say most analyzer package updates potentially add new warnings that didn't exist before. Or simple solution is just make the default a suggestion vs. warning. Then they can opt into making it a warning/error.

(edit: sorry, I'm at work and was just checking comments at lunch and added this from my other GH account.)

[byte2pixel]

Just to confirm the above was me.

I closed the PR thanks for the consideration.

Cheers,
Mel

[sliekens]

@byte2pixel I don't think @phil-scott-78 meant that this analyzer is not wanted, but rather that it shouldn't raise a warning when EscapeMarkup is used in the interpolation.

[phil-scott-78]

Yep, just trying to dial in a good design. I think it's a good thing to have, just has some edge cases to be considered to not give existing users a bad experience too.

[MelDommer]

Ah my bad I misinterpreted, I thought it was not desired to add any analyzer to MarkupLine due to it being most likely used over the Interpolated one.

Well if you do decide what is desired, and how you would like it handled. Let me know I would be willing to try again.

[MelDommer]

@byte2pixel I don't think @phil-scott-78 meant that this analyzer is not wanted, but rather that it shouldn't raise a warning when EscapeMarkup is used in the interpolation.

I'm not familiar with EscapeMarkup since I only ever used Spectre.Console with MarkupLineInterpolated, that makes sense now that I reread it and understand it's another method being called. I just glossed over what this meant and took it a different way.

[phil-scott-78]

No worries, I've spent way more time than healthy thinking about this problem so i probably didn't explain myself well enough. I have a blog post on my defunct blog getting into some of the design and perf if one needs some historical backing off some of the design decisions and technical reasons we needed a new method instead of an overload - https://github.com/phil-scott-78/thirty25-statiq/blob/main/input%2Fposts%2F2021%2F12%2Fspectre.console-and-interpolatedstringhandlers.md

[byte2pixel]

Just read it all., was an interesting read, thanks.