Skip to content
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

Handle expressions and method calls #52127

Conversation

ryzngard
Copy link
Contributor

@ryzngard ryzngard commented Mar 24, 2021

The service should now handle tracking expressions and method calls from previous items correctly. It's hard to explain all done here, but the tests added should highlight new functionality pretty well.

TODO: VB Tests

@ryzngard ryzngard added the Feature - IDE Value Tracking Work being done for Value Tracking in the IDE label Mar 24, 2021
@ryzngard ryzngard marked this pull request as ready for review March 25, 2021 22:10
@ryzngard ryzngard requested a review from a team as a code owner March 25, 2021 22:10
@runfoapp runfoapp bot mentioned this pull request Mar 25, 2021

namespace Microsoft.CodeAnalysis.ValueTracking
{
internal interface IValueTrackingService : IWorkspaceService
{
Task<ImmutableArray<ValueTrackedItem>> TrackValueSourceAsync(Solution solution, ISymbol symbol, CancellationToken cancellationToken);
Task TrackValueSourceAsync(Solution solution, ISymbol symbol, ValueTrackingProgressCollector progressCollector, CancellationToken cancellationToken);
Task<ImmutableArray<ValueTrackedItem>> TrackValueSourceAsync(TextSpan selection, Document document, CancellationToken cancellationToken);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't the non-progress-reporting version just an extension/helper method that just collects the items and returns them all in one go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It basically is... I can add as an implementation in the interface rather than leaving it as a contract. That seems more clear than an extension method just for code navigation

var node = location.FindNode(cancellationToken);
var sourceDoc = document.Project.Solution.GetRequiredDocument(location.SourceTree);
var syntaxFacts = sourceDoc.GetRequiredLanguageService<ISyntaxFactsService>();
var returnStatements = node.DescendantNodesAndSelf().Where(n => syntaxFacts.IsReturnStatement(n)).ToImmutableArray();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to ensure it doesn't descend into nested local functions or lambdas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure yet, after finishing up some of the expression and method call situations lambdas and locals are next


public ValueTask OnFindInDocumentStartedAsync(Document document) => new();
await TrackExpressionAsync(operation, sourceDoc, progressCollector, cancellationToken).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is identical to the if block?

var expression = node.DescendantNodesAndSelf().First(syntaxFacts.IsMethodBody);
if (expression is null)
{
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be continue, so processing out params isn't skipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me get a test for out params, but I think you're right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does need to continue, but there are some nuances that need to be handled for adding tests. I'm going to do that in the next PR and address this issue. Will link once the PR is up

Co-authored-by: David Wengier <david.wengier@microsoft.com>
Copy link
Contributor

@jmarolf jmarolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@ghost ghost merged commit ca1e841 into dotnet:features/value_tracking Mar 30, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE auto-merge Feature - IDE Value Tracking Work being done for Value Tracking in the IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants