Add check for asserts without an explanatory message

[wilzbach]

From the NG: http://forum.dlang.org/post/uagjlugnczfegbuxdjzr@forum.dlang.org

[CyberShadow]

Awesome! enforce too, please?

[ghost]

Asking to put a message in assert(0), assert(false), etc, is not reasonable, e.g in default cases of switches and other similar uses.

Also it's very easy to cheat on the check: assert(0, " ")`.

[PetarKirov]

I agree with @bbasile that assert(0) and assert(false) could be left out.

[PetarKirov]

But in general, when reasonable people may disagree, it's better to leave it to the end-user to decide. So I guess the solution should be to add assert0_without_msg in addition to assert_without_msg and analogically for enforce.

Hopefully there should be an easy way to reuse the same analyzer for all of them. I wonder if something along the lines of this would work:

enum AssertCheck
{
    normalAssert,
    unreachableAssert,
    normalEnforce,
    unreachableEnforce
}

class AssertWithoutMessageCheck : AssertWithoutMessageCheckImpl!(AssertCheck.normalAssert) { }
class Assert0WithoutMessageCheck : AssertWithoutMessageCheckImpl!(AssertCheck.unreachableAssert) { }
class EnforceWithoutMessageCheck : AssertWithoutMessageCheckImpl!(AssertCheck.normalEnforce) { }
class Enforce0WithoutMessageCheck : AssertWithoutMessageCheckImpl!(AssertCheck.unreachableEnforce) { }

class AssertWithoutMessageCheckImpl(AssertCheck config) : BaseAnalyzer
{
    static if (config == AssertCheck.normalAssert)
    {
        enum string KEY = "dscanner.style.assert_without_msg";
        enum string MESSAGE = "An assert(cond) should have an explanatory message";
    }
    else static if (config == AssertCheck.unreachableAssert)
    {
        enum string KEY = "dscanner.style.assert0_without_msg";
        enum string MESSAGE = "An assert(false) should have an explanatory message";
    }
    else static if (config == AssertCheck.normalEnforce)
    {
        enum string KEY = "dscanner.style.enforce_without_msg";
        enum string MESSAGE = "An enforce(cond) should have an explanatory message";
    }
    else static if (config == AssertCheck.unreachableEnforce)
    {
        enum string KEY = "dscanner.style.enforce0_without_msg";
        enum string MESSAGE = "An enforce(false) should have an explanatory message";
    }
    else
        static assert(0, "Invalid AssertCheck template argument: " ~ config.stringof);

    // ...
}

Sounds good?

---

Edit: For some reason, I expected some fancy metaprogramming that would automatically instantiate decedents of BaseAnalyzer based on their enum KEY, but looking src/analysis/run.d reveals that this is not the case. So I guess the above could be simplified to:

enum AssertCheck
{
    normalAssert,
    unreachableAssert,
    normalEnforce,
    unreachableEnforce
}

class AssertWithoutMessageCheckImpl(AssertCheck config) : BaseAnalyzer
{
    enum string KEY = "dscanner.style.assert_without_msg";
    enum string MESSAGE = "Asserts and `enforce`s should have an explanatory message";
    // ...
}

[ghost]

@wilzbach, did you try the new check with phobos ? Maybe the results, the false positive and how much solvable they look can help to convince you but it's only two cases to skip, 0 and false and empty string literals too.

[ghost]

I don't agree with this change. Wasn't the check designed for phobos in first place ?

[ghost]

Meaning that disabled was okay.

[wilzbach]

Awesome! enforce too, please?

Done.

@wilzbach, did you try the new check with phobos ? Maybe the results, the false positive and how much solvable they look can help to convince you but it's only two cases to skip, 0 and false and empty string literals too.

Good idea. Full list is here: http://sprunge.us/MaGf

A couple of examples (abbreviated):

assert(false)

From std.functional:

