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

mavasani · 2018-09-26T21:14:18Z

Implementation of the custom column support has been kept extensible, so that each reference item can have customized key-values info, where key corresponds to the name of the custom dynamic column and values are set of one or more values the table entry. Columns become dynamically visible if at least one row in the table has a value for it.

Fixes #24877

TODOs:

Finalize localization and terminology for the reference kinds (will need design review): Do we need to use localizable resource strings? Should the values be lot more succinct (like 'ref', 'out', 'in' 'read', 'write', etc.) or current descriptive terms are preferred?
The new column does not show up in the visible part of the FAR window by default, and user needs to scroll and move the column for visibility (see image below). This seems to be due to the fact that underlying table control does not refresh/update the column widths when a dynamic column becomes visible. I will follow up with the platform team to ensure the default view shows the custom columns, whenever marked visible.

Implementation of the custom column support has been kept extensible, so that each reference item can have customized key-values info, where key corresponds to the name of the custom dynamic column and values are set of one or more values the table entry. Columns become dynamically visible if at least one row in the table has a value for it. Fixes dotnet#24877

heejaechang · 2018-09-26T21:21:32Z

src/Features/Core/Portable/FindUsages/SourceReferenceItem.cs

+        /// 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 ImmutableDictionary<string, ImmutableArray<string>> ReferenceInfo { get; }


I believe this type will held in memory until find all reference window get cleared. so, might be good to not create new dictionary with same vaules again and again? but re-use dictionary with same values. #Resolved

I agree with heejae. Also, i think we already have a MultiDictionary. We should probably just use that. #Resolved

heejaechang · 2018-09-26T21:22:21Z

src/Features/Core/Portable/FindUsages/SourceReferenceItem.cs

+            ReferenceInfo = referenceInfo ?? throw new ArgumentNullException(nameof(referenceInfo));
+        }
+
+        internal SourceReferenceItem(DefinitionItem definition, DocumentSpan sourceSpan, ValueUsageInfo valueUsageInfo)


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

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

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

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)

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

Is MultiDictionary mutable

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

Darn... sorry about that :(

#Resolved

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)

No issues :) moved back to ImmutableDictionary.

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

heejaechang · 2018-09-26T21:23:18Z

src/Features/Core/Portable/FindUsages/SourceReferenceItem.cs

