Defer computing the unrooted symbol set to the point that we actually produce a final compilation to hand out.#50879
Conversation
4fac45d to
aea9e53
Compare
… produce a final compilation to hand out.
aea9e53 to
aa89603
Compare
| declarationOnlyCompilation: null, | ||
| generatedDocuments: ImmutableArray<SourceGeneratedDocumentState>.Empty, | ||
| generatedDocumentsAreFinal: false, | ||
| unrootedSymbolSet: null); |
There was a problem hiding this comment.
i do like how all the constructors of State don't need to figure out what to pass along.
| if (state.FinalCompilationWithGeneratedDocuments != null && state.FinalCompilationWithGeneratedDocuments.TryGetValue(out var compilationOpt) && compilationOpt.HasValue) | ||
| { | ||
| compilation = compilationOpt.Value; | ||
| state.InitializeUnrootedSymbolSet(compilation); |
There was a problem hiding this comment.
i don't love that we expose compilations in effectively two separate codepaths (TryGetCompilation and GetCompilationAsync), and i need to update both. feels fragile.
There was a problem hiding this comment.
I'm worried there's also potentially a third place now: CompilationTracker.GetPartialMetadataReference also hands out compilations which I guess get stitched into P2P references if you're freezing some consuming project. I'm highly suspicious of that code now that I'm aware it exists because I suspect you can get compilations that hadn't called RecordAssemblySymbols on it either and that might be bad in it's own right.
I think a slightly more defensive way to write this (if this is indeed broken) is update RecordAssemblySymbols to also take a State parameter, and that method should update the the unrooted symbols for that compilation. The existing calls to RecordAssemblySymbols are updated with the states they have, and we also update GetPartialMetadataReference as well...I think.
| { | ||
| return await BuildCompilationInfoAsync(solution, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
same logic as before, just extracted into helper.
|
@jasonmalinowski do you have any recommendations on how to avoid having to patch two entrypoints here. it makes me feel like this is rather fragile :( |
| declarationOnlyCompilation: null, | ||
| generatedDocuments, | ||
| generatedDocumentsAreFinal: false, // since we have a set of transformations to make, we'll always have to run generators again | ||
| GetUnrootedSymbols(inProgressCompilation)) |
There was a problem hiding this comment.
Was there a reason we did this versus just passing null for the unrooted symbols in the first place? Because I would have imagined the only times we ever handed out a compilation was after it had been passed to a FinalState. In the case of a compilation being made for frozen partial semantics, we still create a FinalState with the in-progress compilation before handing it out, so I would have thought that would work.
The only place I can potentially imagine that invariant being broken is in GetPartialMetadataReference where it looks like we're just more or less returning a compilation directly, and it looks like there's already multiple other bugs there that we may need to fix.
Review with whitespace changes off.