Add a Workspace API to represent source generated documents #49198

jasonmalinowski · 2020-11-05T23:42:54Z

High level, this allows us to represent source generated documents as a Document object so it can be used in other code without too much special work. Commit-at-a-time recommended. A spec is included too.

Fixes #45449
Fixes #49448

jasonmalinowski · 2020-11-13T20:13:57Z

docs/ide/api-designs/Workspace and Source Generated Documents.md

+
+The only surprise you might run into is taking a `SyntaxTree` from an earlier `Solution`, passing it to a newer `Solution`, and getting a document that no longer exists because in the later `Solution` is no longer generating this document. However, passing a `SyntaxTree` between snapshots like that is questionable in the first place. Any code that is working with multiple snapshots and knew that a Document between the snapshots had the same tree could have done something like this, but that code is potentially broken _anyways_ with source generators since now one tree can change due to another tree changing.
+
+## APIs on `Workspace`


Note the open file handling isn't implemented in this PR, yet. The proposal is still open to review of course!

jasonmalinowski · 2020-11-13T20:21:18Z

...e/Workspace/Solution/SolutionState.CompilationAndGeneratorDriverTranslationAction_Actions.cs

@@ -36,24 +35,17 @@ public override Task<Compilation> TransformCompilationAsync(Compilation oldCompi

            internal sealed class TouchAdditionalDocumentAction : CompilationAndGeneratorDriverTranslationAction
            {
+#pragma warning disable IDE0052 // Remove unread private members


To explain a bit of the history of the code here: the source generator API from the compiler originally had an API for doing incremental updates; that was hastily commented out in a preview release, and as a result the TransformGeneratorDriver functions weren't doing anything. I've removed the TransformGeneratorDriver code for now, but am leaving the additional actions in place because there's already been a certain amount of investment in creating these actions correctly in all of the various calls where we fork snapshots. Since the expectation is the compiler API will get worked on soonish, I'm going to leave the actions in place for now.

jasonmalinowski · 2020-11-13T20:31:03Z

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


        [ImportingConstructor]
        [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
        public VisualStudioDocumentNavigationService(
            IThreadingContext threadingContext,
            SVsServiceProvider serviceProvider,
-            IVsEditorAdaptersFactoryService editorAdaptersFactoryService)
+            IVsEditorAdaptersFactoryService editorAdaptersFactoryService,
+            Lazy<SourceGeneratedFileManager> sourceGeneratedFileManager /* lazy to avoid circularities */)


As of this commit, the SourceGeneratedFileManager imports VisualStudioDocumentNavigationService, and the latter also imports the former. This isn't great but longer term I'm hoping the "interesting" bits of SourceGeneratedFileManager move down a few layers so they can be shared with other hosts, and the leftovers merge into the VisualStudioDocumentNavigationService.

This clarifies it also makes compilations weak too.

jasonmalinowski · 2020-11-13T23:55:52Z

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

-            => _context.OnReferenceFoundAsync(reference.Rehydrate(_solution, GetDefinition(reference.DefinitionId)));
+        public async ValueTask OnReferenceFoundAsync(SerializableSourceReferenceItem reference)
+        {
+            var rehydrated = await reference.RehydrateAsync(_solution, GetDefinition(reference.DefinitionId), _context.CancellationToken).ConfigureAwait(false);


It's not clear to me if _context.CancellationToken is usable here: this would mean that if the client has requested cancellation, that we'll throw here, but that may end up remoting the exception to the server which then breaks OOP.

i would expect that to be ok as any callbacks must be cancellation safe as well. if not, we have serious issues on our hands. @tmat ?

So while testing this I think I got an assert once, which was worrisome. My worry is the callback may be expecting a different token than this one -- basically the callback creates it's own linked token in some way, and we need to be throwing with that one.

Had a quick idea: while we wait for @tmat's answer on this, I can just add a try/catch here and eat the cancellation exception: there's nothing the remote side could do with the cancelation exception anyways, and presumably it's having it's own cancellation process which will also cancel the work off.

docs/ide/api-designs/Workspace and Source Generated Documents.md

chsienki · 2020-11-17T19:51:51Z

...e/Workspace/Solution/SolutionState.CompilationAndGeneratorDriverTranslationAction_Actions.cs

@@ -155,38 +147,32 @@ public override Task<Compilation> TransformCompilationAsync(Compilation oldCompi

            internal sealed class AddAnalyzerReferencesAction : CompilationAndGeneratorDriverTranslationAction
            {
+#pragma warning disable IDE0052 // Remove unread private members
+                // https://github.com/dotnet/roslyn/issues/44161: right now there is no way to tell a GeneratorDriver that an analyzer reference has been added


There is GeneratorDriver.{Add,Remove}Generators() that can be used for this purpose

@chsienki Do we get any benefit from it though at this point?

chsienki · 2020-11-17T19:53:19Z

...e/Workspace/Solution/SolutionState.CompilationAndGeneratorDriverTranslationAction_Actions.cs

@@ -200,14 +186,6 @@ public AddAdditionalDocumentsAction(ImmutableArray<TextDocumentState> additional
                {
                    _additionalDocuments = additionalDocuments;


There is GeneratorDriver.{Add,Remove}AdditionalTexts() for this scenario.

Ditto: we're not really doing incremental use of this at this point, so it doesn't seem helpful? I agree we'll go back to that at some point...

chsienki · 2020-11-17T20:07:37Z

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

+
+                // The particular choice of crypto algorithm here is arbitrary and can be always changed as necessary.
+                var hash = System.Security.Cryptography.SHA256.Create().ComputeHash(hashInput.ToArray());
+                Array.Resize(ref hash, 16);


It says above we're using a crypto hash to avoid collisions, but if we're only using the first 16 bits, then that isn't guaranteed. Should the comment above be 'to reduce the likelihood of collisions?'

16 bytes, not bits. I tried (and failed) to find the similar aproach in the compiler where we compute the MVID in deterministic build cases -- do you know where that is?

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

docs/ide/api-designs/Workspace and Source Generated Documents.md

This will be returned from the Workspace API to represent syntax trees that are generated by a source generator. It still acts like a normal Document in that you can ask it for text or a tree, but it gives you some additional information about how it was generated. The intent is this also provides easy way for somebody to do a type check if the document is actually source generated, so we can take appropriate actions, like skipping a refactoring.

…tedDocumentsAsync() This rips out the TrackedGeneratorDriver code, which was implemented as an initial attempt to work with the compiler's incremental support for updating. Since the incremental support was removed this type didn't have much reason to still be around.

If we had already transitioned the tracker to a final state, and then the compilation was garbage collected, we still have the generated result that is correct. We can just reuse it in that case.

Right now the caller doesn't have to do anything to start getting source generated documents; this allows code that assumes that all SyntaxTrees have documents still functions reasonably well. Fixes dotnet#45449

Now that we have proper documents to represent source generated documents, we can clean this up. Previously we were opening source generated documents in the symbol navigation service, now we can do this in the document navigaton service, which the symbol service sits atop.

We want the DocumentId generated for a generated output to be stable between Compilations; this is so features that track a document by DocumentId can find it after some change has happened that requires generators to run again.

This code will need fixing to update it for source generators, but I wanted to ensure the unchecked null reference could first be found by the compiler.

This is equivalent to Project.GetSourceGeneratedDocumentAsync() but saves you a step.

This is just replacing some calls to GetDocument() to also fetch the generated document if that fails; most of the work is just spreading some asynchrony.

…in generated output

…f available

…be expected While we sort out precisely which cancellation token to use, for now we can just eat it to ensure stability.

jasonmalinowski · 2020-11-20T23:30:20Z

At this point I'm going to merge this PR since I think I've addressed most of @CyrusNajmabadi's concerns (and especially the bugs he found). @tmat and @chsienki there's some unanswered questions which I don't believe will fundamentally change the PR and are easy to address in a mop-up. And it's now fixing two dogfood-reported bugs so I think it's a net win, and it's also something I want to get in before we snap Preview 2.

tmat · 2020-12-01T03:41:19Z

        ValueTask OnReferenceFoundAsync(RemoteServiceCallbackId callbackId, SerializableSourceReferenceItem reference);

All these methods should take cancellationToken as a parameter.

Refers to: src/Features/Core/Portable/FindUsages/IRemoteFindUsagesService.cs:31 in 1a72d4d. [](commit_id = 1a72d4d, deletion_comment = False)

Dotnet-GitSync-Bot added the Area-IDE label Nov 5, 2020

jasonmalinowski self-assigned this Nov 12, 2020

jasonmalinowski force-pushed the source-generator-document-api branch from 3f61c17 to 97b67c0 Compare November 13, 2020 20:11

jasonmalinowski commented Nov 13, 2020

View reviewed changes

jasonmalinowski added 2 commits November 13, 2020 13:00

Add some comments to explain what the Optionals are for

94dd4d8

Rename CreateWorkspaceWithRecoverableSyntaxTrees

b4dfbfb

This clarifies it also makes compilations weak too.

jasonmalinowski force-pushed the source-generator-document-api branch 3 times, most recently from cc0e31b to cb6b20a Compare November 13, 2020 23:53

jasonmalinowski marked this pull request as ready for review November 13, 2020 23:54

jasonmalinowski requested a review from a team as a code owner November 13, 2020 23:54

jasonmalinowski commented Nov 13, 2020

View reviewed changes

jasonmalinowski force-pushed the source-generator-document-api branch from cb6b20a to e91f380 Compare November 14, 2020 00:15

runfoapp bot mentioned this pull request Nov 14, 2020

Hang in AppDomain.Unload causes tests to time out #49035

Closed

davidwengier reviewed Nov 16, 2020

View reviewed changes

docs/ide/api-designs/Workspace and Source Generated Documents.md Outdated Show resolved Hide resolved

jasonmalinowski force-pushed the source-generator-document-api branch from e91f380 to 75ec025 Compare November 16, 2020 19:29

runfoapp bot mentioned this pull request Nov 17, 2020

Roslyn infrastructure failures cause CI to fail #49023

Open

chsienki reviewed Nov 17, 2020

View reviewed changes

docs/ide/api-designs/Workspace and Source Generated Documents.md Show resolved Hide resolved

chsienki reviewed Nov 17, 2020

View reviewed changes