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.

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

DisposableFieldsSuppressor and Analyzer #576

Merged
merged 11 commits into from
Sep 6, 2023
Merged

DisposableFieldsSuppressor and Analyzer #576

merged 11 commits into from
Sep 6, 2023

Conversation

manfred-brands
Copy link
Member

@manfred-brands manfred-brands commented Aug 21, 2023 •
edited
Loading

Fixes #568

There is a new DiagnosticSuppressor that suppresses the CA1001 error if the class is a TestFixture.
There is also a new DiagnosticsAnalyzer which analyzes fields.

  1. If an IDisposable field is set in the OneTimeSetUp method it is only Disposed in the OneTimeTearDown method.
  2. All other IDisposable fields must be disposed in TearDown method.
  3. Add support for IAsyncDisposable

Disposal detection is limited, but I think probably sufficient for a TearDown method.
As long as field.Dispose() or field?.Dispose() is called, it is deemed disposed.
It could be that the field is conditionally assigned and therefore conditionally Disposed.
It is way beyond the scope to find out that the conditions are different.

@manfred-brands manfred-brands marked this pull request as draft August 21, 2023 03:06
@manfred-brands manfred-brands force-pushed the Issue568DisposableFields branch from 471ce36 to d67a14d Compare August 29, 2023 05:38
@manfred-brands manfred-brands marked this pull request as ready for review August 29, 2023 06:27
@manfred-brands manfred-brands changed the title Initial DisposableFieldsSuppressor DisposableFieldsSuppressor amd Analyzer Aug 29, 2023
@mikkelbu mikkelbu changed the title DisposableFieldsSuppressor amd Analyzer DisposableFieldsSuppressor and Analyzer Aug 29, 2023
mikkelbu
mikkelbu reviewed Aug 29, 2023
View reviewed changes
Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

I still have to review most of DisposeFieldsInTearDownAnalyzer (I think I've reviewed one quarter of it). I hope I've time to do it tomorrow when my head is not overloaded.

So far it looks great 👍

src/nunit.analyzers/Extensions/ITypeSymbolExtensions.cs Outdated Show resolved Hide resolved
src/nunit.analyzers.tests/DisposeFieldsInTearDown/DisposeFieldsInTearDownAnalyzerTests.cs Outdated Show resolved Hide resolved
src/nunit.analyzers/DisposeFieldsInTearDown/DisposeFieldsInTearDownAnalyzer.cs Outdated Show resolved Hide resolved
mikkelbu
mikkelbu approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

Great work @manfred-brands. I only have some minor comments, questions, suggestions

src/nunit.analyzers/DisposeFieldsInTearDown/DisposeFieldsInTearDownAnalyzer.cs Outdated Show resolved Hide resolved
src/nunit.analyzers/DisposeFieldsInTearDown/DisposeFieldsInTearDownAnalyzer.cs Outdated Show resolved Hide resolved
src/nunit.analyzers/DisposeFieldsInTearDown/DisposeFieldsInTearDownAnalyzer.cs Outdated Show resolved Hide resolved
src/nunit.analyzers/DisposeFieldsInTearDown/DisposeFieldsInTearDownAnalyzer.cs Outdated Show resolved Hide resolved
src/nunit.analyzers/DisposeFieldsInTearDown/DisposeFieldsInTearDownAnalyzer.cs Outdated Show resolved Hide resolved
src/nunit.analyzers/DisposeFieldsInTearDown/DisposeFieldsInTearDownAnalyzer.cs Outdated Show resolved Hide resolved
@manfred-brands manfred-brands force-pushed the Issue568DisposableFields branch from d67a14d to 92687c5 Compare August 31, 2023 04:06
@mikkelbu
Copy link
Member

Looks good @manfred-brands. I'm happy to merge this when you are

@manfred-brands
Copy link
Member Author

I'll do some test on existing code bases to see if it picks things up as expected.

@manfred-brands
Copy link
Member Author

I get some false positives on writers that are Closed but not Disposed. CA2000 is happy with Close. I will add support for that.

@manfred-brands
Copy link
Member Author

TODO: Items found during letting it loose on a big project

  • Fix await (including .ConfigureAwait(false) suffix)
  • Add support for Disposal in try/finally.
  • Only analyze TestFixtures. It now actually complain about non-test fixtures.
  • Make Exception for 'Task'. It is Disposable, but shouldn't be disposed.
  • Do not complain about variables assigned using 'copy' assignments. Only check 'new' and returns of static factory methods.

@manfred-brands
Copy link
Member Author

@mikkelbu Can you please re-review. I made several changes after letting it loose on our main project.

I also found two other bugs in the current version and created two separate small PRs for them.

@mikkelbu
Copy link
Member

mikkelbu commented Sep 5, 2023

@mikkelbu Can you please re-review. I made several changes after letting it loose on our main project.

I also found two other bugs in the current version and created two separate small PRs for them.

I've reviewed the other PRs, and I'll take a look at this tonight (as it requires some more time)

mikkelbu
mikkelbu approved these changes Sep 5, 2023
View reviewed changes
Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

I did the review one commit at a time and you corrected the few minor nitpicks I found in subsequent commits, so great work 👍. So I only have one typo left.

...alyzers/DisposeFieldsAndPropertiesInTearDown/DisposeFieldsAndPropertiesInTearDownAnalyzer.cs Outdated Show resolved Hide resolved
@manfred-brands
Add DisposableFieldsSuppressor and DisposeFieldsInTearDownAnalyzer
dab02a7
@manfred-brands
Code review changes.
68ca208
@manfred-brands
Add support for more cases
16daf9f 
* await
* ConfigureAwait
* try
* finally
* Close is recognized as Dispose method
@manfred-brands
Enhancements:
70ab1b5 
* Ignore classes that are not a TestFixture
* Ignore some types that don't need Disposing, e.g. Task
* Only check fields assigned from constructors and methods, not from copy and other expressions.
@manfred-brands
Add support for properties and constructors/initializers
9081f44
@manfred-brands
Add support for conditional Dispose
a60221b
@manfred-brands
Add support for field with a TypeParameter type
3015d1b
@manfred-brands
Rename 'fields' with better name.
9a6ee66
@manfred-brands
Added Parameters class so we don't need to pass in lots of fields.
27bcdef 
Add support for Release(field) and allow configuring extra release methods.
@manfred-brands
Changed to use single HashSet per operation.
5237532 
Previously it created several and then merged them.
@manfred-brands
Code review commit
9fee186
@manfred-brands manfred-brands force-pushed the Issue568DisposableFields branch from 0e49e22 to 9fee186 Compare September 6, 2023 01:02
@manfred-brands
Copy link
Member Author

Thanks @mikkelbu for your reviews.

@manfred-brands manfred-brands merged commit 81c26ba into nunit:master Sep 6, 2023
3 checks passed
@manfred-brands manfred-brands deleted the Issue568DisposableFields branch September 6, 2023 01:11
@mikkelbu mikkelbu added this to the Release 3.7 / 2.7 milestone Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikkelbu mikkelbu mikkelbu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 3.7 / 2.7
Development

Successfully merging this pull request may close these issues.

Feature request: suppress CA1001 when Dispose is called in the TearDown method
2 participants
@manfred-brands @mikkelbu