CA1508 - False positive for null check in finally block

[bartdesmet]

static void Repro()
{
    D d = null;
    try
    {
        d = new D();
    }
    finally
    {
        d?.Dispose();
    }
}

class D : IDisposable
{
    public D() => throw new InvalidOperationException();

    public void Dispose() { }
}

The constructor of D can throw and leave d to be null, so the finally block correctly checks for null. However, CA1508 suggests to remove the null check.

[Youssef1313]

@bartdesmet Because there is no catch clause, the above program should crash and the finally block won't be reachable with null value.

[Youssef1313]

Ok I now see that if Repro is called in a try clause that has a catch the null check will be making a difference.

[Youssef1313]

@mavasani Should the following check be done regardless it's compiler generated or not? or will this introduce false negatives?

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode.cs

Lines 155 to 156 in 60fec5c

	// Skip IsNull operations in compiler generated finally region for using statement
	if (block.IsFirstBlockOfCompilerGeneratedFinally(cfg))

[mavasani]

or will this introduce false negatives?

Yes, that is correct. We cannot assume that first block of finally cannot have redundant null checks. For example, if the original repro is adjusted a bit such that d is guaranteed to be non-null prior to try block, d?.Dispose() indeed has a redundant ?. which needs to be flagged.

This issue likely needs further debugging through the dataflow analysis for try/catch/finally blocks in

roslyn-analyzers/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DataFlowAnalysis.cs

Lines 173 to 185 in 7edc7c3

	private void RunCore(
	ControlFlowGraph cfg,
	PooledSortedSet<int> worklist,
	PooledSortedSet<int> pendingBlocksNeedingAtLeastOnePass,
	TAnalysisData? initialAnalysisData,
	DataFlowAnalysisResultBuilder<TAnalysisData> resultBuilder,
	PooledHashSet<BasicBlock> uniqueSuccessors,
	PooledDictionary<int, List<BranchWithInfo>> finallyBlockSuccessorsMap,
	PooledDictionary<ControlFlowRegion, TAnalysisData> catchBlockInputDataMap,
	PooledDictionary<int, TAnalysisData> inputDataFromInfeasibleBranchesMap,
	PooledDictionary<int, (int Ordinal, ControlFlowConditionKind BranchKind)?> blockToUniqueInputFlowMap,
	PooledDictionary<int, int> loopRangeMap,
	bool exceptionPathsAnalysisPostPass)

[skrysm]

@bartdesmet

Because there is no catch clause, the above program should crash and the finally block won't be reachable with null value.

I think this is wrong. The finally block is always executed - even on exceptions (that's what finally is for).

[skrysm]

On a different note: I'm getting CA1508 for this code:

X509Certificate2? pubCert = null;

try
{
    pubCert = new X509Certificate2(publicKeyBytes);

    if (pubCert.HasPrivateKey)
    {
        throw new InvalidOperationException("This certificate already has a private key.");
    }
}
catch (Exception)
{
    pubCert?.Dispose(); // <-- CA1508: 'pubCert' is never 'null'.
    throw;
}

This is wrong because the constructor of X509Certificate2 can throw exceptions in which case pubCert would be null.

I'm guessing this is the same problem as in this issue? Or should I create a separate issue for this?

[genlu]

I think it's likely the same issue, no need to create a separate one for now.