Improve S2259: Support DoesNotReturnIf for custom assertions

llfab commented 2 years ago

Description

S2259 is raised even if tested before with custom assertion method. This issue therefore is similar to https://github.com/SonarSource/sonar-dotnet/issues/349 with the difference that I am referring to a custom method.

Repro steps

Assert.That(systems != null, "Cannot get pose for coordinate key - no coordinate systems defined!");
return systems.GetPose(coordinateKey.Value, Core.Geometry3D.CoordinateKey.World) * Pose;

//...
// with assertion method
public static void That(bool condition, string message = "Precondition failed!")
{
  if (!condition) { throw new AssertionException(message); }
}

Expected behavior

The sonar C# scanner should detect that the custom assertion will check for != null.

Actual behavior

The sonar C# scanner does not detect and raise an issue both in SonarCloud and in SonarLint.

Known workarounds

None. I tried the AssertionMethodAttribute, but that did not work.

public class AssertionMethodAttribute : Attribute { }

//...
[AssertionMethod]
public static void That(bool condition, string message = "Precondition failed!")
{
    if (!condition) { throw new AssertionException(message); }
}

Related information

llfab commented 2 years ago

Please note that a simple Attribute like ValidatedNotNullAttribute would not fix the problem even if it worked because the custom assertion is able to check any kind of boolean construction.

llfab commented 2 years ago

I found one workaround which is very specific however:

Assert.NotNull(systems, "Text");
return systems.GetPose(coordinateKey.Value, Core.Geometry3D.CoordinateKey.World) * Pose;

//...
public class AssertionMethodAttribute : Attribute { }
public sealed class ValidatedNotNullAttribute : Attribute { }

//...
[AssertionMethod]
public static void NotNull([ValidatedNotNull] object obj, string message = "Object must not be null!") => Assert.That(obj != null, message);

The (more generic) AssertionMethodAttribute did not help. The more specific ValidateNotNullAttribute did help.

I would be better however if this was doable also with more general means (like AssertionMethodAttribute).

pavel-mikula-sonarsource commented 2 years ago

This is an interesting scenario. Thank you for reporting it.

Methods decorated with AssertionMethodAttribute should assume that their first bool argument is always true. And try to learn constraints from the expression that is passed into it. We should be able to support the inline expressions, however, we will not be able to backtrack usages like

var result = systems != null;
Assert.That(result, "Something");

martin-strecker-sonarsource commented 2 years ago

The attribute @llfab is talking about is most likely the AssertionMethodAttribute from ReSharper. It goes in combination with AssertionConditionAttribute. We should also support Debug.Assert from the runtime, even though that one is not annotated.

Another attribute, that behaves in a comparable manner is DoesNotReturnIfAttribute, with a small difference:

AssertionMethodAttribute

Assert.That(someCondition); // void That([AssertionConditionType(AssertionConditionType.IS_TRUE)] condition)
RemainingMethdBody();

should be interpreted as

if (someCondition)
{
    RemainingMethdBody();
}

DoesNotReturnIfAttribute

Assert.That(someCondition); // void That([DoesNotReturnIf(true)] condition)
RemainingMethdBody();

should be interpreted as

if (someCondition)
{
    throw new Exception("");
}
RemainingMethdBody();

martin-strecker-sonarsource commented 2 years ago

SonarSource / sonar-dotnet