address review comments and add some more test coverage

Author: hnrklssn

clang/include/clang/AST/TypeBase.h

@@ -9006,10 +9006,9 @@ inline bool Type::isSafePointerType() const {
 }
 
 inline bool Type::isImplicitSafePointerType() const {
-  if (auto AT = this->getAs<AttributedType>()) {
+  if (const auto *AT = this->getAs<AttributedType>()) {
     if (AT->getAttrKind() == attr::PtrAutoAttr) {
-      assert(isSafePointerType());
-      return true;
+      return isSafePointerType();
     }
     return AT->getEquivalentType()->isImplicitSafePointerType();
   }

clang/include/clang/Sema/Sema.h

@@ -1203,7 +1203,7 @@ class Sema final : public SemaBase {
   bool findMacroSpelling(SourceLocation &loc, StringRef name);
 
   /// Calls \c Lexer::getLocForEndOfToken()
-  SourceLocation getLocForEndOfToken(SourceLocation Loc, unsigned Offset = 0) const;
+  SourceLocation getLocForEndOfToken(SourceLocation Loc, unsigned Offset = 0);
 
   /// Calls \c Lexer::findNextToken() to find the next token, and if the
   /// locations of both ends of the token can be resolved it return that
@@ -2815,7 +2815,7 @@ class Sema final : public SemaBase {
   /// Emit fix-it updating BTy to have the same CountAttributedType kind as ATy.
   void fixCountAttributedTypeKind(Sema::SemaDiagnosticBuilder &D,
                                   const CountAttributedType *ATy,
-                                  const CountAttributedType *BTy) const;
+                                  const CountAttributedType *BTy);
 
   /// Used to record or retrieve if a VarDecl/FieldDecl has a BoundsSafety FixIt
   /// emitted on it. The primary use case for this is preventing emitting

clang/include/clang/Sema/SemaFixItUtils.h

@@ -158,7 +158,7 @@ class BoundsSafetyFixItUtils {
 
   static void fixCountAttributedTypeLocsInFunctionSignature(
       Sema &S, Sema::SemaDiagnosticBuilder &D, QualType QA, QualType QB,
-      FunctionDecl *ADecl, FunctionDecl *BDecl, bool ExprMismatch,
+      const FunctionDecl *ADecl, FunctionDecl *BDecl, bool ExprMismatch,
       bool KindMismatch);
 
 private:

clang/lib/Sema/Sema.cpp

@@ -94,7 +94,7 @@ bool Sema::isCXXSafeBuffersBoundsSafetyInteropEnabledAt(
 }
 /* TO_UPSTREAM(BoundsSafety) OFF */
 
-SourceLocation Sema::getLocForEndOfToken(SourceLocation Loc, unsigned Offset) const {
+SourceLocation Sema::getLocForEndOfToken(SourceLocation Loc, unsigned Offset) {
   return Lexer::getLocForEndOfToken(Loc, Offset, SourceMgr, LangOpts);
 }
 

clang/lib/Sema/SemaBoundsSafety.cpp

@@ -272,7 +272,7 @@ bool Sema::CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, bool CountInBytes,
 
 /* TO_UPSTREAM(BoundsSafety) ON*/
 static SourceRange SourceRangeFor(const CountAttributedType *CATy,
-                                  const Sema &S, bool AttrNameOnly) {
+                                  Sema &S, bool AttrNameOnly) {
   // Note: This implementation relies on `CountAttributedType` being unique.
   // E.g.:
   //
@@ -833,7 +833,7 @@ bool Sema::BoundsSafetyCheckCountAttributedTypeHasConstantCountForAssignmentOp(
 
 void Sema::fixCountAttributedTypeKind(Sema::SemaDiagnosticBuilder &D,
                                       const CountAttributedType *ATy,
-                                      const CountAttributedType *BTy) const {
+                                      const CountAttributedType *BTy) {
   SourceRange SR = SourceRangeFor(BTy, *this, /*AttrNameOnly=*/true);
   if (SR.isInvalid()) {
     return;

clang/lib/Sema/SemaDecl.cpp

@@ -3743,9 +3743,9 @@ static void adjustDeclContextForDeclaratorDecl(DeclaratorDecl *NewD,
 
 /* TO_UPSTREAM(BoundsSafety) ON*/
 // FIXME: Rename to something more accurate
-struct TransposeDynamicBoundsExpr
-    : public TreeTransform<TransposeDynamicBoundsExpr> {
-  using BaseClass = TreeTransform<TransposeDynamicBoundsExpr>;
+struct CheckSameParameterNames
+    : public TreeTransform<CheckSameParameterNames> {
+  using BaseClass = TreeTransform<CheckSameParameterNames>;
 
   FunctionDecl *NewFD;
   bool Success = true;
@@ -3761,12 +3761,12 @@ struct TransposeDynamicBoundsExpr
     if (Invalid)
       return false;
 
-    TransposeDynamicBoundsExpr Checker(SemaRef, NewFD);
+    CheckSameParameterNames Checker(SemaRef, NewFD);
     Checker.TransformExpr(E);
     return Checker.Success;
   }
 
-  TransposeDynamicBoundsExpr(Sema &SemaRef, FunctionDecl *NewFD)
+  CheckSameParameterNames(Sema &SemaRef, FunctionDecl *NewFD)
       : BaseClass(SemaRef), NewFD(NewFD) {}
 
   ExprResult TransformDeclRefExpr(DeclRefExpr *E) {
@@ -3788,20 +3788,19 @@ struct TransposeDynamicBoundsExpr
   }
 };
 
-struct TransposeDynamicBoundsExprForReal
-    : public TreeTransform<TransposeDynamicBoundsExprForReal> {
-  using BaseClass = TreeTransform<TransposeDynamicBoundsExprForReal>;
+struct TransposeDynamicBoundsExpr
+    : public TreeTransform<TransposeDynamicBoundsExpr> {
+  using BaseClass = TreeTransform<TransposeDynamicBoundsExpr>;
 
   FunctionDecl *NewFD;
-  bool Success = true;
 
   /// Rewrite the expr `E` in the context of `NewFD`.
   /// If `E` refers to any function parameters, the rewritten version will refer
   /// to the corresponding parameters in `NewFD`.
   static bool Rewrite(Sema &SemaRef, FunctionDecl *NewFD, Expr *E,
                       std::string &Result) {
 
-    TransposeDynamicBoundsExprForReal Rewriter(SemaRef, NewFD);
+    TransposeDynamicBoundsExpr Rewriter(SemaRef, NewFD);
     ExprResult ExprRes = Rewriter.TransformExpr(E);
     if (ExprRes.isInvalid())
       return false;
@@ -3812,16 +3811,15 @@ struct TransposeDynamicBoundsExprForReal
     return true;
   }
 
-  TransposeDynamicBoundsExprForReal(Sema &SemaRef, FunctionDecl *NewFD)
+  TransposeDynamicBoundsExpr(Sema &SemaRef, FunctionDecl *NewFD)
       : BaseClass(SemaRef), NewFD(NewFD) {}
 
   ExprResult TransformDeclRefExpr(DeclRefExpr *E) {
-    ASTContext &ctx = SemaRef.getASTContext();
-    assert(isa<ParmVarDecl>(E->getDecl()));
-    auto *OldParm = cast<ParmVarDecl>(E->getDecl());
+    const ASTContext &ctx = SemaRef.getASTContext();
+    const auto *OldParm = cast<ParmVarDecl>(E->getDecl());
     auto *NewParm = NewFD->getParamDecl(OldParm->getFunctionScopeIndex());
     if (!ctx.hasSameType(OldParm->getType(), NewParm->getType()))
-      return {};
+      return ExprError();
     return DeclRefExpr::Create(ctx, NestedNameSpecifierLoc(), SourceLocation(),
                                NewParm, false, SourceLocation(),
                                NewParm->getType(), E->getValueKind());
@@ -3893,7 +3891,7 @@ static void fixBoundsSafetyTypeLocs(Sema &S, Sema::SemaDiagnosticBuilder &D,
     KS << "__bidi_indexable";
   } else if (auto DCPTA = QA->getAs<CountAttributedType>()) {
     std::string Rewritten;
-    if (!TransposeDynamicBoundsExpr::Check(S, BDecl, DCPTA->getCountExpr(),
+    if (!CheckSameParameterNames::Check(S, BDecl, DCPTA->getCountExpr(),
                                              Rewritten))
       return;
     const char *Keyword =
@@ -3904,7 +3902,7 @@ static void fixBoundsSafetyTypeLocs(Sema &S, Sema::SemaDiagnosticBuilder &D,
     KS << '(' << Rewritten << ')';
   } else if (auto EndPointer = GetEndPointer(QA)) {
     std::string Rewritten;
-    if (!TransposeDynamicBoundsExpr::Check(S, BDecl, EndPointer, Rewritten))
+    if (!CheckSameParameterNames::Check(S, BDecl, EndPointer, Rewritten))
       return;
     KS << "__ended_by(" << Rewritten << ')';
   } else {
@@ -3931,10 +3929,10 @@ static void fixBoundsSafetyTypeLocs(Sema &S, Sema::SemaDiagnosticBuilder &D,
 /// to be called once for each direction.
 void BoundsSafetyFixItUtils::fixCountAttributedTypeLocsInFunctionSignature(
     Sema &S, Sema::SemaDiagnosticBuilder &D, QualType QA, QualType QB,
-    FunctionDecl *ADecl, FunctionDecl *BDecl, bool ExprMismatch,
+    const FunctionDecl *ADecl, FunctionDecl *BDecl, bool ExprMismatch,
     bool KindMismatch) {
-  auto *ATy = QA->getAs<CountAttributedType>();
-  auto *BTy = QB->getAs<CountAttributedType>();
+  const auto *ATy = QA->getAs<CountAttributedType>();
+  const auto *BTy = QB->getAs<CountAttributedType>();
   assert(ATy && BTy);
   if (!ATy || !BTy)
     return;
@@ -3944,7 +3942,7 @@ void BoundsSafetyFixItUtils::fixCountAttributedTypeLocsInFunctionSignature(
 
   if (ExprMismatch) {
     std::string Rewritten;
-    if (!TransposeDynamicBoundsExprForReal::Rewrite(
+    if (!TransposeDynamicBoundsExpr::Rewrite(
             S, BDecl, ATy->getCountExpr(), Rewritten))
       return; // don't emit a KindMismatch fix-it either if this fails
     D << FixItHint::CreateReplacement(BTy->getCountExpr()->getSourceRange(),
@@ -3979,8 +3977,8 @@ static void fixBoundsSafetyFunctionDecl(Sema &S, Sema::SemaDiagnosticBuilder &D,
   }
 }
 
-static bool diagnoseConflictingAllocSizeAttribute(FunctionDecl *New,
-                                                  FunctionDecl *Old,
+static bool diagnoseConflictingAllocSizeAttribute(const FunctionDecl *New,
+                                                  const FunctionDecl *Old,
                                                   Sema &Self) {
   // System headers that haven't adopted bounds safety are allowed to break
   // the rules for compatibility reasons, since errors there are hard to fix.
@@ -3991,8 +3989,8 @@ static bool diagnoseConflictingAllocSizeAttribute(FunctionDecl *New,
   // return types. Merging of attributes is done after type checking, but we
   // want to emit the root cause of the type error rather than an error for
   // mismatcing (implicit) return types.
-  if (auto OldASA = Old->getAttr<AllocSizeAttr>()) {
-    if (auto NewASA = New->getAttr<AllocSizeAttr>()) {
+  if (const auto *OldASA = Old->getAttr<AllocSizeAttr>()) {
+    if (const auto *NewASA = New->getAttr<AllocSizeAttr>()) {
       if (!OldASA->getElemSizeParam().equals(NewASA->getElemSizeParam()) ||
           !OldASA->getNumElemsParam().equals(NewASA->getNumElemsParam())) {
         Self.Diag(NewASA->getLoc(), diag::err_mismatched_alloc_size)

clang/lib/Sema/SemaDeclAttr.cpp

@@ -6559,6 +6559,7 @@ class MakeStartedByPointerType
 
 class ConstructDynamicRangePointerType :
   public ConstructDynamicBoundType<ConstructDynamicRangePointerType> {
+  using BaseClass = ConstructDynamicBoundType<ConstructDynamicRangePointerType>;
   std::optional<TypeCoupledDeclRefInfo> StartPtrInfo;
 
 public:
@@ -6647,7 +6648,7 @@ class ConstructDynamicRangePointerType :
       ConstructedType = RetTy->getAs<BoundsAttributedType>();
       return RetTy;
     }
-    return VisitDynamicRangePointerType(T);
+    return BaseClass::VisitDynamicRangePointerType(T);
   }
 
   QualType DiagnoseConflictingType(const CountAttributedType *T) {

clang/test/BoundsSafety/Sema/ptrs-with-multiple-range-attrs.c

@@ -4,6 +4,9 @@
 // RUN: %clang_cc1 -fsyntax-only -fexperimental-bounds-safety-attributes -x c++ -verify=expected,cxx %s
 // RUN: %clang_cc1 -fsyntax-only -fexperimental-bounds-safety-attributes -x objective-c -verify=expected,c %s
 // RUN: %clang_cc1 -fsyntax-only -fexperimental-bounds-safety-attributes -x objective-c++ -verify=expected,cxx %s
+//
+// RUN: not %clang_cc1 -fsyntax-only -fbounds-safety -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s --implicit-check-not 'fix-it:"'
+// RUN: not %clang_cc1 -fsyntax-only -fexperimental-bounds-safety-attributes -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s --implicit-check-not 'fix-it:"'
 
 #include <ptrcheck.h>
 
@@ -27,33 +30,42 @@ int *__counted_by(4) bazz() {} // ok - same count expr
 
 // expected-note@+1{{previous declaration is here}}
 int *bazy();
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-1]]:6-[[@LINE-1]]:6}:"__counted_by(4) "
 // expected-error@+1{{conflicting '__counted_by' attribute with the previous function declaration}}
 int *__counted_by(4) bazy() {} // ok
 
 // expected-note@+1 2{{previous declaration is here}}
 int *__counted_by(len) bayz(unsigned len);
 // expected-error@+1{{conflicting '__counted_by' attribute with the previous function declaration}}
 int *bayz(unsigned len) {}
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-1]]:6-[[@LINE-1]]:6}:"__counted_by(len) "
 // expected-error@+2{{conflicting '__sized_by' attribute with the previous function declaration}}
 // expected-note@+1{{conflicting attributes were '__counted_by(len)' and '__sized_by(len)'}}
 int *__sized_by(len) bayz(unsigned len) {}
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-7]]:6-[[@LINE-7]]:18}:"__sized_by"
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-2]]:6-[[@LINE-2]]:16}:"__counted_by"
 
 // expected-note@+1{{previous declaration is here}}
 int *baxz(unsigned len, int **pptr);
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-1]]:30-[[@LINE-1]]:30}:"__counted_by(len)"
 // expected-error@+1{{conflicting '__counted_by' attribute with the previous function declaration}}
 int *baxz(unsigned len, int *__counted_by(len)* pptr) {} // ok
 
 // expected-note@+1{{previous declaration is here}}
 int *baxy(unsigned len, int *__counted_by(len)* pptr);
 // expected-error@+1{{conflicting '__counted_by' attribute with the previous function declaration}}
 int *baxy(unsigned len, int ** pptr) {}
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-1]]:30-[[@LINE-1]]:30}:"__counted_by(len)"
 
+typedef int canonically_int_t;
 char *__counted_by(len) qux(int len);
 // expected-note@+1{{previous declaration is here}}
 char *__sized_by(len) qux(int len);
 // expected-error@+2{{conflicting '__counted_by' attribute with the previous function declaration}}
 // expected-note@+1{{conflicting attributes were '__sized_by(len)' and '__counted_by(4)'}}
-char *__counted_by(4) qux(int len) {}
+char * _Nonnull __counted_by(4) qux(canonically_int_t len) {}
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-4]]:18-[[@LINE-4]]:21}:"4"
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-2]]:30-[[@LINE-2]]:31}:"len"
 
 int *__counted_by(len) __counted_by(len) quux(int len) {} // ok - same count expr
 
@@ -67,6 +79,7 @@ void corge(int *__counted_by(len) ptr, int len) {} // ok
 
 // expected-note@+1{{previous declaration is here}}
 int ** grault(int len);
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-1]]:8-[[@LINE-1]]:8}:"__counted_by(len) "
 // expected-error@+1{{conflicting '__counted_by' attribute with the previous function declaration}}
 int *__counted_by(len) grault(int len) {}
 
@@ -98,6 +111,7 @@ void end1(int *__ended_by(b) a, int *b); // OK redeclaration
 void end1(int *__ended_by(b) a, int *b) {} // OK definition over declaration
 
 void end1(int *a, int *b); // expected-error{{conflicting '__ended_by' attribute with the previous function declaration}}
+// CHECK: fix-it:{{.*}}ptrs-with-multiple-range-attrs.c":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"__ended_by(b) "
 void end1(int *a, int *__ended_by(a) b); // expected-error 2{{conflicting '__ended_by' attribute with the previous function declaration}} mismatch on both a and b
 void end1(int *__ended_by(b) a, int *__ended_by(a) b); // expected-error{{conflicting '__ended_by' attribute with the previous function declaration}}