Guards and Debug.Asserts are Inconsistent

[justin-millman]

There is currently an inconsistency in the way that Guard.Against and Debug.Assert are being used throughout Kvasir and the associated projects. Some functions use both, some use one or the other, some use neither; and, which of those it is is inconsistent.

The rules should be:

• Guard.Against is used when evaluating function arguments to public functions or virtual protected functions
• Debug.Assert is used when evaluating function arguments to private functions or internal functions

The rationale here is simple: if a function is private or internal, it can only be invoked by the Framework itself, so the checks are purely defensive and should be compiled out in Release builds. However, public functions or virtual protected functions may be invoked with arguments provided by consumers: they therefore must be checked to ensure that preconditions are upheld.

[djinnstar]

Happy to give this a shot, BUT I wanted to confirm that this example is what we're looking to accomplish: [in src/Kvasir/Annotations/StringLengthAttribute.cs, at line 103]

                Debug.Assert(converters is not null);
                Debug.Assert(settings is not null);

should become

                Guard.Against.Null(converters);
                Guard.Against.Null(settings);

Let me know if I'm misunderstanding something!

[justin-millman]

You've got the right idea, but for the case you point out it's actually the reverse. That function is public, yes, but it's in a private struct, which means it's still only accessible to code in the Framework itself.

The general rule I want to live by is:

• If the condition is something that is checking user-provided values, it should be a Guard.Against call
• If the condition is something that is checking values provided by Kvasir itself, it should be a Debug.Assert call

Basically, I want the defensive programming in place to catch my own bugs, but I don't want to pay for the checks when actually running the code.

I think for this issue, I just want to apply that rule against the access modifiers that are already in place. Separately, I think I want to revisit what I'm making public and what is internal - I think I've got too much that is visible to users, even if I've made the constructors internal to prevent them from instantiating the types. When I do that, I'll obviously have to re-evaluate the check styles, but that can be done in a second step.

[justin-millman]

I'm actually rethinking this a bit.

For Atropos and Cybele, I think using the Guard Clauses in publicly accessible APIs is fine, since those are intended to be user-facing support libraries. For Kvasir, though, we have three different groupings:

• Things that are purely user-facing (like the custom attributes)
• Things that are purely internal (like most of the constructors)
• Things that are user-facing but not really part of the Framework (like the relations)

For the first group, we should be throwing KvasirExceptions (or a more specific derived type). For the second group, we should be using exclusively Debug.Assert. For the third group, we can use Ardalis.Guard.Against the way we would for e.g. Cybele and still use Debug.Assert in non-public APIs.

In this light, I think that the effort to revisit what is public is actually part of this - so I think we'll want to actually readdress the visibility patterns first and then do a pass over the error management. Otherwise I think we'll end up having too much bona fide exception logic (for things that will ultimately end up non-public).