Skip to content

Enable CA2213: Disposable fields should be disposed #15805

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.

Sign up for GitHub

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

Jump to bottom
Closed
xtqqczze wants to merge 2 commits into from
Closed

Enable CA2213: Disposable fields should be disposed #15805

xtqqczze wants to merge 2 commits into from

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jul 20, 2021 •
edited
Loading

Enable CA2213: Disposable fields should be disposed

  • Suppress existing violations with #pragma warning disable
  • Disable at project level for Microsoft.PowerShell.Commands.Utility due to error that was not suppressible with #pragma:
CSC : error CA2213: 'FrontEndCommandBase' contains field 'implementation' that is of IDisposable type 'ImplementationCommandBase', but it is never disposed. Change the Dispose method on 'FrontEndCommandBase' to call Close or Dispose on this field. [X:\src\Microsoft.PowerShell.Commands.Utility\Microsoft.PowerShell.Commands.Utility.csproj]

Contributes to #15803.

Related issue: dotnet/roslyn-analyzers#4407

cc: @iSazonov

@ghost ghost assigned daxian-dbw Jul 20, 2021
@xtqqczze xtqqczze mentioned this pull request Jul 20, 2021
Open
32 tasks
@xtqqczze xtqqczze closed this Jul 20, 2021
@xtqqczze xtqqczze reopened this Jul 20, 2021
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 20, 2021

CodeFactor "complex method" issues are false positives, see #15721.

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 21, 2021

I'd qualify these all as bugs - if a class is IDisposable we should implement and call Dispose().

@xtqqczze xtqqczze closed this Jul 21, 2021
@xtqqczze xtqqczze reopened this Jul 21, 2021
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 21, 2021

I'd qualify these all as bugs - if a class is IDisposable we should implement and call Dispose().

Unfortunately it is not quite that simple, some disposables are not owned by the class as they are injected via a property, etc.

In any case, I've annotated each suppression with a link to #15803, so we can track these issues.

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 21, 2021

I'd qualify these all as bugs - if a class is IDisposable we should implement and call Dispose().

Unfortunately it is not quite that simple, some disposables are not owned by the class as they are injected via a property, etc.

In any case, I've annotated each suppression with a link to #15803, so we can track these issues.

We need to review every case and either fix as bug or suppress with description why it is not bug and maybe add more info in (XML) docs.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 21, 2021 •
edited
Loading

PowerShell-CI-linux test failure, see #15814.

@xtqqczze xtqqczze marked this pull request as ready for review July 21, 2021 18:44
@ghost ghost added the Review - Needed The PR is being reviewed label Jul 29, 2021
@ghost
Copy link

ghost commented Jul 29, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@xtqqczze xtqqczze closed this Jul 29, 2021
@xtqqczze xtqqczze reopened this Jul 29, 2021
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 29, 2021

We need to review every case and either fix as bug or suppress with description why it is not bug and maybe add more info in (XML) docs.

We could merge this PR to enable the rule, and then review each case in #15803,

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 30, 2021

/rebase

@ghost ghost removed the Review - Needed The PR is being reviewed label Jul 30, 2021
@ghost ghost added the Review - Needed The PR is being reviewed label Aug 6, 2021
@ghost
Copy link

ghost commented Aug 6, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Aug 6, 2021

@iSazonov Can we merge this, and then resolve the suppression issues tracked by #15803?

iSazonov
iSazonov reviewed Aug 7, 2021
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/Microsoft.PowerShell.Commands.Utility.csproj
<Description>PowerShell's Microsoft.PowerShell.Commands.Utility project</Description>
<NoWarn>$(NoWarn);CS1570;CA1416</NoWarn>
<!-- CA2213: https://github.com/PowerShell/PowerShell/issues/15803 -->
<NoWarn>$(NoWarn);CS1570;CA1416;CA2213</NoWarn>
Copy link
Collaborator

@iSazonov iSazonov Aug 7, 2021

Choose a reason for hiding this comment

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

Why do we need this if we suppress with pragma in files?

Copy link
Contributor Author

@xtqqczze xtqqczze Aug 7, 2021

Choose a reason for hiding this comment

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

I could not suppress with pragma as the error was not tracible to a specific line of code, please see the PR description.

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 7, 2021

Since the issues are in different subsystems they will be analyzed and fixed by different experts. So I think it make sense to enumerate the issues with code references in #15803 as task list.

@ghost ghost removed the Review - Needed The PR is being reviewed label Aug 7, 2021
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Aug 9, 2021

@iSazonov Issues are listed with code references in #15803.

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 9, 2021

@xtqqczze Thanks! I think we should fix bugs before we merge the PR to avoid hiding the bugs.

@ghost ghost added the Review - Needed The PR is being reviewed label Aug 16, 2021
@ghost
Copy link

ghost commented Aug 16, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@daxian-dbw
Copy link
Member

daxian-dbw commented Nov 2, 2021

@xtqqczze and @iSazonov, thank both of you. I completely agree that the exposed violation needs to be reviewed and potentially fixed before moving on enabling this rule.

@ghost ghost removed the Review - Needed The PR is being reviewed label Nov 2, 2021
@daxian-dbw
Copy link
Member

daxian-dbw commented Nov 2, 2021

I updated the issue description of #15803 with the following words:

TODO: after reviewing and fixing all violations listed here, please re-open #15805 to enable the CA2213 rule so as to make sure no violations in future.

For now, I will close this PR for housekeeping purpose.

@daxian-dbw daxian-dbw closed this Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13 TravisEz13 is a code owner

Assignees

@daxian-dbw daxian-dbw

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xtqqczze @iSazonov @daxian-dbw