Connect open source generated files to the workspace #52094

jasonmalinowski · 2021-03-23T23:54:56Z

Implements the core of #50676.

The overall approach here is to make GetOpenDocumentInCurrentContextWithChanges work to return SourceGeneratedDocuments like anything else. The strange bit is we have to ensure the source generated document matches the text that's currently in the buffer. Consider a case where:

The user makes a change in another file which means the generator will produce new results. That generator is running async.
The user switches back to the generated file and immediately invokes a command on it before we refresh the buffer contents.

In that case, the buffer contents are stale, but we need to ensure the SourceGeneratedDocument given to all our features is in sync with the actual text buffer, or otherwise spans and everything won't align. This is very similar to the general concept of the "with changes" portion of that API: you may always get a forked version of the world that doesn't represent an entirely consistent view of the entire world, but it's at least going to ensure your document matches your starting point.

In the "regular case" that the generator has already ran and we're able to confirm the contents of the text buffer still matches the generated output, this (like for regular documents) doesn't fork anything at all -- you end up with the Solution object that matches Workspace.CurrentSolution.

The implementation approach here is to ensure that when we do fork a snapshot the final Compilation has the tree matching the text present no matter what. One approach would have been to fork the compilation tracker with some extra special state that remembers to fix that up in the end but I had two concerns with that approach:

The compilation tracker implementation is already crazy complicated.
Forking the compilation tracker while a generator is running potentially now running generators twice depending on the timing.

I decided to take the approach that CompilationTracker has an interface extracted for it's actual surface area, and then when we do the forked solution we create a different implementation of that interface that forwards to the underlying implementation and then replaces out the tree at the very end. This means we don't ever have generators running twice, and the magic of swapping out the tree is all contained in the special implementation and the core implementation is untouched. I absolutely concede that the interface isn't fun (translation: I hate it too) and if somebody has a better idea please speak up!

Still to do:

Integration tests and maybe some unit tests too
Further investigation of whether we need to implement some more of the ICompilationTracker methods. In my limit testing I don't see features currently relying on it.

CyrusNajmabadi · 2021-03-24T20:50:42Z

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.LazySyntaxTree.cs

+            public override string FilePath
+            {
+                get { return _path; }
+            }


CyrusNajmabadi · 2021-03-24T21:38:23Z

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.LazySyntaxTree.cs

+            }
+        }
+    }
+}


this is pretty complex. i have not reviewed it closely.

CyrusNajmabadi · 2021-03-24T21:38:39Z

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.cs

+        internal static SyntaxTree ParseTextLazy(
+            SourceText text,
+            CSharpParseOptions? options = null,
+            string path = "")


can we make these non optional, since this is an internal API?

CyrusNajmabadi · 2021-03-24T21:38:48Z

src/Compilers/Core/Portable/InternalUtilities/EnumerableExtensions.cs

@@ -11,6 +11,7 @@
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Roslyn.Utilities;
+using System.Threading.Tasks;


SYstem first.

src/Compilers/Core/Portable/InternalUtilities/EnumerableExtensions.cs

CyrusNajmabadi · 2021-03-24T21:39:52Z

src/Compilers/VisualBasic/Portable/Syntax/VisualBasicSyntaxTree.LazySyntaxTree.vb

+Namespace Microsoft.CodeAnalysis.VisualBasic
+
+    Partial Public Class VisualBasicSyntaxTree
+
+        Private NotInheritable Class LazySyntaxTree


Suggested change

Namespace Microsoft.CodeAnalysis.VisualBasic

Partial Public Class VisualBasicSyntaxTree

Private NotInheritable Class LazySyntaxTree

Namespace Microsoft.CodeAnalysis.VisualBasic

Partial Public Class VisualBasicSyntaxTree

Private NotInheritable Class LazySyntaxTree

CyrusNajmabadi · 2021-03-24T21:40:11Z

src/Compilers/VisualBasic/Portable/Syntax/VisualBasicSyntaxTree.LazySyntaxTree.vb

+            End Function
+        End Class
+    End Class
+End Namespace


looks like lots of duplication of the C# side. can we share anything?

CyrusNajmabadi · 2021-03-24T21:40:27Z

src/Compilers/VisualBasic/Portable/Syntax/VisualBasicSyntaxTree.vb

@@ -201,6 +201,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
                cloneRoot:=False)
        End Function

+        Friend Shared Function ParseTextLazy(text As SourceText,
+                                         Optional options As VisualBasicParseOptions = Nothing,
+                                         Optional path As String = "") As SyntaxTree


consider making non optinoal. also, sad indentation is sad.

CyrusNajmabadi · 2021-03-24T21:40:40Z

src/EditorFeatures/Core/ReferenceHighlighting/ReferenceHighlightingViewTaggerProvider.cs

