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

Add support for displaying reference kind in Find All References window. #30155

Merged
merged 8 commits into from
Oct 3, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public async Task OnReferenceFoundAsync(Document document, TextSpan span)
var documentSpan = await ClassifiedSpansAndHighlightSpanFactory.GetClassifiedDocumentSpanAsync(
document, span, _context.CancellationToken).ConfigureAwait(false);
await _context.OnReferenceFoundAsync(new SourceReferenceItem(
_definition, documentSpan, isWrittenTo: false)).ConfigureAwait(false);
_definition, documentSpan, ValueUsageInfo.None)).ConfigureAwait(false);
}

public Task ReportProgressAsync(int current, int maximum)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ public static async Task<SourceReferenceItem> TryCreateSourceReferenceItemAsync(
var documentSpan = await ClassifiedSpansAndHighlightSpanFactory.GetClassifiedDocumentSpanAsync(
document, sourceSpan, cancellationToken).ConfigureAwait(false);

return new SourceReferenceItem(definitionItem, documentSpan, referenceLocation.IsWrittenTo);
return new SourceReferenceItem(definitionItem, documentSpan, referenceLocation.ValueUsageInfo);
}

private static SymbolDisplayFormat GetFormat(ISymbol definition)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ protected override async Task<ImmutableArray<FinderLocation>> FindReferencesInDo
// FAR on the Delegate type and use those results to find Invoke calls

var syntaxFactsService = document.GetLanguageService<ISyntaxFactsService>();
var semanticFactsService = document.GetLanguageService<ISemanticFactsService>();

var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var nodes = root.DescendantNodes();
Expand All @@ -104,7 +105,8 @@ protected override async Task<ImmutableArray<FinderLocation>> FindReferencesInDo
.Where(e => semanticModel.GetSymbolInfo(e, cancellationToken).Symbol.OriginalDefinition == methodSymbol);

