Proposal - Introduce IDisposable ownership annotations

[jeremyVignelles]

Hi,

I have thought about this for a while, and I finally got time for that, so I'm posting here. Please tell me if that's not the right place to ask for that.

Rationale

I am an average developper that wants to make sure that I don't forget to call Dispose() on IDisposable. What is great is that there's a roslyn analyzer for that : https://github.com/DotNetAnalyzers/IDisposableAnalyzers

However, sometimes, that analyzer doesn't know how things work, and a signature like

public MyDisposableClass GetInstance()

will trigger a warning, while sometime it's just returning a cached copy which is owned by the class.

There is no way for the analyser to know the semantic of what should be disposed and what should not.

Then came the idea that we could provide some attributes that could help the analyzer.

The problem

How to redistribute that kind of attributes?

• Embedding the attributes with the analyzer? The analyzer itself is a development dependency, and we shouldn't leak that kind of implementation details into the project.
• Having a package dependency for this kind of annotations? What about version conflicts between two projects that uses different versions of the annotations?
• Having a .cs file that the project that wants to check embeds, similar to LibLog ? How would the analyzer check the attributes? with only the class name?
• Maintaining external annotations like JetBrains ? That would be probably feasible, and makes it possible to annotate existing code, but we are not JetBrains...

Each of those options have drawbacks, and probably more than exposed above. Then we thought about it in a different way : If we truly want to analyze the behavior of the code, what if we find a more universal way : introduce the annotations directly in .net.

The proposition

