[-Wnullable-to-nonnull-conversion] should warn in more cases, and shouldn't in some

[alejandro-colomar]

I was trying the feature, but it is trivial to silence warnings, and it also warns in some cases where it shouldn't:

$ cat nonnull.c 
int *_Nonnull f1(int *_Nullable p)
{
	return p;  // We got a warning :)
}

int *_Nonnull f2(int *_Nullable p)
{
	int *_Null_unspecified q;

	q = p;  // Expected a warning, but not :/

	return q;  // Expected a warning, but not :/
}

int f3(int *_Nullable p)
{
	return *p;  // Expected a warning, but not :/
}

int *_Nonnull f4(int *_Nullable p, int *_Nonnull q)
{
	if (p)
		return p;  // Ideally, shouldn't get a warning here :(
	return q;
}

$ clang -Weverything -Wno-nullability-extension -Wno-missing-prototypes -S -o /dev/null nonnull.c
nonnull.c:3:9: warning: implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull' [-Wnullable-to-nonnull-conversion]
        return p;  // We got a warning :)
               ^
nonnull.c:23:10: warning: implicit conversion from nullable pointer 'int * _Nullable' to non-nullable pointer type 'int * _Nonnull' [-Wnullable-to-nonnull-conversion]
                return p;  // Ideally, shouldn't get a warning here :(
                       ^
2 warnings generated.

[tbaederr]

Could you please change the title to something that could be considered professional and constructive instead of "weak and dumb"?

[alejandro-colomar]

Could you please change the title to something that could be considered professional and constructive instead of "weak and dumb"?

Sure. Done. Sorry I was a bit unimaginative for that yesterday. :)

[dwblaikie]

I would guess that f2 is working as intended, that "unspecified" is meant to capture the old semantics and implicitly convert in either direction.

f3 and f4 I don't understand/can't justify as well. https://clang.llvm.org/docs/analyzer/developer-docs/nullability.html suggests that the intent is to catch f3 - maybe it never got fully implemented? Or there's some bug here - might be worth checking the existing clang test cases to see if any dereferencing cases are diagnosed?

I could understand if f4 was only addressable by a more expensive static analysis like in Clang's Static Analyzer, rather than by classic compiler warnings, but can't explain the missing warning in f3.

@DougGregor

[MosheBerman]

tldr

1. I think this is f3 expected behavior because of null_unspecified and also the checker tries hard to avoid noise.

2. f4 can be relatively easily implemented in the static analyzer but I'm not sure about the Semantic analyzer.

3. Happy to (tentatively) look at implementing a fix for f4, time permitting.

In Detail

@dwblaikie, the page you linked says that dereferences are supposed to be handled when they are annotated. My reading is that this is f3 is expected behavior.

Put differently,the checker only checks for nullable and nonnull conversions, not nullable to null_unspecified conversions.

null_unspecified is the default annotation, and f3 has an unannotated return type.

Similar to the page you linked, this page has the description and the ensuing discussion.

The source is in NullabilityChecker.cpp and there are some useful comments which address the design choices.

There is also some logic in SemaType.cpp, around local variables and declaration shadowing. This is where I suspect the complexity around f4 comes in.

