Skip to content

Commit

Permalink
Fix ValueTracking for index parameters (dotnet#57727)
Browse files Browse the repository at this point in the history
Index values weren't properly handled previously, and it was assumed all parameters existed in a method. In reality some exist in properties. Who knew!??!

No need to worry about the cast, symbol finder will do the right thing. Code does need to be updated to handle the fact that we want to track potentially two things with index: the argument passed in as the index in callsites, and potentially the item being indexed to. Update the FindReferencesProgress to handle references for index specifically.

Fixes dotnet#57100
  • Loading branch information
ryzngard authored Nov 16, 2021
1 parent 4d98fa2 commit c7521c9
Show file tree
Hide file tree
Showing 4 changed files with 197 additions and 3 deletions.
95 changes: 95 additions & 0 deletions src/EditorFeatures/Test/ValueTracking/CSharpValueTrackingTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -959,5 +959,100 @@ public static int GetM()

await ValidateChildrenEmptyAsync(workspace, items.Single());
}

[Theory]
[CombinatorialData]
public async Task TestIndex(TestHost testHost)
{
var code =
@"
class Test
{
public int this[string $$key] => 0;
public int M(Test localTest)
{
var assignedVariable = this[""test""];
System.Console.WriteLine(this[""test""]);
return localTest[""test""];
}
}";

//
// |> public int this[string [|key|]] => 0; [Code.cs:4]
// |> return [|localTest|][[|"test"|]]; [Code.cs:10]
// |> System.Console.WriteLine(this[[|"test"|]]); [Code.cs:8]
// |> var [|assignedVariable = this["test"]|]; [Code.cs:7]
using var workspace = CreateWorkspace(code, testHost);

var items = await ValidateItemsAsync(
workspace,
itemInfo: new[]
{
(3, "string key") // |>public int this[[|string key|]] => 0; [Code.cs:4]
});

items = await ValidateChildrenAsync(
workspace,
items.Single(),
childInfo: new[]
{
(10, "localTest"), // return [|localTest|]["test"]; [Code.cs:10] (This is included because it is part of a return statement, and follows same logic as other references for if it is tracked)
(10, "\"test\""), // return localTest[[|"test"|]]; [Code.cs:10]
(8, "\"test\""), // System.Console.WriteLine(this[[|"test"|]]); [Code.cs:8]
(7, "\"test\""), // var assignedVariable = this[[|"test"|]]; [Code.cs:7]
});

await ValidateChildrenEmptyAsync(workspace, items[0]);
await ValidateChildrenEmptyAsync(workspace, items[1]);
await ValidateChildrenEmptyAsync(workspace, items[2]);
await ValidateChildrenEmptyAsync(workspace, items[3]);
}

[Theory]
[CombinatorialData]
public async Task TestPropertyValue(TestHost testHost)
{
var code =
@"
class Test
{
private int _i;
public int I
{
get => _i;
set
{
_i = $$value;
}
}
public int M(Test localTest)
{
localTest.I = 5;
}
}";
// _i = [|value|]; [Code.cs:9]
// |> localTest.I = [|5|]; [Code.cs:15]
using var workspace = CreateWorkspace(code, testHost);

var items = await ValidateItemsAsync(
workspace,
itemInfo: new[]
{
(9, "value") // _i = [|value|]; [Code.cs:9]
});

items = await ValidateChildrenAsync(
workspace,
items.Single(),
childInfo: new[]
{
(15, "5") // localTest.I = [|5|]; [Code.cs:15]
});

await ValidateChildrenEmptyAsync(workspace, items.Single());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,58 @@ End Class
ValidateItem(initialItems[1], 3);
}