@@ -163,7 +163,8 @@ protected override Task ProduceTagsAsync(TaggerContext<NavigableHighlightTag> co
            var document = documentHighlights.Document;

            var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
-            var textSnapshot = text.FindCorrespondingEditorTextSnapshot();
+
+            var textSnapshot = context.SpansToTag.FirstOrDefault(s => s.Document == document).SnapshotSpan.Snapshot;
            if (textSnapshot == null)


what happened here?

So for normal open documents, while the file is open the document text is directly linked to a text snapshot. In the case of generated document it's a bit murkier -- they're not connected to a snapshot if we're just using text that was generated in the background. If the background-computed text matches the open text, I was reusing the existing snapshot rather than forking it with identical text. So in the snapshot you get FindCorrespondingEditorTextSnapshot may not work. I could fork it anyways but such a fork would throw away the red nodes for tree parsed for the source generated file. Rather than using that helper, we're already given a list of documents and corresponding snapshots, so we can just use that directly.

src/VisualStudio/Core/Def/Implementation/Workspace/SourceGeneratedFileManager.cs

CyrusNajmabadi · 2021-03-24T21:43:02Z

src/VisualStudio/Core/Def/Implementation/Workspace/SourceGeneratedFileManager.cs

-            generatedSourceHintName = fileInfo.Name;
-
-            return true;
+            return Guid.TryParse(fileInfo.Directory.Name, out var guid) &&


what is going on here. how do you know the directory is a guid?

So the Visual Studio shell (today) isn't a fan of files that don't really exist; we always need a file name that at least sometimes has to exist or we can't really open things. So how us opening the dynamic files works is we generate a temporary file on disk, and then tell the shell to open it. When the file is opened, we go "ah, that's our file!" and then wire up from there. We just always make the containing directory name of that a GUID.

CyrusNajmabadi · 2021-03-24T21:43:38Z

src/Workspaces/Core/Portable/Workspace/Solution/Document.cs

@@ -440,7 +440,7 @@ public ImmutableArray<DocumentId> GetLinkedDocumentIds()
        ///
        /// Use this method to gain access to potentially incomplete semantics quickly.
        /// </summary>
-        internal Document WithFrozenPartialSemantics(CancellationToken cancellationToken)
+        internal virtual Document WithFrozenPartialSemantics(CancellationToken cancellationToken)


:'( terrifying.

src/Workspaces/Core/Portable/Workspace/Solution/Solution.cs

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs

...s/Core/Portable/Workspace/Solution/SolutionState.GeneratedFileReplacingCompilationTracker.cs

CyrusNajmabadi · 2021-03-24T21:45:35Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.ICompilationTracker.cs

+{
+    internal partial class SolutionState
+    {
+        private interface ICompilationTracker


def doc this type. feel free to write a treatise :)

this still applies.

CyrusNajmabadi · 2021-03-24T21:46:05Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.ICompilationTracker.cs

+            /// compilation and may choose to throw it away knowing that it could be reconstructed at a
+            /// later point if necessary.
+            /// </summary>
+            bool HasCompilation { get; }


do we actually use this? terrifying.

This is used to implement

roslyn/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs

Lines 1548 to 1562 in 02428e3

/// <summary>

/// Gets a copy of the solution isolated from the original so that they do not share computed state.

///

/// Use isolated solutions when doing operations that are likely to access a lot of text,

/// syntax trees or compilations that are unlikely to be needed again after the operation is done.

/// When the isolated solution is reclaimed so will the computed state.

/// </summary>

public SolutionState GetIsolatedSolution()

{

var forkedMap = ImmutableDictionary.CreateRange<ProjectId, CompilationTracker>(

_projectIdToTrackerMap.Where(kvp => kvp.Value.HasCompilation)

.Select(kvp => new KeyValuePair<ProjectId, CompilationTracker>(kvp.Key, kvp.Value.Clone())));

return this.Branch(projectIdToTrackerMap: forkedMap);

}

And yes, it's kinda terrifying. It's a public API that it looks like we don't actually use anywhere. Maybe we should just deprecate it.

yes. i would be 100% with us deprecating and saying: this is no longer supported. i know why we made this method. but i have no belief that anyone else is using it (Esp. as we are not).

CyrusNajmabadi · 2021-03-24T21:46:21Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.ICompilationTracker.cs

+            bool HasCompilation { get; }
+            ProjectState ProjectState { get; }
+
+            ICompilationTracker Clone();


what'st he value in cloning? since these are immutable already, right?

This is used to implement

roslyn/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs

Lines 1548 to 1562 in 02428e3

/// <summary>

/// Gets a copy of the solution isolated from the original so that they do not share computed state.

///

/// Use isolated solutions when doing operations that are likely to access a lot of text,

/// syntax trees or compilations that are unlikely to be needed again after the operation is done.

/// When the isolated solution is reclaimed so will the computed state.

/// </summary>

public SolutionState GetIsolatedSolution()

{

var forkedMap = ImmutableDictionary.CreateRange<ProjectId, CompilationTracker>(

_projectIdToTrackerMap.Where(kvp => kvp.Value.HasCompilation)

.Select(kvp => new KeyValuePair<ProjectId, CompilationTracker>(kvp.Key, kvp.Value.Clone())));

return this.Branch(projectIdToTrackerMap: forkedMap);

}

let's remove in a followup pr.

CyrusNajmabadi · 2021-03-24T21:47:26Z

Turns out my brain is too tired for this right now. Let's have a 15 min chat tomorrow about what's going on here, and then i go back dn review.

davidwengier

I have left a comment on every line of code I feel qualified to comment on :)

src/Workspaces/Core/Portable/Workspace/Solution/SourceGeneratedDocumentIdentity.cs

…d files

Now that the compiler doesn't actually parse generated files eagerly, we can use the same parsing mechanism we do for parsing regular trees. This removes some duplication and also will set us up to have forked solutions where we have generated trees that didn't come from the compiler at all.

We pass these around as a group on a fairly regular basis, so let's just make them a single type.

…ontainers

This is just the invocation of the extract interface refactoring and then some immediate follow-ups to trivially retarget types.

…documents The overall approach here is to make GetOpenDocumentInCurrentContextWithChanges work to return SourceGeneratedDocuments like anything else. The strange bit is we have to ensure the source generated document matches the text that's currently in the buffer. Consider a case where: 1. The user makes a change in another file which means the generator will produce new results. That generator is running async. 2. The user switches back to the generated file and immediately invokes a command on it before we refresh the buffer contents. In that case, the buffer contents are stale, but we need to ensure the SourceGeneratedDocument given to all our features is in sync with the actual text buffer, or otherwise spans and everything won't align. This is very similar to the general concept of the "with changes" portion of that API: you may always get a forked version of the world that doesn't represent an entirely consistent view of the entire world, but it's at least going to ensure your document matches your starting point. In the "regular case" that the generator has already ran and we're able to confirm the contents of the text buffer still matches the generated output, this (like for regular documents) doesn't fork anything at all -- you end up with the Solution object that matches Workspace.CurrentSolution. The implementation approach here is to ensure that when we do fork a snapshot the final Compilation has the tree matching the text present no matter what. One approach would have been to fork the compilation tracker with some extra special state that remembers to fix that up in the end but I had two concerns with that approach: 1. The compilation tracker implementation is already crazy complicated. 2. Forking the compilation tracker while a generator is running potentially now running generators twice depending on the timing. I decided to take the approach that CompilationTracker has an interface extracted for it's actual surface area, and then when we do the forked solution we create a different implementation of that interface that forwards to the underlying implementation and then replaces out the tree at the very end. This means we don't ever have generators running twice, and the magic of swapping out the tree is all contained in the special implementation and the core implementation is untouched.

…uleOrDynamic works This is a naive implementation that simply ensures we don't get different behavior from the main implementation. Most of this change is just moving the creation and matching logic into UnrootedSymbolSet.cs itself.

This works the same as the GetOpenDocumentInCurrentContextWithChanges. This fixes navigation bars specifically which use this.

We were holding onto the ISourceGenerator instance and passing it around for various reasons; this decomposes it into holding onto the assembly name and type name strings, so we can serialize this across processes.

We don't know up-front whether a generated file exists in the workspace, and our APIs to grab a document from a buffer are intended to complete quickly. If it turns out later the file isn't in the workspace anymore we will fake it and add it back so that way features aren't surprised by this. However, that means semantics in that file may be inaccurate. Disconnecting the document means that once the file is gone, then we won't be in this incorrect state for long.

It's a checksum of the attributes, not info, which can be confusing here.

This isn't actually used by anything.

We already had an assert that we shouldn't read one, but this adds the same assert on the sending side where it's easier to debug the source of the problem.

There are a number of asserts which try to ensure that we don't try to sync a null across the wire during solution sync. If you wanted to try using Checksum.Null as a placeholder for an optional value during synchronization, you'd hit these asserts because not everything would filter them out. This change filters them out during some parts of the synchronization process, allowing it to be used as an optional value.

The solution has the concept of "frozen" source generated documents where we force a solution snapshot to have a source generated document of a certain content, even if the generator is producing something new. This allows us to isolate features operating on a source generated document open in the editor that hasn't been updated yet -- we freeze the contents of the source generated file to match the open buffer, so taggers and the like can still function normally. This commit ensures that we synchronize those to the OOP process so everything stays in sync. The primary counter-intuitive bit is why we're holding onto this information at the SolutionState level, but that's because the place where the information is ultimately needed is the compilation trackers, which are held by the Solution object and nowhere else. This allows us to reuse the underlying project states for maximum efficiency.

Since we want to put this into 16.10 but we're past the feature cutoff, we'll put this behind an experimental flag.

jasonmalinowski self-assigned this Mar 23, 2021

dotnet-issue-labeler bot added the Area-IDE label Mar 23, 2021

jasonmalinowski marked this pull request as ready for review March 24, 2021 00:07

jasonmalinowski requested review from a team as code owners March 24, 2021 00:07