Add a missing check for AccessorDeclaration#51409
Add a missing check for AccessorDeclaration#51409Cosifne merged 1 commit intodotnet:release/dev16.10from
Conversation
4e3ae9e to
b7f96f2
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@Youssef1313 Yes, I just realized I paste a wrong link here |
|
I should probably also ping @CyrusNajmabadi to review this |
|
L:ooking |
| } | ||
| } | ||
| } | ||
| }"); |
There was a problem hiding this comment.
can you explain how i should read these tests? i'm not really getting what they're trying to indicate.
There was a problem hiding this comment.
The first string passed to Test is the resulting string you get if you invoke Smart Break Line at any of the positions marked with $$ in the second string.
There was a problem hiding this comment.
:) Sam has answered the question
There was a problem hiding this comment.
Ick. That's super confusing :-)
| => accessorDeclarationNode.Body != null | ||
| && accessorDeclarationNode.Body.Statements.IsEmpty() | ||
| && accessorDeclarationNode.ExpressionBody == null | ||
| && accessorDeclarationNode.Parent != null |
There was a problem hiding this comment.
technically, this check isn't necessary. teh only thing that ever has a null parent is structured trivia or a compilation unit.
There was a problem hiding this comment.
:) I was just always trying to avoid use the '!' operator to suppress the null check. But indeed its parent is not null here. Given this PR is probably going to QB I feel it is better to keep the check here. I could open a PR targeting master to remove the check here
|
QB has approved the bug. And this needs an insertion. FYI @RikkiGibson |
Fix the bug found by Sam and it is caused because a missing check.
Also, add a few tests around the braces removal scenarios.
Tracking: #51404