False report CA1821 (Remove empty Finalizers)

[mavasani]

See https://github.com/dotnet/roslyn/pull/19806/files#diff-4fddad076e8fb11cecceca523ee2aeb5R120 and https://github.com/dotnet/roslyn/pull/19806/files#diff-9318b6e7e0f109083f0de57890db9edfR148

[nguerrera]

I don't think this is a false report though maybe the messaging could be improved. Finalizers like that should have #if CONTRACTS_FULL/#if DEBUG around them. When the conditional calls get compiled away, there will be empty finalizers left behind, and that will cause the GC to do unnecessary work, and objects to be promoted to another generation unnecessarily.

[mavasani]

@nguerrera Thanks, you are right that the analyzer is reporting a valid issue on release build. However, there is still an issue in the analyzer - it shouldn't fire if the enclosing method is also conditionally excluded. For example, the following still fires the diagnostic in both release and debug builds.

#if DEBUG
        ~InvisibleEditor()
        {
            Debug.Assert(Environment.HasShutdownStarted, GetType().Name + " was leaked without Dispose being called.");
        }
#endif

[sharwell]

For example, the following still fires the diagnostic in both release and debug builds.

How does the diagnostic fire in this case for release builds?

[333fred]

The problem here is that we explicitly flag bodies with just methods marked as conditional, but we don't then check to see if the method is excluded conditionally. I've filed https://github.com/dotnet/roslyn/issues/24569 for exposing the ability to get preprocessor info for a given syntax node, so we can check to see if the method is guarded by #if DEBUG flags.

[sharwell]

The problem here is that we explicitly flag bodies with just methods marked as conditional, but we don't then check to see if the method is excluded conditionally.

My understanding is you currently treat all conditional methods as not included in the compilation output. After the change, you will start to treat them as included if the conditional symbol applied to the method definition is defined at the point of the call. Is this correct?

[333fred]

Yep, that's correct. The idea is that if the user has a method with just Debug calls in it, we can then tell them to wrap with #if DEBUG.

[Cyberboss]

Here are a few repros: https://github.com/tgstation/tgstation-server/blob/5d0c165f9202f8e98dc2518eb6b766de44577fc4/src/Tgstation.Server.Host/Components/Watchdog/SessionController.cs#L230

https://github.com/tgstation/tgstation-server/blob/5d0c165f9202f8e98dc2518eb6b766de44577fc4/src/Tgstation.Server.Host/Core/SemaphoreSlimContext.cs#L47

FxCop Nuget: 2.6.2

[Evangelink]

@mavasani We could use a workaround while waiting for a more reliable solution. My idea is to count invocations to method with the conditional attribute if the destructor has any directive.

This would result in potentially some false negative but would certainly kill the false positives raised here. Besides, I think that the current implementation might be a little to strict anyways because a method marked with ConditionalAttribute specifies which pre-processor symbol is expected while our current implementation rules them all out. WDYT?

[sharwell]

@Evangelink the methods linked by @Cyberboss do not involve preprocessor symbols. Can you clarify what your proposed change is, and which scenarios it would address?

@Cyberboss Both of your finalizers involve invalid calls to other managed objects. The most straightforward way to address this problem is to remove both of the finalizers.

1. SessionController.Dispose(bool) should not be calling process.Terminate or logger.LogCritical when disposing is false. Since these are the only significant calls made on the finalization path, the finalizer should be removed.
2. SemaphoreSlimContext.Dispose() should not be calling lockedSemaphore.Release on the finalization path. Since this is the only significant call made on the finalization path, the finalizer should be removed.

Depending on how your code is structured, it's possible that your application is currently relying on these finalizers for some normal scenarios. Even though they are working in some cases, they are generally not guaranteed to work under any particular scenario so it would be good to remove them.

[Evangelink]

@sharwell I was talking about the case where the finalizer is wrapped in a Debug directive (i.e. cases described by @mavasani, in the doc and in #3336).