Why does UnnecessaryWithoutSuggestionDescriptor still show the smart tag, contrary to docs?

[jnm2]

Version Used: VS 16.5-p3

When we implemented 'Simplify interpolation' in #40070, I specifically wanted to fade out multiple text segments. There is a nice API for that, but even though we used it, each segment gets its own smart tag.

Is this a Roslyn bug, or did we miss something? What confuses me the most, besides the fact that the nice API doesn't seem to do what it says, is what the difference actually is between UnnecessaryWithSuggestionDescriptor and UnnecessaryWithoutSuggestionDescriptor. They are identical except that the former gets the diagnostic ID IDE0071 and the latter gets the diagnostic ID IDE0071WithoutSuggestion.

That already seems odd, but I also searched all files in the repo for the string WithoutSuggestion and I don't see code that looks like it is doing an EndsWith("WithoutSuggestion") check. Could it have been removed at some point?

Here's the nice API:

roslyn/src/CodeStyle/Core/Analyzers/AbstractCodeStyleDiagnosticAnalyzer.cs

Lines 16 to 34 in 019f8df

	/// <summary>
	/// Diagnostic descriptor for code you want to fade out *and* want to have a smart-tag
	/// appear for. This is the common descriptor for code that is being faded out
	/// </summary>
	protected readonly DiagnosticDescriptor UnnecessaryWithSuggestionDescriptor;
	/// <summary>
	/// Diagnostic descriptor for code you want to fade out and do *not* want to have a smart-tag
	/// appear for. This is uncommon but useful in some cases. For example, if you are fading
	/// out pieces of code before/after another piece of code *on the same line*, then you will
	/// only want one usage of <see cref="UnnecessaryWithSuggestionDescriptor"/> and multiple
	/// usages of <see cref="UnnecessaryWithoutSuggestionDescriptor"/>.
	///
	/// That's because if you use <see cref="UnnecessaryWithSuggestionDescriptor"/> for all the
	/// faded out code then that will mean the user will see multiple code actions to fix the
	/// same issue when they bring up the code action on that line. Using these two descriptors
	/// helps ensure that there will not be useless code-action overload.
	/// </summary>
	protected readonly DiagnosticDescriptor UnnecessaryWithoutSuggestionDescriptor;

Here's our implementation:

roslyn/src/Features/Core/Portable/SimplifyInterpolation/AbstractSimplifyInterpolationDiagnosticAnalyzer.cs

Lines 70 to 78 in 019f8df

	for (var i = 0; i < unnecessaryLocations.Length; i++)
	{
	context.ReportDiagnostic(DiagnosticHelper.Create(
	i == 0 ? UnnecessaryWithSuggestionDescriptor : UnnecessaryWithoutSuggestionDescriptor,
	unnecessaryLocations[i],
	severity,
	additionalLocations: locations,
	properties: null));
	}

[jnm2]

Looks like the first occurrence of the text WithoutSuggestion in the repository is @CyrusNajmabadi's PR: https://github.com/dotnet/roslyn/pull/13927/files#diff-3612a4d72dedca97a42766687ce9889eR39

It was a while back. Does anyone remember why this worked at the time?

[jnm2]

Oh, maybe the severity is what's wrong. I noticed that other references to UnnecessaryWithoutSuggestionDescriptor such as this do not specify a severity:

roslyn/src/Features/CSharp/Portable/UseDefaultLiteral/CSharpUseDefaultLiteralDiagnosticAnalyzer.cs

Lines 47 to 61 in b494eec

	// Create a normal diagnostic that covers the entire default expression.
	context.ReportDiagnostic(
	DiagnosticHelper.Create(
	Descriptor,
	defaultExpression.GetLocation(),
	preference.Notification.Severity,
	additionalLocations: null,
	properties: null));
	// Also fade out the part of the default expression from the open paren through
	// the close paren.
	context.ReportDiagnostic(
	Diagnostic.Create(
	UnnecessaryWithoutSuggestionDescriptor,
	syntaxTree.GetLocation(fadeSpan)));

If this is what it is, I think the docs for UnnecessaryWithoutSuggestionDescriptor should specify that it's only half the story and that no severity should be specified if this descriptor is used.
Maybe even prefer to make UnnecessaryWithoutSuggestionDescriptor private and add a helper method CreateUnnecessaryWithoutSuggestionDiagnostic so that severity can't be specified?

[jnm2]

Yep, that was the problem. Screenshot, fix, and proposed (internal) API change in #41854.