+        {
+            var referenceInfo = ImmutableDictionary<string, ImmutableArray<string>>.Empty;
+
+            var values = valueUsageInfo.ToValues();


here, you might have some cache to reuse existing dictionary? or most common one? or like last 10 unique ones? something like that. #Resolved

Sure. #Resolved

heejaechang · 2018-09-26T21:26:13Z

...ualStudio/Core/Def/Implementation/FindReferences/AbstractFindUsagesCustomColumnDefinition.cs

+    /// </summary>
+    internal abstract class AbstractFindUsagesCustomColumnDefinition : TableColumnDefinitionBase
+    {
+        protected AbstractFindUsagesCustomColumnDefinition()


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

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

heejaechang · 2018-09-26T21:28:12Z

...ualStudio/Core/Def/Implementation/FindReferences/FindUsagesValueUsageInfoColumnDefinition.cs

+    /// </summary>
+    [Export(typeof(ITableColumnDefinition))]
+    [Name(ColumnName)]
+    internal sealed class FindUsagesValueUsageInfoColumnDefinition : AbstractFindUsagesCustomColumnDefinition


@olegtk so this will not load roslyn when like C++ do find all references? right? #Pending

Definitely need to figure this out as well. #Resolved

The column doesn't seem to show unless explicitly added to the table control. MEF-exporting just seems to add it to the table column definition manager.

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

It's still not clear to me though when it is MEF imported. If it is imported even when C#/VB haven't been loaded... then that's not good :-/

Ah, get your point. Probably @olegtk can answer that. Note that our approach is same as what C++ itself has been using for it's custom read/write column, which has been in place for at least a release.

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

heejaechang · 2018-09-26T21:29:24Z

...Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/PropertySymbolReferenceFinder.cs

@@ -171,8 +172,9 @@ private static bool IsForEachProperty(IPropertySymbol symbol)
                    }

                    var location = argumentList.SyntaxTree.GetLocation(new TextSpan(argumentList.SpanStart, 0));
+                    var valueUsageInfo = semanticModel.GetValueUsageInfo(node, semanticFacts, cancellationToken);


curious, why semanticFacts is needed? #ByDesign

It will not be needed once #30123 is merged. For now, we use it for bunch of helpers. #Resolved

heejaechang · 2018-09-26T21:32:42Z

src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs

@@ -501,8 +504,9 @@ bool isRelevantDocument(SyntaxTreeIndex syntaxTreeInfo)
                if (deconstructMethods.Any(m => Matches(m, originalUnreducedSymbolDefinition)))
                {
                    var location = syntaxFacts.GetDeconstructionReferenceLocation(node);
+                    var valueUsageInfo = semanticModel.GetValueUsageInfo(node, semanticFacts, cancellationToken);


it has document. so might make sense to have exteion method on document then? document.GetValueUsageIfoAsync(node, cancellation)? rahter than one to retrieve things and then call on semanticmodel?

just nit thing since feels like it can save typing. #ByDesign

I added it on SemanticModel based on @CyrusNajmabadi suggestion here. Eventually it should probably go into the compiler layer. #Resolved

yes. i would prefer tihs be on semanticmodel. #Resolved

heejaechang · 2018-09-26T21:33:30Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs


                // After adding us as the source, the manager should immediately call into us to
                // tell us what the data sink is.
                Debug.Assert(_tableDataSink != null);
+
+                // Initialize and update custom column states.
+                _customColumnStatesMap = GetInitialCustomColumnStates(findReferencesWindow.TableControl.ColumnStates, customColumns, customColumnNames);


so mostly work is just adding this custom column to table control. so, review from editor team will be helpful @olegtk #Pending

Yes. I also need their help for the TODO (2) mentioned in PR description. Let me start an internal thread with them. #Pending

feels super weird to pass two sets along here. DOes hte GetInitialCustomColumnStates code assume anything about hte relation of their order? #Resolved

Passing the columnNames set as there is a contains check in it. I can also refactor to pass a single set. #Resolved

CyrusNajmabadi · 2018-09-26T23:18:17Z

...ualStudio/Core/Def/Implementation/FindReferences/AbstractFindUsagesCustomColumnDefinition.cs

+            }
+
+            return base.TryGetFilterItems(entry, out filterItems);
+        }


what does this method do? #Resolved

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

Added comment

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

CyrusNajmabadi · 2018-09-26T23:19:08Z

...ualStudio/Core/Def/Implementation/FindReferences/AbstractFindUsagesCustomColumnDefinition.cs

+
+        // Default implementation for column display value.
+        public virtual string GetDisplayStringForColumnValues(ImmutableArray<string> values) => string.Join(", ", values);
+        protected virtual IEnumerable<string> SplitColumnDisplayValue(string displayValue) => displayValue.Split(',').Select(v => v.Trim());


i'm concerned about this all being allocation heavy. do we do this once per item? Or just once per column/window? if it's per item, we should cache the results since tehre will likely only ever be a handful of actual distinct instances. #Resolved

Sure, I can optimize here as we have fixed set of combinations. #Resolved

CyrusNajmabadi · 2018-09-26T23:19:22Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

+            /// <summary>
+            /// Column states for custom columns.
+            /// </summary>
+            private readonly Dictionary<string, ColumnState2> _customColumnStatesMap;
            #endregion


nit: blank line above #endregion. #Resolved

just to be clear. You want this in this region right? i.e. this is intended ot be protected by _gate? if so, great. but i just want to check. #Resolved

Yes, correct - it currently has all uses protected by _gate.

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

CyrusNajmabadi · 2018-09-26T23:20:01Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

+            /// <summary>
+            /// Column states for custom columns.
+            /// </summary>
+            private readonly Dictionary<string, ColumnState2> _customColumnStatesMap;


Update docs (or name) to say what hte mapping is to/from. i don't know what this string actually is here. maybe columnTitleToStateMap? (if it is the title... just guessing here). #Resolved

columnTitleToStateMap? (if it is the title... just guessing here).

That is correct. Will change #Resolved

CyrusNajmabadi · 2018-09-26T23:20:48Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

            #endregion

            protected AbstractTableDataSourceFindUsagesContext(
                 StreamingFindUsagesPresenter presenter,
-                 IFindAllReferencesWindow findReferencesWindow)
+                 IFindAllReferencesWindow findReferencesWindow,
+                 ImmutableHashSet<AbstractFindUsagesCustomColumnDefinition> customColumns)


