Merge pull request #65732 from jasonmalinowski/fix-lsif-issues

Fix potential race where we're expecting a moniker to be attached

src/Features/Core/Portable/LanguageServiceIndexFormat/SymbolMoniker.cs

@@ -18,34 +18,42 @@ public SymbolMoniker(string scheme, string identifier)
             this.Identifier = identifier;
         }
 
-        public static SymbolMoniker? TryCreate(ISymbol symbol)
+        public static bool HasMoniker(ISymbol symbol)
         {
-            // This uses the existing format that earlier prototypes of the Roslyn LSIF tool implemented; a different format may make more sense long term, but changing the
-            // moniker makes it difficult for other systems that have older LSIF indexes to the connect the two indexes together.
+            // We don't create monikers for symbols with type substitutions
+            if (!symbol.OriginalDefinition.Equals(symbol))
+                return false;
 
-            // Skip all local things that cannot escape outside of a single file: downstream consumers simply treat this as meaning a references/definition result
+            // Don't create monikers for all local things that cannot escape outside of a single file: downstream consumers simply treat this as meaning a references/definition result
             // doesn't need to be stitched together across files or multiple projects or repositories.
             if (symbol.Kind is SymbolKind.Local or
                 SymbolKind.RangeVariable or
                 SymbolKind.Label or
                 SymbolKind.Alias)
             {
-                return null;
+                return false;
             }
 
-            // Skip built in-operators. We could pick some sort of moniker for these, but I doubt anybody really needs to search for all uses of
+            // Don't create monikers for built in-operators. We could pick some sort of moniker for these, but I doubt anybody really needs to search for all uses of
             // + in the world's projects at once.
             if (symbol is IMethodSymbol method && method.MethodKind == MethodKind.BuiltinOperator)
-            {
-                return null;
-            }
+                return false;
 
             // TODO: some symbols for things some things in crefs don't have a ContainingAssembly. We'll skip those for now but do
             // want those to work.
             if (symbol.Kind != SymbolKind.Namespace && symbol.ContainingAssembly == null)
-            {
-                return null;
-            }
+                return false;
+
+            return true;
+        }
+
+        public static SymbolMoniker Create(ISymbol symbol)
+        {
+            // This uses the existing format that earlier prototypes of the Roslyn LSIF tool implemented; a different format may make more sense long term, but changing the
+            // moniker makes it difficult for other systems that have older LSIF indexes to the connect the two indexes together.
+
+            if (!HasMoniker(symbol))
+                throw new InvalidOperationException($"'{symbol}' does not have a moniker.");
 
             // Namespaces are special: they're just a name that exists in the ether between compilations
             if (symbol.Kind == SymbolKind.Namespace)

src/Features/Lsif/Generator/Generator.cs

@@ -253,7 +253,7 @@ SymbolKind.RangeVariable or
 
                     if (declaredSymbol != null)
                     {
-                        var definitionResultsId = symbolResultsTracker.GetResultIdForSymbol(declaredSymbol, Methods.TextDocumentDefinitionName, () => new DefinitionResult(idFactory));
+                        var definitionResultsId = symbolResultsTracker.GetResultIdForSymbol(declaredSymbol, Methods.TextDocumentDefinitionName, static idFactory => new DefinitionResult(idFactory));
                         lsifJsonWriter.Write(new Item(definitionResultsId.As<DefinitionResult, Vertex>(), lazyRangeVertex.Value.GetId(), documentVertex.GetId(), idFactory));
 
                         // If this declared symbol also implements an interface member, we count this as a definition of the interface member as well.
@@ -286,12 +286,12 @@ SymbolKind.RangeVariable or
                         void MarkImplementationOfSymbol(ISymbol baseMember)
                         {
                             // First we create a definition link for the reference results for the base member
-                            var referenceResultsId = symbolResultsTracker.GetResultSetReferenceResultId(baseMember.OriginalDefinition, idFactory);
+                            var referenceResultsId = symbolResultsTracker.GetResultSetReferenceResultId(baseMember.OriginalDefinition);
                             lsifJsonWriter.Write(new Item(referenceResultsId.As<ReferenceResult, Vertex>(), lazyRangeVertex.Value.GetId(), documentVertex.GetId(), idFactory, property: "definitions"));
 
                             // Then also link the result set for the method to the moniker that it implements
-                            referenceResultsId = symbolResultsTracker.GetResultSetReferenceResultId(declaredSymbol.OriginalDefinition, idFactory);
-                            var implementedMemberMoniker = symbolResultsTracker.GetResultIdForSymbol<Moniker>(baseMember.OriginalDefinition, "moniker", () => throw new Exception("When we produced the resultSet, we should have already created a moniker for it."));
+                            referenceResultsId = symbolResultsTracker.GetResultSetReferenceResultId(declaredSymbol.OriginalDefinition);
+                            var implementedMemberMoniker = symbolResultsTracker.GetMoniker(baseMember.OriginalDefinition, semanticModel.Compilation);
                             lsifJsonWriter.Write(new Item(referenceResultsId.As<ReferenceResult, Vertex>(), implementedMemberMoniker, documentVertex.GetId(), idFactory, property: "referenceLinks"));
                         }
                     }
@@ -302,7 +302,7 @@ void MarkImplementationOfSymbol(ISymbol baseMember)
                         // symbol but the range can point a different symbol's resultSet. This can happen if the token is
                         // both a definition of a symbol (where we will point to the definition) but also a reference to some
                         // other symbol.
-                        var referenceResultsId = symbolResultsTracker.GetResultSetReferenceResultId(referencedSymbol.GetOriginalUnreducedDefinition(), idFactory);
+                        var referenceResultsId = symbolResultsTracker.GetResultSetReferenceResultId(referencedSymbol.GetOriginalUnreducedDefinition());
                         lsifJsonWriter.Write(new Item(referenceResultsId.As<ReferenceResult, Vertex>(), lazyRangeVertex.Value.GetId(), documentVertex.GetId(), idFactory, property: "references"));
                     }
 

