Fix S2259 FP: "Null pointer dereference" should not raise if the variable was tested with Debug.Assert before

[valhristov]

RSPEC-2259

• [ ] Reintroduce learning from Debug.Assert
• [ ] Revert #397
• [ ] Introduce means to not raise "Expression always true/false" on values that were checked with Debug.Assert. Perhaps this could be done with a special constraint that will notify the SE engine that we learned from a Debug.Assert call.

Consider the following examples:

Debug.Assert(a == null);
a.Foo(); // Noncompliant, a is null

Debug.Assert(a == null);
if (a == null) {} // Compliant, expression always true/false is not raised

• Debug.Assert falls under cross-procedural analysis because it is a method call that we know the behavior of (invokes the debugger if the expression evaluates to false)

Related info: The commit that removed learning from Debug.Assert The corresponding ticket Ticket that removes Debug.Assert from CFG #397

[killergege]

We have the same false-positive issue, but with Nunit.

var item = myList.FirstOrDefault();
Assert.IsNotNull(item);
Assert.IsTrue(item.Sample.Contains(p)); //'item' is null on at least one execution path

The last row raises the S2259 error but item will never be null, as the Assert will throw an Exception before.

Should I create another issue, or is it more or less the same ?

[valhristov]

@killergege the problem sounds the same, even though in the newer versions of the SonarC# analyzers, this rule should not run on test projects.

What version of SonarC# are you using (or the nuget if you are referencing it directly)?

We are not running this rule on tests because if a test fails with a NullReferenceException it will be easily caught and the users of the application will not be affected.

[StasPerekrestov]

I just kindly ask you, is it possible to add an ability to specify test methods as a configurable option?

So, developers may be able to use their own assert statements. Debug.Assert EnsureThat.IsNotNull ... etc.

[batkaevruslan]

In addition to @StasPerekrestov question I can give the example of using resharper's attribute. We have method [ContractAnnotation("argument:null => halt")] public static void IsNotNull<T>(T? argument){ ...} When resharper sees call like EnsureThat.IsNotNull(fooObject) resharper knows that any following call to fooObject property is safe as fooObject is garanteed to be not null.

[valhristov]

Hey guys, thanks for the feedback.

@batkaevruslan You could actually create such attribute:

using System;

public sealed class ValidatedNotNullAttribute : Attribute { }

public static class Guard
{
    public static void NotNull<T>([ValidatedNotNull] this T value, string name) where T : class
    {
        if (value == null)
            throw new ArgumentNullException(name);
    }
}

public static class Utils
{
    public static string ToUpper(string value)
    {
        Guard.NotNull(value, nameof(value));
        if (value == null)
        {
            return value.ToString(); // No issue raised here
        }
        return value.ToUpper();
    }
}

It is not as flexible as the R#'s contract annotations, but should work for the majority of the cases. We added this functionality as part of S3900 and apparently we omitted to update the related rules that use it as well. I added a new ticket to address this omission: #1675

@StasPerekrestov this ticket is not about unit testing and assertion frameworks, but about System.Diagnostics.Debug.Assert(). The problem with it is, that when using DEBUG configuration the method is executed and could break the execution when the expression is false. However, in RELEASE builds, the method is not executed at all. The conundrum is that we should both learn and not learn from expressions contained in Debug.Assert methods and there are good reasons for doing and not doing it...

Currently the rule is hardcoded to not run on test code, for the reasons I explained in my previous reply. We might add ability to configure whether to run it on tests or not, but I cannot say when at this point.

[StasPerekrestov]

Makes sense. Thanks for the explanation.

[pavel-mikula-sonarsource]

Comment from #397

Also worth considering to support Microsoft.VisualStudio.TestTools.UnitTesting.Assert class and its methods, eg. Assert.IsNotNull.

Or if it's possible then find a more generic solution (if there is none exists yet) which tells the S2259 checker that "after running this method (eg. Assert.IsNotNull) this parameter (eg. value) won't be null", this way SonarSource will be more extendable and won't be opinionated towards a specific test framework.

[tbutler-qontigo]

We would like support for NUnit Assert.That(xx, Is.Not.Null) - the rule should detect that the test will not pass this point if xx is null so there is no need to warn on subsequent accesses of xx. This is an explicit null check but there is also an implict case: Assert.That(xx.SomeProp, Is.Not.Null)

Here, if xx.SomrProp or indeed xx?.SomeProp is not null then, by implication, xx is not null so again, it is a false positive to warn about possible null access later in the test

[martin-strecker-sonarsource]

see also #5824