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

Mark diagnostics as being 'live' or not in LSP #70402

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

CyrusNajmabadi
Copy link
Member

This allows the LSP client to know how to treat diagnotics that we produce against those that may be produced from other 'non-live' source (like CPS).

Specifically, this ensures that if the user does the following steps that they get the right results:

  1. FSA is off.
  2. User invokes "run code analysis". This produces non-live errors.
  3. User makes a change.
  4. User invokes a build. This produces non-live errors.

Currently, roslyn continues to report the results of '2' for the "last computed workspace results" when queried for workspace diagnostics in step 3 and beyond.
 
However, by adding this flag, we ensure that '4' supersedes the results of '2' by letting the client know that both are stale results and thus more recent stale results shoudl beat out less recent ones.

In all cases our 'live' diagnostics continue to beat all stale diagnostic sources.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner October 17, 2023 15:51
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 17, 2023
// Mark this also as a build error. That way an explicitly kicked off build from a source like CPS can
// override it.
if (!isLiveSource)
result.Add(VSDiagnosticTags.BuildError);
Copy link
Member Author

Choose a reason for hiding this comment

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

@mavasani should isLiveSource = false always imply potentialDuplicate?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the implication of adding PotentialDuplicate tag? Does it mark it for de-duping?

Copy link
Member Author

Choose a reason for hiding this comment

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

Taking offline to discuss.

@mavasani
Copy link
Contributor

@CyrusNajmabadi overall LGTM, though I'd want to confirm if you are able to validate the exact 4-step scenario mentioned in the PR description works fine after this PR.

@kayle
Copy link

kayle commented Oct 17, 2023

I think this only works if msbuild & Roslyn use the same identifier/hashing for diagnostics. LSP & Build diagnostics are two unrelated sources, so the client can only dedupe if the diagnostics reuse the same identifier string.

@CyrusNajmabadi
Copy link
Member Author

@adrianvmsft was doing the work to ensure we had the same ID i believe. Did that work go in @adrianvmsft ? Do we still need to do anything there to ensure we're doing things the same way?

@CyrusNajmabadi
Copy link
Member Author

@mavasani this may depend on #70410 going through.

@CyrusNajmabadi
Copy link
Member Author

Merging in. We still want this so that "run code analysis" results show up in the 'build' tab. Deduping isn't currently needed because when a build is kicked off we will clear out these results. We can consider further improvements in this area if desired (balanced against cost/complexity).

@CyrusNajmabadi CyrusNajmabadi merged commit ce04f4c into dotnet:main Oct 20, 2023
@ghost ghost added this to the Next milestone Oct 20, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the liveSource branch October 24, 2023 15:55
@jjonescz jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants