S1186: also inspect empty set and init and empty local functions #8584
Conversation
antonioaversa
force-pushed
the
antonio/S1186-improvement-empty-set-and-init
branch
5 times, most recently
from
f508ae5 to
64b148a
Compare
antonioaversa
changed the title
antonioaversa
force-pushed
the
antonio/S1186-improvement-empty-set-and-init
branch
3 times, most recently
from
6dbbfe2 to
63d629d
Compare
| class Inherited : Base | ||
| { | ||
| protected override int VirtualEmptyInitProp { | ||
| init |
There was a problem hiding this comment.
Note for the reviewer: in the presence of accessors with empty bodies, I have decided to go only one level up w.r.t. the empty body, when formatting the code fix, instead of formatting at the property level.
That is because the user may also have other accessors defined in the property, that are not involved in the code fix.
In that scenario, a code fix on the body of an accessor would end up formatting a potentially large portion of user code, that may not be what the user wants.
int AProperty
{
get { /* ... */ } // Get accessor with code, that the user may not want to be reformatted
init { }
}Let me know if you think that it would be better, in your opinion, to take the risk of reformatting unrelated user code, and have better formatting overall, and I will make the change.
|
Uncovered lines in ReturnEmptyCollectionInsteadOfNull.cs and SyntaxNodeExtensions.cs are not new, only refactored. |
antonioaversa
added
Sprint: Hardening
antonioaversa
changed the title
| @@ -68,41 +68,32 @@ void ReportIfAny(List<Location> nullOrDefaultLiterals) | |||
| } | |||
| } | |||
|
|
|||
| private static BlockSyntax GetBody(SyntaxNode node) => | |||
There was a problem hiding this comment.
Can't we remove this method and use the SyntaxExtensions?
There was a problem hiding this comment.
Discussed offline, this always returns the body of the get accessor. We would have to return an array instead of a single BlockSyntax.
| { | ||
| var symbol = context.SemanticModel.GetDeclaredSymbol(context.Node); | ||
| return symbol is IPropertySymbol property ? property.Type : ((IMethodSymbol)symbol).ReturnType; | ||
| } | ||
|
|
||
| private static ArrowExpressionClauseSyntax GetExpressionBody(SyntaxNode node) => |
There was a problem hiding this comment.
Wouldn't be better to move this to SyntaxNodeExtensions?
There was a problem hiding this comment.
Similar with above.
antonioaversa
force-pushed
the
antonio/S1186-improvement-empty-set-and-init
branch
from
a0eb27b to
1ae9e62
Compare
|
|
| }; | ||
|
|
||
| private static AccessorDeclarationSyntax GetAccessor(PropertyDeclarationSyntax property) => | ||
| property.AccessorList?.Accessors.FirstOrDefault(a => a.IsKind(SyntaxKind.GetAccessorDeclaration)); | ||
| property.AccessorList.Accessors.FirstOrDefault(x => x.IsKind(SyntaxKind.GetAccessorDeclaration)); |
There was a problem hiding this comment.
While the grammar defines the accessor_list production rule as optional, and an alternative to ((arrow_expression_clause | equals_value_clause) ';')), the AccessorList results non-null even on arrow expressions.
Since I can't produce any example of a property where AccessorList ends up being null (not even with an incomplete code example), I am removing the null propagation operator.
|
After the changes to the testing framework, coverage figures are not comparable with the ones we used to have. |
antonioaversa
deleted the
antonio/S1186-improvement-empty-set-and-init
branch
Fixes #3753