private uint _ctfeSkipOp(ref string op)
{
    if (!__ctfe) assert(false);
...

assert(0)

From std.math:

        else version (Solaris)
        {
            asm pure nothrow @nogc // assembler by W. Bright
            {
                // EDX = (A.length - 1) * real.sizeof
...
            }
        }
        else
        {
            static assert(0);
        }

One from std.process:

    void remove(in char[] name) @trusted nothrow @nogc // TODO: @safe
    {
        version (Windows)    SetEnvironmentVariableW(name.tempCStringW(), null);
        else version (Posix) core.sys.posix.stdlib.unsetenv(name.tempCString());
        else static assert(0);
    }

From std.socket:

else
{
    static assert(0);     // No socket support yet.
}

Imho it would be better to have e.g. the message "No socket support yet" directly appear on the CLI, hence I don't see assert(0) as a false positive.

Also it's very easy to cheat on the check: assert(0, " ")`.

At least for peer-reviewed library, these cheats will/should be noticed.
I added a simple "cheat prevention", so " " will be detected .

I don't agree with this change. Wasn't the check designed for phobos in the first place ?

Fair enough.

[wilzbach]

Hopefully there should be an easy way to reuse the same analyzer for all of them.

I think there's no usecase to prevent enforce(0) and allow assert(0), so we could simplify it to to checks.

I wonder if something along the lines of this would work:

DScanner isn't used as an API, but through the command-line and thus the dscanner.ini is the only way to customize the behavior. There's already a default string defined for all tests:

enum Check: string
{
    /// Check is disabled.
    disabled    = "disabled",
    /// Check is enabled.
    enabled     = "enabled",
    /// Check is enabled but not operated in the unittests.
    skipTests   = "skip-unittest"
}

But we could create a new type for this check - sth. like this would work fine:

enum AssertWithoutMsgCheck: string
{
    /// Check is disabled.
    disabled    = "disabled",
    /// Check is enabled.
    enabled     = "enabled",
    /// Check is enabled but not operated in the unittests.
    skipTests   = "skip-unittest",
   /// Check is enabled, but not for assert(0) or assert(false)
   skipAssert0   = "skip-assert0",
   /// Check is enabled, but not for assert(0) or assert(false) and unittest
   skipTestsAssert0   = "skip-unittest-assert0",
}

[CyberShadow]

IMHO this is a waste of code.

[wilzbach]

I agree, but it wasn't hard and it helps to get this through the review.

[CyberShadow]

The cheating complaint is nonsensical, someone inclined to "cheat" can just as well do it with assert(cond, "fgsfds");. But the entire idea of "cheat prevention" in such tools is completely meaningless. There is no need to waste any time on it.

[ghost]

which you would also detect while reviewing Phobos, if this assert thing was really an issue (caugh caugh).

[CyberShadow]

It's easy to write an assertion or enforce without a message by accident, negligence, or simple unawareness of best practices; however, cheating the tool would take malevolent intent. We pretty much only care about the first three.

[Hackerpilot]

I agree with @CyberShadow, we should protect against accidents, not malice.

[ghost]

I still dont see the code that test for assert(0) or assert(false).

[CyberShadow]

I still dont see the code that test for assert(0) or assert(false).

It seems like we don't need that for Phobos.

If you'd like it for general use of DScanner, perhaps you could implement that after this PR is merged?

[wilzbach]

@bbasile FWIW I test for assert(0) - just not in the way you would like it to be.

[ghost]

I opt out from this review. The only thing that's important to my eyes is that it's disabled by default.

[ghost]

Sorry, hit the wrong button.

[ghost]

So nobody merges this ? Was it supposed to be finished or WIP ?

[wilzbach]

Was it supposed to be finished or WIP ?

I never had time to remove the "cheating check", but AFAICT nothing else is blocking this.

[ghost]

So removes it and let's get the merge.

[wilzbach]

So removes it and let's get the merge.

Finally removed!