-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Update 'use auto property' to support using the field
keyword
#74629
Changes from 119 commits
9afc7e1
62bdd75
538db7e
55da141
96b23b5
dee9526
4d227d3
0ede824
5f40ed7
afcbacb
4919f39
b300536
7924bbe
05c024d
0ea409b
b957b4c
6f61596
0461361
94d3381
6626a89
412382a
2de28ec
ef1b6fa
fd03d65
4a01d5a
f0d6151
117fe3b
6cfd56a
50c45b2
33543b1
1c55beb
f165c4f
2b068b2
7ed1c3d
3b372c4
774a128
0b5c055
118e271
35c9449
0227b36
198e936
5124ead
de3bd01
68379e2
e45c15d
39214ab
dfba3ec
51b2ff0
977afb7
5217fa8
fced203
d89097b
294a6a6
b6ee184
49f99ab
a41fa31
0a62a2b
1181126
8723be2
39365cd
219d868
b4717dc
74f6f88
fe4a559
e44b3df
cad7236
89f1063
247c82c
749e60b
c8f29d5
1858006
57d8b4d
1dc3a9b
64e8faa
85991db
b56c6c5
51e5848
81dcce4
3c56e61
7c894b2
3d9ff31
5af808c
169248a
495e059
9f92651
4b7b54d
117790d
8932350
c7e3c17
fdb4c3c
5ede96b
2764566
6f0818a
5f96092
a1a2064
8de2fec
cf5921c
961e6a7
868a104
b9fc597
84be2ba
a578fa8
d682631
8dbe835
f19ef31
0bab2c9
044defd
d083c2d
58ada63
0465a2e
3af227d
5f5769d
fe14219
a9587e0
dc71576
9f72f65
8344b18
9d56f29
c1e5941
fb9f627
03f275b
edd7fad
1e0c11a
4ec0be4
6f13ed9
b00be4e
f7a30a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using System.Collections.Concurrent; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Threading; | ||
|
@@ -29,6 +30,12 @@ internal sealed class CSharpUseAutoPropertyAnalyzer : AbstractUseAutoPropertyAna | |
protected override SyntaxKind PropertyDeclarationKind | ||
=> SyntaxKind.PropertyDeclaration; | ||
|
||
protected override bool CanExplicitInterfaceImplementationsBeFixed | ||
=> false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just a move up. |
||
|
||
protected override bool SupportsFieldAttributesOnProperties | ||
=> true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new support. We used to block converting a field/property pair to an auto prop if hte field had attributes. Now we suppor this in c# as you can convert the attribute to |
||
|
||
protected override ISemanticFacts SemanticFacts | ||
=> CSharpSemanticFacts.Instance; | ||
|
||
|
@@ -38,31 +45,49 @@ protected override bool SupportsReadOnlyProperties(Compilation compilation) | |
protected override bool SupportsPropertyInitializer(Compilation compilation) | ||
=> compilation.LanguageVersion() >= LanguageVersion.CSharp6; | ||
|
||
protected override bool CanExplicitInterfaceImplementationsBeFixed() | ||
=> false; | ||
protected override bool SupportsFieldExpression(Compilation compilation) | ||
=> compilation.LanguageVersion() >= LanguageVersion.CSharp13; | ||
|
||
protected override ExpressionSyntax? GetFieldInitializer(VariableDeclaratorSyntax variable, CancellationToken cancellationToken) | ||
=> variable.Initializer?.Value; | ||
|
||
protected override void RegisterIneligibleFieldsAction( | ||
protected override bool ContainsFieldExpression(PropertyDeclarationSyntax propertyDeclaration, CancellationToken cancellationToken) | ||
{ | ||
foreach (var node in propertyDeclaration.DescendantNodes()) | ||
{ | ||
if (node.IsKind(SyntaxKind.FieldExpression)) | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
protected override void RecordIneligibleFieldLocations( | ||
HashSet<string> fieldNames, | ||
ConcurrentSet<IFieldSymbol> ineligibleFields, | ||
ConcurrentDictionary<IFieldSymbol, ConcurrentSet<SyntaxNode>> ineligibleFields, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we used to just mark a field as being ineligible depending on how it was used in teh file. Now we keep track of the places that previously made it ineligible. If those are outside of the property being converted, we still can't convert (As those external locations can't access the field anymore). But if they are inside of hte proeprty being converted, it is fine as inside the property we can use the new |
||
SemanticModel semanticModel, | ||
SyntaxNode codeBlock, | ||
CancellationToken cancellationToken) | ||
{ | ||
foreach (var argument in codeBlock.DescendantNodesAndSelf().OfType<ArgumentSyntax>()) | ||
{ | ||
// An argument will disqualify a field if that field is used in a ref/out position. | ||
// We can't change such field references to be property references in C#. | ||
// We can't change such field references to be property references in C#, unless we | ||
// are converting to the `field` keyword. | ||
if (argument.RefKindKeyword.Kind() != SyntaxKind.None) | ||
AddIneligibleFieldsForExpression(argument.Expression); | ||
|
||
// Use of a field in a nameof(...) expression can't *ever* be converted to use `field`. | ||
// So hard block in this case. | ||
if (argument.Expression.IsNameOfArgumentExpression()) | ||
AddIneligibleFieldsForExpression(argument.Expression, alwaysRestricted: true); | ||
} | ||
|
||
foreach (var refExpression in codeBlock.DescendantNodesAndSelf().OfType<RefExpressionSyntax>()) | ||
AddIneligibleFieldsForExpression(refExpression.Expression); | ||
|
||
// Can't take the address of an auto-prop. So disallow for fields that we do `&x` on. | ||
// Can't take the address of an auto-prop. So disallow for fields that we do `&x` on. Unless we are converting | ||
// to the `field` keyword. | ||
foreach (var addressOfExpression in codeBlock.DescendantNodesAndSelf().OfType<PrefixUnaryExpressionSyntax>()) | ||
{ | ||
if (addressOfExpression.Kind() == SyntaxKind.AddressOfExpression) | ||
|
@@ -72,60 +97,74 @@ protected override void RegisterIneligibleFieldsAction( | |
foreach (var memberAccess in codeBlock.DescendantNodesAndSelf().OfType<MemberAccessExpressionSyntax>()) | ||
{ | ||
if (CouldReferenceField(memberAccess)) | ||
AddIneligibleFieldsIfAccessedOffNotDefinitelyAssignedValue(semanticModel, memberAccess, ineligibleFields, cancellationToken); | ||
AddIneligibleFieldsIfAccessedOffNotDefinitelyAssignedValue(memberAccess); | ||
} | ||
|
||
return; | ||
|
||
bool CouldReferenceField(ExpressionSyntax expression) | ||
{ | ||
// Don't bother binding if the expression isn't even referencing the name of a field we know about. | ||
var rightmostName = expression.GetRightmostName()?.Identifier.ValueText; | ||
return rightmostName != null && fieldNames.Contains(rightmostName); | ||
} | ||
|
||
void AddIneligibleFieldsForExpression(ExpressionSyntax expression) | ||
void AddIneligibleFieldsForExpression(ExpressionSyntax expression, bool alwaysRestricted = false) | ||
{ | ||
if (!CouldReferenceField(expression)) | ||
return; | ||
|
||
var symbolInfo = semanticModel.GetSymbolInfo(expression, cancellationToken); | ||
AddIneligibleFields(ineligibleFields, symbolInfo); | ||
AddIneligibleFields(symbolInfo, expression, alwaysRestricted); | ||
} | ||
} | ||
|
||
private static void AddIneligibleFieldsIfAccessedOffNotDefinitelyAssignedValue( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved helpers to be inner functions. |
||
SemanticModel semanticModel, MemberAccessExpressionSyntax memberAccess, ConcurrentSet<IFieldSymbol> ineligibleFields, CancellationToken cancellationToken) | ||
{ | ||
// `c.x = ...` can't be converted to `c.X = ...` if `c` is a struct and isn't definitely assigned as that point. | ||
void AddIneligibleFieldsIfAccessedOffNotDefinitelyAssignedValue( | ||
MemberAccessExpressionSyntax memberAccess) | ||
{ | ||
// `c.x = ...` can't be converted to `c.X = ...` if `c` is a struct and isn't definitely assigned as that point. | ||
|
||
// only care about writes. if this was a read, then it must be def assigned and thus is safe to convert to a prop. | ||
if (!memberAccess.IsOnlyWrittenTo()) | ||
return; | ||
// only care about writes. if this was a read, then it must be def assigned and thus is safe to convert to a prop. | ||
if (!memberAccess.IsOnlyWrittenTo()) | ||
return; | ||
|
||
// this only matters for a field access off of a struct. They can be declared unassigned and have their | ||
// fields directly written into. | ||
var symbolInfo = semanticModel.GetSymbolInfo(memberAccess, cancellationToken); | ||
if (symbolInfo.GetAnySymbol() is not IFieldSymbol { ContainingType.TypeKind: TypeKind.Struct }) | ||
return; | ||
// this only matters for a field access off of a struct. They can be declared unassigned and have their | ||
// fields directly written into. | ||
var symbolInfo = semanticModel.GetSymbolInfo(memberAccess, cancellationToken); | ||
if (symbolInfo.GetAnySymbol() is not IFieldSymbol { ContainingType.TypeKind: TypeKind.Struct }) | ||
return; | ||
|
||
var exprSymbol = semanticModel.GetSymbolInfo(memberAccess.Expression, cancellationToken).GetAnySymbol(); | ||
if (exprSymbol is not IParameterSymbol and not ILocalSymbol) | ||
return; | ||
var exprSymbol = semanticModel.GetSymbolInfo(memberAccess.Expression, cancellationToken).GetAnySymbol(); | ||
if (exprSymbol is not IParameterSymbol and not ILocalSymbol) | ||
return; | ||
|
||
var dataFlow = semanticModel.AnalyzeDataFlow(memberAccess.Expression); | ||
if (dataFlow != null && !dataFlow.DefinitelyAssignedOnEntry.Contains(exprSymbol)) | ||
AddIneligibleFields(ineligibleFields, symbolInfo); | ||
} | ||
var dataFlow = semanticModel.AnalyzeDataFlow(memberAccess.Expression); | ||
if (dataFlow != null && !dataFlow.DefinitelyAssignedOnEntry.Contains(exprSymbol)) | ||
AddIneligibleFields(symbolInfo, memberAccess); | ||
} | ||
|
||
private static void AddIneligibleFields(ConcurrentSet<IFieldSymbol> ineligibleFields, SymbolInfo symbolInfo) | ||
{ | ||
AddIneligibleField(symbolInfo.Symbol); | ||
foreach (var symbol in symbolInfo.CandidateSymbols) | ||
AddIneligibleField(symbol); | ||
void AddIneligibleFields( | ||
SymbolInfo symbolInfo, | ||
SyntaxNode location, | ||
bool alwaysRestricted = false) | ||
{ | ||
AddIneligibleField(symbolInfo.Symbol, location, alwaysRestricted); | ||
foreach (var symbol in symbolInfo.CandidateSymbols) | ||
AddIneligibleField(symbol, location, alwaysRestricted); | ||
} | ||
|
||
void AddIneligibleField(ISymbol? symbol) | ||
void AddIneligibleField( | ||
ISymbol? symbol, | ||
SyntaxNode location, | ||
bool alwaysRestricted) | ||
{ | ||
// If the field is always restricted, then add the compilation unit itself to the ineligibility locations. | ||
// that way we never think we can convert this field. | ||
if (symbol is IFieldSymbol field) | ||
ineligibleFields.Add(field); | ||
{ | ||
AddFieldUsage(ineligibleFields, field, alwaysRestricted | ||
? location.SyntaxTree.GetRoot(cancellationToken) | ||
: location); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -181,7 +220,7 @@ private static bool CheckExpressionSyntactically(ExpressionSyntax expression) | |
=> accessorDeclaration is { Body.Statements: [T statement] } ? statement : null; | ||
|
||
protected override ExpressionSyntax? GetSetterExpression( | ||
IMethodSymbol setMethod, SemanticModel semanticModel, CancellationToken cancellationToken) | ||
SemanticModel semanticModel, IMethodSymbol setMethod, CancellationToken cancellationToken) | ||
{ | ||
// Setter has to be of the form: | ||
// | ||
|
@@ -213,4 +252,24 @@ protected override SyntaxNode GetFieldNode( | |
? fieldDeclaration | ||
: variableDeclarator; | ||
} | ||
|
||
protected override void AddAccessedFields( | ||
SemanticModel semanticModel, | ||
IMethodSymbol accessor, | ||
HashSet<string> fieldNames, | ||
HashSet<IFieldSymbol> result, | ||
CancellationToken cancellationToken) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new helper that analyzed the code inside an accessor to see which fields (from teh containing type) that were accessed. These fields will then be checked to see if any could be replaced with |
||
{ | ||
var syntax = accessor.DeclaringSyntaxReferences[0].GetSyntax(cancellationToken); | ||
foreach (var descendant in syntax.DescendantNodesAndSelf()) | ||
{ | ||
cancellationToken.ThrowIfCancellationRequested(); | ||
|
||
if (descendant is IdentifierNameSyntax identifierName) | ||
{ | ||
result.AddIfNotNull(TryGetDirectlyAccessedFieldSymbol( | ||
semanticModel, identifierName, fieldNames, cancellationToken)); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,11 @@ | |
namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UseAutoProperty; | ||
|
||
[Trait(Traits.Feature, Traits.Features.CodeActionsUseAutoProperty)] | ||
public sealed class UseAutoPropertyTests(ITestOutputHelper logger) | ||
public sealed partial class UseAutoPropertyTests(ITestOutputHelper logger) | ||
: AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor(logger) | ||
{ | ||
private readonly ParseOptions CSharp12 = CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp12); | ||
|
||
internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace) | ||
=> (new CSharpUseAutoPropertyAnalyzer(), GetCSharpUseAutoPropertyCodeFixProvider()); | ||
|
||
|
@@ -667,7 +669,7 @@ class Class | |
} | ||
|
||
[Fact] | ||
public async Task TestGetterWithMutipleStatements() | ||
public async Task TestGetterWithMultipleStatements_CSharp12() | ||
{ | ||
await TestMissingInRegularAndScriptAsync( | ||
""" | ||
|
@@ -684,11 +686,11 @@ int P | |
} | ||
} | ||
} | ||
"""); | ||
""", new TestParameters(parseOptions: CSharp12)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not supported in c# 12. but will work in c# 13. tests for those are in teh sibling file added. |
||
} | ||
|
||
[Fact] | ||
public async Task TestSetterWithMutipleStatements() | ||
public async Task TestSetterWithMultipleStatements_CSharp12() | ||
{ | ||
await TestMissingInRegularAndScriptAsync( | ||
""" | ||
|
@@ -731,7 +733,7 @@ int P | |
} | ||
} | ||
} | ||
"""); | ||
""", new TestParameters(parseOptions: CSharp12)); | ||
} | ||
|
||
[Fact] | ||
|
@@ -1153,9 +1155,9 @@ partial class Class | |
} | ||
|
||
[Fact] | ||
public async Task TestNotWithFieldWithAttribute() | ||
public async Task TestWithFieldWithAttribute() | ||
{ | ||
await TestMissingInRegularAndScriptAsync( | ||
await TestInRegularAndScriptAsync( | ||
""" | ||
class Class | ||
{ | ||
|
@@ -1170,6 +1172,13 @@ int P | |
} | ||
} | ||
} | ||
""", | ||
""" | ||
class Class | ||
{ | ||
[field: A] | ||
int P { get; } | ||
} | ||
"""); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we now support this in any version of hte language. |
||
} | ||
|
||
|
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.
view with whitespace off.