IDisposableAnnotations

[JohanLarsson]

Fixes #126

[JohanLarsson]

DisposedByCaller is longer but perhaps clearer?

[retailcoder]

Agree. Would MustDispose be clearer (while still short)?

[jnm2]

To come up with an alternative for the sake of discussion: [BecomesOwner]? [TransferOwnership]?

[Disposes] sounds like the method itself does the disposing. Is that always the case or might it be saved for the class to dispose later?

[bclothier]

I think we need to make it a verb to be more explicit for the callers. MustBeDisposed and NotDisposable?

[jnm2]

Just spaghetti: [OwnedByCaller]? [CallerOwns]?

[JohanLarsson]

I think having Disposable somewhere in the name is good.

[jnm2]

I don't like the contraction. [NotOwnedByCaller]?

[retailcoder]

I'd spell it out. DoNotDispose 😉

[bclothier]

One too many n there. ;-)

[bclothier]

Probably better if we change should to must to be consistent with the attribute's name.

[bclothier]

Any progress on this? We would love to see this in action.

[bclothier]

I want to check and see if this will be coming soon? This is a blocking issue for me.

[JohanLarsson]

I don't really remember where we left this but we discussed that it would be nasty if we require the attribute dll to be shipped with applications or the attribute package a package dependency for libraries.

We also discussed opening an issue about adding the attributes to the framework but I don't remember what happened with that.

[jeremyVignelles]

@JohanLarsson : I asked the question on their gitter, but didn't have any official answer. I have never taken time to ask on the repo

[bclothier]

The way I see it if we dont want to ship attributes, this leaves us with two alternatives:

1. we overload some existing attribute. For instance, use the DataContract and DataMember and assign a special meaning to a XML namespace.

2. make use of comments, perhaps even custom elements in the xmldocs. IINM, analyzers can still get the comments, so it can be used without needing an attribute class.

[jnm2]

JetBrains' annotations package puts [Conditional("JETBRAINS_ANNOTATIONS")] on every attribute: https://www.fuget.org/packages/JetBrains.Annotations/2018.3.0/lib/netstandard2.0/JetBrains.Annotations.dll/JetBrains.Annotations/PureAttribute?code=true'

This means that analyzers can see the annotations by using syntax even though the attributes are never emitted at compile time (unless you define the symbol JETBRAINS_ANNOTATIONS).

If you add such a NuGet package, you can also include a .targets that keeps the attributes assembly from being copied to build output. I wrote a build trimming tool, so I can help with that.

[jeremyVignelles]

I finally asked the question there : https://github.com/dotnet/coreclr/issues/24668

[bclothier]

What is the current blocking issue here?

[JohanLarsson]

Not sure there is anything blocking, did not really like the design and have not had much time.

[jnm2]

Now that the C# nullability attributes have shipped, I wonder if a similar naming convention could be used.

[JohanLarsson]

What would the names be then?

[jnm2]

https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis?view=netcore-3.0
Actually, they seem less helpful than I was thinking. We can't use Disposable in place of Null because disposing is never optional. You either must or must not dispose, right? There's no "may dispose" for the caller and there's no "might dispose" for the callee.

Taking the concept of directing some at inputs (AllowNull, DisallowNull) and others at outputs (NotNull, MaybeNull), here's a straw man based on the ideas above:

MustDispose, MustNotDispose: applies to the caller's subsequent use of the value

WillDispose, WillNotDispose: signals whether or not the operation is transferring ownership away from the caller.

If and When variants could follow just like in System.Diagnostics.CodeAnalysis if they prove useful.

[jnm2]

I'd actually like to see something like this added to the framework and understood by Roslyn's and fxcop's dispose analyzers.

[jeremyVignelles]

You either must or must not dispose, right? There's no "may dispose" for the caller and there's no "might dispose" for the callee.

I came across the case once where I wondered if it was mandatory to dispoed, because I think I read a code in asp.net core where it isn't dispoed:

https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.primitives.ichangetoken.registerchangecallback?view=aspnetcore-3.0

The purpose of the IDisposable here is to say "I don't want to be notified anymore". If the registration is meant to live as long as the application, is the Dispose() really mandatory?

I'd actually like to see something like this added to the framework and understood by Roslyn's and fxcop's dispose analyzers.

There does not seem to be a big progress here : https://github.com/dotnet/corefx/issues/37873 . I've seen they put a milestone on that but...

[Bouke]

It would be great if we could annotate our APIs with ownership information. Besides our own code, it would be nice if we could somehow annotate third party code as well. See for example StreamReader which is currently hard-coded in the analyzer, but what about other classes -- we can't expect this analyzer to hard-code every third-party API out there.

I agree with the point posted here that we should avoid having to ship a dll in our release builds. However the annotations should be clear as well, I'm not sure how such a thing can be achieved.

For completeness we would need a way to annotate leaveOpen (optional ownership). Maybe something like TakesOwnershipWhenAttribute(string, bool), e.g.:

public StreamReader(
    [TakesOwnershipWhen("leaveOpen", false)] Stream stream,
    bool leaveOpen)
{ }

[jnm2]

https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1813: avoid unsealed attributes

[AraHaan]

After thinking about this, what if the analyzer contained a source generator that would generate the annotation codes that people can use to control the analyzer part of itself?