return invocations.Concat(convertedAnonymousFunctions).SelectAsArray(
n => new FinderLocation(n, new ReferenceLocation(document, null, n.GetLocation(), isImplicit: false, isWrittenTo: false, candidateReason: CandidateReason.None)));
n => new FinderLocation(n, new ReferenceLocation(document, null, n.GetLocation(), isImplicit: false,
valueUsageInfo: semanticModel.GetValueUsageInfo(n, semanticFactsService, cancellationToken), candidateReason: CandidateReason.None)));
}
}
}
45 changes: 45 additions & 0 deletions src/Features/Core/Portable/FindUsages/SourceReferenceItem.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Concurrent;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.FindUsages
{
Expand All @@ -10,6 +12,11 @@ namespace Microsoft.CodeAnalysis.FindUsages
/// </summary>
internal sealed class SourceReferenceItem
{
// We can have only a handful of different values for ValueUsageInfo flags enum, so the maximum size of this dictionary is capped.
// So, we store this as a static dictionary which will be held in memory for the lifetime of the process.
private static readonly ConcurrentDictionary<ValueUsageInfo, MultiDictionary<string, string>> s_valueUsageInfoToReferenceInfoMap
= new ConcurrentDictionary<ValueUsageInfo, MultiDictionary<string, string>>();

/// <summary>
/// The definition this reference corresponds to.
/// </summary>
Expand All @@ -25,11 +32,49 @@ internal sealed class SourceReferenceItem
/// </summary>
public bool IsWrittenTo { get; }

/// <summary>
/// Additional information about the reference.
/// Each entry represents a key-values pair of data. For example, consider the below entry:
/// { "ValueUsageInfo" } = { "Read", "Write" }
/// This entry indicates that the reference has additional value usage information which indicate
/// it is a read/write reference, such as say 'a++'.
/// </summary>
public MultiDictionary<string, string> ReferenceInfo { get; }

[Obsolete]
public SourceReferenceItem(DefinitionItem definition, DocumentSpan sourceSpan, bool isWrittenTo)
{
Definition = definition;
SourceSpan = sourceSpan;
IsWrittenTo = isWrittenTo;
ReferenceInfo = GetOrCreateReferenceInfo(ValueUsageInfo.None);
}

public SourceReferenceItem(DefinitionItem definition, DocumentSpan sourceSpan, MultiDictionary<string, string> referenceInfo)
{
Definition = definition;
SourceSpan = sourceSpan;
ReferenceInfo = referenceInfo ?? throw new ArgumentNullException(nameof(referenceInfo));
}

internal SourceReferenceItem(DefinitionItem definition, DocumentSpan sourceSpan, ValueUsageInfo valueUsageInfo)
Copy link
Contributor

@heejaechang heejaechang Sep 26, 2018

Choose a reason for hiding this comment

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

how about having ctro that contains both isWrittenTo and ReferenceInfo as parameter? #ByDesign

Copy link
Contributor Author

@mavasani mavasani Sep 26, 2018

Choose a reason for hiding this comment

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

I have maked the the ctor with IsWrittenTo as obsolete. @CyrusNajmabadi told me that TS uses it, so I did not remove it. Hopefully they can move to new ctor and then we can completely remove IsWrittenTo and obsolete ctor. #ByDesign

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Sep 27, 2018

Choose a reason for hiding this comment

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

Note: i'm not certain if they use it. might as well check. if tehy don't, def just remove :) #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified that TypeScript does still use the constructor public SourceReferenceItem(DefinitionItem definition, DocumentSpan sourceSpan, bool isWrittenTo). I am going to retain the existing approach.


In reply to: 220759901 [](ancestors = 220759901)

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 2, 2018

Choose a reason for hiding this comment

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

on the fence about ValueUsageInfo being baked into this API directly. I think i'd prefer if it just took a MultiDictionary<string, string> ReferenceInfo as an argument instead.

Other minor note: Is MultiDictionary mutable? If so... i may have to retract my position about using it since we'd want immutable data here. If it is immutbale, great! #Resolved

Copy link
Contributor Author

@mavasani mavasani Oct 2, 2018

Choose a reason for hiding this comment

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

Is MultiDictionary mutable

Yes, it is. I will revert it back to ImmutableDictionary<string, ImmutableArray<string>>. #Resolved

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 2, 2018

Choose a reason for hiding this comment

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

Darn... sorry about that :(

#Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We invoke this from multiple places and we are caching the mapping from ValueUsageInfo to the reference info values map here, so for now I will retain this here.


In reply to: 222055535 [](ancestors = 222055535)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No issues :) moved back to ImmutableDictionary.


In reply to: 222063382 [](ancestors = 222063382)

: this(definition, sourceSpan, GetOrCreateReferenceInfo(valueUsageInfo))
{
IsWrittenTo = valueUsageInfo.IsWrittenTo();
}

private static MultiDictionary<string, string> GetOrCreateReferenceInfo(ValueUsageInfo valueUsageInfo)
=> s_valueUsageInfoToReferenceInfoMap.GetOrAdd(valueUsageInfo, CreateReferenceInfo);
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 3, 2018

Choose a reason for hiding this comment

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

i can't remember. will this cache the dleegate for CreateReferenceInfo? I don't think it will. So we can either fix this to explicitly cache that delegate, or switch to a simple lambda that calls the function (since hte compiler will cache htat). #Resolved


private static MultiDictionary<string, string> CreateReferenceInfo(ValueUsageInfo valueUsageInfo)
{
var referenceInfo = new MultiDictionary<string, string>();
foreach (var value in valueUsageInfo.ToLocalizableValues())
{
referenceInfo.Add(nameof(ValueUsageInfo), value);
}

return referenceInfo;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ private static bool IsFieldWrite(IFieldReferenceOperation fieldReference, ISymbo
{
// Check if the underlying member is being written or a writable reference to the member is taken.
var valueUsageInfo = fieldReference.GetValueUsageInfo();
if (!valueUsageInfo.ContainsWriteOrWritableRef())
if (!valueUsageInfo.IsWrittenTo())
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Collections.Immutable;
Expand Down Expand Up @@ -263,7 +263,7 @@ private void OnSymbolEnd(SymbolAnalysisContext symbolEndContext, bool hasInvalid
// Check if the underlying member is neither read nor a readable reference to the member is taken.
// If so, we flag the member as either unused (never written) or unread (written but not read).
if (TryRemove(member, out var valueUsageInfo) &&
!valueUsageInfo.ContainsReadOrReadableRef())
!valueUsageInfo.IsReadFrom())
{
Debug.Assert(IsCandidateSymbol(member));
Debug.Assert(!member.IsImplicitlyDeclared);
Expand Down Expand Up @@ -299,7 +299,7 @@ private void OnSymbolEnd(SymbolAnalysisContext symbolEndContext, bool hasInvalid
}

// Report IDE0051 or IDE0052 based on whether the underlying member has any Write/WritableRef/NonReadWriteRef references or not.
var rule = !valueUsageInfo.ContainsWriteOrWritableRef() && !valueUsageInfo.ContainsNonReadWriteRef() && !symbolsReferencedInDocComments.Contains(member)
var rule = !valueUsageInfo.IsWrittenTo() && !valueUsageInfo.IsNameReference() && !symbolsReferencedInDocComments.Contains(member)
? s_removeUnusedMembersRule
: s_removeUnreadMembersRule;

Expand Down Expand Up @@ -466,4 +466,4 @@ private bool IsEntryPoint(IMethodSymbol methodSymbol)
methodSymbol.ReturnType.OriginalDefinition == _genericTaskType);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using Microsoft.VisualStudio.Shell.TableControl;
using Roslyn.Utilities;

namespace Microsoft.VisualStudio.LanguageServices.FindUsages
{
/// <summary>
/// Implementation of a custom, dynamic column for the Find All References window.
/// </summary>
internal abstract class AbstractFindUsagesCustomColumnDefinition : TableColumnDefinitionBase
{
protected AbstractFindUsagesCustomColumnDefinition()
Copy link
Contributor

@heejaechang heejaechang Sep 26, 2018

Choose a reason for hiding this comment

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

tagging @olegtk for eyes from editor team. I can't find github alias for david pugh. #Pending

Copy link
Contributor Author

@mavasani mavasani Sep 26, 2018

Choose a reason for hiding this comment

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

Thanks, yes confirmation from Oleg and David will help. By the way, I verified that just defining a new column with export attribute doesn't cause it to be automatically displayed in FAR window even for us. Platform code explicitly adds only the fixed column set and any variable column has to be explicitly added using the TableControl.SetControlStates API. #Resolved

{
DefaultColumnState = new ColumnState2(Name, isVisible: false, DefaultWidth);
}

public ColumnState2 DefaultColumnState { get; }

public override bool TryGetFilterItems(ITableEntryHandle entry, out IEnumerable<string> filterItems)
{
// Determine the constituent strings for the display value in column, which should be used for applying the filter.
// For example, if value "Read, Write" is displayed in column for an entry, we will return "Read" and "Write" here,
// so filtering based on individual filter terms can be done.
if (IsFilterable &&
entry.TryGetValue(Name, out var value) &&
value is string displayString &&
!string.IsNullOrEmpty(displayString))
{
filterItems = SplitColumnDisplayValue(displayString);
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 2, 2018

Choose a reason for hiding this comment

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

how often is this called? once per item in the row? if so, we may want to cache the split values (and, if you've already done this, ignore this comment). #Resolved

Copy link
Contributor Author

@mavasani mavasani Oct 2, 2018

Choose a reason for hiding this comment

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

Yep, SplitColumnDisplayValue is an abstract method and its implementation caches the mapping from display value to split values. #Resolved

return true;
}

return base.TryGetFilterItems(entry, out filterItems);
}
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Sep 26, 2018

Choose a reason for hiding this comment

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

  1. what does this method do? #Resolved

Copy link
Contributor Author

@mavasani mavasani Sep 26, 2018

Choose a reason for hiding this comment

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

This basically determines the constituent strings for the display value in column, which should be used for applying the filter. For example, value "Read, Write" displayed in column for an entry, we will return "Read" and "Write" here, so filtering based on individual filter terms can be done. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment


In reply to: 220752126 [](ancestors = 220752126)


public abstract string GetDisplayStringForColumnValues(MultiDictionary<string, string>.ValueSet values);
protected abstract IEnumerable<string> SplitColumnDisplayValue(string displayValue);

protected static string JoinValues(MultiDictionary<string, string>.ValueSet values) => string.Join(", ", values.Order());
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 2, 2018

Choose a reason for hiding this comment

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

'Order' seems like an odd name... i can guess what it likely means, but it's not super clear. That said, htis is exceptionally minor, so i don't really care if we can't make something better. #Resolved

Copy link
Contributor Author

@mavasani mavasani Oct 2, 2018

Choose a reason for hiding this comment

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

'Order' seems like an odd name.

This will go away when I switch from MultiDictionary<string, string> to ImmutableDictionary<string, ImmutableArray<string> as the value array should be already in preferred order. #Resolved

protected static IEnumerable<string> SplitAndTrimValue(string displayValue) => displayValue.Split(',').Select(v => v.Trim());
}
}
Loading