Tokens not replaced in: DF0100 C# {0} implements IDisposable but return value {1} does not.

asmilelikethat commented 5 years ago

Prerequisites

[x] I have written a descriptive issue title
[x] I have verified that I am running the latest version of DisposableFixer: 2.1.0
[x] I have searched open and closed issues to ensure it has not already been reported

Description

Latest version has message for DF0100 with unreplaced tokens '{0}, {1}, etc.' as above, also see screenshot. Also notice in screenshot there's a false positive as the using statement will call Dispose on the SHA256 provider, right?

Source Code

Screenshot

DisposableFixer-2019-09-21_0-11-45

BADF00D commented 5 years ago

Thanks for reporting! I definitely forgot to replace the tokens. But I'm not yet sure if this is a false positive. The screenshots shows to less code to tell this.

I introduced a new diagnostics, that warns you, if you are returning a IDisposable, but the return values of the method/func/local function is not IDisposable. E.g:

public object Create() => new MemoryStream();

That's what I call an factory method. This would pass the responsibility to destroy it to the calling instance. But since object does not implements IDisposable the caller has no change to detect this problem.

This diagnostic would make sense, if you are return (the already disposed) local variable sha256. But I don't think you did this. Maybe you returned some part of the variable like return sha256.SomeProperty. This would be perfectly valid code and should not trigger the diagnostics, but at the moment I'm just guessing. Can you show more of your code? Or if you can't do that, can you reproduce the problem with other code?

Best Greeting

asmilelikethat commented 5 years ago

Sure, here's that whole function, a simple extension method for the FileInfo class:

DisposableFixer-2019-09-21_11-24-25

I should probably make that file read async at some point but it's been working as is for many years. ToHexString() is another extension method that returns a hex string representation of a byte array.

The same DF0100 is showing up in a bunch of similar places that I thought were OK, but tell me what you think:

DisposableFixer-2019-09-21_11-44-01

BADF00D commented 5 years ago

Ok, thanks for the input and the additional screenshots, that shows some more bugs. From my point of view (I'm have no experience in crypto stuff), your code is perfectly OK.

The problem lies within the analyzer. Currently I only check if the variable that carries the IDisposable instance is a decedent of a return statement. So I can detect situations like:

void Create() => new MemoryStream();
void Create2() {
   return new MemoryStream();
}
void Create3(){
    var mem = new MemoryStream();
    return mem;
}

But in your code the instance is not returned, it used get get properties of it or in an argument. These situations should not trigger the diagnostics. Buts since they are actually part of an return statement (from a syntactical point of view), they do.

Should be no big deal to fix this. Note to myself - add tests for local variable

that is used as argument in return statement
that is used as identifier in a InvocationExpression
that has a field that gets returned
that has a property that gets returned
check behavior of this diagnostic in async context (e.g Task<object> Create()=> Task.FromResult(new MemoryStream())

Btw. async is useful when using IO function, but in order to gain any profit of it, the whole call-stack have to be async. Otherwise you gain nothing of make a single method async.

BADF00D commented 5 years ago

Hi, I just published a new version that should fix all the bugs you reported. The only thing I moved to a new issue (#128) was the support to detect hidden disposables in async code.

BADF00D / DisposableFixer