[Theory]
[CombinatorialData]
public async Task TestPropertyValue(TestHost testHost)
{
var code =
@"
Class C
Private _s As String
Public Property S() As String
Get
Return _s
End Get
Set(ByVal value As String)
_s = $$value
End Set
End Property
Public Sub SetS(s As String)
Me.S = s
End Sub
Public Function GetS() As String
Return Me.S
End Function
End Class
";

//
// _s = value [Code.vb:8]
// |> Me.S = s [Code.vb:14]
//
using var workspace = CreateWorkspace(code, testHost);

var items = await ValidateItemsAsync(
workspace,
itemInfo: new[]
{
(8, "value") // _s = [|value|] [Code.vb:8]
});

var childItems = await ValidateChildrenAsync(
workspace,
items.Single(),
childInfo: new[]
{
(14, "s") // Me.S = [|s|] [Code.vb:14]
});

await ValidateChildrenEmptyAsync(workspace, childItems.Single());
}

[Theory]
[CombinatorialData]
public async Task TestField(TestHost testHost)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public async ValueTask OnReferenceFoundAsync(SymbolGroup _, ISymbol symbol, Refe
else if (location.IsWrittenTo)
{
var syntaxFacts = location.Document.GetRequiredLanguageService<ISyntaxFactsService>();
var node = location.Location.FindNode(CancellationToken.None);
var node = location.Location.FindNode(cancellationToken);

// Assignments to a member using a "this." or "Me." result in the node being an
// identifier and the parent of the node being the member access expression. The member
Expand All @@ -86,6 +86,53 @@ public async ValueTask OnReferenceFoundAsync(SymbolGroup _, ISymbol symbol, Refe
await _operationCollector.VisitAsync(operation, cancellationToken).ConfigureAwait(false);
}
}
else if (symbol is IPropertySymbol { IsIndexer: true } propertySymbol)
{
// If the location isn't written to but it's an index accessor we still want to follow
// it as a source to be tracked for the property itself. Specifically, we want to check
// invocation sites and track the arguments being used in the invocation and potentially
// the expression being indexed to, if it is relavent (as determined by the OperationCollector).
var node = location.Location.FindNode(cancellationToken);
var syntaxFacts = location.Document.GetRequiredLanguageService<ISyntaxFactsService>();

var elementAccess = node.FirstAncestorOrSelf<SyntaxNode>(syntaxFacts.IsElementAccessExpression);
if (elementAccess is null)
{
return;
}

syntaxFacts.GetPartsOfElementAccessExpression(elementAccess, out var expression, out var argumentList);
var semanticModel = await location.Document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);

if (argumentList is not null)
{
foreach (var argument in syntaxFacts.GetArgumentsOfArgumentList(argumentList))
{
var argumentOperation = semanticModel.GetOperation(argument, cancellationToken);
if (argumentOperation is not null)
{
await _operationCollector.VisitAsync(argumentOperation, cancellationToken).ConfigureAwait(false);
}
}
}

if (expression is not null)
{
// We do not want to track "this" as part of an index operation, but we do
// want to track other variables that are accessed. Arguably they "contribute" even if
// not specifically to the argument of the element access.
if (syntaxFacts.IsThisExpression(expression))
{
return;
}

var expressionOperation = semanticModel.GetOperation(expression, cancellationToken);
if (expressionOperation is not null)
{
await _operationCollector.VisitAsync(expressionOperation, cancellationToken).ConfigureAwait(false);
}
}
}
}

public ValueTask OnStartedAsync(CancellationToken _) => new();
Expand Down
4 changes: 2 additions & 2 deletions src/Features/Core/Portable/ValueTracking/ValueTracker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,10 @@ private static async Task TrackParameterSymbolAsync(
OperationCollector collector,
CancellationToken cancellationToken)
{
var containingMethod = (IMethodSymbol)parameterSymbol.ContainingSymbol;
var containingSymbol = parameterSymbol.ContainingSymbol;
var findReferenceProgressCollector = new FindReferencesProgress(collector);
await SymbolFinder.FindReferencesAsync(
containingMethod,
containingSymbol,
collector.Solution,
findReferenceProgressCollector,
documents: null, FindReferencesSearchOptions.Default, cancellationToken).ConfigureAwait(false);
Expand Down

0 comments on commit c7521c9

Please sign in to comment.