False positives

[bclothier]

Thank you very much for sharing the analyzer. I'm in process of trying this out with Rubberduck-VBA. This project involves quite a lot of manual management due to deep COM interop and a large part of architecture relies on IDisposable interface to explicitly control the lifetime of wrapped COM objects.

I've found few false positives. I do not know if those are something we can fix or enhance but I thought I'd at least point them out.

With IDISP001 rule we are flagged here:

var component = _state.ProjectsProvider.Component(target.QualifiedName.QualifiedModuleName);

The component here is a IDisposable object which we obtain via the _projectsProvider.Component. The intention with the ProjectsRepository is that the class is responsible for the lifetime of its objects, so thus we shouldn't dispose anything we get out of the ProjectsRepository.

We have a similar situation here:

var subclass = Subclasses.Subclass(hwnd);

As above, Subclass returns a IDisposable object from a cached collection.

With IDSIP007 rule we also get flagged here:

using (var module = component.CodeModule)
{
  ...
}

In this case, CodeModule is similarly a IDisposable object. However, the CodeModule property reads:

public ICodeModule CodeModule => new CodeModule(IsWrappingNullReference ? null : Target.CodeModule);

Thus, using is actually appropriate in this case since we get a new object and thus are responsible for disposing it.

With IDISP003, we have a false positive for this code:

        void ComTypes.ITypeInfo.GetRefTypeInfo(int hRef, out ComTypes.ITypeInfo ppTI)
            => ppTI = GetSafeRefTypeInfo(hRef);

This flags the ppTI as needing to be disposed. What I don't get, though is that the variable has type of ITypeInfo, which does not implement IDisposable. If I try to apply the quickfix, I get the following code:

        void ComTypes.ITypeInfo.GetRefTypeInfo(int hRef, out ComTypes.ITypeInfo ppTI)
        {
            (ppTI as IDisposable)?.Dispose();
            return ppTI = GetSafeRefTypeInfo(hRef);
        }

Which does not look at all appropriate to me.

Another example is here for this section of code:

                            IHostApplication result;
                            if (hostAppControl.IsWrappingNullReference)
                            {
                                result = null;
                            }
                            else
                            {
                                switch (hostAppControl.Caption)
                                {
                                    case "Microsoft Excel":
                                        result = new ExcelApp();
                                        break;
                                    ...<several other cases>....
                                    default:
                                        result = null;
                                        break;
                                }
                            }

                            _host = result;
                            return result;

For each case, the result is flagged in spite of the fact that it's a local variable and thus cannot be been previously assigned.

I have not gone though all other issues and will update as I proceed.

I know we can add suppress message but I want to avoid suppressing it everything. We got about 1500 warnings and we're going to have to assess them one by one because of some false positives.

[JohanLarsson]

Thanks for reporting!

[JohanLarsson]

I cloned rubberduck and had a look.

var subclass = Subclasses.Subclass(hwnd);

Why not do return _subclasses.GetOrAdd(hwnd, x => Create(x)); inside Subclasses.Subclass(IntPtr)? Then you get a thread safe implementation and I think this analyzer will figure the code out.

I'm not sure about teaching the analyzer about TryAdd so it understands the way your code is written.
I think there is a point in the analyzer being a bit opinionated pointing to stuff like this as there is a race condition in the code.

[JohanLarsson]

About:

var component = _state.ProjectsProvider.Component(target.QualifiedName.QualifiedModuleName);

It is tricky as there are interfaces involved. The analyzer falls back on heuristics and assumes that disposables from methods are created. We need to add annotations to handle this case. Annotations have been discussed before but we have not pulled the trigger on them.

[JohanLarsson]

using (var module = component.CodeModule)
{
}

Reason IDISP007 nags here is again heuristics, it assumes that a property does not return a created disposable that the ~caller owns. Needs annotations #126

A property returning a created reference type is not awesome design if you ask me.

[JohanLarsson]

I have not gone though all other issues and will update as I proceed.

For me it is easiest with many small issues. Also for discussion we have a gitter, everyone welcome!

[JohanLarsson]

// IDISP003
void ComTypes.ITypeInfo.GetRefTypeInfo(int hRef, out ComTypes.ITypeInfo ppTI)
            => ppTI = GetSafeRefTypeInfo(hRef);

This looks like a bug, I'll try to repro and fix.

[JohanLarsson]

Looks like I'm dumbing analysis in the switch also.

[bclothier]

Thank you very much for the responses! In future I will split them as one issue per.

Regarding the annotations - to be honest, we were kicking around the same idea of annotating our project repository class? interface? to help us track whether it is the caller's responsibility to dispose the provided IDIsposable objects. As you see, there is a large burden placed on the caller to get it all right. I agree that returning a created reference type isn't always the best design but there are few reasons:

1. Those wrap a single COM interface. Each different COM interface must be individually tracked and managed.
2. Not all are needed, and not all are meant to have the same lifetime/scope.
3. We are working with a COM host, so therefore we must deal with transient COM objects; simply caching them cause far more problems than it solves.

Those all are managed by SafeComWrapper abstract class, which implements IDisposable. Nonetheless, it does make it a challenge to track who's responsible for what.

While we already have written some Roslyn analyzers for our project, this kind of analysis would be far too complicated and as such, I'd much rather piggyback on your excellent analyzers which already has identified a good number of legitimate misuses.

[JohanLarsson]

As you see, there is a large burden placed on the caller to get it all right.

If we add support for annotations we will have the analyzer check usages so burden will not be huge. Adding annotations is not going to be very hard, it mostly stuck on deciding on names of attributes etc. I created an issue for discussing annotations, please join use and bikeshed :D

[bclothier]

Confirming -- I noticed a similar false positives for IDISP007 as the case with the IDISP001 reported above. I assume this is due to using same heuristics so fixing the issue would address both rules? If that's not the case, I will make a separate issue.

[JohanLarsson]

Closing this, open new issues if I forgot things.