Invoke generators in the CompilationTracker #42373

jasonmalinowski · 2020-03-13T00:22:40Z

This moves the invocation of generators into the CompilationTracker so generators are properly included in all Compilations handed out by the Workspace. This will allows us to take advantage of the incremental update support of the compiler APIs.

The TrackedGeneratorDriver is a wrapper type to track some information that right now is not tracked by GeneratorDriver for now; cleaning up that API will be addressed in a future PR and the TrackedGeneratorDriver type will go away.

jasonmalinowski · 2020-03-13T00:33:08Z

@chsienki This is my current prototype to get the invocation of the generators moved into the correct layer so we have better caching/generation support. This isn't quite done yet as I'm not actually trying to call TryApplyEdits yet, as how the CompilationTracker works a bit more funky than I expected. I'm about 50% sure this is better than your prototype, but it's still not remotely "good" yet. (It's also possible it's worse... 😄)

jasonmalinowski · 2020-03-17T00:10:57Z

@dotnet/roslyn-ide: some background on this: when @chsienki started doing the source generator work, he did a quick hack to get source generators to run after we produced a compilation in the compilation tracker. Since this was happening after the core workspace processed stuff, this had various problems: TryGetCompilation and GetCompilationAsync would have returned different things, P2P references probably would have been broken, etc. This tries to move the core running into the compilation tracker so there aren't so many things broken with it.

This is still a prototype although one that compiler is interested in dogfooding. The primary goals are:

The compilation tracker should behave the same as usual if you aren't on the preview langver, since this all gets short-circuited there.
Starting to test out the APIs so we can get some initial experimentation with them.

Out of scope of this PR is:

Fleshing out the prototype (you'll see a number of places where the compiler hasn't implemented something, and so we have to punt work because of that.)
Trying to use the compiler's incremental update API -- that'll take more work. We've also identified a few API gaps too that need to be addressed before we can do that, we think.
Any sort of remotely smart performance handling.
Any representation of these in a smart way in the Workspaces layer.

Once this is in to unblock compiler dogfooding, then we'll tackle those. This is emphatically a "look, we're walking now" PR where the goal is just to make things be Not Entirely Wrong(tm).

@chsienki and I have been going back and forth on this so we now have some additional feedback on his APIs. Expect those PROTOTYPE comments to go away.

jasonmalinowski · 2020-03-17T00:17:29Z

And oh there was one more goal: re-learn (at least for me) how the CompilationTracker even works, since any source generator design is potentially going to have non-trivial impacts to it. This I think is the "smallest" impact to it in a way that is functionally correct while trying to not be smart about perf in any way. But now that I've paged back into memory how it works I can have some hope at estimating tasks for actually making things smarter too.

jaredpar · 2020-03-17T03:20:40Z

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/ISolutionExtensions.cs

