-Wnullable-to-nonnull-conversion no longer understands if-statements

[CodingMarkus]

With -Wnullable-to-nonnull-conversion enabled and set to error (-Werror), the following code has always been failing since _Nullable has been introduced to Xcode:

dispatch_cancel(csrc);

with the type of csrc being dispatch_source_t _Nullable. However, this code did not fail so far:

if (csrc) dispatch_cancel(csrc);

as the if-statement guarantees that csrc is only passed to the function, if it is not NULL and note that csrc is a stack variable and no reference to it has ever left the current the stack frame, so it's impossible that this variable could become NULL after the if-statement and before the function call.

Starting with Xcode 14.3 (LLVM 15.0.0), all of a sudden this line fails as well, which seems a huge step backwards to me. It seems like clang/LLVM got worse as understanding code logic. This forces developer to implicitly cast csrc to _Nonnull instead of just having to add an appropriate if-check, which is way better code.

BTW, the release notes of LLVM 15.0.0 imply nothing about such a change.

[CodingMarkus]

I just discovered that this behavior even violates the specification:

On a branch, where a nullable pointer is known to be non null, the checker treat it as a same way as a pointer annotated as nonnull.

Source: https://clang.llvm.org/docs/analyzer/developer-docs/nullability.html

Which clearly is not the case in my sample above and the same issue as already reported #27058 which was fixed in between (I'm pretty sure ternary statements worked correctly in the past a well), so I think this issue actually a real regression.

Working around it also is not that easy unless you want to write hundreds of casts by hand. I used to have a macro for that which broke as well. I tried to update that macro to work under the new conditions but the problem is that this won't work for blocks (the old one worked for every type). Here are the old and new versions.

[shafik]

CC @rjmccall

[CodingMarkus]

Maybe related: The behavior of nullability and auto typing was fundamentally changed in https://reviews.llvm.org/D110216 and https://reviews.llvm.org/D116342.

Original source: http://www.openradar.me/36877120

Also there is a related thread on Apple's developer forum: https://developer.apple.com/forums/thread/726000

[CodingMarkus]

Here's a minimal sample showing the change in behavior:

#include <stdlib.h>
#include <stdio.h>

static char *__nullable string = NULL;

void
print ( char * __nonnull string )
{
    printf("%s\n", string);
}

int main ( )
{
    srandomdev();
    if (random() % 2 == 0) {
        string = "Hello World";
    }

    // (1)
    if (string) {
        print(string);
    }

    // (2)
    __auto_type autoStrChecked = string;
    if (autoStrChecked) {
        print(autoStrChecked);
    }

    // (3)
    __auto_type autoStr = string;
    print(autoStr);

    // (4)
    print(string ?: "");

    return 0;
}

IMO only (3) should produce an error. In all other cases the compiler knows that the value passed is not NULL. (1) might produce an error but only if string is volatile or publicly visible; here it is neither (not volatile and static).

Compiling with Xcode 14.2, which is using clang-14, only (1) produces an error:

# CC=clang ; ($CC --version && $CC -Wall -Werror=nullable-to-nonnull-conversion -o test test.c)
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Volumes/Temp/Xcode/App/Xcode_14.2.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
test.c:23:9: error: implicit conversion from nullable pointer 'char * _Nullable' to non-nullable pointer type 'char * _Nonnull' [-Werror,-Wnullable-to-nonnull-conversion]
                print(string);
                      ^
1 error generated.

Compiling with Xcode 14.3, which uses clang-15 or any later build (e.g. clang-16) produces three errors:

clang version 16.0.5
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /opt/local/libexec/llvm-16/bin
test.c:23:9: error: implicit conversion from nullable pointer 'char * _Nullable' to non-nullable pointer type 'char * _Nonnull' [-Werror,-Wnullable-to-nonnull-conversion]
                print(string);
                      ^
test.c:29:9: error: implicit conversion from nullable pointer 'char * _Nullable' to non-nullable pointer type 'char * _Nonnull' [-Werror,-Wnullable-to-nonnull-conversion]
                print(autoStrChecked);
                      ^
test.c:34:8: error: implicit conversion from nullable pointer 'char * _Nullable' to non-nullable pointer type 'char * _Nonnull' [-Werror,-Wnullable-to-nonnull-conversion]
        print(autoStr);
              ^
3 errors generated.

Having to manually cast ever single time a nullable pointer is used for a non-nullable function is extremely tedious, as this happens extremely often. Most pointers in source code today can be NULL, most functions don't want a NULL pointer to be passed. The usual way to fix that is

if (value) process(value);

but that's not enough anymore. Now you must cast, despite the if-check ensuring that value is not NULL here. So we used the following work around in all our code so far:

#define iflet( dst, src )        \
    __auto_type  dst = src;  \
    if (dst)                 \

int main ( )
{
    iflet (str, string) {
        print(str);
    }

    return 0;
}

Which mimicked the if let construct in Swift but this isn't possible anymore.

test2.c:27:9: error: implicit conversion from nullable pointer 'char * _Nullable' to non-nullable pointer type 'char * _Nonnull' [-Werror,-Wnullable-to-nonnull-conversion]
                print(str);

And there is no way to fix that. You cannot use

__auto_type dst = (__nonnull)src;
--> error: nullability specifier '_Nonnull' cannot be applied to non-pointer type 'int'

__auto_type __nonnull dst = src;
-->  error: implicit conversion from nullable pointer 'char * _Nullable' to non-nullable pointer type 'char * _Nullable _Nonnull' [-Werror,-Wnullable-to-nonnull-conversion]

The following works

__auto_type dst = (typeof(*src) *__nonnull)src;

but only if the type of src can be de-referenced.

static void (^ block )( ) = ^{ };

int main ( )
{
    iflet (b, block) {

Results in

error: indirection requires pointer operand ('void (^)()' invalid)

The macro did work with block and function pointers, as well as with C and Objective-C pointers all alike. And disabling nullability checks means dropping the best C language extension that clang ever got, as it eliminates 99% of all NULL-pointer issues which are among the most prominent issues in C and Obj-C code.

[NSProgrammer]

Can we get anyone to take a look at this? We have been forced to downgrade -Wnullable-to-nonnull-conversion from an error to a warning.

[shafik]

CC @Bigcheese

[drobilla]

I'm also stuck between having to disable this (otherwise very beneficial) warning or adding noisy/opaque casts, no matter how obvious the condition is. With clang 16.0.6 on x64 Linux.

[martinboehme]

With -Wnullable-to-nonnull-conversion enabled and set to error (-Werror), the following code has always been failing since _Nullable has been introduced to Xcode:

dispatch_cancel(csrc);

with the type of csrc being dispatch_source_t _Nullable. However, this code did not fail so far:

if (csrc) dispatch_cancel(csrc);

Are you sure that this used to work?

In godbolt, I see no difference between Clang 14.0.0 (godbolt) and Clang 15.0.0 (godbolt) in this regard.

Also, when looking at the code that implements this warning, there doesn't seem to have been any change in the fundamental approach since it was introduced nine years ago. The code simply looks at the nullability of the source and destination expressions and makes no attempt to perform any kind of flow-sensitive analysis (i.e. to see whether the expression's value can be null at this specific point in the program).

The change discussed above (https://reviews.llvm.org/D110216) seems to have changed the behavior of __auto_type, but didn't do anything to change the behavior of -Wnullable-to-nonnull-conversion, as far as I can tell.

So because this isn't a regression, I think this isn't simply a case of going back to some hypothetical previous version of the code (that does a flow-sensitive analysis), because such a version doesn't exist. So I'm somewhat sceptical that the behavior will change anytime soon.