surprised this is a HashSet. any reason for that versus just an array? when i see 'set' it makes me thing "we'll have duplicates, and we need to make them unique." But it's unclear to me why we'd have multiple copies of the same column. #Resolved

Yes, will change to immutable array. #Resolved

CyrusNajmabadi · 2018-09-26T23:22:08Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

+
+                // Initialize and update custom column states.
+                _customColumnStatesMap = GetInitialCustomColumnStates(findReferencesWindow.TableControl.ColumnStates, customColumns, customColumnNames);
+                TableControl.SetColumnStates(_customColumnStatesMap.Values);


.values will be unordered here. is that what we want? if there are multiple columns, we could get non-deterministic ordering behavior now/in-the-future. #Resolved

I don't know if SetColumnStates can affect column ordering or just affects the state. @olegtk? #Resolved

I confirmed by looking at the editor code that order of the input column states is irrelevant for SetColumnStates.

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

I will add a comment.

In reply to: 221027314 [](ancestors = 221027314,220752779)

CyrusNajmabadi · 2018-09-26T23:22:34Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

+            {
+                var customColumnStatesMap = new Dictionary<string, ColumnState2>(customColumnNames.Count);
+
+                // If there is an existing column state for the custom column, flip it to be non-visible by default.


why...? #Resolved

The column will be completely empty for FAR on things such as types, literals, methods, no references found case, etc. Current implementation dynamically changes the column visibility based on whether at least one entry has a non-default value, and it seems to work pretty well. I can provide a gif animation for the experience here. #Resolved

CyrusNajmabadi · 2018-09-26T23:23:29Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

@@ -267,7 +306,8 @@ protected async Task<(Guid, string projectName, SourceText)> GetGuidAndProjectNa
            protected async Task<Entry> CreateDocumentSpanEntryAsync(
                RoslynDefinitionBucket definitionBucket,
                DocumentSpan documentSpan,
-                HighlightSpanKind spanKind)
+                HighlightSpanKind spanKind,
+                ImmutableDictionary<string, ImmutableArray<string>> customColumnsDataOpt = null)


Ideally, avoid optional params in internal code. It makes it too easy to miss a case when someone should not get the default, and doesn't know they need to pass this along. #Resolved

CyrusNajmabadi · 2018-09-26T23:24:18Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

+                    projectName, guid, sourceText, classifiedSpansAndHighlightSpan, GetAggregatedCustomColumnsData());
+
+                // Local functions.
+                ImmutableDictionary<string, string> GetAggregatedCustomColumnsData()


not certain what this is actually returning... what are the keys, whare the expected values? It's not clear what 'aggregated' means here. #Resolved

It's basically aggregating values. For example:

{ { "Column1", {"Value1", "Value2"} }, { "Column2", {"Value3", "Value4"} } }

will transform to:

