Verify arguments length for diagnostic messages by cston · Pull Request #60925 · dotnet/roslyn

cston · 2022-04-23T18:36:27Z

Verify DiagnosticInfo() arguments array length matches the expected value from the diagnostic message (C# only).

Fixes #60862 for C#.

Youssef1313 · 2022-04-23T19:28:24Z

src/Compilers/Core/Portable/Diagnostic/DiagnosticInfo.cs

        }

+        [Conditional("DEBUG")]
+        internal static void AssertExpectedMessageArgumentsLength(CommonMessageProvider messageProvider, int errorCode, int actualLength)


Is this path for compiler diagnostics only?

I think it would be beneficial to have the assert for all diagnostics (to prevent IDExxxx from having bugs too)

Then the following analyzer will need to be excluded:

roslyn/src/Compilers/Test/Core/Diagnostics/CommonDiagnosticAnalyzers.cs

Lines 894 to 914 in 8defa4a

[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]

public sealed class AnalyzerReportingMisformattedDiagnostic : DiagnosticAnalyzer

{

public static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(

"ID1",

"Title1",

"Symbol Name: {0}, Extra argument: {1}",

"Category1",

defaultSeverity: DiagnosticSeverity.Warning,

isEnabledByDefault: true);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

public override void Initialize(AnalysisContext context)

{

context.RegisterSymbolAction(symbolContext =>

{

// Report diagnostic with incorrect number of message format arguments.

symbolContext.ReportDiagnostic(Diagnostic.Create(Rule, symbolContext.Symbol.Locations[0], symbolContext.Symbol.Name));

}, SymbolKind.NamedType);

}

}

Not sure if that makes sense? #Resolved

I've limited the change to C# compiler diagnostics for now, to keep the scope manageable. We can consider addressing other diagnostics, separately, as needed.

I think it would be beneficial to have the assert for all diagnostics (to prevent IDExxxx from having bugs too)

I have no objection, as long as this is a debug-only assertion. It's possible we have runtime scenarios where the diagnostic intentionally provides more placeholders than are used by the message.

Youssef1313 · 2022-04-23T22:35:30Z

src/Compilers/Core/Portable/Diagnostic/DiagnosticInfo.cs

+            if (errorCode <= 0)
+            {
+                return;
+            }


When can this happen?
#Resolved

For ErrorCode.Void and ErrorCode.Unknown which are used as placeholders for errors.

Youssef1313 · 2022-04-23T22:37:52Z

src/Compilers/Core/Portable/Diagnostic/DiagnosticInfo.cs

+            {
+                return;
+            }
+            string message = messageProvider.LoadMessage(errorCode, language: null) ?? string.Empty;


Can LoadMessage return null for a reason other than a bug? #Resolved

Youssef1313 · 2022-04-23T22:40:51Z

src/Compilers/Core/Portable/Diagnostic/DiagnosticInfo.cs

+            {
+                expectedLength = Math.Max(int.Parse(match.Value[1..^1]) + 1, expectedLength);
+            }
+            Debug.Assert(expectedLength == actualLength);


Would Debug.Assert(expectedLength == matches.Count) be valid (in addition to the existing assert)? #Resolved

matches has entries for each argument use, even duplicates, so matches.Count may not equal expectedLength. But it makes sense to check that all arguments are used, so I've used a BitVector instead, thanks.

There is one resource string that fails this new check: ERR_IdentifierExpectedKW. I've disabled the check for that ErrorCode, rather than changing the resource string and the arguments array, to avoid breaking translated resources. The assert is useful for new cases where arguments are unused though.

Youssef1313 · 2022-04-23T22:41:01Z

src/Compilers/Core/Portable/Diagnostic/DiagnosticInfo.cs

+        [Conditional("DEBUG")]
+        internal static void AssertExpectedMessageArgumentsLength(CommonMessageProvider messageProvider, int errorCode, int actualLength)
+        {
+#if DEBUG


Is it needed? #Resolved

It's not strictly necessary but it seemed reasonable to avoid emitting the method body in release builds.

Youssef1313 · 2022-04-23T22:57:36Z

src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs

                return TypeWithAnnotations.Create(new ExtendedErrorTypeSymbol(
                    Compilation.Assembly.GlobalNamespace, identifierValueText, 0,
-                    new CSDiagnosticInfo(ErrorCode.ERR_SingleTypeNameNotFound)));
+                    new CSDiagnosticInfo(ErrorCode.ERR_SingleTypeNameNotFound, identifierValueText)));


Which test failed this?

I think this change fixes #59417 but how can we observe that through a test?