src/Features/Lsif/Generator/Graph/HoverResult.cs

@@ -8,7 +8,7 @@
 namespace Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator.Graph
 {
     /// <summary>
-    /// Represents a foverResult vertex for serialization. See https://github.com/Microsoft/language-server-protocol/blob/main/indexFormat/specification.md#more-about-request-textdocumenthover for further details.
+    /// Represents a hoverResult vertex for serialization. See https://github.com/Microsoft/language-server-protocol/blob/main/indexFormat/specification.md#more-about-request-textdocumenthover for further details.
     /// </summary>
     internal sealed class HoverResult : Vertex
     {

src/Features/Lsif/Generator/ResultSetTracking/DelegatingResultSetTracker.cs

@@ -16,7 +16,7 @@ public DelegatingResultSetTracker(Func<ISymbol, IResultSetTracker> chooseTracker
             _chooseTrackerForSymbol = chooseTrackerForSymbol;
         }
 
-        public Id<T> GetResultIdForSymbol<T>(ISymbol symbol, string edgeKind, Func<T> vertexCreator) where T : Vertex
+        public Id<T> GetResultIdForSymbol<T>(ISymbol symbol, string edgeKind, Func<IdFactory, T> vertexCreator) where T : Vertex
         {
             return _chooseTrackerForSymbol(symbol).GetResultIdForSymbol(symbol, edgeKind, vertexCreator);
         }

src/Features/Lsif/Generator/ResultSetTracking/IResultSetTracker.cs

@@ -21,13 +21,13 @@ internal interface IResultSetTracker
         /// Returns an ID of a vertex that is linked from a result set. For example, a <see cref="ResultSet"/> has an edge that points to a <see cref="ReferenceResult"/>, and
         /// item edges from that <see cref="ReferenceResult"/> are the references for the range. This gives you the ID of the <see cref="ReferenceResult"/> in this case.
         /// </summary>
-        Id<T> GetResultIdForSymbol<T>(ISymbol symbol, string edgeKind, Func<T> vertexCreator) where T : Vertex;
+        Id<T> GetResultIdForSymbol<T>(ISymbol symbol, string edgeKind, Func<IdFactory, T> vertexCreator) where T : Vertex;
 
         /// <summary>
-        /// Similar to <see cref="GetResultIdForSymbol{T}(ISymbol, string, Func{T})"/>, but instead of creating the vertex (if needed) and adding an edge, this
+        /// Similar to <see cref="GetResultIdForSymbol{T}"/>, but instead of creating the vertex (if needed) and adding an edge, this
         /// simply tracks that this method has been called, and it's up to the caller that got a true return value to create and add the vertex themselves. This is handy
         /// when the actual identity of the node isn't needed by any other consumers, or the vertex creation is expensive and we don't want it running under the lock that
-        /// <see cref="GetResultIdForSymbol{T}(ISymbol, string, Func{T})"/> would have to take.
+        /// <see cref="GetResultIdForSymbol{T}"/> would have to take.
         /// </summary>
         bool ResultSetNeedsInformationalEdgeAdded(ISymbol symbol, string edgeKind);
     }

src/Features/Lsif/Generator/ResultSetTracking/IResultSetTrackerExtensions.cs

@@ -17,7 +17,36 @@ internal static class IResultSetTrackerExtensions
         /// <summary>
         /// Returns the ID of the <see cref="ReferenceResult"/> for a <see cref="ResultSet"/>.
         /// </summary>
-        public static Id<ReferenceResult> GetResultSetReferenceResultId(this IResultSetTracker tracker, ISymbol symbol, IdFactory idFactory)
-            => tracker.GetResultIdForSymbol(symbol, Methods.TextDocumentReferencesName, () => new ReferenceResult(idFactory));
+        public static Id<ReferenceResult> GetResultSetReferenceResultId(this IResultSetTracker tracker, ISymbol symbol)
+            => tracker.GetResultIdForSymbol(symbol, Methods.TextDocumentReferencesName, static idFactory => new ReferenceResult(idFactory));
+
+        /// <summary>
+        /// Fetches the moniker node for a symbol; this should only be called on symbols where <see cref="SymbolMoniker.HasMoniker"/> returns true.
+        /// </summary>
+        public static Id<Moniker> GetMoniker(this IResultSetTracker tracker, ISymbol symbol, Compilation sourceCompilation)
+        {
+            return tracker.GetResultIdForSymbol(symbol, "moniker", idFactory =>
+            {
+                var moniker = SymbolMoniker.Create(symbol);
+
+                string? kind;
+
+                if (symbol.Kind == SymbolKind.Namespace)
+                {
+                    kind = null;
+                }
+                else if (symbol.ContainingAssembly.Equals(sourceCompilation.Assembly))
+                {
+                    kind = "export";
+                }
+                else
+                {
+                    kind = "import";
+                }
+
+                // Since we fully qualify everything, all monitors are unique within the scheme
+                return new Moniker(moniker.Scheme, moniker.Identifier, kind, unique: "scheme", idFactory);
+            });
+        }
     }
 }

