Page MenuHomePhabricator

[Sema] Add warning for implicitly casting a null constant to a non null pointer type
Needs ReviewPublic

Authored by ahatanak on Jul 14 2016, 5:56 PM.

Details

Summary

This patch makes clang issue a warning when a null constant is used in a context where a non null expression is expected. For example:

int * _Nonnull p0 = 0; // warning expected here
int * _Nonnull p1;
int * _Nonnull p2 = c ? p1 : 0; // warning expected here

A new function Sema::diagnoseNullPtrToNonnullCast is defined, which checks whether a null pointer constant is being cast to a _Nonnull pointer type, and called before ImplicitCastExprs are created.

rdar://problem/24724255
rdar://problem/22074116

Diff Detail

Repository
rC Clang

Event Timeline

ahatanak updated this revision to Diff 64070.Jul 14 2016, 5:56 PM
ahatanak retitled this revision from to [Sema] Add warning for implicitly casting a null constant to a non null pointer type .
ahatanak updated this object.
ahatanak added a reviewer: doug.gregor.
ahatanak added a subscriber: cfe-commits.
doug.gregor edited edge metadata.Jul 14 2016, 7:55 PM

I think this check should go into SemaChecking.cpp

ahatanak updated this revision to Diff 65446.Jul 25 2016, 5:24 PM
ahatanak edited edge metadata.

