BADF00D / DisposableFixer

This is a Visual Studio Extension and NuGet package that should identify and fix problems as memleaks while using IDisposables.
Other
35 stars 7 forks source link

Null-condition operations aren't recognised #48

Closed teh-hippo closed 7 years ago

teh-hippo commented 7 years ago

Hey mate;

LOVE the extension. It's so easy to forget to properly dispose of objects, and I appreciate the time you've put in getting this one done.

It seems pretty good to me, but I've found it doesn't recognise null-condition operations. For example:

private IDisposable _toDispose;

public void OptionalCall() {
    _toDispose = File.Open("");
}

public void Dispose() {
    _toDispose?.Dispose();
}

Is that something that can be added?

BADF00D commented 7 years ago

Thanks for the compliment and the bug report. Actually I stumbled upon this error a while ago. But I thought I fixed it in issue #33. Can you check which version you are using? It should be fixed since version 0.21.

I can see, that the test that verifies this issue only test local variables, maybe its not correctly detected when used on fields.

teh-hippo commented 7 years ago

I'm not able to check; I've left the office for the day, so it'll be my tomorrow. l did only install it about 10 hours ago however; should that be the latest?

I'm on Visual Studio 2017, C# 7.0.

On 12 Jul 2017, at 7:07 pm, BADF00D notifications@github.com wrote:

Thanks for the compliment and the bug report. Actually I stumbled upon this error a while ago. But I thought I fixed it in issue #33. Can you check which version you are using? It should be fixed since version 0.21.

I can see, that the test that verifies this issue only test local variables, maybe its not correctly detected when used on fields.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

BADF00D commented 7 years ago

It should be version 0.24. So I thinks its a bug when using null-conditional operator on fields.

@badf00d: added test for null-conditional operator for

teh-hippo commented 7 years ago

Hey @BADF00D; checked, and I'm running 0.24. How can I help you work this out?

BADF00D commented 7 years ago

It would be nice if you could create a small code sample that reproduces the problem using only classes from mscorelib. You can see such an example in this test.

Usually I work test driven, so you can spare me time writing the test. Thanks for your help

teh-hippo commented 7 years ago

Sure; here you go.

Marker: Field is not disposed.

using System.IO;
using FluentAssertions;
using Microsoft.CodeAnalysis;
using NUnit.Framework;

namespace DisposableFixer.Test.IssueTests
{
    [TestFixture]
    internal class If_Analyser_runs_on_code_from_issue_48 : IssueSpec
    {
        private const string Code = @"
using System;
using System.Reactive.Disposables;

namespace MyNamespace
{
    internal class MyClass : IDisposable
    {
        private readonly IDisposable _exampleDisposable;

        public MyClass()
        {
            _exampleDisposable = Disposable.Create(() => { });
        }

        public void Dispose()
        {
            _exampleDisposable?.Dispose();
        }
    }
}";

        private Diagnostic[] _diagnostics;

        protected override void BecauseOf()
        {
            _diagnostics = MyHelper.RunAnalyser(Code, Sut);
        }

        [Test]
        public void Then_there_should_be_no_Diagnostics()
        {
            _diagnostics.Length.Should().Be(0);
        }
    }
}
BADF00D commented 7 years ago

I just uploaded version 0.25 that should fix the problem. I found out, that there was a problem with MethodInvocation, too.

The following example yields an error before, but it shouldn't:

using System.IO;
using System.Threading.Tasks;
namespace DisFixerTest.Async
{
    internal class MyClass : IDisposable
    {
        public MyClass()
        {
            Create()?.Dispose();
        }

        public IDisposable Create()
        {
            return new System.IO.MemoryStream();
        }
    }
}

This and your bug are fixed now.

teh-hippo commented 7 years ago

Verified! Thanks @BADF00D