[NFC] Fix potential dereferencing of nullptr.

[schittir]

Summary

Replace getAs with castAs and add assert if needed.

[schittir]

Fix typo

[Fznamznon]

clang/lib/Parse/ParseStmt.cpp

887 ↗

(On Diff #532528)

The assert that looks like assert(x && "x should not be null") seems strange. Failed assert(x) implies that x should not be null. If there is a message, a message saying what is wrong and why is much more useful.

clang/lib/Sema/SemaExprObjC.cpp

2441

That seems to be a strange place before and after changes. With or without change, when ReceiverType.isNull() is true we just end up passing nullptr as receiverTypeInfo to the BuildClassMessage which doesn't seem to be checking its non-nullness before dereferencing it, even though its description says that receiverTypeInfo can be null.
I guess it is fine to pass nullptr to a function whose description says so, but the non-nullness check inside of it should be probably a bit more obvious than it is right now.

[schittir]

clang/lib/Parse/ParseStmt.cpp

887 ↗

(On Diff #532528)

Would you suggest removing the message and changing it to

assert(DeepestParsedCaseStmt);

I am not sure what message would be useful here.

clang/lib/Sema/SemaExprObjC.cpp

2441

I see your point about passing ReceiverTypeInfo as nullptr to BuildClassMessage method - it seems ok to do that.
Would it make sense to add an assert(ReceiverTypeInfo); inside the method as way of making the non-nullness check more obvious?

[aaron.ballman]

clang/lib/Parse/ParseStmt.cpp

886–887 ↗

(On Diff #532528)

The assert doesn't add much value, this seems to be a false positive in the analysis.

If we have no top-level case statement yet, then we get into the if branch on line 884, but that then sets DeepestParsedCaseStmt on line 890, and that must be non-null because we verified that Case is valid above on line 876. So by the time we get into the else branch, DeepestParsedCaseStmt must be nonnull.

I don't think changes are needed here at all; WDYT?

clang/lib/Sema/SemaExprObjC.cpp

2441

I think there's a different assertion needed here. The invariant that BuildClassMessage() has is that the first argument (the TypeSourceInfo * needs to be nonnull if the third argument (the SourceLocation) is invalid. So I think this assert should be:

assert(((isSuperReceiver && Loc.isValid()) || receiverTypeInfo) &&
       "Either the super receiver location needs to be valid or the receiver needs valid type source information");

clang/lib/Sema/SemaType.cpp

2710–2712

Slightly better fix so we don't do "isa" followed by "cast".

[schittir]

Changes to address comments.

clang/lib/Parse/ParseStmt.cpp

886–887 ↗

(On Diff #532528)

This makes sense. I didn't thoroughly consider the Case.isInvalid() check above in my analysis. Thank you for catching this! I removed this change.

[aaron.ballman]

LGTM!

[schittir]

Thank you @aaron.ballman and @Fznamznon for reviewing! I landed this patch with commit d2fafa79ef08f9ef9cd0108a6caa7fc61a31bdeb

clang/lib/Sema/SemaExprObjC.cpp

ExprResult Sema::BuildClassMessageImplicit(QualType ReceiverType,
                                            SourceLocation Loc,
                                            Selector Sel,
                                            ObjCMethodDecl *Method,
                                            MultiExprArg Args) {
   TypeSourceInfo *receiverTypeInfo = nullptr;
   if (!ReceiverType.isNull())
     receiverTypeInfo = Context.getTrivialTypeSourceInfo(ReceiverType);
 
+  assert(((isSuperReceiver && Loc.isValid()) || receiverTypeInfo) &&
+         "Either the super receiver location needs to be valid or the receiver "
+         "needs valid type source information");
   return BuildClassMessage(receiverTypeInfo, ReceiverType,
                           /*SuperLoc=*/isSuperReceiver ? Loc : SourceLocation(),
                            Sel, Method, Loc, Loc, Loc, Args,
                            /*isImplicit=*/true);
 }
 
 static void applyCocoaAPICheck(Sema &S, const ObjCMessageExpr *Msg,
                                unsigned DiagID,

clang/lib/Sema/SemaObjCProperty.cpp

if (Synthesize) {
     property->setPropertyIvarDecl(Ivar);
 
     QualType IvarType = Context.getCanonicalType(Ivar->getType());
 
     // Check that type of property and its ivar are type compatible.
     if (!Context.hasSameType(PropertyIvarType, IvarType)) {
       if (isa<ObjCObjectPointerType>(PropertyIvarType)
           && isa<ObjCObjectPointerType>(IvarType))
-        compat =
-          Context.canAssignObjCInterfaces(
-                                  PropertyIvarType->getAs<ObjCObjectPointerType>(),
-                                  IvarType->getAs<ObjCObjectPointerType>());
+        compat = Context.canAssignObjCInterfaces(
+            PropertyIvarType->castAs<ObjCObjectPointerType>(),
+            IvarType->castAs<ObjCObjectPointerType>());
       else {
         compat = (CheckAssignmentConstraints(PropertyIvarLoc, PropertyIvarType,
                                              IvarType)
                     == Compatible);
       }
       if (!compat) {
         Diag(PropertyDiagLoc, diag::err_property_ivar_type)
           << property->getDeclName() << PropType

clang/lib/Sema/SemaType.cpp

QualType Sema::BuildVectorType(QualType CurType, Expr *SizeExpr,
   if ((!CurType->isDependentType() &&
        (!CurType->isBuiltinType() || CurType->isBooleanType() ||
         (!CurType->isIntegerType() && !CurType->isRealFloatingType())) &&
        !CurType->isBitIntType()) ||
       CurType->isArrayType()) {
     Diag(AttrLoc, diag::err_attribute_invalid_vector_type) << CurType;
     return QualType();
   }
   // Only support _BitInt elements with byte-sized power of 2 NumBits.
-  if (CurType->isBitIntType()) {
-    unsigned NumBits = CurType->getAs<BitIntType>()->getNumBits();
+  if (const auto *BIT = CurType->getAs<BitIntType>()) {
+    unsigned NumBits = BIT->getNumBits();
     if (!llvm::isPowerOf2_32(NumBits) || NumBits < 8) {
       Diag(AttrLoc, diag::err_attribute_invalid_bitint_vector_type)
           << (NumBits < 8);
       return QualType();
     }
   }
 
   if (SizeExpr->isTypeDependent() || SizeExpr->isValueDependent())