Skip to content
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

Fix source-generator reload when in 'balanced' mode #75831

Merged
merged 16 commits into from
Nov 9, 2024

Conversation

CyrusNajmabadi
Copy link
Member

Previously, if a source generator changed on disk, and hte user was in "balanced" mode, we wouldn't reload it until the next save/build point. This isn't a great experience, especially as changing on disk is a rare occurrence, and we want users to see those updates happen immediately to know that the new SG was picked up.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner November 8, 2024 19:54
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 8, 2024
/// will have their source generator versions updated. If the solution is specified, then all projects will have
/// their versions updated.
/// </summary>
public static SourceGeneratorExecutionVersionMap GetUpdatedSourceGeneratorVersions(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a move.

@@ -846,7 +846,7 @@ public ProjectState WithMetadataReferences(IReadOnlyList<MetadataReference> meta
return With(projectInfo: ProjectInfo.With(metadataReferences: metadataReferences).WithVersion(Version.GetNewerVersion()));
}

public ProjectState WithAnalyzerReferences(IEnumerable<AnalyzerReference> analyzerReferences)
public ProjectState WithAnalyzerReferences(IReadOnlyList<AnalyzerReference> analyzerReferences)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stronger typing, and this ensures we're not lazily producing these values every time.

}

return WithCompilationState(CompilationState.AddAnalyzerReferences(this.SolutionState.AddAnalyzerReferences(projectId, collection), collection));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only expose Add/Remove at the top level. internally, these all boil down to 'With' calls. this makes it so we have a single chokepoint to add some new logic.

@@ -713,17 +713,6 @@ public SolutionCompilationState WithProjectMetadataReferences(
forkTracker: true);
}

/// <inheritdoc cref="SolutionState.AddAnalyzerReferences(ProjectId, ImmutableArray{AnalyzerReference})"/>
public SolutionCompilationState AddAnalyzerReferences(StateChange stateChange, ImmutableArray<AnalyzerReference> analyzerReferences)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed. This all goes through WithProjectAnalyzerReferences now

@@ -748,22 +737,11 @@ public SolutionCompilationState WithAnalyzerReferences(IReadOnlyList<AnalyzerRef
return Branch(this.SolutionState.WithAnalyzerReferences(analyzerReferences));
}

/// <inheritdoc cref="SolutionState.RemoveAnalyzerReference(ProjectId, AnalyzerReference)"/>
public SolutionCompilationState RemoveAnalyzerReference(ProjectId projectId, AnalyzerReference analyzerReference)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed. This all goes through WithProjectAnalyzerReferences now

/// Create a new solution instance with the project specified updated to include the
/// specified analyzer references.
/// </summary>
public StateChange AddAnalyzerReferences(ProjectId projectId, ImmutableArray<AnalyzerReference> analyzerReferences)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed. This all goes through WithProjectAnalyzerReferences now

@@ -56,72 +56,5 @@ await this.SetCurrentSolutionAsync(
cancellationToken).ConfigureAwait(false);

return;

static SourceGeneratorExecutionVersionMap GetUpdatedSourceGeneratorVersions(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to helper used both here (when we hear about saves/builds), and by the compilation-state instance wqhen it is told that analyzer references changed.


[Theory, CombinatorialData]
internal async Task UpdatingAnalyzerReferenceReloadsGenerators(
SourceGeneratorExecutionPreference executionPreference)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test that validates that boht automatic and balanced works.

Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable


// As we're changing the references for a project we want to force generators to rerun for it. This way the
// user automatically sees the new generators values without having to take any more explicit steps.
var updatedMap = GetUpdatedSourceGeneratorVersions(this,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible that there are not actually any changed references here, and we end up force re-generation needlessly? Or does the caller guarantee that there are definitely changed references?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind - seems like the callers are guaranteeing that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. callers guarantee

// As we're changing the references for a project we want to force generators to rerun for it. This way the
// user automatically sees the new generators values without having to take any more explicit steps.
var updatedMap = GetUpdatedSourceGeneratorVersions(this,
ImmutableSegmentedList<(ProjectId? projectId, bool forceRegeneration)>.Empty.Add((projectId, true)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this is faster than just [(projectId, true)]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately, that type doesn't expose collection initializer support yet. @sharwell for segmented list support.

/// their versions updated.
/// </summary>
public static SourceGeneratorExecutionVersionMap GetUpdatedSourceGeneratorVersions(
SolutionCompilationState solution, ImmutableSegmentedList<(ProjectId? projectId, bool forceRegeneration)> projectIds)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImmutableSegmentedList

Consider moving off segmented. Doesn't seem like it would be needed.

}

/// <summary>
/// Given the current state of a <paramref name="solution"/>, produced an updated version of the source generator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

produced

super nit: typo

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@CyrusNajmabadi CyrusNajmabadi marked this pull request as draft November 8, 2024 21:58
auto-merge was automatically disabled November 8, 2024 21:58

Pull request was converted to draft

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review November 8, 2024 22:15
@CyrusNajmabadi CyrusNajmabadi merged commit 51b22b5 into dotnet:main Nov 9, 2024
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 9, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the sgReloadBalanced branch November 11, 2024 16:37
@jjonescz jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants