Respond with updated diagnostics when EnC diagnostics state is different#60081
Conversation
| { | ||
| _previouslyHadDiagnostics = false; | ||
| _diagnosticsVersion++; | ||
| } |
There was a problem hiding this comment.
any concern about threadsafety?
There was a problem hiding this comment.
These methods are called in exactly one spot, and the caller didn't seem to worry, so I didn't worry. I'm sure @tmat can comment more though
src/Features/Core/Portable/EditAndContinue/EditAndContinueDiagnosticUpdateSource.cs
Show resolved
Hide resolved
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
honestly, this looks fine to me. To @dibarbet if he has concerns. It's a bit ugly that diagnostics has to know about enc, but i can't think of a better way myself.
I agree. A better checksum that knew when a build had been done would be ideal I think. |
src/Features/Core/Portable/EditAndContinue/EditAndContinueDiagnosticUpdateSource.cs
Outdated
Show resolved
Hide resolved
…nosticUpdateSource.cs
...otocol/Handler/Diagnostics/Experimental/ExperimentalDocumentPullDiagnosticHandlerProvider.cs
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache`2.cs
Outdated
Show resolved
Hide resolved
I agree this is fine for now - lets wait to see if we encounter more similar issues before doing something more complex |
| // Even though we only report diagnostics, and not rude edits, we still need to | ||
| // ensure that the presence of rude edits are considered when we decide to update | ||
| // our version number. | ||
| if (diagnostics.Any() || rudeEdits.Any(d => d.Diagnostics.Any())) |
tmat
left a comment
There was a problem hiding this comment.
Seems ok as a workaround. Hopefully we'll be able to replace this with a better system once we remove solution crawler and fully switch to pull diagnostics.
…are value types or reference types.
|
/azp run roslyn-integration-corehost |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run roslyn-integration-corehost |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Build finally green. Conflicts. FML. |
|
/azp run roslyn-integration-corehost |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run roslyn-integration-corehost |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run roslyn-integration-corehost |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes AB#1481208
See discussion in LSPPod (which for some reason I can't link to ¯\_(ツ)_/¯) for context. If there is an obviously better solution here, I'm all ears!