There were a number of tests that failed, including IOperationTests_IConversionExpression.ConversionExpression_Implicit_ReferenceConversion_InvalidSyntax and IOperationTests_ISizeOfExpression.TestSizeOfExpression_MissingArgument. The DiagnosticInfo is used in the resulting ExtendedErrorTypeSymbol but it's not clear how to construct a test where the ExtendedErrorTypeSymbol.ErrorInfo is reported from such cases though.

jcouv · 2022-04-25T20:22:02Z

src/Compilers/CSharp/Portable/Errors/MessageProvider.cs

+                0 => false,
+                ErrorCode.Unknown => false,
+                ErrorCode.Void => false,
+                ErrorCode.WRN_XMLParseError => false, // should investigate failures here


Tracking issue? #Resolved

Investigated and added comment here.

jcouv · 2022-04-25T20:30:13Z

src/Compilers/Core/Portable/Diagnostic/DiagnosticInfo.cs

+            int expectedLength = 0;
+            foreach (object? m in matches)
+            {
+                if (m is Match { } match)


{ }

{ } unnecessary #Resolved

jcouv

LGTM Thanks (iteration 7) with a couple of nits to consider

AlekseyTs

LGTM (commit 7)

AlekseyTs

LGTM (commit 10)

Verify arguments length for diagnostic messages

d21c0f3

ghost added the Area-Compilers label Apr 23, 2022

Youssef1313 reviewed Apr 23, 2022

View reviewed changes

Fix some tests

3711fe6

Youssef1313 reviewed Apr 23, 2022

View reviewed changes

cston force-pushed the 60862 branch 2 times, most recently from 5184fb4 to 5a6a9aa Compare April 24, 2022 15:45

cston added 3 commits April 24, 2022 13:20

ERR_SyntaxError

fdc9dd9

Fix tests

5c419a2

Skip VisualBasic

eeb0df1

cston force-pushed the 60862 branch from 5a6a9aa to eeb0df1 Compare April 24, 2022 20:22

Misc.

2fbf3f7

cston force-pushed the 60862 branch from d682705 to 2fbf3f7 Compare April 24, 2022 21:41

Fix tests

b104c6b

cston marked this pull request as ready for review April 25, 2022 01:05

cston requested review from a team as code owners April 25, 2022 01:05

cston requested a review from CyrusNajmabadi April 25, 2022 17:45

jcouv reviewed Apr 25, 2022

View reviewed changes

jcouv self-assigned this Apr 25, 2022

jcouv approved these changes Apr 25, 2022

View reviewed changes

AlekseyTs approved these changes Apr 28, 2022

View reviewed changes

cston added 3 commits April 29, 2022 13:26

Address PR feedback

de84738

Merge remote-tracking branch 'upstream/main' into 60862

f68083f

Fix tests

ccb3c5d

AlekseyTs approved these changes May 2, 2022

View reviewed changes

cston merged commit 0c31b36 into dotnet:main May 2, 2022

cston deleted the 60862 branch May 2, 2022 21:18

ghost added this to the Next milestone May 2, 2022

Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022

Youssef1313 mentioned this pull request Jun 23, 2022

Assert diagnostics pass correct number of arguments #59419

Open

	[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
	public sealed class AnalyzerReportingMisformattedDiagnostic : DiagnosticAnalyzer
	{
	public static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(
	"ID1",
	"Title1",
	"Symbol Name: {0}, Extra argument: {1}",
	"Category1",
	defaultSeverity: DiagnosticSeverity.Warning,
	isEnabledByDefault: true);

	public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
	public override void Initialize(AnalysisContext context)
	{
	context.RegisterSymbolAction(symbolContext =>
	{
	// Report diagnostic with incorrect number of message format arguments.
	symbolContext.ReportDiagnostic(Diagnostic.Create(Rule, symbolContext.Symbol.Locations[0], symbolContext.Symbol.Name));
	}, SymbolKind.NamedType);
	}
	}

Conversation

cston commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 Apr 23, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Apr 29, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Apr 23, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Apr 23, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Apr 23, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 29, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Apr 23, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Apr 23, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 29, 2022

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 25, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 29, 2022

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 25, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cston commented Apr 23, 2022 •

edited

Loading

Youssef1313 Apr 23, 2022 •

edited by cston

Loading

cston Apr 29, 2022 •

edited

Loading

Youssef1313 Apr 23, 2022 •

edited by cston

Loading

Youssef1313 Apr 23, 2022 •

edited by cston

Loading

Youssef1313 Apr 23, 2022 •

edited by cston

Loading

Youssef1313 Apr 23, 2022 •

edited by cston

Loading

cston Apr 23, 2022 •

edited

Loading

Youssef1313 Apr 23, 2022 •

edited by cston

Loading

jcouv Apr 25, 2022 •

edited by cston

Loading

jcouv Apr 25, 2022 •

edited by cston

Loading