-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Convert AssignOutParametersAboveReturnTests to the new test framework #41758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| char M(out int i, out string s) | ||
| { | ||
| [|return 'a';|] | ||
| {|CS0177:{|CS0177:return 'a';|}|} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, duplicate diagnostics need nested annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interested in hearing what this is about as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All diagnostics must be declared. There is no such thing as a "nested" annotation; this is just a visual artifact caused by one diagnostic's span being a superset of another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one diagnostic's span being a superset of another.
but in this case, the spans are identical.
- does this indicate an issue with the analyzer (producing duplicate diagnostics)
- if they're identical, does the test need to declare both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no analyzer here. The compiler reported both diagnostics.
All diagnostics must be declared. It is common for distinct diagnostics to appear in the same location, e.g. when the message differs.
| { | ||
| var generator = editor.Generator; | ||
|
|
||
| if (exprOrStatement is LocalFunctionStatementSyntax { ExpressionBody: { } localFunctionExpressionBody }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tagging @CyrusNajmabadi for the product change, as I believe he added these code fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results here were totally wrong. The code fix made a change in an unrelated method, with two impacts:
- The original diagnostic was still reported in the same place, with a code fix offered
- The iterative application of code fixes was an infinite loop, adding more incorrect code with each iteration
There was no way to port the test of local functions without this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yikes!
mavasani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test changes LGTM
src/EditorFeatures/CSharpTest/AssignOutParameters/AssignOutParametersAboveReturnTests.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/AssignOutParameters/AssignOutParametersAboveReturnTests.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/AssignOutParameters/AssignOutParametersAboveReturnTests.cs
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/AssignOutParameters/AssignOutParametersAboveReturnTests.cs
Show resolved
Hide resolved
...eatures/CSharp/Portable/AssignOutParameters/AssignOutParametersAboveReturnCodeFixProvider.cs
Show resolved
Hide resolved
|
@sharwell because of the largely mechanical nature of the test changes, it's somewhat tedious to review. If possible, would you be willing to go through and call out anything of particular interest (in the test changes) to make the review more expedient? That way we can focus on the important bits, skim the rest, and get through a lot faster. Thanks! |
| void X() | ||
| { | ||
| char D(out int i) => [|'a';|] | ||
| char {|CS0177:D|}(out int i) => 'a'; | ||
| D(out _); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Prior to my change, the code fix for this produced this method:
void X()
{
i = 0;
char D(out int i) => 'a';
D(out _);
}
@CyrusNajmabadi I've now commented on the PRs sent to date. For future changes, I'll submit the PR as a draft, and include the annotations before marking it ready for review. |
No description provided.