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

S3966: Remove FP on streams special handling #3647

Merged
merged 5 commits into from
Oct 8, 2020

Conversation

costin-zaharia-sonarsource
Copy link
Member

Fixes #3644

@costin-zaharia-sonarsource costin-zaharia-sonarsource changed the title S3966: Remove streams special handling S3966: Remove FP with streams special handling Oct 7, 2020
@costin-zaharia-sonarsource costin-zaharia-sonarsource changed the title S3966: Remove FP with streams special handling S3966: Remove FP on streams special handling Oct 7, 2020
Copy link
Contributor

@andrei-epure-sonarsource andrei-epure-sonarsource left a comment

Choose a reason for hiding this comment

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

Just to drop a few remarks.

The StreamWriter constructor docs says

Unless you set the leaveOpen parameter to true, the StreamReader object calls Dispose() on the provided Stream object when StreamReader.Dispose is called.

In practice it calls Close() but close eventually calls Dispose(). In the remarks for the Close() docs

This method calls Dispose, specifying true to release all resources. You do not have to specifically call the Close method. Instead, ensure that every Stream object is properly disposed. You can declare Stream objects within a using block (or Using block in Visual Basic) to ensure that the stream and all of its resources are disposed, or you can explicitly call the Dispose method.

Even in the StreamWriter itself, close() calls Dispose(true).

So the reasoning that Close() isn't the same thing as Dispose() is wrong.

I guess it's still ok to remove this example because we were suggesting to replace it with some cumbersome , not nice try-catch replacement.

@@ -1,33 +1,20 @@
<p>A proper implementation of <code>IDisposable.Dispose</code> should allow for it to be called multiple times on the same object, however this is not
guaranteed and could result in an exception being thrown.</p>
<p>It is best not to rely on this behaviour and therefore make sure an object is disposed only once on all execution paths. This is particularly true
<p>It is best not to rely on this behavior and therefore make sure an object is disposed only once on all execution paths. This is particularly true
Copy link
Contributor

Choose a reason for hiding this comment

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

is the nested using statements still valid?

This is particularly true when dealing with nested using statements.

I would check with @nicolas-harraudeau-sonarsource on how it's best to word the new implementation

Choose a reason for hiding this comment

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

Thanks for pinging me @andrei-epure-sonarsource.
I rewrote the rule description with the help of @costin-zaharia-sonarsource.

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussion with @nicolas-harraudeau-sonarsource we decided to add the rule back to Sonar Way. With the latest changes most of the FPs will disappear and the found issues on peach reveal code which is error prone.

@andrei-epure-sonarsource
Copy link
Contributor

After these changes... will this rule only catch when a resource is manually disposed a second time via .Dispose() , after a using? In this case, the rule wouldn't be that bad because we'd detect a code smell (hard to read - why Dispose if already using has been used)?

@costin-zaharia-sonarsource
Copy link
Member Author

I guess it's still ok to remove this example because we were suggesting to replace it with some cumbersome , not nice try-catch replacement.

Additionally, calling dispose twice in this case is not a problem since there is no exception thrown.

@costin-zaharia-sonarsource
Copy link
Member Author

After these changes... will this rule only catch when a resource is manually disposed a second time via .Dispose() , after a using? In this case, the rule wouldn't be that bad because we'd detect a code smell (hard to read - why Dispose if already using has been used)?

Yes, what's left now is the check that the Dispose was explicitly called twice or after a using.

@sonarcloud
Copy link

sonarcloud bot commented Oct 8, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@andrei-epure-sonarsource andrei-epure-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Oct 8, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit 2a4dbc2 into master Oct 8, 2020
@pavel-mikula-sonarsource pavel-mikula-sonarsource deleted the costin/3644 branch October 8, 2020 16:16
Corniel pushed a commit to Corniel/sonar-csharp that referenced this pull request Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule S3966: FP when using StreamWriter
4 participants