Addressed review comment and made a couple of other changes.

  • Move Sema::diagnoseNullPtrToNonnullCast to SemaChecking.cpp.
  • In diagnoseNullPtrToNonnullCast, call "Type::isAnyPointerType" instead of "Type::isPointerType" so that the function can diagnose objective-c pointers in addition to c/c++ pointers.
  • Instead of adding the new warning to the existing "NullableToNonNullConversion" group, which would cause clang to warn about nil returns from objective-c methods (which is undesirable according to r240153's commit log), create a new diagnostic group for "-Wnull-const_to-nonnull".
ahatanak updated this revision to Diff 85651.Jan 24 2017, 4:25 PM
ahatanak added a reviewer: jordan_rose.

Rebase.

jordan_rose added inline comments.Jan 24 2017, 4:39 PM
include/clang/Basic/DiagnosticGroups.td
346

Nitpick: Using "const" here makes me think of the qualifier. Is there a reason not to just spell out "constant"?

include/clang/Basic/DiagnosticSemaKinds.td
9161

Why "DefaultIgnore"? This seems like a good warning to be on all the time.

ahatanak added inline comments.Jan 24 2017, 5:03 PM
include/clang/Basic/DiagnosticGroups.td
346

I probably just wanted it to be shorter, but I'll change it to constant as you suggested.

include/clang/Basic/DiagnosticSemaKinds.td
9161

I think I added DefaultIgnore just because the one above (warn_nullability_lost) had it too. I agree that it probably should be on all the time.

ahatanak updated this revision to Diff 87016.Feb 3 2017, 2:35 PM
ahatanak marked 2 inline comments as done.

Turning the warning on by default caused clang to issue warnings in many other cases, including Objective-C methods returning nil, which was something r240153 tried to avoid. If we want to disable the warning in Sema::ImpCastExprToType when the cast is part of a return statement of an ObjC method, we'll probably have to pass down a flag or something, which is probably not a good idea.

Instead of trying to issue the warning in Sema::ImpCastExprToType, the new patch checks whether there is a null constant in Sema::AddInitializerToDecl (which is needed to issue the first warning in null_constant_to_nonnull.c) and computeConditionalNullability (which is needed for the second warning). Also, it adds method Sema::checkNonNullExpr so that CheckNonNullExpr can be called from files other than SemaChecking.cpp.

ahatanak updated this revision to Diff 92583.Mar 21 2017, 6:33 PM

Rebase and ping.

jordan_rose edited edge metadata.Mar 22 2017, 6:14 PM

This looks like it's only coming up for declarations. What about assignments?

int x;
int * _Nonnull p = &x;
p = NULL; // warn here?
lib/Sema/SemaExpr.cpp
7117

This isn't quite correct, unfortunately. (_Nonnull id)nil should be considered non-nullable, since it's the canonical way to avoid all these warnings. It might just be good enough to move this check below the getNullability one, though.

ahatanak updated this revision to Diff 150218.Jun 6 2018, 4:25 PM
ahatanak marked an inline comment as done.
ahatanak added a reviewer: dcoughlin.
ahatanak set the repository for this revision to rC Clang.

Sorry for the delay in responding. I've addressed Jordan's review comments.

I had to make changes to a couple of tests in Analysis. In particular, I'm not sure whether we should try to avoid producing extra diagnostics in test/Analysis/nullability_nullonly.mm or whether it's possible to do so in Sema.

ahatanak added inline comments.Jun 6 2018, 4:30 PM
lib/Sema/SemaExpr.cpp
7117

Sema::CheckNonNullExpr checks the nullability of the type of the expression first and returns false if there is a cast to _Nonnull.

Sorry for the delay. I think this is mostly good, but I do still have a concern about the diagnostics change.

lib/Sema/SemaExpr.cpp
7117

Hm, okay, I see you have a test. Sorry for the noise.

7220

Nitpick: you could use const on the Exprs here.

11199

This could come later, but what about struct members or ObjC properties or ObjC subscripts? Seems like you could just check the type of the LHS.

test/Analysis/nullability_nullonly.mm
103

@dergachev.a, what do you think here? Is it okay that the analyzer diagnoses this in addition to the new warning?

test/Sema/conditional-expr.c
20

This seems like an unfortunate change to make, since most people do not bother with nullability.

NoQ edited subscribers, added: NoQ; removed: dergachev.a.Jul 10 2018, 12:04 PM
NoQ added inline comments.
test/Analysis/nullability_nullonly.mm
103

We're usually trying to avoid this when we notice it, but there are many cases where we didn't notice it because both warnings and the analyzer are becoming better independently. I guess you could just give us a heads up with a bug report if you don't want to bother with this.

In this case i think it should be easy to fix though, because the analyzer already has static isARCNilInitializedLocal() that suppresses implicit null initializers, we could teach it to suppress all null initializers instead.

ahatanak updated this revision to Diff 154908.Jul 10 2018, 6:02 PM
ahatanak marked 3 inline comments as done.

Address review comments.

lib/Sema/SemaExpr.cpp
11199

I removed the check for DeclRef so that it warns on struct members (see test case in null_constant_to_nonnull.c).

CheckNonNullArgument already checks null arguments passed to ObjC properties and subscripts setter methods. I added a new test case to test/SemaObjC/nullability.m.

test/Analysis/nullability_nullonly.mm
103

I think the warning in nullability-no-arc.mm should be suppressed too? I made changes to isARCNilInitializedLocal so that it returns true when the initializer is explicit or when it is not ARC. I'm not sure whether this is the right way to fix it, but it doesn't cause any regression tests to fail.

test/Sema/conditional-expr.c
20

Yes, this is unfortunate, but I'm not sure what's the right way to avoid printing nullability specifiers in the diagnostic message. Do you have any suggestions?

NoQ added inline comments.Jul 11 2018, 6:09 PM
test/Analysis/nullability_nullonly.mm
103

Yep, that makes sense, thank you!

It seems that we also need to check that the initializer is a null/nil literal, in a manner similar to how the warning checks it, otherwise we lose an analyzer positive on

int *foo() {
  int *x = 0;
  int *_Nonnull y = x; // expected-warning{{Null assigned to a pointer which is expected to have non-null value}}
  return y;
}

(which wasn't there in the test suite, so i had to make it up)

And i guess you'll need to rename the function and update comments, and parameter C also becomes unused (not sure it'll remain this way when the aforementioned test is fixed).

ahatanak added inline comments.Jul 16 2018, 11:19 AM
test/Sema/conditional-expr.c
20

It looks like I can use PrintingPolicy to print the nullability specifier only when needed.

I think it's also possible to add a flag to Expr that indicates the Expr is possibly null. For example, when an Expr is an IntegerLiteral of value 0, or a CastExpr or a ConditionalOperator has a subexpression whose flag is set. This could be a better solution than the current solution in this patch.

jordan_rose added inline comments.Jul 16 2018, 11:22 AM
test/Sema/conditional-expr.c
20

Whew. I hadn't had the chance to look at PrintingPolicy and was afraid you'd have to add a new flag to diagnostics or something to specify whether nullability was relevant.

An additional flag on Expr seems like overkill to me, given that isNullPointerConstant already exists. But I don't work in Clang these days, so maybe I'm wrong and it is something worth caching.