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

Remove unnecessary type for wrapping analyzer references. #74739

Merged

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner August 13, 2024 17:45
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 13, 2024
// Dispose of any analyzers that were removed now that we've applied the changes.
foreach (var analyzer in _analyzersRemovedInBatch)
{
analyzer.Dispose();
Copy link
Member Author

Choose a reason for hiding this comment

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

no need to Dispose, as Dispose ended up doing nothing. Will demosntrate later.

}
}

public void Dispose()
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the crux of this change. Note what DIspose does. It clears teh internal state of this type, grabs any load diagnostics produced... then DISCARDS them (loadErrors) is not read at all.

As such, this type does all this work to connect to events to listen for things that happen, only to utterly ignore them. As such, we can safely remove anything related to load errors. Since we can ignore those, we don't need to subscribe to AnalyzerLoadFailed event either. Which means this type has no purpose at all and can be entirely removed.

Note: perhaps it's a bug that we're missing LoadFailed errors. But that's the current state of the world, so this isn't making anything worse. If we do want to get these errors, we need that to likely happen in OOP, and have things reported from that end.

@Cosifne
Copy link
Member

Cosifne commented Aug 13, 2024

#43008

Does this mean this work item is completed? 🤔

@CyrusNajmabadi
Copy link
Member Author

@Cosifne Yup. I'll take care of that :)

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.

3 participants