Enable assert_without_msg DScanner check

[wilzbach]

dlang-community/D-Scanner#495 is now on the phobos branch of DScanner.

Assert messages are very helpful to a user and we should try our best to avoid things like

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

or

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

See also the NG thread with this idea or dlang-community/D-Scanner#495

FWIW the blacklist might look long, but (1) the opposite is long as well and (2) we should try to improve the status quo - module per module (many modules only contain a few assert(0);

CC @CyberShadow @andralex

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[JackStouffer]

Hmmm, is this a standard we want to apply to every assert?

Ping @andralex. What's your opinion on this?

[andralex]

A message on static assert is nice but not always required - e.g. it often refers to an implementation matter that is obvious from context. I'm not strong pro or con. FWIW C++ does require a message, which people often provide as "".

[andralex]

A message on each non-static assert would be nice to have though.

[CyberShadow]

I think having to write static assert(false, "Unsupported platform"); shouldn't be too bad. The message doesn't need to be specific; anything is better than "Assertion failed" or "Enforcement failed".

[jmdavis]

IMHO, if it's clear from the assertion's condition what is being checked, then a message is pointless. e.g. what are you going to do with assert(!range.empty)? Something like assert(!range.empty, "the range was empty when it wasn't suppose to be")? It's not the end of the world if we require messages, but I really don't think that it's going to help much in a lot of cases, because it's frequently obvious from the assertion itself - especially when you look at the code, which you really need to do with most assertions anyway.

Now, if the message can make it unnecessary to look at the code, then great (function pre-conditions could be a good example of this), and it is certainly the case sometimes that the condition needs extra explanation, in which case a message it definitely warranted. But IMHO, an assertion message is often pointless. So, I don't think that requiring that all assertions have messages makes a lot of sense.

[jmdavis]

I think having to write static assert(false, "Unsupported platform"); shouldn't be too bad.

I think that that's an example of one where a message helps clarify the situation and potentially eliminates the need to look at the code, so I agree that it makes sense. I just don't agree that it's always an improvement to have a message. Sometimes, it's just redundant, or you'd have to look at the code to understand the issue anyway, so the message doesn't help.

[CyberShadow]

IMHO, if it's clear from the assertion's condition what is being checked, then a message is pointless. e.g. what are you going to do with assert(!range.empty)?

That's a fair point, but the bigger picture question is whether enforcing this rule (and fixing the instances it identifies) will have a net positive impact in the end. Some redundant assertion messages may be a bit annoying, but being 100% sure that a D user will never again see an "assertion failed" message inside Phobos code with no further explanation sounds like a goal worth pursuing.

[CyberShadow]

A bug in favor of this change:

https://issues.dlang.org/show_bug.cgi?id=17458

[CyberShadow]

@jmdavis I think all things considered we would gain more by going forward with this, what do you think?

[MetaLang]

What's the holdup on this? There is never a good reason to not include an assert message; it's the difference between getting an error message immediately on the command line and having to open up the code to the error location to figure out what went wrong. IMO this is a no-brainer that affects basic usability.

[MetaLang]

ping @wilzbach

[wilzbach]

What's the holdup on this? There is never a good reason to not include an assert message; it's the difference between getting an error message immediately on the command line and having to open up the code to the error location to figure out what went wrong. IMO this is a no-brainer that affects basic usability.

As newer DScanner versions contain this check (at least the phobos branch), this is as simple as setting a blacklist.
While I was at this PR, I already removed a couple of modules from the blacklist (std.algorithm.mutation, std.json, std.zip, std.zlib), but the blacklist offers still a lot of work.