[NFC][Clang] Fix static analyzer concern about null pointer dereference

[eandrews]

Summary

Param->getTypeConstraint() can return nullptr, and was dereferenced without a check.

I followed the logic for invalid constraints and set Status as concepts::ExprRequirement::SS_ExprSubstitutionFailure if getTypeConstraint() returns null unexpectedly. I am not a 100% certain about this change since I am unfamiliar with concepts.

[aaron.ballman]

This feels a bit more like a functional change than a non-functional change because it seems like we should be able to test this case (whereas, if we think TC can never be null in reality, we could add an assert for it and not add test coverage). That said, I'm not certain how to induce a failure here. Adding @erichkeane in case he has ideas.

[eandrews]

In D157554#4576478, @aaron.ballman wrote:

This feels a bit more like a functional change than a non-functional change because it seems like we should be able to test this case (whereas, if we think TC can never be null in reality, we could add an assert for it and not add test coverage). That said, I'm not certain how to induce a failure here. Adding @erichkeane in case he has ideas.

Yea I agree. I see that this is inside ReturnTypeRequirement.isTypeConstraint() so maybe Param should always have a type constraint? I'm just naively guessing here though

[aaron.ballman]

In D157554#4576720, @eandrews wrote:

In D157554#4576478, @aaron.ballman wrote:

This feels a bit more like a functional change than a non-functional change because it seems like we should be able to test this case (whereas, if we think TC can never be null in reality, we could add an assert for it and not add test coverage). That said, I'm not certain how to induce a failure here. Adding @erichkeane in case he has ideas.

Yea I agree. I see that this is inside ReturnTypeRequirement.isTypeConstraint() so maybe Param should always have a type constraint? I'm just naively guessing here though

As I read the code, I think an assert is sufficient -- if the param is a type constraint, I believe we expect a non-null type constraint and a null one is a sign of a bug.

[eandrews]

In D157554#4577504, @aaron.ballman wrote:

In D157554#4576720, @eandrews wrote:

In D157554#4576478, @aaron.ballman wrote:

This feels a bit more like a functional change than a non-functional change because it seems like we should be able to test this case (whereas, if we think TC can never be null in reality, we could add an assert for it and not add test coverage). That said, I'm not certain how to induce a failure here. Adding @erichkeane in case he has ideas.

Yea I agree. I see that this is inside ReturnTypeRequirement.isTypeConstraint() so maybe Param should always have a type constraint? I'm just naively guessing here though

As I read the code, I think an assert is sufficient -- if the param is a type constraint, I believe we expect a non-null type constraint and a null one is a sign of a bug.

Alright. I'll make the change then. Thanks!

[eandrews]

Applied review comments

[tahonermann]

Looks good to me!

[eandrews]

Thanks for the reviews!

clang/lib/Sema/SemaExprCXX.cpp

else if (ReturnTypeRequirement.isTypeConstraint()) {
     Args.push_back(TemplateArgument(MatchedType));
 
     auto *Param = cast<TemplateTypeParmDecl>(TPL->getParam(0));
 
     TemplateArgumentList TAL(TemplateArgumentList::OnStack, Args);
     MultiLevelTemplateArgumentList MLTAL(Param, TAL.asArray(),
                                          /*Final=*/false);
     MLTAL.addOuterRetainedLevels(TPL->getDepth());
-    Expr *IDC = Param->getTypeConstraint()->getImmediatelyDeclaredConstraint();
-    ExprResult Constraint = SubstExpr(IDC, MLTAL);
+    const TypeConstraint *TC = Param->getTypeConstraint();
+    if (!TC) {
+      Status = concepts::ExprRequirement::SS_ExprSubstitutionFailure;
+    } else {
+      ExprResult Constraint =
+          SubstExpr(TC->getImmediatelyDeclaredConstraint(), MLTAL);
       if (Constraint.isInvalid()) {
         Status = concepts::ExprRequirement::SS_ExprSubstitutionFailure;
       } else {
         SubstitutedConstraintExpr =
             cast<ConceptSpecializationExpr>(Constraint.get());
         if (!SubstitutedConstraintExpr->isSatisfied())
           Status = concepts::ExprRequirement::SS_ConstraintsNotSatisfied;
       }
     }
+  }
   return new (Context) concepts::ExprRequirement(E, IsSimple, NoexceptLoc,
                                                  ReturnTypeRequirement, Status,
                                                  SubstitutedConstraintExpr);
 }
 
 concepts::ExprRequirement *
 Sema::BuildExprRequirement(
     concepts::Requirement::SubstitutionDiagnostic *ExprSubstitutionDiagnostic,