[PATCH] D14938: Fix implicit conversion from _Nullable to _Nonnull warnings in C++

[Justin Bogner via cfe-commits]

George Burgess IV via cfe-commits <cfe-commits at lists.llvm.org> writes:
> george.burgess.iv created this revision.
> george.burgess.iv added a reviewer: doug.gregor.
> george.burgess.iv added subscribers: cfe-commits, doug.gregor.
>
> Currently, code like this compiles cleanly in C++, but with warnings
> (as it should) in C:

Are there already tests in place for the C version? I assume so, but
please double check.

>
> ```
> int foo() {
>   void *_Nullable p;
>   void *_Nonnull n = p;
> }
> ```
>
> This patch makes us emit the appropriate warnings in C++.

LGTM with a couple of nits, below.

> @doug.gregor: `arc` said you would be best to review this; if you're
> unable to, I'm happy to ping others. :)
>
> http://reviews.llvm.org/D14938
>
> Files:
>   include/clang/Sema/Sema.h
>   lib/Sema/Sema.cpp
>   lib/Sema/SemaExpr.cpp
>   lib/Sema/SemaExprCXX.cpp
>   test/SemaCXX/nullability.cpp
>
> Index: test/SemaCXX/nullability.cpp
> ===================================================================
> --- test/SemaCXX/nullability.cpp
> +++ test/SemaCXX/nullability.cpp
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wno-nullability-declspec %s -verify
> +// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wno-nullability-declspec %s -verify -Wnullable-to-nonnull-conversion
>  
>  #if __has_feature(nullability)
>  #else
> @@ -67,3 +67,33 @@
>  }
>  
>  template void test_accepts_nonnull_null_pointer_literal_template<&accepts_nonnull_4>(); // expected-note{{instantiation of function template specialization}}
> +
> +void TakeNonnull(void *_Nonnull);
> +// Check different forms of assignment to a nonull type from a nullable one.
> +void AssignAndInitNonNull() {
> +  void *_Nullable nullable;
> +  void *_Nonnull p(nullable); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  void *_Nonnull p2{nullable}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  void *_Nonnull p3 = {nullable}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  void *_Nonnull p4 = nullable; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  void *_Nonnull nonnull;
> +  nonnull = nullable; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  nonnull = {nullable}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +
> +  TakeNonnull(nullable); //expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull}}
> +  TakeNonnull(nonnull); // OK
> +}
> +
> +void *_Nullable ReturnNullable();
> +
> +void AssignAndInitNonNullFromFn() {
> +  void *_Nonnull p(ReturnNullable()); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  void *_Nonnull p2{ReturnNullable()}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  void *_Nonnull p3 = {ReturnNullable()}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  void *_Nonnull p4 = ReturnNullable(); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  void *_Nonnull nonnull;
> +  nonnull = ReturnNullable(); // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +  nonnull = {ReturnNullable()}; // expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull'}}
> +
> +  TakeNonnull(ReturnNullable()); //expected-warning{{implicit conversion from nullable pointer 'void * _Nullable' to non-nullable pointer type 'void * _Nonnull}}
> +}
> Index: lib/Sema/SemaExprCXX.cpp
> ===================================================================
> --- lib/Sema/SemaExprCXX.cpp
> +++ lib/Sema/SemaExprCXX.cpp
> @@ -3118,6 +3118,7 @@
>      ToType = ToAtomic->getValueType();
>    }
>  
> +  QualType InitialFromType = FromType;
>    // Perform the first implicit conversion.
>    switch (SCS.First) {
>    case ICK_Identity:
> @@ -3488,6 +3489,12 @@
>                               VK_RValue, nullptr, CCK).get();
>    }
>  
> +  // If this conversion sequence succeeded and involved implicitly converting a
> +  // _Nullable type to a _Nonnull one, complain.
> +  if (CCK == CCK_ImplicitConversion)
> +    diagnoseNullableToNonnullConversion(ToType, InitialFromType,
> +                                        From->getLocStart());
> +
>    return From;
>  }
>  
> Index: lib/Sema/SemaExpr.cpp
> ===================================================================
> --- lib/Sema/SemaExpr.cpp
> +++ lib/Sema/SemaExpr.cpp
> @@ -11823,8 +11823,8 @@
>  
>    switch (ConvTy) {
>    case Compatible:
> -      DiagnoseAssignmentEnum(DstType, SrcType, SrcExpr);
> -      return false;
> +    DiagnoseAssignmentEnum(DstType, SrcType, SrcExpr);
> +    return false;

This looks unrelated / please commit formatting cleanup separately.

>  
>    case PointerToInt:
>      DiagKind = diag::ext_typecheck_convert_pointer_int;
> Index: lib/Sema/Sema.cpp
> ===================================================================
> --- lib/Sema/Sema.cpp
> +++ lib/Sema/Sema.cpp
> @@ -349,6 +349,16 @@
>    AnalysisWarnings.PrintStats();
>  }
>  
> +void Sema::diagnoseNullableToNonnullConversion(QualType DstType,
> +                                               QualType SrcType,
> +                                               SourceLocation Loc) {
> +  if (auto ExprNullability = SrcType->getNullability(Context))
> +    if (*ExprNullability == NullabilityKind::Nullable)
> +      if (auto TypeNullability = DstType->getNullability(Context))
> +        if (*TypeNullability == NullabilityKind::NonNull)
> +          Diag(Loc, diag::warn_nullability_lost) << SrcType << DstType;
> +}

This might be more readable with an early return or two. If it doesn't
seem any more readable to you it's fine as is though.

> +
>  /// ImpCastExprToType - If Expr is not of type 'Type', insert an implicit cast.
>  /// If there is already an implicit cast, merge into the existing one.
>  /// The result is of the given category.
> @@ -372,18 +382,7 @@
>    assert((VK == VK_RValue || !E->isRValue()) && "can't cast rvalue to lvalue");
>  #endif
>  
> -  // Check whether we're implicitly casting from a nullable type to a nonnull
> -  // type.
> -  if (auto exprNullability = E->getType()->getNullability(Context)) {
> -    if (*exprNullability == NullabilityKind::Nullable) {
> -      if (auto typeNullability = Ty->getNullability(Context)) {
> -        if (*typeNullability == NullabilityKind::NonNull) {
> -          Diag(E->getLocStart(), diag::warn_nullability_lost)
> -            << E->getType() << Ty;
> -        }
> -      }
> -    }
> -  }
> +  diagnoseNullableToNonnullConversion(Ty, E->getType(), E->getLocStart());
>  
>    QualType ExprTy = Context.getCanonicalType(E->getType());
>    QualType TypeTy = Context.getCanonicalType(Ty);
> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h
> +++ include/clang/Sema/Sema.h
> @@ -3505,6 +3505,11 @@
>    void DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
>                          SourceLocation OpLoc);
>  
> +  /// \brief Warn if we're implicitly casting from a _Nullable pointer type to a
> +  /// _Nonnull one.
> +  void diagnoseNullableToNonnullConversion(QualType DstType, QualType SrcType,
> +                                           SourceLocation Loc);
> +
>    ParsingDeclState PushParsingDeclaration(sema::DelayedDiagnosticPool &pool) {
>      return DelayedDiagnostics.push(pool);
>    }
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits