[clang] Fix incorrect inferred lifetime_capture_by attr on STL

[hokein]

We incorrectly annotate the iterator parameter for insert method (void insert(const_iterator, const value_type& value)), because iterator is also a gsl-pointer type.

This patch fixes it.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

We incorrectly annotate the iterator parameter for insert method (void insert(const_iterator, const value_type& value)), because iterator is also a gsl-pointer type.

This patch fixes it.

---

Full diff: https://github.com/llvm/llvm-project/pull/118013.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaAttr.cpp (+6-1)
• (modified) clang/test/AST/attr-lifetime-capture-by.cpp (+28-38)

diff --git a/clang/lib/Sema/SemaAttr.cpp b/clang/lib/Sema/SemaAttr.cpp
index b0849c74e375ed..d3cf42251be2e7 100644
--- a/clang/lib/Sema/SemaAttr.cpp
+++ b/clang/lib/Sema/SemaAttr.cpp
@@ -287,7 +287,12 @@ void Sema::inferLifetimeCaptureByAttribute(FunctionDecl *FD) {
     if (PVD->hasAttr<LifetimeCaptureByAttr>())
       return;
   for (ParmVarDecl *PVD : MD->parameters()) {
-    if (sema::isPointerLikeType(PVD->getType().getNonReferenceType())) {
+    // Methods in standard containers that capture values typically accept
+    // reference-type parameters, e.g., `void push_back(const T& value)`.
+    // We only apply the lifetime_capture_by attribute to parameters of
+    // pointer-like reference types (`const T&`, `T&&`).
+    if (PVD->getType()->isReferenceType() &&
+        sema::isPointerLikeType(PVD->getType().getNonReferenceType())) {
       int CaptureByThis[] = {LifetimeCaptureByAttr::THIS};
       PVD->addAttr(
           LifetimeCaptureByAttr::CreateImplicit(Context, CaptureByThis, 1));
diff --git a/clang/test/AST/attr-lifetime-capture-by.cpp b/clang/test/AST/attr-lifetime-capture-by.cpp
index c3afe267301ad7..2a568b0f00d3ae 100644
--- a/clang/test/AST/attr-lifetime-capture-by.cpp
+++ b/clang/test/AST/attr-lifetime-capture-by.cpp
@@ -37,67 +37,56 @@ struct vector {
 struct [[gsl::Pointer()]] View {};
 std::vector<View> views;
 // CHECK:   ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
-// CHECK:       TemplateArgument type 'View'
-// CHECK-NOT:   LifetimeCaptureByAttr
 
 // CHECK:       CXXMethodDecl {{.*}} push_back 'void (const View &)'
-// CHECK:           ParmVarDecl {{.*}} 'const View &'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
-// CHECK-NOT:   LifetimeCaptureByAttr
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'const View &'
+// CHECK-NEXT:               LifetimeCaptureByAttr {{.*}} Implicit
 
 // CHECK:       CXXMethodDecl {{.*}} push_back 'void (View &&)'
-// CHECK:           ParmVarDecl {{.*}} 'View &&'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'View &&'
+// CHECK-NEXT:               LifetimeCaptureByAttr {{.*}} Implicit
 
 // CHECK:       CXXMethodDecl {{.*}} insert 'void (iterator, View &&)'
-// CHECK:           ParmVarDecl {{.*}} 'iterator'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
-// CHECK:           ParmVarDecl {{.*}} 'View &&'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
-// CHECK-NOT:   LifetimeCaptureByAttr
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'iterator'
+// CHECK-NOT:               LifetimeCaptureByAttr {{.*}} Implicit
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'View &&'
+// CHECK-NEXT:               LifetimeCaptureByAttr {{.*}} Implicit
 
 template <class T> struct [[gsl::Pointer()]] ViewTemplate {};
 std::vector<ViewTemplate<int>> templated_views;
-// CHECK:   ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
-// CHECK:       TemplateArgument type 'ViewTemplate<int>'
-// CHECK-NOT:   LifetimeCaptureByAttr
+// CHECK:       ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
 
 // CHECK:       CXXMethodDecl {{.*}} push_back 'void (const ViewTemplate<int> &)'
-// CHECK:           ParmVarDecl {{.*}} 'const ViewTemplate<int> &'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'const ViewTemplate<int> &'
+// CHECK-NEXT:               LifetimeCaptureByAttr {{.*}} Implicit
 // CHECK-NOT:   LifetimeCaptureByAttr
 
 // CHECK:       CXXMethodDecl {{.*}} push_back 'void (ViewTemplate<int> &&)'
-// CHECK:           ParmVarDecl {{.*}} 'ViewTemplate<int> &&'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'ViewTemplate<int> &&'
+// CHECK-NEXT:               LifetimeCaptureByAttr {{.*}} Implicit
 
 // CHECK:       CXXMethodDecl {{.*}} insert 'void (iterator, ViewTemplate<int> &&)'
-// CHECK:           ParmVarDecl {{.*}} 'iterator'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
-// CHECK:           ParmVarDecl {{.*}} 'ViewTemplate<int> &&'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
-// CHECK-NOT:   LifetimeCaptureByAttr
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'iterator'
+// CHECK-NOT:               LifetimeCaptureByAttr {{.*}} Implicit
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'ViewTemplate<int> &&'
+// CHECK-NEXT:               LifetimeCaptureByAttr {{.*}} Implicit
 
 std::vector<int*> pointers;
 // CHECK:   ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
-// CHECK:       TemplateArgument type 'int *'
-// CHECK-NOT:   LifetimeCaptureByAttr
 
 // CHECK:       CXXMethodDecl {{.*}} push_back 'void (int *const &)'
-// CHECK:           ParmVarDecl {{.*}} 'int *const &'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
-// CHECK-NOT:   LifetimeCaptureByAttr
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'int *const &'
+// CHECK-NEXT:               LifetimeCaptureByAttr {{.*}} Implicit
 
 // CHECK:       CXXMethodDecl {{.*}} push_back 'void (int *&&)'
-// CHECK:           ParmVarDecl {{.*}} 'int *&&'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'int *&&'
+// CHECK-NEXT:               LifetimeCaptureByAttr {{.*}} Implicit
 
 // CHECK:       CXXMethodDecl {{.*}} insert 'void (iterator, int *&&)'
-// CHECK:           ParmVarDecl {{.*}} 'iterator'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
-// CHECK:           ParmVarDecl {{.*}} 'int *&&'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
-// CHECK-NOT:   LifetimeCaptureByAttr
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'iterator'
+// CHECK-NOT:               LifetimeCaptureByAttr {{.*}} Implicit
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'int *&&'
+// CHECK-NEXT:               LifetimeCaptureByAttr {{.*}} Implicit
 
 std::vector<int> ints;
 // CHECK:   ClassTemplateSpecializationDecl {{.*}} struct vector definition implicit_instantiation
@@ -110,6 +99,7 @@ std::vector<int> ints;
 // CHECK-NOT:   LifetimeCaptureByAttr
 
 // CHECK:       CXXMethodDecl {{.*}} insert 'void (iterator, int &&)'
-// CHECK:           ParmVarDecl {{.*}} 'iterator'
-// CHECK:               LifetimeCaptureByAttr {{.*}} Implicit
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'iterator'
+// CHECK-NOT:               LifetimeCaptureByAttr {{.*}} Implicit
+// CHECK-NEXT:           ParmVarDecl {{.*}} 'int &&'
 // CHECK-NOT:   LifetimeCaptureByAttr

[usx95]

Can you also add a lit test for:

std::vector<std::string> getVector();
std::set<std::string> s;
s.insert(getVector().begin(), getVector().end()); // FIXME: taking iterator of a temporary container without immediate dereference is almost always wrong.

[usx95]

maybe skip this line if CHECK-NEXT is possible.
same in line 70, 87

[hokein]

ah, right, this is even better. Done.

[hokein]

Can you also add a lit test for:

std::vector<std::string> getVector();
std::set<std::string> s;
s.insert(getVector().begin(), getVector().end()); // FIXME: taking iterator of a temporary container without immediate dereference is almost always wrong.

I think this is a different issue, I'd prefer not adding it to this test file.