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

When we're creating a ResultSet for a symbol, we always attach
a moniker on it once it's created. We do that outside the main lock
against the dictionary for all symbols, so that way the creation
of the moniker isn't holding up operations for unrelated symbols.
This was fine because nothing cared about the moniker existing, until
we added additional support for inheritence, and the new spec now
needs us to link to monikers in some cases. There I put in an incorrect
assumption that the moniker might be there, which isn't the case
if a different thread is still making it.

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

@@ -291,7 +291,7 @@ void MarkImplementationOfSymbol(ISymbol baseMember)
 
                             // Then also link the result set for the method to the moniker that it implements
                             referenceResultsId = symbolResultsTracker.GetResultSetReferenceResultId(declaredSymbol.OriginalDefinition);
-                            var implementedMemberMoniker = symbolResultsTracker.GetResultIdForSymbol<Moniker>(baseMember.OriginalDefinition, "moniker", static (_, _) => throw new Exception("When we produced the resultSet, we should have already created a moniker for it."));
+                            var implementedMemberMoniker = symbolResultsTracker.GetMoniker(baseMember.OriginalDefinition, semanticModel.Compilation);
                             lsifJsonWriter.Write(new Item(referenceResultsId.As<ReferenceResult, Vertex>(), implementedMemberMoniker, documentVertex.GetId(), idFactory, property: "referenceLinks"));
                         }
                     }

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

@@ -19,5 +19,34 @@ internal static class IResultSetTrackerExtensions
         /// </summary>
         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", (symbol, 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,48 +72,14 @@ 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;