{ { "Column1", "Value1, Value2" }, { "Column2", "Value3, Value4" } } ``` #Resolved

ok. doc comment then :) #Resolved

CyrusNajmabadi · 2018-09-26T23:27:49Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

+                // Update the column states if required.
+                if (newColumnStatesOpt != null)
+                {
+                    TableControl.SetColumnStates(newColumnStatesOpt);


is this safe on the BG thread?

You might be hearing about this from multiple threads simultaneously (find-refs is heavily multi-threaded). Is this safe? #Resolved

@olegtk? #Resolved

I just verified from looking at Editor layer code that this API forces switch to UI thread, so it should be safe to call here. Also note that we will call it only once for each new custom column to add for each FAR query - the lock above this guarantees that newColumnStatesOpt is going to be non-null only for the first FAR result that has a non-empty column value. #Resolved

great. consider doc'ing that to help make it clear this is all intentional and safe. thanks! #Resolved

CyrusNajmabadi · 2018-09-26T23:29:40Z

...ualStudio/Core/Def/Implementation/FindReferences/FindUsagesValueUsageInfoColumnDefinition.cs

+                                                                .Cast<ValueUsageInfo>()
+                                                                .Where(value => value != ValueUsageInfo.None && (value & (value - 1)) == 0) // Single bit set
+                                                                .Select(v => v.ToString())
+                                                                .ToImmutableArray();


it's unclear to me what the localization concerns are here. Are these filters shown to the user? If so, we likely cna't use v.toString here and we need to actually provide appropriate localized values depending on the enum value. #Pending

Yes, the values here are directly from the ValueUsageInfo enum. We either need to define corresponding string constants or localizable resource strings here to display in FAR column. #Pending

gotcha. as a WIP this is fine then :) #Pending

This is pending design issue, which will be addressed once we finalize the design: allowable values in this column AND localizable or not.

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

Cute! #Resolved

CyrusNajmabadi · 2018-09-26T23:30:16Z

src/VisualStudio/Core/Def/ServicesVSResources.resx

@@ -1069,4 +1069,7 @@ I agree to all of the foregoing:</value>
  <data name="Prefer_compound_assignments" xml:space="preserve">
    <value>Prefer compound assignments</value>
  </data>
+  <data name="Kind" xml:space="preserve">
+    <value>Kind</value>


probably needs a localization note. this is pretty vague :) #Pending

CyrusNajmabadi · 2018-09-26T23:31:21Z

src/Workspaces/Core/Portable/Extensions/ValueUsageInfo.cs

+                }
+            }
+
+            return builder.ToImmutable();


it would probably be very good to cache this. Also, is this passed back to the editor as an interface type (like IList/IReadOnlyList?). If so, we should cache in that form so we don't box on converting the immutablearray to the interface. #Resolved

This is now cached in the FAR column which invokes this method.

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

CyrusNajmabadi · 2018-09-26T23:32:46Z

src/Workspaces/Core/Portable/FindSymbols/ReferenceLocation.cs

@@ -50,14 +56,14 @@ public struct ReferenceLocation : IComparable<ReferenceLocation>, IEquatable<Ref

        public CandidateReason CandidateReason { get; }

-        internal ReferenceLocation(Document document, IAliasSymbol alias, Location location, bool isImplicit, bool isWrittenTo, CandidateReason candidateReason)
+        internal ReferenceLocation(Document document, IAliasSymbol alias, Location location, bool isImplicit, ValueUsageInfo valueUsageInfo, CandidateReason candidateReason)


this may be called from another language (not sure, but we need to check). If so, we might need to leave the old constructor, and have it delegate to a new one.

However, if this is truly C#/VB only, then this is safe. i just dont remember anymore. #Resolved

Verified this is only used within Roslyn.sln.

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

CyrusNajmabadi · 2018-09-26T23:33:13Z

src/Workspaces/Core/Portable/Shared/Extensions/SemanticModelExtensions.cs

+            this SemanticModel semanticModel,
+            SyntaxNode node,
+            ISemanticFactsService semanticFacts,
+            CancellationToken cancellationToken)


did this move from somewhere? #Resolved

No, it was added here so we don't need to track multiple boolean flags. Additionally, I am hoping that this method will completely go away soon when we merge in #30123

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

ok great. let's track that in #30123 if you're not already doing that :) #Resolved

Added comment #30123 (comment)

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

CyrusNajmabadi · 2018-09-26T23:34:50Z

Done with first pass. Other notes from the original post:

Should the values be lot more succinct (like 'ref', 'out', 'in' 'read', 'write', etc.) or current descriptive terms are preferred?

Yes. Read, Write and Read/Write are likely the max complexity we should expose here. For things like nameof(...) it should just be nothing. #Resolved

CyrusNajmabadi · 2018-09-26T23:35:48Z

The new column does not show up in the visible part of the FAR window by default, and user needs to scroll and move the column for visibility (see image below). This seems to be due to the fact that underlying table control does not refresh/update the column widths when a dynamic column becomes visible. I will follow up with the platform team to ensure the default view shows the custom columns, whenever marked visible.

If we do this, let' make sure that the column does nto disappear/reappear as you scroll. Once the column is there, it should just stay there. #Resolved

CyrusNajmabadi · 2018-10-02T18:22:37Z

src/Workspaces/Core/Portable/Extensions/OperationExtensions.cs

            }
            else if (operation.Parent is INameOfOperation ||
                     operation.Parent is ITypeOfOperation ||
                     operation.Parent is ISizeOfOperation)
            {
-                return ValueUsageInfo.NonReadWriteRef;
+                return ValueUsageInfo.NameReference;


not a huge fan of th ename. but it's also likely good/clear enough, so i'm not bothered enough to want it changed :) #Resolved

CyrusNajmabadi · 2018-10-02T18:22:52Z

src/Workspaces/Core/Portable/Extensions/ValueUsageInfo.cs

        /// </summary>
-        WritableRef = 0x01000,
+        Reference = 0x0100,


nice. i like this more. #Resolved

@jasonmalinowski's suggestion :) #Resolved

CyrusNajmabadi · 2018-10-02T18:23:55Z

src/Workspaces/Core/Portable/Extensions/ValueUsageInfo.cs

        /// For example, 'nameof(x)' does not read or write the underlying value stored in 'x'.
        /// </summary>
-        NonReadWriteRef = 0x10000,
+        NameReference = 0x1000,


this one doesn't really fit now since it uses 'Reference' in the name, but isn't or'ed with 'Reference'. I would be ok with ValueUsage.NameOnly perhaps #Resolved

or maybe 'AsName', or maybe 'NotAsValue'. #Resolved

Seems reasonable, and felt little odd to me too. I am going to make your suggested change to use NameOnly #Resolved

Going to stick with NameOnly for now and we can make the resource string change later if we feel it is still confusing.

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

CyrusNajmabadi · 2018-10-02T18:27:02Z

src/Workspaces/Core/Portable/WorkspacesResources.resx

@@ -747,4 +747,16 @@
  <data name="Refactoring_Only" xml:space="preserve">
    <value>Refactoring Only</value>
  </data>
+  <data name="NameReference" xml:space="preserve">
+    <value>NameReference</value>


this is a strange resource name. is it just pending something better? :) #Resolved

CyrusNajmabadi · 2018-10-02T18:28:21Z

Wow. This was super clean. I really really like how it came out. All my comments are basic nits, or general questoins i just want to get clarity around. It actually looks like there shouldn't be any concerns, but i want to double-check some things. Once i hear back, i can sign off asap! Thanks!

CyrusNajmabadi · 2018-10-02T18:32:14Z

Also paging @jnm2 as i think he'll be really happy to see this :)

mavasani · 2018-10-02T19:25:29Z

mavasani · 2018-10-03T01:33:11Z

@dotnet/roslyn-infrastructure @jaredpar Seems Linux_Test mono is failing roslyn-ci with the following publish error:

Publishing build artifacts failed with an error: Not found PathtoPublish: /data/agent/_work/9/s/Binaries/$(_configuration)/Logs

jaredpar · 2018-10-03T03:00:17Z

That is the secondary issue. The actual failure is above that running tests. Mono is crashing running one of the assemblies. This is a known issue and I have a PR out to
Move to a newer mono that fixes this.

Short term just rerun the VSTS build

mavasani · 2018-10-03T05:55:24Z

@jaredpar I have tried to requeue the build few times, but it still fails the Linux Mono test.

jaredpar · 2018-10-03T13:35:01Z

@mavasani sigh, now it's failing for a different reason: docker zombie container 😦 This is an issue I'm working with AzDev on. Let me know when you're ready to merge and I'll override the checks.

mavasani · 2018-10-03T13:39:10Z

Thanks @jaredpar. @CyrusNajmabadi any more concerns/feedback? We can merge once Cyrus sign's off.

jaredpar · 2018-10-03T14:11:38Z

@mavasani this PR will fix this problem #30286

CyrusNajmabadi · 2018-10-03T16:01:16Z

@mavasani Will do one more pass to make sure everything looks good :)

CyrusNajmabadi · 2018-10-03T16:02:50Z

src/Features/Core/Portable/FindUsages/SourceReferenceItem.cs


 namespace Microsoft.CodeAnalysis.FindUsages
 {
+    using ReferenceInfoMap = ImmutableDictionary<string, ImmutableArray<string>>;


i like this for now. If we ever open this stuff up though, we'll likely want to create a new type for this. #Pending

Sure, makes sense.

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

CyrusNajmabadi · 2018-10-03T16:04:11Z

src/Features/Core/Portable/FindUsages/SourceReferenceItem.cs

+        }
+
+        private static ReferenceInfoMap GetOrCreateReferenceInfo(ValueUsageInfo valueUsageInfo)
+            => s_valueUsageInfoToReferenceInfoMap.GetOrAdd(valueUsageInfo, CreateReferenceInfo);


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

CyrusNajmabadi · 2018-10-03T16:04:25Z

src/Features/Core/Portable/RemoveUnusedMembers/AbstractRemoveUnusedMembersDiagnosticAnalyzer.cs

@@ -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.IsNameOnly() && !symbolsReferencedInDocComments.Contains(member)


i much prefer these new name! :) #Resolved

CyrusNajmabadi · 2018-10-03T16:08:26Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

+
+                // Local functions.
+                AbstractFindUsagesCustomColumnDefinition GetCustomColumn(string columnName)
+                    => (AbstractFindUsagesCustomColumnDefinition)TableControl.ColumnDefinitionManager.GetColumnDefinition(columnName);


i assume this cast is always safe? i have'nt been able to convince myself it is. what if some other feature/language already made this column? #Resolved

Valid point. I will filter out the key-value pairs for such cases.

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

CyrusNajmabadi · 2018-10-03T16:09:39Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

+                        // Also note that we will call it only once for each new custom column to add for
+                        // each find references query - the lock above guarantees that newColumnStatesOpt is
+                        // going to be non-null only for the first result that has a non-empty column value.
+                        TableControl.SetColumnStates(newColumnStates);


note: this is only safe if SetColumnStates doesn't hold onto newColumnStates. i was thnking there would still be a .ToImmutable here. #Resolved

Sure.

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

CyrusNajmabadi · 2018-10-03T16:10:28Z

...ualStudio/Core/Def/Implementation/FindReferences/FindUsagesValueUsageInfoColumnDefinition.cs

+        // Allow filtering of the column by each ValueUsageInfo kind.
+        private static readonly ImmutableArray<string> s_defaultFilters = Enum.GetValues(typeof(ValueUsageInfo))
+                                                                .Cast<ValueUsageInfo>()
+                                                                .Where(value => value != ValueUsageInfo.None && value.IsSingleBitSet())


value != ValueUsageInfo.None seems redundant since you check IsSingleBitSet. #Resolved

CyrusNajmabadi

LGTM. Minor concerns. Feel free to fix or not if you want. I do think we need to ensure we don't load Roslyn unintentionally in non-Roslyn scenarios.

mavasani added Area-IDE Need Design Review The end user experience design needs to be reviewed and approved. labels Sep 26, 2018

mavasani requested review from CyrusNajmabadi and a team September 26, 2018 21:14

heejaechang reviewed Sep 26, 2018

View reviewed changes

CyrusNajmabadi reviewed Sep 26, 2018

View reviewed changes

CyrusNajmabadi reviewed Oct 2, 2018

View reviewed changes

Address PR feedback

f821e24

heejaechang approved these changes Oct 2, 2018

View reviewed changes

CyrusNajmabadi reviewed Oct 3, 2018

View reviewed changes

CyrusNajmabadi approved these changes Oct 3, 2018

View reviewed changes

mavasani added 2 commits October 3, 2018 09:44

Address feedback

e0fbebf

Merge remote-tracking branch 'upstream/master' into FAR_Column

041e939

mavasani merged commit 6ebb41d into dotnet:master Oct 3, 2018

mavasani deleted the FAR_Column branch October 3, 2018 23:12

mavasani mentioned this pull request Oct 4, 2018

Request: Find All References broken down by use #22545

Closed

jinujoseph added this to the 16.0.P2 milestone Oct 4, 2018

mavasani mentioned this pull request Dec 11, 2018

Find All References 'Kind' column values for Namespaces and Types. #31682

Merged

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

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

Conversation

mavasani commented Sep 26, 2018

heejaechang Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

heejaechang Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 27, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Oct 2, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Oct 2, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

CyrusNajmabadi Oct 3, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

heejaechang Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

CyrusNajmabadi Oct 2, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

mavasani Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 26, 2018 • edited by mavasani Loading

Choose a reason for hiding this comment

heejaechang Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

heejaechang Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 27, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Oct 2, 2018 •

edited by mavasani

Loading

mavasani Oct 2, 2018 •

edited

Loading

CyrusNajmabadi Oct 2, 2018 •

edited by mavasani

Loading

heejaechang Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

heejaechang Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

heejaechang Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Oct 3, 2018 •

edited by mavasani

Loading

heejaechang Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

heejaechang Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

heejaechang Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Oct 2, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 27, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 27, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

mavasani Sep 26, 2018 •

edited

Loading

CyrusNajmabadi Sep 27, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Oct 2, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Oct 2, 2018 •

edited by mavasani

Loading

CyrusNajmabadi commented Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi commented Sep 26, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Oct 2, 2018 •

edited by mavasani

Loading