dlang-community / D-Scanner

Swiss-army knife for D source code
Boost Software License 1.0
238 stars 80 forks source link

Warnings for implicit* casts to bool (for `if` & `!`), and `!item in list` #873

Open HuskyNator opened 1 year ago

HuskyNator commented 1 year ago

If statements & 'implicit' explicit casts

This issue mostly pertains to 'implicit' casts in if statements though it is not limited to this domain. *To double check the behavior I originally intended to reference documentation with regards to implicit casting from pointers to booleans and the use of logical not expressions. I was however unable to find either! This post however seems to describe part of it rather well, assuming it is still up to date, which it seems to be given some local testing

In general, if statements make implicit explicit casts to booleans. Meaning invalid implicit casts are still permitted. It should be quite obvious how this could be cause for serious issues. (I was actually able to find at least 2 requests to change this behaviour. It is even related to issue #210. As such, it seems reasonable to warn against this 'implicit' cast to bool inside an if statement. This would however contain the following edge case.

Membership

A common use of implicit casts in if statements is checking for membership:

if(key in assocArray)

With in yielding either a pointer to the value associated with the key, or null if not applicable. The pointer is then implicitly cast to bool. This way, equality with null does not need to be tested explicitly. Additionally this allows for the assignment syntax:

if(auto identifier = key in assocArray)

Apart from this use case (in expressions), most (if not all) other use cases have been part of buggy code in my experience. One less common use case, if(pointer), also comes to mind, though this is semantically identical to the preferential, more expressive & less error prone if(pointer !is null).

Incorrect Membership

Due to precedence rules, the statement if(!entry in list) is very misleading, as it is not identical to if(entry !in list). In most cases this will not be cause for concern as it will simply not compile. In the cases where it does compile (such as when implicit casts from bool to the entry type are permitted, eg. most (if not all) integer types), it is however always a bug (the only practically valid case is an associative array of type bool). The origin of this issue is related to the issue described below. The construct itself can however be considered for the addition of explicit warnings.

Logical Not ! casts

Apart from if statements, logical not expressions also (seem to*) behave as described above:

bool condition = wronglyTypedPointer; // invalid
bool condition = !wronglyTypedPointer; // valid
size_t offset = !wronglyTypedPointer; // also valid!
size_t offset = !3.5f; // also valid!!

This makes code that should semantically not compile, or is otherwise very likely to contain typos, run without issue. As you may have noticed, this pitfall is especially deep due to the way booleans can be further implicitly cast to other types, essentially creating an implicit chain of casts (eg. float->bool->size_t).

Again, note the initial cast to bool does not behave as a normal implicit cast, but rather as if cast(bool) were inserted.

Simple Examples

One can think of many ways these behaviors could lead to issues. Especially when (incorrectly) combined. Just from the top of my head:

bool test = !"false";
bool* condition;
...
if(condition) // or !condition
string[char] dictionary;
if(!'a' in dictionary)

Sidenote

The original reason I wrote this request is the following piece of code.

Set!size_t removeLanguages; // Indices of languages to remove (Set is a wrapper for void[0][size_t])
string[] languages;
...
string[] header;
foreach (language; languages)
    if (!language in removeLanguages)
        header ~= language;
...

This compiles & runs fine, until one finds out all languages have been removed. This piece of code actually contains 2 mistakes, the first of which however hides the second:

  1. The !entry in list construct
  2. The use of string language, instead of its size_t index (eg. languages.countUntil(language) or comparable)

Originally this was supposed to be just about this first incorrect construct. Reading up on the issue, especially previous mentions, and upon not finding any reasonable documentation apart from the referenced forum note, I however expanded this request to also contain the implicit casts described. Apologies if this request ended up being bigger than may be desirable. - HN