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

Support for VS 2017? #30

Closed ag-eitilt closed 7 years ago

ag-eitilt commented 7 years ago

Disposables are the aspect of C# that I have the most trouble with, and an extension like this would be very helpful. Unfortunately, I've switched to VS 2017, and this only supports 2015. I don't know how drastically the APIs have changed between the two, but unless it would require completely rewriting the code, it would be nice to extend support to the newer version -- and if it is that different, a note in the Readme saying that support isn't planned probably wouldn't go awry.

BADF00D commented 7 years ago

Up to now, I didn't plan to support VS2017 because at home, I bought a Resharper licence that does not support VS2017 and at work we use VS2015. Furthermore there was no request to do so. So for me there was no reason to upgrade. I just installed VS2017 CE and give it a try. I don't think they changed much. But I would remind you, that this project has still no major release. I found a couple of memleaks in my companies production code, but don't expect to much: Its is and will probably always be a static code analysis.

I will keep you up to date.

ag-eitilt commented 7 years ago

Good to keep in mind. For me, though, any reminder will help out. Thanks!

BADF00D commented 7 years ago

I was able to install the extension via Marketplace. Please give it a try.

ag-eitilt commented 7 years ago

It installs perfectly well, but I'm not seeing it indicate that a Stream is undisposed. Where and how is that supposed to show up?

BADF00D commented 7 years ago

I just opened my test project and everything works as expected: image

All errors got marked via green curly lines underneath it. If you hover these, you get an hit about was got wrong. You can also see them in the error list:

image

Maybe you stumbled upon an error. Can you post the code?

ag-eitilt commented 7 years ago

Playing around with your code above, the extension is installed and (usually) functions correctly. There seems to be two separate things I'm running into:

public StorageFile File { get; }
public async Task<IEnumerable<object>> Data() {
    var stream = await File.OpenReadAsync();
    return await DataFormat.ParseAsync(stream.AsStream());
}

The above hold true even if I extract everything into its own variable. For a better-isolated example, the next displays a warning only under fileStream.AsStream(), while the one after it is accepted without any warning, despite fileStream needing to be disposed:

public async Task<IEnumerable<MetadataTag>> Data() {
    var fileStream = await File.OpenReadAsync();
    var stream = fileStream.AsStream();  // Warning on this line
    await MetadataFormat.ParseAsync(stream);
    return null;
}

 

public async Task<IEnumerable<MetadataTag>> Data() {
    var fileStream = await File.OpenReadAsync();
    using (var stream = fileStream.AsStream())
        await MetadataFormat.ParseAsync(stream);
    return null;
}

I guess it could be an issue with the await, but that's not the impression I got from moving the .AsStream() around.

ag-eitilt commented 7 years ago

As another slight enhancement, it seems to be overzealously matching some expression-bodied methods:

public IEnumerator<KeyValuePair<TKey, TValue>> GetEnumerator() =>
    Dictionary.GetEnumerator();  // "Method returns an anonymous object that is not disposed"

public IEnumerator<KeyValuePair<TKey, TValue>> GetEnumerator() {
    return Dictionary.GetEnumerator();  // No warning
}

Unless I'm severely misunderstanding that part of the language, not only should the first be equivalent to the second, but I wouldn't want to dispose the returned object.

BADF00D commented 7 years ago

I think the problem ist, that await "unpacks" the result of a task, the extension does not recognize this. I made a simple code snipped to verify this:

using System.IO;
using System.Threading.Tasks;
namespace DisFixerTest.Async
{
    class AsyncFileStream
    {
        public async Task<bool> Data()
        {
            var stream = await FileAsync.OpenReadAsync();

            return false;
        }
        public class FileAsync
        {
            public static async Task<MemoryStream> OpenReadAsync()
            {
                var memStream = new MemoryStream();

                return memStream;
            }
        }
    }
}
BADF00D commented 7 years ago

Since the "support VS2017" problem ist solved, I close this ticket. The problem mentioned later is now part of issue #31.