+        public static Document GetRequiredDocument(this Project project, DocumentId documentId)
+        {
+            var document = project.GetDocument(documentId);
+            if (document == null)


Embrace the is null lifestyle 😄

Let me fix that for you:

=> project.GetDocument(documentId) ?? throw new InvalidOperationException(WorkspaceExtensionsResources.The_solution_does_not_contain_the_specified_document)

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

jaredpar · 2020-03-17T03:28:19Z

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

@@ -516,7 +528,7 @@ private async Task<Compilation> GetOrBuildDeclarationCompilationAsync(SolutionSt
                    compilation = compilation.AddSyntaxTrees(trees);
                    trees.Free();

-                    WriteState(new FullDeclarationState(compilation), solution);
+                    WriteState(new FullDeclarationState(compilation, new TrackedGeneratorDriver(generatorDriver: null)), solutionServices);


Nit: rest of PR uses new TrackedGenerator(null). Not opposed to the name but seems like it should be consistent.

As an aside the pattern seems odd. Feels like it would be better as a named factory of sorts say TrackedGeneratorDriver.CreateNeedsFullGeneration

TrackedGeneratorDriver is a workaround for a API problem right now with GeneratorDriver. GeneratorDriver has a way to reuse a previous driver with a known set of edits, which you can then apply with TryApplyEdits. But if there's some change that happens to a compilation that doesn't have a corresponding edit, it's just up to the user of the driver to remember to not call TryApplyEdits. I've asked @chsienki to instead offer some API to "poison" a generator driver which would mean:

Any stored edits are tossed, because there's no point in holding onto them anymore. (It's otherwise potentially just leaking memory.)

Any future calls to TryApplyEdits just fails, so a full generation has to be ran instead. Since the driver requires that as a fallback no matter what (the host has to be ready for TryApplyEdits to fail at any time, so this isn't extra work for us.)

At this point the struct is just going away.

jaredpar · 2020-03-17T03:30:14Z

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

+                            compilationFactory.CreateGeneratorDriver(
+                                this.ProjectState.ParseOptions!,
+                                generators,
+                                additionalTexts));


This will drop the value of generatorDriver.NeedsFullGeneration if it was false. Is there ever a case where that could happen?

The NeedsFullGeneration flag is to track whether or not the GeneratorDriver.TryApplyEdits would be useful; that is only useful if you had an existing generator around, so once we're creating a new one we need a full generation no matter what.

chsienki · 2020-03-17T18:51:55Z

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

+        // PROTOTYPE: this mostly exists as a way to track if a GeneratorDriver has been mutated in a way
+        // which TryApplyEdit can no longer be used. We should just revise GeneratorDriver to better handle
+        // this pattern.
+        public readonly struct TrackedGeneratorDriver


I think I understand the intent of this class, but it doesn't actually seem to be used? If we're not checking the NeedsFullGeneration property, why is it needed?

Ah I just saw your comments in response to Jared. The 'poison' api is trivial. Do we want to add it into this PR and drop the tracker here instead?

Yes, I tried to use it and then realized I have further problems to sort out. 😦 So it's left in so we can use it, just not quite yet.

(The problems are mostly that I need further understand how exactly the compilation tracker reuses compilations from full states, versus proper in-progress states. It's a bit funky.)

Ah ok, makes sense

do you guys think you'll still need this in the future?

chsienki

genlu · 2020-03-20T21:50:41Z

src/Workspaces/Core/Portable/Workspace/Host/CompilationFactory/ICompilationFactoryService.cs

@@ -10,12 +12,9 @@ namespace Microsoft.CodeAnalysis.Host
    internal interface ICompilationFactoryService : ILanguageService
    {
        Compilation CreateCompilation(string assemblyName, CompilationOptions options);
-        Compilation CreateSubmissionCompilation(string assemblyName, CompilationOptions options, Type hostObjectType);
-        Compilation GetCompilationFromCompilationReference(MetadataReference reference);


Why are these removed? Is it related to this PR?

Turns out these were unused; it was easier to remove them than null annotate them.

i'm a fan of pulling those sorts of chnages out into a simultaneous PR that is easy to review/merge. jsut a thought for the future.

This was broken down commit-by-commit; my bad for not mentioning that in the original PR message.

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

genlu · 2020-03-20T23:22:51Z

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

+
+                    RecordAssemblySymbols(compilation, metadataReferenceToProjectId);
+
+                    this.WriteState(


OK, just trying to understand this change, we basically made the after-generation compilation the only possible final compilation of state to make everything consistent?

The CompilationTracker always moved Compilations through a bunch of states, finally reaching the "Final State". Any call to GetCompilationAsync() would simply push through to the FinalState and then use that one. The FinalState was also the only one that had P2P references wired up as you can see, so essentially that's strengthened here a bit.

The main change was that any given state is also allowed to provide a "good enough" compilation that it has at any point. That could be some partial compilation, or in the case of a FinalState it is the final compilation. I'm now being more explicit and FinalState (if there are generators) now holds onto both the final compilation (post generator) and also the pre-generator compilation, where the latter is used for another later generation pass.

We'll probably have to tweak this further because the generator driver is also going to allow us to have incremental updating, which means we'll need to pull a post-generation compilation through some transformation states as well.

tmat · 2020-03-25T20:04:41Z

@chsienki Why is GeneratorDriverState separate from GeneratorDriver?

chsienki · 2020-03-25T20:15:37Z

Mainly so that CSharpGeneratorDriver doesn't have to have copy of all the ctor params. (we've talked about moving everything down into the C# layer though so can combine if/when we do that)

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

tmat · 2020-03-25T20:49:42Z

Yeah, I think combining these types would be better.

...paces/Core/Portable/Workspace/Solution/SolutionState.CompilationTranslationAction.Actions.cs

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

CyrusNajmabadi · 2020-03-26T04:06:06Z

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

@@ -295,7 +304,13 @@ public CompilationTracker FreezePartialStateWithTree(SolutionState solution, Doc
                }

                inProgressProject = inProgressProject.AddProjectReferences(newProjectReferences);
-                inProgressCompilation = UpdateCompilationWithNewReferencesAndRecordAssemblySymbols(inProgressCompilation, metadataReferences, metadataReferenceToProjectId);
+
+                if (!Enumerable.SequenceEqual(inProgressCompilation.ExternalReferences, metadataReferences))


what's an ExtenralReference?

It's the list of references that excludes references that get added via #r. Those are computed automatically and added into the Compilation's direct "References" collection, but in this case we're explicitly only mutating the ones that came elsewhere.

(This code just moved, but it's a great question nonetheless.)

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

CyrusNajmabadi · 2020-03-26T04:08:08Z

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

-                        inProgressCompilation = await intermediateProjects[0].action.InvokeAsync(inProgressCompilation, cancellationToken).ConfigureAwait(false);
+                        var compilationTranslationAction = intermediateProjects[0].action;
+                        inProgressCompilation = await compilationTranslationAction.TransformCompilationAsync(inProgressCompilation, cancellationToken).ConfigureAwait(false);
+                        inProgressGeneratorDriver = compilationTranslationAction.TransformGeneratorDriver(inProgressGeneratorDriver);


interesting that this is cancellable.

What do you mean? I guess the idea is if somebody asks for a compilation and they stop then we stop processing until somebody were to ask again.

...paces/Core/Portable/Workspace/Solution/SolutionState.CompilationTranslationAction.Actions.cs

CyrusNajmabadi · 2020-03-26T04:16:57Z

...paces/Core/Portable/Workspace/Solution/SolutionState.CompilationTranslationAction.Actions.cs

+                public override TrackedGeneratorDriver TransformGeneratorDriver(TrackedGeneratorDriver generatorDriver)
+                {
+                    // PROTOTYPE: right now we have no way to remove additional files so we have to do one from scratch
+                    return new TrackedGeneratorDriver(generatorDriver: null);


not certain i understand hte comment.

Reworded to be a bit clearer, hopefully?

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

CyrusNajmabadi · 2020-03-26T04:19:04Z

src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs

+            private string GetGeneratedFileName(string path) => $"{Path.GetFileNameWithoutExtension(path)}.generated";
+        }
+    }
+}


i appreciate test. one thing that we may need to invest in IMO is a way to do fine-grained compilation tracker testing. i.e to actually validate that we are or aren't doing specific pieces of work at specific times.

src/Compilers/CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests.cs

ryzngard

Looks reasonable to my understanding of the generator APIs (and good discussions made in comments)

…onAsync This class is going to get more methods on it so we'll want a more descriptive name now -- it was fine to have a general name when there was only one method

WriteState was taking a SolutionState when all it really needed was a SolutionServices since it needed to do some caching stuff. This tweaks the code so it's not passing a SolutionState unless actually needed, which helps ensure we don't accidentally use any solution-wide state except during finalization of a compilation.

Nothing is using this anymore, so it can be removed. We explicitly don't want a GeneratorDriver holding onto Compilations so as to avoid rooting them.

This moves the invocation of generators into the CompilationTracker so generators are properly included in all Compilations handed out by the Workspace. This also allows us to take advantage of the incremental update support of the compiler APIs. The TrackedGeneratorDriver is a wrapper type to track some information that right now is not tracked by GeneratorDriver for now; cleaning up that API will be addressed in a future PR and the TrackedGeneratorDriver type will go away.

Fixing these is tracked by dotnet#42823.

jasonmalinowski self-assigned this Mar 13, 2020

jasonmalinowski force-pushed the generate-sources-in-the-compilation-tracker branch 3 times, most recently from c936b56 to 2f25fcf Compare March 16, 2020 23:54

jasonmalinowski marked this pull request as ready for review March 17, 2020 00:04

jasonmalinowski requested review from a team as code owners March 17, 2020 00:04

jinujoseph added the Area-IDE label Mar 17, 2020

jaredpar reviewed Mar 17, 2020

View reviewed changes

chsienki reviewed Mar 17, 2020

View reviewed changes

jasonmalinowski force-pushed the generate-sources-in-the-compilation-tracker branch 2 times, most recently from 8d4257a to fc83b73 Compare March 18, 2020 02:00

chsienki approved these changes Mar 19, 2020

View reviewed changes

genlu reviewed Mar 20, 2020

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/Solution.cs Outdated Show resolved Hide resolved

genlu reviewed Mar 20, 2020

View reviewed changes

jasonmalinowski force-pushed the generate-sources-in-the-compilation-tracker branch 2 times, most recently from 84f2ce4 to 6182bd7 Compare March 25, 2020 03:18

tmat reviewed Mar 25, 2020

View reviewed changes

...paces/Core/Portable/Workspace/Solution/SolutionState.CompilationTranslationAction.Actions.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Mar 26, 2020

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs Show resolved Hide resolved