src/Features/Lsif/Generator/ResultSetTracking/SymbolHoldingResultSetTracker.cs

@@ -72,54 +72,20 @@ private TrackedResultSet GetTrackedResultSet(ISymbol symbol)
             //
             // This we do outside the lock -- whichever thread was the one to create this was the one that
             // gets to write out the moniker, but others can use the ResultSet Id at this point.
-            if (symbol.OriginalDefinition.Equals(symbol))
+            if (SymbolMoniker.HasMoniker(symbol))
             {
-                var monikerVertex = TryCreateMonikerVertexForSymbol(symbol);
-
-                if (monikerVertex != null)
-                {
-                    // Attach the moniker vertex for this result set
-                    _ = GetResultIdForSymbol(symbol, "moniker", () => monikerVertex);
-                }
+                _ = this.GetMoniker(symbol, _sourceCompilation);
             }
 
             return trackedResultSet;
         }
 
-        private Moniker? TryCreateMonikerVertexForSymbol(ISymbol symbol)
-        {
-            var moniker = SymbolMoniker.TryCreate(symbol);
-
-            if (moniker == null)
-            {
-                return null;
-            }
-
-            string? kind;
-
-            if (symbol.Kind == SymbolKind.Namespace)
-            {
-                kind = null;
-            }
-            else if (symbol.ContainingAssembly.Equals(_sourceCompilation.Assembly))
-            {
-                kind = "export";
-            }
-            else
-            {
-                kind = "import";
-            }
-
-            // Since we fully qualify everything, all monitors are unique within the scheme
-            return new Moniker(moniker.Scheme, moniker.Identifier, kind, unique: "scheme", _idFactory);
-        }
-
         public Id<ResultSet> GetResultSetIdForSymbol(ISymbol symbol)
         {
             return GetTrackedResultSet(symbol).Id;
         }
 
-        public Id<T> GetResultIdForSymbol<T>(ISymbol symbol, string edgeKind, Func<T> vertexCreator) where T : Vertex
+        public Id<T> GetResultIdForSymbol<T>(ISymbol symbol, string edgeKind, Func<IdFactory, T> vertexCreator) where T : Vertex
         {
             return GetTrackedResultSet(symbol).GetResultId(edgeKind, vertexCreator, _lsifJsonWriter, _idFactory);
         }
@@ -156,7 +122,7 @@ public TrackedResultSet(Id<ResultSet> id)
                 Id = id;
             }
 
-            public Id<T> GetResultId<T>(string edgeLabel, Func<T> vertexCreator, ILsifJsonWriter lsifJsonWriter, IdFactory idFactory) where T : Vertex
+            public Id<T> GetResultId<T>(string edgeLabel, Func<IdFactory, T> vertexCreator, ILsifJsonWriter lsifJsonWriter, IdFactory idFactory) where T : Vertex
             {
                 lock (_edgeKindToVertexId)
                 {
@@ -172,7 +138,7 @@ public Id<T> GetResultId<T>(string edgeLabel, Func<T> vertexCreator, ILsifJsonWr
                         return new Id<T>(existingId.Value.NumericId);
                     }
 
-                    var vertex = vertexCreator();
+                    var vertex = vertexCreator(idFactory);
                     _edgeKindToVertexId.Add(edgeLabel, vertex.GetId().As<T, Vertex>());
 
                     lsifJsonWriter.Write(vertex);