Here comes the IDisposable ownership attributes. The first formal definition was made here, and I think that the API shape is understandable.

    /// <summary>
    /// The return value must be disposed by the caller.
    /// </summary>
    [AttributeUsage(AttributeTargets.ReturnValue | AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
    public class GivesOwnershipAttribute : Attribute
    {
    }

    /// <summary>
    /// The return value must not be disposed by the caller.
    /// </summary>
    [AttributeUsage(AttributeTargets.ReturnValue | AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
    public class KeepsOwnershipAttribute : Attribute
    {
    }

    /// <summary>
    /// The ownership of instance is transferred and the receiver is responsible for disposing.
    /// </summary>
    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
    public class TakesOwnershipAttribute : Attribute
    {
    }

Let's dig into this : By defining these attributes, we state that "The owner of the object is responsible to Dispose() it".

Ownership can be given (i.e. transferred to caller):

public class ResourceFactory {
   [return: GivesOwnership]
   public Resource Create()
   {
        return new MyResource();
   }
}

It can be taken

public class ResourceHandler : IDisposable {
   private Resource _resource;
   public void SetResource([TakesOwnership] Resource resource) => this._resource = resource;
   public void Dispose()
   {
      this._resource.Dispose();
   }
}

new ResourceHandler().SetResource(new Resource);

We can also say explicitely that the resource is kept:

public class ResourceCache : IDisposable {
    private Resource _resource;
    [return: KeepsOwnership]
    public Resource GetResource()
    {
       return this._resource;
    }
}

Why in .NET?

Having a way to express ownership is really nice to know what is happening, and many places in the framework itself could benefit from that.

Take this code for example:

var stream = File.Open...;
var reader = new StreamReader(stream);

Which one should I dispose?

If StreamReader declared this constructor as TakesOwnership, the question is no more.

Another question I often have is : "When using IServiceCollection.AddSingleton(), are my objects correctly Dispose() 'd?"

Potential uses

• Documentation : The documentation could automatically state that the method has a special behavior regarding the object ownership
• Analyzers : IDisposableAnalyzers would be one of them, but maybe something more integrated, like in the compiler or in VS ?

What do you think?

[huoyaoyuan]

Are there any works to do for CLR on this feature? Due to the high-compatibility designs, it's extremely hard to let CLR force some new rules. Otherwise, this issue could just be done by analyzers.

First of all, IDisposable is even not CLR-forced thing. You can write and execute classes with inconsistent finalizers and disposals.

[jeremyVignelles]

Hi,

Are there any works to do for CLR on this feature?

I think so, or maybe that belongs to the corefx to provide such things. I see it very similar to the introduction of the non-nullable types : It's something that helps write you better code, and if first applied to .net itself, that would greatly help the adoption of such features.

Due to the high-compatibility designs, it's extremely hard to let CLR force some new rules. Otherwise, this issue could just be done by analyzers.

My proposal is not about rules, but more about helping user to spot usages that are different from the intent. Right now, when you look at a method that returns IDisposable, how do you know if you should dispose the object or not? I think people often deal with that in one of the following ways:

• Reading the doc : if you're motivated enough to read it and if you are lucky enough to have that written in the doc, then you could know
• Reading through the source code : This is not something the average developer would do
• Try it, and see if that fails : I guess most of people would fit in that category. Either they dispose it too often and see if it throws, or not enough, and they end up with leaks.

Implementing that in an analyser would probably be doable, however to be efficient:

• The analyzer must have knowledge of the developer intent. Otherwise, there will be a lot of false positives or, more importantly, false negatives.
• Annotations must be present on each potentially problematic API, which will not likely be the case with third party libraries. Why such a library would bother to expose the API in an annotated way (with the additional dependency) if that's not something widely recognized in the .net community? The framework itself is widely recognized.

First of all, IDisposable is even not CLR-forced thing. You can write and execute classes with inconsistent finalizers and disposals.

I don't want to force that, I want library authors to be able to express the intent : "Hey, the return of that method must be disposed", or "Give me a Disposable, and I will take care of Disposing it", and do that in a way that can be understood by analyzers. Authors can still (but are discouraged to) write code that doesn't follow the patterns.

[huoyaoyuan]

Implementing that in an analyser would probably be doable, however to be efficient: The analyzer must have knowledge of the developer intent. Otherwise, there will be a lot of false positives or, more importantly, false negatives. Annotations must be present on each potentially problematic API, which will not likely be the case with third party libraries. Why such a library would bother to expose the API in an annotated way (with the additional dependency) if that's not something widely recognized in the .net community? The framework itself is widely recognized.

CLR has even less knowledge than analyzer. I guess you want to let CLR "auto-detect" a method's ownership of a disposable. But it's even much more harder than verifying and enforcing.

If you want something make people to follow the pattern easily, it falls into language design. Many people may have the same idea of this proposal, but it's nothing to do with it in CLR now.

[jeremyVignelles]

You might be right, this proposal would probably better fit in csharplang, but it will probably have implications in corefx (where annotations will likely be declared). Can someone move that proposal to the appropriate repository?

[RussKeldorph]

@karelz Please either move as requested or close this issue and advise opening another following https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md.

[Gnbrkm41]

I think one use case of this could be HttpClient (for the aforementioned IDisposableAnalyzers), since it's one of the types that implement IDisposable however it seems to be recommended to use single instance instead of instantiating a new one every time, hence caching.

side note regarding 'Why in .NET?' section: just dispose of both of the instances, since calling Dispose() multiple times won't cause any problems 😝

using (Stream stream = File.Open())
using (StreamReader reader = new StreamReader(stream))
{
    // Some nice codes come here
}

[jeremyVignelles]

Stream is likely not the best example of usage because you can pass it into a stream reader and transfer ownership, or specify leaveOpen. The scenario is complex there.

If I understand what you mean, you're telling me that it doesn't hurt to dispose the item twice? Is that always true, or only on Stream? I'm not sure that is something that is always implemented, and I'm pretty sure that I have in my code objects that are not meant to be disposed twice.

Anyway, there are still the case where you get a IDisposable object as a result of a method, and where you must not dispose it because that would otherwise close it for the rest of the code.

[Gnbrkm41]

If I understand what you mean, you're telling me that it doesn't hurt to dispose the item twice?

Yes, that is correct. It is advised against throwing anything in Dispose/only making it possible to disposing once to ensure cleanup of unmanaged resources. See Implementing a Dispose method:

To help ensure that resources are always cleaned up appropriately, a Dispose method should be callable multiple times without throwing an exception.

So I would expect it to be always implemented in such way. But regardless, I think it's a fair point, hence just a side note.

[jeremyVignelles]

Hi, is there anything new here? Any chance to see this coming in .net 5?

Has this even been triaged/discussed?

[ericstj]

I'm not sure starting in libraries is the right way to move this issue forward. Seems the roslyn-anlyzers is better to work on a change that adds support to an analyzer and detects attributes by name. Typically these types of annotations don't rely on strong typing in specific assembly but support the attribute definition in any assembly and match by namespace.name to allow folks to use it on older frameworks that can't change the API. The same was true for async, nullable, and others. Work out the kinks, drive consensus, and demonstrate the value. Once that happens we could consider adding such a feature to some of the API in dotnet/runtime.

We've done investment in analyzers in net5 not sure if there has been explicit consideration for IDisposable. @stephentoub @bartonjs @jeffhandley may know.

[bartonjs]

not sure if there has been explicit consideration for IDisposable.

Since this is the only code-analyzer issue with IDisposable in it, probably not :smile:.

I agree that this will require a very similar shape to nullability annotations, in that it requires a lot of attributes to be placed throughout the codebase, and we need a very complex set of attributes; like [KeepsOwnershipUnless(nameof(leaveOpen))].

For inputs (including this):

• Called member takes ownership unconditionally
• Called member takes ownership conditionally
• Called member does not take ownership
• Called member disposes the object.
• Called member relinquishes ownership, it returns to the previous owner (or, absent any real owner, the caller/the GC). (e.g. removing something from a DisposableCollection)

For outputs:

• Caller should dispose
  • RSACertificateExtensions.GetRSAPrivateKey
• Caller should not dispose
  • X509Certificate2.get_PrivateKey
• Whether or not a caller should dispose depends on some other state
  • StreamReader..ctor(Stream, ..., bool)
• Return value is an input parameter, caller maintains ownership transparently ([NotNullIfNotNull], basically)
  • (Having trouble coming up with a public example of this one off the top of my head)

There might be other states.

It's as least as complex as nullability, because of something like

internal sealed class ReturnWrapper : IDisposable
{
    private IDisposable? _disposable;

    internal ReturnWrapper([TakesOwnership] IDisposable disposable)
    {
        _disposable = disposable;
    }

    public void Dispose()
    {
        _disposable?.Dispose();
        _disposable = null;
    }

    [HowDoWeDescribeThisState]
    internal void SetSuccess()
    {
        _disposable = null;
    }
}

...

using (FileStream stream = File.Open(...))
using (ReturnWrapper wrapper = new ReturnWrapper(stream))
{
    ...
    wrapper.SetSuccess();
    return stream;
}

The SetSuccess() method undoes an ownership transfer from a constructor. How does that get described in annotations? (The relationship for this class's members seems pretty easy to wrap my head around, but as a special case, I can't describe what it does generally). And, like nullability, no one can really determine how complex it actually is until they try decorating all of the shared runtime, since their samples will end up being a bit overly simplistic and/or operating on incomplete data.

[mavasani]

FYI: We already have some IDisposable analyzers for detecting dispose leaks in roslyn-analyzers repo.

1. CA2000: Dispose objects before losing scope. Implementation at https://github.com/dotnet/roslyn-analyzers/blob/0a6e4b3935bbcdde79eb90d1760368e89ffff826/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DisposeObjectsBeforeLosingScope.cs
2. CA2213: Disposable fields should be disposed. Implementation at https://github.com/dotnet/roslyn-analyzers/blob/0a6e4b3935bbcdde79eb90d1760368e89ffff826/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DisposableFieldsShouldBeDisposed.cs
3. CA2215: Dispose methods should call base class dispose. Implementation at https://github.com/dotnet/roslyn-analyzers/blob/f15404b312295e7cee16fb40b4c8d3f91f10f087/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DisposeMethodsShouldCallBaseClassDispose.cs

Current implementation of these analyzers primarily relies on the following for improved analysis precision:

1. Dataflow analysis (also supports interprocedural analysis - only within the current source assembly, not referenced assemblies)
2. Configurable options for generic disposable ownership transfer semantics to be used by analysis. See here and here for the supported options.

However, these analyzers still lead to false positives and negatives. These would definitely benefit from attribute based annotations for an improved analysis precision.

Typically these types of annotations don't rely on strong typing in specific assembly but support the attribute definition in any assembly and match by namespace.name to allow folks to use it on older frameworks that can't change the API. The same was true for async, nullable, and others. Work out the kinks, drive consensus, and demonstrate the value. Once that happens we could consider adding such a feature to some of the API in dotnet/runtime.

Agreed. Tagging @Evangelink who has shown interest in this space and is heavily involved in contributions to roslyn-analyzers repo - would you be interested in working with @bartonjs on the initial attribute design and a prototype in enhancing the existing disposable analyzers and/or adding new disposable analyzers in roslyn-analyzers repo?

[Evangelink]

I would love to :)

[Evangelink]

Another case that needs to be handled by the attribute is the case of overloads with bool leaveOpen parameters (see https://docs.microsoft.com/en-us/dotnet/api/system.io.streamwriter.-ctor?view=netcore-3.1#System_IO_StreamWriter__ctor_System_IO_Stream_System_Text_Encoding_System_Int32_System_Boolean_)

[MartyIX]

This would be really great to have. Are there any plans to add this functionality?

[jeremyVignelles]

Anything new on this? How could the leaveOpen be handled?

EDIT: Why has this issue been assigned to the "team IoT pod" dashboard?

[Evangelink]

How could the leaveOpen be handled?

It depends the direction we take. If we assume everything gets annotated (marked with attributes) then we could go in the direction of not having a special case. We could also have analyzers trying to recognize a pattern (as it is done with the try-pattern), for example we could assume that a method taking something disposable and having a boolean parameter named leaveOpen would mean that the disposable parameter is not supposed to be disposed. It could also be a parameter to the attribute that would say which boolean parameter is allowing the keep-open functionality. That's just a couple of examples without giving much thoughts.

[robindegen]

I would love for this to be added. There is no good way to indicate ownership at the moment and the analyzer is going crazy about it.