Skip to content

Assert diagnostics pass correct number of arguments#59419

Open
Youssef1313 wants to merge 7 commits into
dotnet:mainfrom
Youssef1313:patch-12
Open

Assert diagnostics pass correct number of arguments#59419
Youssef1313 wants to merge 7 commits into
dotnet:mainfrom
Youssef1313:patch-12

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

Related to #59418

@ghost ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Analyzers labels Feb 9, 2022
@Youssef1313 Youssef1313 changed the title Assert IDExxxx and ENCxxxx passes correct number of arguments Assert diagnostics passes correct number of arguments Feb 9, 2022
@Youssef1313 Youssef1313 changed the title Assert diagnostics passes correct number of arguments Assert diagnostics pass correct number of arguments Feb 9, 2022
@davidwengier
Copy link
Copy Markdown
Member

I appreciate the thought, but this would not have helped uncover the linked issue, so I'm not sure this has much value from an ENC standpoint. I guess someone from @dotnet/roslyn-compiler might have a different opinion?

@Youssef1313
Copy link
Copy Markdown
Member Author

@davidwengier I'm surprised the assertion didn't fail for ENC0088. I'll try to take a look and understand why.

@davidwengier
Copy link
Copy Markdown
Member

@davidwengier I'm surprised the assertion didn't fail for ENC0088. I'll try to take a look and understand why.

That diagnostic is only reported when an unknown error happens. We don't have tests for unknown errors, because if we knew what the error was we'd fix it :)

@Youssef1313
Copy link
Copy Markdown
Member Author

I thought this test is "faking" the unexpected exception via the faultInjector parameter:

var expectedDiagnostic = outOfMemory ?
Diagnostic(RudeEditKind.MemberBodyTooBig, "public static void G()", FeaturesResources.method) :
Diagnostic(RudeEditKind.MemberBodyInternalError, "public static void G()", FeaturesResources.method);

@davidwengier
Copy link
Copy Markdown
Member

Well now I'm confused.. if we had a test for that scenario, then why is it passing at all, given the bug?

I will have to dig in more

@davidwengier
Copy link
Copy Markdown
Member

@Youssef1313 I updated #59443 to validate all EnC diagnostics.

Do you think there is still value in this PR for general compiler scenarios?

@Youssef1313
Copy link
Copy Markdown
Member Author

@davidwengier I think it will be beneficial, not only for compiler messages, but also IDExxxx.

I'll have to make sure this actually fails for both scenarios.

@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Jun 23, 2022

I think it will be beneficial, not only for compiler messages, but also IDExxxx.

Compiler part was already done in #60925. This PR is now about analyzers.

@Youssef1313 Youssef1313 marked this pull request as ready for review June 23, 2022 20:53
@Youssef1313 Youssef1313 requested review from a team as code owners June 23, 2022 20:53
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approve

…dateFormatStringDiagnosticAnalyzer.cs

Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
Copy link
Copy Markdown
Member Author

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wait on merging this. I think this won't detect everything because of the early return for _messageArgs.Length == 0. Fixed in cfd414c. Waiting on CI to see if there are additional failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Analyzers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants