Fix issue with source generator diagnostics not refreshing on save in… #79507
Conversation
| public override async Task<(int globalStateVersion, VersionStamp? dependentVersion, Checksum? executionVersion)> ComputeCheapVersionAsync(DiagnosticsRequestState state, CancellationToken cancellationToken) | ||
| { | ||
| return (state.GlobalStateVersion, await state.Project.GetDependentVersionAsync(cancellationToken).ConfigureAwait(false)); | ||
| Checksum? executionVersion = state.Project.Solution.CompilationState.SourceGeneratorExecutionVersionMap.Map.TryGetValue(state.Project.Id, out var sourceGeneratorExecutionVersion) ? |
There was a problem hiding this comment.
This was the problem - the cheap version is not incrementing when the sg execution version changed (the expensive one did and does, that was added in #77271).
A couple of alternatives to this, not sure if they are better
Project.GetDependentVersionAsynccould include the execution version (may have downstream impacts on other callers)- Remove the cheap version check entirely in favor of
Project.GetDiagnosticChecksumAsyncwhich checks execution version. Downside is potentially more computation of the checksums
There was a problem hiding this comment.
1 seems possible. Though I'm not actually sure how it would work. Wouls the sg execution map also map to a Version that changed whenever the value charged?
That actually seems somewhat reasonable...
I do like 2 though. It just removes complexity here. I'm not concerned about checksum cost. We're constantly computing (and caching those values), so I don't think this fast path is generally that helpful.
So let's do 2.
There was a problem hiding this comment.
- works for me, and the same checksums are also getting computed for code actions quite frequently anyway already.
|
Also, I realize I'm not understanding something. The point of the two
checks is so that we could fast path knowing if something changed. So if
the version check says "it changed" then we recompute. That's cheap and
let's us easily recompute for many ops. But if the version says it is the
same, then we need to do the full and accurate checksum check.
So it should be fine if the execution info isn't in the cheap version check
right? That still means we'll run the checksum check in that case.
…On Tue, Jul 22, 2025, 3:15 AM David Barbet ***@***.***> wrote:
@dibarbet <https://github.com/dibarbet> requested review from
@dotnet/roslyn-ide on: #79507
<#79507> Fix issue with source
generator diagnostics not refreshing on save in… as a code owner.
—
Reply to this email directly, view it on GitHub
<#79507 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABC2MY5ADEENJ7NRHGSOP6T3JWGBZAVCNFSM6AAAAACCBJ6DS2VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJYG42DAMJRGAZDAMY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Not quite that - if the cheap check matches, that is supposed to mean that definitely nothing changed (because its essentially the versionstamp which almost always increments on anything changed). Then if the cheap check does not match, we do the more expensive checksum check to see if anything is different that might affect diagnostics. |
well in that case, we def need executionmap info to roll into the 'version' info for a project. that's morally fine. -- but, i would love to get us onto checksums entirely. versionstamps are just... bleagh :) |
dibarbet
force-pushed
the
sg_refresh_save
branch
from
0e7a193 to
e58e14f
Compare
3 participants
… balanced mode
Resolves #75586
Support for this was originally added in #77271
However it seems like
Project.GetDependentVersionAsyncchanged to no longer increment when the sg execution version changed, and due to a test issue, the problem was not caught.Now:
clean val - https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/656041