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

Fix S2930 FP: Do not report on fields of IDisposable classes #8336

Closed
pavel-mikula-sonarsource opened this issue Nov 8, 2023 · 1 comment
Labels
Area: C# C# rules related issues. Area: CFG/SE CFG and SE related issues. Area: VB.NET VB.NET rules related issues. Type: False Positive Rule IS triggered when it shouldn't be.

Comments

@pavel-mikula-sonarsource
Copy link
Contributor

public class Sample : IDisposable
{
    private readonly CancellationTokenSource source;

    public Sample() =>
        source = new(); // FP

   public void Dispose() =>
        source.Dispose();
}

and also

public class Sample : IDisposable
{
    private readonly CancellationTokenSource source;

    public Sample() =>
        source = new(); // FP, the fact that disposable field was not disposed should be another rule

   public void Dispose()
   {
        // Did we forget to dispose something here?
   }
}
@pavel-mikula-sonarsource pavel-mikula-sonarsource added Area: VB.NET VB.NET rules related issues. Area: C# C# rules related issues. Type: CFG/SE FPs Rule IS triggered when it shouldn't be for CFG and SE rules. Area: CFG/SE CFG and SE related issues. labels Nov 8, 2023
@Tim-Pohlmann Tim-Pohlmann added the Sprint: Hardening Fix FPs/FNs/improvements label Apr 8, 2024
@Tim-Pohlmann Tim-Pohlmann added this to the 9.24 milestone Apr 8, 2024
@Tim-Pohlmann Tim-Pohlmann self-assigned this Apr 9, 2024
@Tim-Pohlmann Tim-Pohlmann added Type: False Positive Rule IS triggered when it shouldn't be. and removed Type: CFG/SE FPs Rule IS triggered when it shouldn't be for CFG and SE rules. labels Apr 9, 2024
@Tim-Pohlmann
Copy link
Contributor

Multiple things:

  1. I removed the CFG/SE label because this rule is not Symbolic Exection-based, nor does it use the CFG. That being said, we could consider rewriting it via SE.
  2. The first sample does not reproduce (anymore?).
  3. Regarding the second sample, I want to challenge the idea of having a separate rule for this case. S2930 is intelligent enough to differentiate between samples Fix dotcover error with weird file paths #1 and Request to pull changes for code coverage statistics please #2. Therefore, I find it unnecessary to add more complexity to the implementation by splitting it into multiple rules.
  4. Under the premise that the second sample should trigger a different rule: The other rule does not exist yet. Thus, suppressing this case would cause more harm than good => I'm closing this issue.

@Tim-Pohlmann Tim-Pohlmann closed this as not planned Won't fix, can't repro, duplicate, stale Apr 9, 2024
@Tim-Pohlmann Tim-Pohlmann removed the Sprint: Hardening Fix FPs/FNs/improvements label Apr 9, 2024
@Tim-Pohlmann Tim-Pohlmann removed this from the 9.24 milestone Apr 9, 2024
@Tim-Pohlmann Tim-Pohlmann removed their assignment Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: CFG/SE CFG and SE related issues. Area: VB.NET VB.NET rules related issues. Type: False Positive Rule IS triggered when it shouldn't be.
Projects
None yet
Development

No branches or pull requests

2 participants