Ensure we sync source-generator versions over properly when doing a cone-sync #73688
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| return Checksum.Create(checksums); | ||
| } |
| AssetPathKind.SolutionSourceGeneratorExecutionVersionMap, newSolutionCompilationChecksums.SourceGeneratorExecutionVersionMap, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| solution = solution.WithSourceGeneratorExecutionVersions(newVersions, cancellationToken); | ||
| } |
There was a problem hiding this comment.
Note: this is something that was missing. but is also a stopgap. the right way to do things is to do teh initial bulk sync, then run the normal 'delta' sync algorithm to ensure that all checksums match. That's what #72860, but it has a problem i'm working with Jason on.
|
|
||
| // For each project, we'll add one checksum for the project id and one for the version map. | ||
| using var _ = ArrayBuilder<Checksum>.GetInstance(2 * supportedCount, out var checksums); | ||
| public SourceGeneratorExecutionVersionMap GetFilteredSourceGenerationExecutionMap(ProjectCone? projectCone) |
There was a problem hiding this comment.
this was very much part of the functional fix. When we're producing the checksum on the host side, we want to filter the map to what's in the cone. Then, on the oop side, we want to be able to ask for this filtered map portion. We then update that part of the OOP side with this filtered set.
| checksums.Add(Checksum.Create(version, static (v, w) => v.WriteTo(w))); | ||
| } | ||
|
|
||
| return Checksum.Create(checksums); |
There was a problem hiding this comment.
a bit off topic: is the ReadOnlySpan overload of Checksum.Create more performant than the ImmutableArray/ArrayBuilder versions? If so, and not that it helps here, couldn't the ImmutableArray overload get the underlying array and use the equivalent of the readonlyspan implementation?
There was a problem hiding this comment.
yes. the IA version should defer to the ROS version.
That said, this is not an IA. This is an ArrayBuilder.
| checksums.Add(Checksum.Create(@this.SourceGeneratorExecutionVersionMap[projectId], static (v, w) => v.WriteTo(w))); | ||
| builder.Remove(projectId); | ||
| } | ||
| else if (projectCone != null && !projectCone.Contains(projectId)) |
There was a problem hiding this comment.
There was a problem hiding this comment.
a project-cone very often spans multiple projects. it represents a project and all the projects that project depends on.
There was a problem hiding this comment.
I'm just wondering whether it would be better to build this up by testing for inclusion rather than reduce this by testing for exclusion.
that would work as well. I don't have strong feelings.
There was a problem hiding this comment.
That was a great explanation, thank you! Can you put a blurb in the code so I don't have that thought again?
|
@ToddGrun @jasonmalinowski i def want to get this in today. |
| Debug.Assert(projectCone.ProjectIds.Count == newVersions.Map.Count); | ||
| Debug.Assert(projectCone.ProjectIds.All(id => newVersions.Map.ContainsKey(id))); |
There was a problem hiding this comment.
Do we not have a SetEquals helper to call? This works but would be shorter/clearer.
There was a problem hiding this comment.
def can clean that up in followup.
| { | ||
| await solution.AppendAssetMapAsync(map, project1.Id, CancellationToken.None); | ||
| var project1Checksum = await solution.CompilationState.GetChecksumAsync(project1.Id, CancellationToken.None); | ||
| var project1SyncedSolution = await remoteWorkspace.GetTestAccessor().GetSolutionAsync(assetProvider, project1Checksum, updatePrimaryBranch: false, CancellationToken.None); |
There was a problem hiding this comment.
Did this need an extra assert?
There was a problem hiding this comment.
No. The impl actually already asserts that the checksums are the same (and throws if not). (so this failed prior to this fix).
No description provided.