False positive on S2583 in C# catch clause

[Daynvheur]

Hello here,
I would wan to report something that looks like a false positive S2583 on SonarLint 8.1.0.95039 for VisualStudio Pro 2022 (not connected mode), when a null check has been performed in a try clause then checked again in the catch clause:

using System;
using System.Collections.Generic;
using System.Globalization;

DateTime? Test_S2583(Dictionary<string, string>? dic = null)
{
	try
	{
		return dic?.ContainsKey("c") == true
			? DateTime.ParseExact(dic["c"], "dd/MM/yyyy HH:mm:ss", CultureInfo.CurrentCulture)
			: null;
	}
	catch (Exception)
	{
		Console.WriteLine(dic is null ? "true" : "false"); // <- s2583 because of the dic?.ContainsKey above
		return null;
	}
}

I’ve seen some other subjects on the topic, but haven’t seen this try…catch error flow.

[sebastien.marichal]

Hello @Daynvheur,

Welcome to the community!

This is a true positive.
dic cannot be null in the catch clause, because all paths that could throw are executed only if dic is not null.

To illustrate more, we can lower your snippet to:

DateTime? Test_S2583(Dictionary<string, string>? dic = null)
{
    try
    {
        return (dic != null && dic.ContainsKey("c")) ? new Nullable<DateTime>(DateTime.ParseExact(dic["c"], "dd/MM/yyyy HH:mm:ss", CultureInfo.CurrentCulture)) : null;
    }
    catch (Exception)
    {
        Console.WriteLine((dic == null) ? "true" : "false");
        return null;
    }
}

In the lowered snippet, it is more clear that dic cannot be null when entering the catch clause.
If you add a statement that can throw before the return, the issue will not raise anymore. This is because, in this scenario, our symbolic execution engine won’t know whether dic is null or not null.

I hope this helps.

Have a nice day!

[Daynvheur]

Thanks for the answer!
In return (dic != null && dic.ContainsKey("c")) ? new Nullable<DateTime>(...) : null;, dic can’t throw an NRE, you’re right.
I had some NRE while parsing the value stored in dic["c"], so here is the intended code on my side:

DateTime? Test_S2583(Dictionary<string, string>? dic = null)
{
    try
    {
        return (dic != null && dic.ContainsKey("c")) ? new Nullable<DateTime>(DateTime.ParseExact(dic["c"], "dd/MM/yyyy HH:mm:ss", CultureInfo.CurrentCulture)) : null;
    }
    catch (Exception)
    {
        Console.WriteLine((dic["c"] == null) ? "true" : "false");
        return null;
    }
}

Without S2583. (But with added CS8602 , requiring dic!["c"] == null to stop warnings.)

I’ll edit the title. I can’t edit the title.

[Daynvheur]

Actually, there may be room for a missing S2589 to report: sharplab sample

using System;
using System.Collections.Generic;

var tst = Test_S2583(new() { { "c", null } }); //CS8625 (this is a warning, not an error)

DateTime? Test_S2583(Dictionary<string, string>? dic = null)
{
    try
    {
        return dic == null ? null : throw new NotImplementedException();
    }
    catch (Exception)
    {
        if (dic == null) ; //positive S2589: dic can't be null here
        else //false positive CS8602: dic can't be null here, as stated previously
            if (dic.ContainsKey("c"))
                if (dic["c"] == null) //<--? false negative S2589: dic["c"] is typed `string` and should not be nullable?
                                      //(the compiler doesn't enforce this, so maybe it's not a false negative after all.)
                    Console.WriteLine("warning CS8625 in #nullable enable enforced call sites");
                else ;

        return null;
    }
}

If you have more intakes about this, otherwise the current issue is closed.