-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Always run the razor generator even in balanced mode #79510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
chsienki
commented
Jul 22, 2025
- Change DoNotCreate to CreateRequired
- Add a flag to remote generation service that allows the caller to specify required generators only
- Pass the flag to OOP based on the creation policy
- Set the creation policy when creating a compilation tracker
- Ensure we add in previous docs that weren't regenerated when running only required generators
- Add a filter that only runs razor when required generators are requested
- Change DoNotCreate to CreateRequired - Add a flag to remote generation service that allows the caller to specify required generators only - Pass the flag to OOP based on the creation policy - Set the creation policy when creating a compilation tracker - Ensure we add in previous docs that weren't regenerated when running only required generators - Add a filter that only runs razor when required generators are requested
@@ -31,8 +31,10 @@ internal interface IRemoteSourceGenerationService | |||
/// <param name="withFrozenSourceGeneratedDocuments">Controls if the caller wants frozen source generator documents | |||
/// included in the result, or if only the most underlying generated documents (produced by the real compiler <see | |||
/// cref="GeneratorDriver"/> should be included.</param> | |||
/// <param name="requiredDocumentsOnly">Controls if the caller only wants to run required generators and use old | |||
/// results for other generators, or if all generators should be run to get new documents.</param> | |||
ValueTask<ImmutableArray<SourceGeneratedDocumentInfo>> GetSourceGeneratedDocumentInfoAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a complaint. But wondering if we should merge the booleans into some options/enum type.
Question (and consider doc'ing). What are the semantics of calling this multiple times, where all the values are the same, but 'requireDocumentsOnly' is true/false.
I'm fairly sure i want the following:
- order doesn't matter. you get the same results regardless of the order you may have called this.
- passing 'requiredDoucmentsOnly=false' always gives a superset of 'requiredDocumentsOnly=true'.
@@ -57,6 +58,6 @@ private readonly record struct CreationPolicy( | |||
/// Do not create up to date source generator docs and do not create up to date skeleton references for P2P | |||
/// references. For both, use whatever has been generated most recently. | |||
/// </summary> | |||
public static readonly CreationPolicy DoNotCreate = new(GeneratedDocumentCreationPolicy.DoNotCreate, SkeletonReferenceCreationPolicy.DoNotCreate); | |||
public static readonly CreationPolicy DoNotCreate = new(GeneratedDocumentCreationPolicy.CreateRequired, SkeletonReferenceCreationPolicy.DoNotCreate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the parameter was requiredDocumentsOnly. perhaps .CreateRequiredOnly. I'm liking the 'only' to indicate very specifically exactly what it is controlling, and waht it isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. As vebose as it might be, i think CreationPolicy CreateRequiredGeneratedDocuments_DoNotCreateSkeletonReferences
is likely appropriate. Having this be called 'DoNotCreate' now feels misleading.
await ValidateSourceGeneratorDocuments(expectedCallback: 0, | ||
expectedRazor: 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await ValidateSourceGeneratorDocuments(expectedCallback: 0, | |
expectedRazor: 0); | |
await ValidateSourceGeneratorDocuments(expectedCallback: 0, | |
expectedRazor: 0); |
i hate this formattign :D. can you either have this be one line, or have it be:
await ValidateSourceGeneratorDocuments(expectedCallback: 0, | |
expectedRazor: 0); | |
await ValidateSourceGeneratorDocuments( | |
expectedCallback: 0, | |
expectedRazor: 0); |
|
||
// Get the documents again and ensure nothing ran | ||
await ValidateSourceGeneratorDocuments(expectedCallback: executionPreference == SourceGeneratorExecutionPreference.Automatic ? 1 : 0, | ||
expectedRazor: 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with all of these.
also: thanks for validating automatic and balanced.
|
||
// Make another change, but this time enqueue an update too | ||
Contract.ThrowIfFalse(workspace.TryApplyChanges(workspace.CurrentSolution.WithDocumentText(tempDoc.Id, SourceText.From("// more new text")))); | ||
workspace.EnqueueUpdateSourceGeneratorVersion(projectId: null, forceRegeneration: false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wouldn't mind a test where we're in balanced, and we force-regen, and we see everything properly updated.
src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CreationPolicy.cs
Show resolved
Hide resolved
@@ -612,7 +612,7 @@ await compilationState.GetCompilationAsync( | |||
if (workspacePreference != SourceGeneratorExecutionPreference.Automatic) | |||
{ | |||
if (creationPolicy.GeneratedDocumentCreationPolicy == GeneratedDocumentCreationPolicy.Create) | |||
creationPolicy = creationPolicy with { GeneratedDocumentCreationPolicy = GeneratedDocumentCreationPolicy.DoNotCreate }; | |||
creationPolicy = creationPolicy with { GeneratedDocumentCreationPolicy = GeneratedDocumentCreationPolicy.CreateRequired }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to complain that i didn't write docs here, and that was my bad. But turns out i did, and they're great :p
So please update the docs above this appropriately. It should be expanded to indicate what this new mode is, and how it affects 'required' and non-required sgs.
--
aside, as i talk about this more, my brain keeps referring to these generators as 'critical' vs 'required'. perhaps that name might be better. but i' mnot pushing for that.
@@ -111,7 +99,7 @@ private sealed partial class RegularCompilationTracker : ICompilationTracker | |||
var infos = infosOpt.Value; | |||
|
|||
// If there are no generated documents, bail out immediately. | |||
if (infos.Length == 0) | |||
if (creationPolicy == GeneratedDocumentCreationPolicy.Create && infos.Length == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it woudl be worth fleshing out the comment above. specifically why the new enum check is in here. (basically, just convey taht, as per the docs, the semantics of .CreateRequired is that we reuse the gen docs from the last run, so we need to now do that below).
if (creationPolicy == GeneratedDocumentCreationPolicy.CreateRequired) | ||
{ | ||
// the documents we got back are only for the required generators, meaning any documents from other generators remain the same. | ||
var generatorsThatRan = infos.Select(di => di.DocumentIdentity.Generator).Distinct().ToImmutableHashSet(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Distinct().ToImmutableHashSet()
is redundant. Can just doToImmutableHashSet()
- We avoid the heavyweight Immutable types (set/dict) unless we intend to store as a field, and we really need to ensure no one messes with it. For local processing, ToSet/ToHashSet is preferred as the types are much ligher.
- i'm tempted to have you literally just ToImmutableArray this, as we should expect that there will legit only be a handful of required generators (if there are more, then the whole 'balanced' mode ends up being rather pointless).
@@ -111,7 +99,7 @@ private sealed partial class RegularCompilationTracker : ICompilationTracker | |||
var infos = infosOpt.Value; | |||
|
|||
// If there are no generated documents, bail out immediately. | |||
if (infos.Length == 0) | |||
if (creationPolicy == GeneratedDocumentCreationPolicy.Create && infos.Length == 0) | |||
return (compilationWithoutGeneratedFiles, TextDocumentStates<SourceGeneratedDocumentState>.Empty); | |||
|
|||
// Next, figure out what is different locally. Specifically, what documents we don't know about, or we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self. man this method is big... but breaking it up may just obfuscate. feels like maybe a helper type might be good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. the method is big enough, and complex enough that i cannot convince myself that the new code is correct.
specifically the new code has bail-out points prior to you doign the work to move the old generated docs forward. For example, it has this check below:

- this is either a bug.
- it's not a bug, but we need DEEP comments explaining why this logic is ok.
--
Aside: I'm not sure if this is possible, but it seems like it might be clearer if the original lgoic here stays mostly unchanged (just extracted to a helper method). Then the method that calls that new helper is the one that takes the results, and does all the manipulations related to the semantics of CreateRequired. So it goes "oh, we only ran a subset of generators, and here are hte results of that. I will now add in the old docs as necessary). That would help keep this code simpler as well, including (i think), still being able to trivially bail out in the infos.Length == 0
case, and whatnot.
The more i say this, the more i want this.
...Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker_Generators.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs
Outdated
Show resolved
Hide resolved
{ | ||
return project.SupportsCompilation | ||
? GetCompilationTracker(project.Id).GetSourceGeneratedDocumentStatesAsync(this, withFrozenSourceGeneratedDocuments, cancellationToken) | ||
? GetCompilationTracker(project.Id, requiredDocumentsOnly).GetSourceGeneratedDocumentStatesAsync(this, withFrozenSourceGeneratedDocuments, cancellationToken) | ||
: new(TextDocumentStates<SourceGeneratedDocumentState>.Empty); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: i'm not opposed to us effectively havign two compilation trackers here depending on what clients may be asking for. What I am trying to figure out is if one compilation tracker should be built from teh other.
Specifically, it feels like we have a bit of a russian doll situation with these systems. 'CreateRequired' runs just the required generators, and 'Create' runs the rest. In other words, 'Create' should be able to pick up the results that 'CreateRequired' produced for the 'required generators'.
--
this actually made me realize something that i absolutely want audited/tested. It seemsl ike it would be bad (imo) if you had the same solution snapshot, and could observe generated required docs (the docs produced from CreateRequired) not being exactly the same if 'Create' then ran and produced docs from teh required generators.
(i don't know if that's actually something possible... since maybe that requires a different fork of the solution. )
src/Workspaces/Remote/ServiceHub/Services/SourceGeneration/RemoteSourceGenerationService.cs
Outdated
Show resolved
Hide resolved
Add assert
…orce the tracker if it needs it
14b67fe
to
21dbf0d
Compare
d77ca12
to
a0a4e82
Compare
Not ready to sign off on this. Can we do a (hopefully) final pass? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address more feedback
@@ -50,6 +50,9 @@ private sealed partial class RegularCompilationTracker : ICompilationTracker | |||
} | |||
else | |||
{ | |||
// Note: If we're in GeneratedDocumentCreationPolicy.CreateRequired it may be possible to skip going to OOP at all. If we know which generators are going to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open an issue.
...Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker_Generators.cs
Outdated
Show resolved
Hide resolved
@@ -50,6 +50,9 @@ private sealed partial class RegularCompilationTracker : ICompilationTracker | |||
} | |||
else | |||
{ | |||
// Note: If we're in GeneratedDocumentCreationPolicy.CreateRequired it may be possible to skip going to OOP at all. If we know which generators are going to run | |||
// we can see upfront if they are present and skip them if so. This is left as a future optimiziation for now. | |||
|
|||
// First try to compute the SG docs in the remote process (if we're the host process), syncing the results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the idea is that because we always jump to OOP, the two states stay in sync with each other by them each transitioning at the same time.
// If this generator has never been executed before or there are no existing results available to return, | ||
// we have it run the generator regardless of the specified creation policy to ensure correctness. | ||
if (priorRunResult?.Results.Any(r => r.Generator == context.Generator && !r.GeneratedSources.IsDefault) == false) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added lots of comments and broken out the logic.
// Either we have no state for the generator, in which case it can't have run. If we do have state, | ||
// the contract from the generator driver is that a generator that hasn't run yet produces a default | ||
// ImmutableArray for GeneratedSources. Note that this is different from an empty array, which | ||
// indicates that the generator ran, but didn't produce any documents: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. This is terrific
Thanks. Those docs are EXTREMELY helpful. I didn't look closely at tests. Ideally they cover all those cases in the ShouldRun function. In particular, it would be good to have a test that is in balanced mode, and pulls on the computer/generators without the razor gen being added. Then it adds razor and pulls again. Based on my reading, that should run razor. Also, do this for non-razor generator. It should run that generator of I'm reading right. Basically, I want to test those cases of hitting the default array of sources. Thanks! |
0a8f208
to
03f9961
Compare
6ed3fef
to
c78beb0
Compare
/backport to release/dev18.0 |
Started backporting to release/dev18.0: https://github.com/dotnet/roslyn/actions/runs/17274211923 |
@chsienki backporting to "release/dev18.0" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Always run 'required' generators: - Change DoNotCreate to CreateRequired - Add a flag to remote generation service that allows the caller to specify required generators only - Pass the flag to OOP based on the creation policy - Set the creation policy when creating a compilation tracker - Ensure we add in previous docs that weren't regenerated when running only required generators - Add a filter that only runs razor when required generators are requested
.git/rebase-apply/patch:163: trailing whitespace.
// the documents we got back are only for the required generators, meaning any documents from other generators remain the same.
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M src/Workspaces/Core/Portable/SourceGeneration/IRemoteSourceGenerationService.cs
M src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker_Generators.cs
M src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs
M src/Workspaces/Remote/ServiceHub/Services/SourceGeneration/RemoteSourceGenerationService.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Workspaces/Core/Portable/SourceGeneration/IRemoteSourceGenerationService.cs
Auto-merging src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker_Generators.cs
Auto-merging src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs
Auto-merging src/Workspaces/Remote/ServiceHub/Services/SourceGeneration/RemoteSourceGenerationService.cs
Applying: Tests
Using index info to reconstruct a base tree...
M src/Compilers/Test/Core/SourceGeneration/TestGenerators.cs
M src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Compilers/Test/Core/SourceGeneration/TestGenerators.cs
CONFLICT (content): Merge conflict in src/Compilers/Test/Core/SourceGeneration/TestGenerators.cs
Auto-merging src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0002 Tests
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
if (creationPolicy.GeneratedDocumentCreationPolicy is GeneratedDocumentCreationPolicy.DoNotCreate) | ||
var canSkipRunningGenerators = creationPolicy.GeneratedDocumentCreationPolicy is GeneratedDocumentCreationPolicy.DoNotCreate | ||
|| (creationPolicy.GeneratedDocumentCreationPolicy is GeneratedDocumentCreationPolicy.CreateOnlyRequired | ||
&& !await HasRequiredGeneratorsAsync(compilationState, cancellationToken).ConfigureAwait(false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so. this is blocking feeddback. my brain cannot figure out stuff like this. please extract a local function. Write simple statements for each case, with simple comments on each explainign it :)
var hasRequiredGenerators = generators.Any(g => g.IsRequiredGenerator()); | ||
return hasRequiredGenerators | ||
? SourceGeneratorPresence.ContainsRequiredSourceGenerators | ||
: SourceGeneratorPresence.OnlyOptionalSourceGenerators; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dedent. yes, i'm that anal.
/// <summary> | ||
/// The project contains no source generators. | ||
/// </summary> | ||
NoSourceGenerators, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
practically, i assume we're at the point that this never happens. So you could really just have this ContainsRequiredSourceGenerators and DoesNotContainRequiredSourceGenerators (just as a named bool) right?
Note: i'm not blocking this. if you feel this is clear and helpful, then we can keep. I'm just thinking out loud and trying to determine if it really would matter in practice.
} | ||
|
||
return false; | ||
return hasGenerators ? SourceGeneratorPresence.OnlyOptionalSourceGenerators | ||
: SourceGeneratorPresence.NoSourceGenerators; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically, i feel like this loop, and the bool is overwrought. but all the alternatives i can think of aren't much better. rimarily though, i feel like i want this to be:
references.Contains(r => r.Presense is .ContainsRequiredSourceGenerators) ? .ContainsRequiredSourceGenerators :
references.Contains(r => r.Presense is .OnlyOptionalSourceGenerators) ? .OnlyOptionalSourceGenerators : .NoSourceGenerators;
it took me a while to understand the looping form. but it's ok with how you have it.
consider just adding a comment indicating that this is the intende dsemantics. s omething like "if at least one reference contains required generators, then we contain required generators. Then if at least one reference has an optional generator, then we have an optional generator. Otherwise, we do nto have generators".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signing off on latest changes again, just with rhe request to clean up that one expression. All the rest is up to you.
} | ||
|
||
return false; | ||
// no required generators, all are optional | ||
return SourceGeneratorPresence.OnlyOptionalSourceGenerators; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's unclear to me if this is correct. what if isolatedReferences.Length > 0
but all of them return .NoGenerators for their generatorPresence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Derp, good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted to the previous loop but added extra comments to explain it.
5861e3d
to
aeb9982
Compare
- Add a CreateOnlyRequired generation state - Add a filter that only runs razor when required generators are requested Manual backport of #79510