Source: I worked on an automatic nullability annotator at Meta last year and spent a ton of time in this code. (Just so you know I'm not making that up, here's an early version that I proposed before mass layoffs ended my work on that. The proposal was reject as-it-was but I had a nicer clang-tidy version in the works. C'est La Vie.)

Edit: I just want to add the caveat that I defer to project maintainers, who I assume of course know more than me.

[alejandro-colomar]

Hi!

tldr

1. I think this is `f3` expected behavior because of `null_unspecified` and also the checker tries hard to avoid noise.

Seems reasonable, especially since you have the #pragmas to tweak the default.

2. `f4` can be relatively easily implemented in the static analyzer but I'm not sure about the Semantic analyzer.

Hmm, since this warning is out of -Wextra, then it also seems reasonable.

BTW, I'd like to ask if there's a way to run the static analyzer as easily as GCC's -fanalyzer. From what I see, Clang's static analyzer is something that takes over your build system and does black magic to it to guess the right thing to do. I'd like instead something very dumb to which I can tell exactly what to do.

Is there some command that I can run on files, as clang-analyzer $CLANG-ANALYZERFLAGS $CFLAGS foo.c and get warnings out of it, similar to how clang-tidy works?

Should I open an issue to request this feature?

3. Happy to (tentatively) look at implementing a fix for `f4`, time permitting.

I'd love that.

In Detail

@dwblaikie, the page you linked says that dereferences are supposed to be handled when they are annotated. My reading is that this is f3 is expected behavior.

Put differently,the checker only checks for nullable and nonnull conversions, not nullable to null_unspecified conversions.

null_unspecified is the default annotation, and f3 has an unannotated return type.

Similar to the page you linked, this page has the description and the ensuing discussion.

The source is in NullabilityChecker.cpp and there are some useful comments which address the design choices.

There is also some logic in SemaType.cpp, around local variables and declaration shadowing. This is where I suspect the complexity around f4 comes in.

Source: I worked on an automatic nullability annotator at Meta last year and spent a ton of time in this code. (Just so you know I'm not making that up, here's an early version that I proposed before mass layoffs ended my work on that. The proposal was reject as-it-was but I had a nicer clang-tidy version in the works. C'est La Vie.)

Thanks a lot!

Alex

[MosheBerman]

BTW, I'd like to ask if there's a way to run the static analyzer as easily as GCC's -fanalyzer.

Yes, but I don't remember offhand. We had these build commands saved to build config files, so my muscle memory is lacking here.

Let me look it up.

From what I see, Clang's static analyzer is something that takes over your build system and does black magic to it to guess the right thing to do. I'd like instead something very dumb to which I can tell exactly what to do.

My limited understanding of the dark "magic" is that it traverses a giant syntax tree. (as part of the compilation, it parses the code into an Abstract Syntax Tree aka AST.) Simplistically, static analyzers are implemented as tree-traversal functions.

Is there some command that I can run on files, as clang-analyzer $CLANG-ANALYZERFLAGS $CFLAGS foo.c and get warnings out of it, similar to how clang-tidy works?

Yes, same as above.

Should I open an issue to request this feature?

I'm not a maintainer/collaborator, so grain of salt, but in my opinion this issue can be used. Let's see what the others think.

[MosheBerman]

🤦‍♂️right, should be simple.

You can add -Wnullability to your regular build command for static nullability warnings.

The list of warnings is here

[alejandro-colomar]

man_facepalmingright, should be simple.

You can add -Wnullability to your regular build command for static nullability warnings.

The list of warnings is here

No, what I meant with "it's not in -Wextra", is that I like that it's not enabled by default. Since it has false positives, like f4, it would be quite bad if it was in -Wextra, but since right now it's just opt-in, it's a good thing.

[alejandro-colomar]

I would like some static analyzer that has no false positives, like hopefully https://clang-analyzer.llvm.org/, to be able to run it in my build system.

If you can find the commands to run it as a simple command on files, without telling it about my build system, it would be great!

[nickdesaulniers]

This came up today on LKML: https://lore.kernel.org/lkml/CAHk-=wiP0983VQYvhgJQgvk-VOwSfwNQUiy5RLr_ipz8tbaK4Q@mail.gmail.com/

Personally, I was surprised that _Nonnull on return types only warns in the caller if it explicitly returns NULL:

#include <stddef.h>

int * _Nonnull foo (void) {
    return &foo;
}

int * _Nonnull baz (void) {
    return NULL;  // null returned from function that requires a non-null return value [-Wnonnull]
}

void bar (void) {
    if (foo() == NULL) // maybe should warn that foo() returns _Nonnull?
        bar();
}

especially since android is adding these to bionic.
https://android-review.googlesource.com/c/platform/bionic/+/2552567/7/libc/include/netinet/ether.h#45
https://android-review.googlesource.com/q/owner:zijunzhao@google.com+Nullability