[Clang][Sema] Earlier type checking for builtin unary operators #90500

sdkrystian · 2024-04-29T17:40:02Z

Currently, clang postpones all semantic analysis of unary operators with operands of pointer/pointer to member/array/function type until instantiation whenever that type is dependent (e.g. T* where T is a type template parameter). Consequently, the uninstantiated AST nodes all have the type ASTContext::DependentTy (which, for the purposes of #90152, is undesirable as that type may be the current instantiation! (e.g. *this))

This patch moves the point at which we perform semantic analysis for such expression to be prior to instantiation.

llvmbot · 2024-04-29T17:40:34Z

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Currently, clang postpones all semantic analysis of unary operators with operands of pointer/pointer to member/array/function type until instantiation whenever that type is dependent (e.g. T* where T is a type template parameter). Consequently, the uninstantiated AST nodes all have the type ASTContext::DependentTy (which, for the purposes of #90152, is undesirable as that type may be the current instantiation! (e.g. *this))

This patch moves the point at which we perform semantic analysis for such expression to be prior to instantiation.

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

9 Files Affected:

(modified) clang/include/clang/AST/Type.h (+5-1)
(modified) clang/lib/Sema/SemaExpr.cpp (+13-8)
(modified) clang/test/AST/ast-dump-expr-json.cpp (+2-2)
(modified) clang/test/AST/ast-dump-expr.cpp (+1-1)
(modified) clang/test/AST/ast-dump-lambda.cpp (+1-1)
(modified) clang/test/CXX/over/over.built/ast.cpp (+5-3)
(modified) clang/test/Frontend/noderef_templates.cpp (+2-2)
(modified) clang/test/SemaCXX/cxx2b-deducing-this.cpp (+2-4)
(modified) clang/test/SemaTemplate/class-template-spec.cpp (+6-6)

diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index dff02d4861b3db..471f78bbdf283c 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -8040,7 +8040,11 @@ inline bool Type::isUndeducedType() const {
 /// Determines whether this is a type for which one can define
 /// an overloaded operator.
 inline bool Type::isOverloadableType() const {
-  return isDependentType() || isRecordType() || isEnumeralType();
+  QualType Canonical = getCanonicalTypeInternal();
+  if (!Canonical->isDependentType())
+    return isRecordType() || isEnumeralType();
+  return !isArrayType() && !isFunctionType() && !isAnyPointerType() &&
+         !isMemberPointerType();
 }
 
 /// Determines whether this type is written as a typedef-name.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 50f92c496a539a..9284ccb3eb7715 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -671,11 +671,12 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E) {
 
   // We don't want to throw lvalue-to-rvalue casts on top of
   // expressions of certain types in C++.
-  if (getLangOpts().CPlusPlus &&
-      (E->getType() == Context.OverloadTy ||
-       T->isDependentType() ||
-       T->isRecordType()))
-    return E;
+  if (getLangOpts().CPlusPlus) {
+    if (T == Context.OverloadTy || T->isRecordType() ||
+        (T->isDependentType() && !T->isAnyPointerType() &&
+         !T->isMemberPointerType()))
+      return E;
+  }
 
   // The C standard is actually really unclear on this point, and
   // DR106 tells us what the result should be but not why.  It's
@@ -11116,7 +11117,7 @@ static bool checkArithmeticIncompletePointerType(Sema &S, SourceLocation Loc,
   if (const AtomicType *ResAtomicType = ResType->getAs<AtomicType>())
     ResType = ResAtomicType->getValueType();
 
-  assert(ResType->isAnyPointerType() && !ResType->isDependentType());
+  assert(ResType->isAnyPointerType());
   QualType PointeeTy = ResType->getPointeeType();
   return S.RequireCompleteSizedType(
       Loc, PointeeTy,
@@ -14287,7 +14288,9 @@ static QualType CheckIncrementDecrementOperand(Sema &S, Expr *Op,
                                                ExprObjectKind &OK,
                                                SourceLocation OpLoc,
                                                bool IsInc, bool IsPrefix) {
-  if (Op->isTypeDependent())
+  if (Op->isTypeDependent() &&
+      (Op->hasPlaceholderType() ||
+       Op->getType()->isSpecificBuiltinType(BuiltinType::Dependent)))
     return S.Context.DependentTy;
 
   QualType ResType = Op->getType();
@@ -14725,7 +14728,9 @@ static void RecordModifiableNonNullParam(Sema &S, const Expr *Exp) {
 static QualType CheckIndirectionOperand(Sema &S, Expr *Op, ExprValueKind &VK,
                                         SourceLocation OpLoc,
                                         bool IsAfterAmp = false) {
-  if (Op->isTypeDependent())
+  if (Op->isTypeDependent() &&
+      (Op->hasPlaceholderType() ||
+       Op->getType()->isSpecificBuiltinType(BuiltinType::Dependent)))
     return S.Context.DependentTy;
 
   ExprResult ConvResult = S.UsualUnaryConversions(Op);
diff --git a/clang/test/AST/ast-dump-expr-json.cpp b/clang/test/AST/ast-dump-expr-json.cpp
index 0fb07b0b434cc3..4b7365e554cb7c 100644
--- a/clang/test/AST/ast-dump-expr-json.cpp
+++ b/clang/test/AST/ast-dump-expr-json.cpp
@@ -4261,9 +4261,9 @@ void TestNonADLCall3() {
 // CHECK-NEXT:                     }
 // CHECK-NEXT:                    },
 // CHECK-NEXT:                    "type": {
-// CHECK-NEXT:                     "qualType": "<dependent type>"
+// CHECK-NEXT:                     "qualType": "V"
 // CHECK-NEXT:                    },
-// CHECK-NEXT:                    "valueCategory": "prvalue",
+// CHECK-NEXT:                    "valueCategory": "lvalue",
 // CHECK-NEXT:                    "isPostfix": false,
 // CHECK-NEXT:                    "opcode": "*",
 // CHECK-NEXT:                    "canOverflow": false,
diff --git a/clang/test/AST/ast-dump-expr.cpp b/clang/test/AST/ast-dump-expr.cpp
index 69e65e22d61d0d..4df5ba4276abd2 100644
--- a/clang/test/AST/ast-dump-expr.cpp
+++ b/clang/test/AST/ast-dump-expr.cpp
@@ -282,7 +282,7 @@ void PrimaryExpressions(Ts... a) {
       // CHECK-NEXT: CompoundStmt
       // CHECK-NEXT: FieldDecl 0x{{[^ ]*}} <col:8> col:8 implicit 'V'
       // CHECK-NEXT: ParenListExpr 0x{{[^ ]*}} <col:8> 'NULL TYPE'
-      // CHECK-NEXT: UnaryOperator 0x{{[^ ]*}} <col:8> '<dependent type>' prefix '*' cannot overflow
+      // CHECK-NEXT: UnaryOperator 0x{{[^ ]*}} <col:8> 'V' lvalue prefix '*' cannot overflow
       // CHECK-NEXT: CXXThisExpr 0x{{[^ ]*}} <col:8> 'V *' this
     }
   };
diff --git a/clang/test/AST/ast-dump-lambda.cpp b/clang/test/AST/ast-dump-lambda.cpp
index ef8789cd97d3e7..a4d3fe4fbda57c 100644
--- a/clang/test/AST/ast-dump-lambda.cpp
+++ b/clang/test/AST/ast-dump-lambda.cpp
@@ -81,7 +81,7 @@ template <typename... Ts> void test(Ts... a) {
 // CHECK-NEXT:    |         | | `-CompoundStmt {{.*}} <col:15, col:16>
 // CHECK-NEXT:    |         | `-FieldDecl {{.*}} <col:8> col:8{{( imported)?}} implicit 'V'
 // CHECK-NEXT:    |         |-ParenListExpr {{.*}} <col:8> 'NULL TYPE'
-// CHECK-NEXT:    |         | `-UnaryOperator {{.*}} <col:8> '<dependent type>' prefix '*' cannot overflow
+// CHECK-NEXT:    |         | `-UnaryOperator {{.*}} <col:8> 'V' lvalue prefix '*' cannot overflow
 // CHECK-NEXT:    |         |   `-CXXThisExpr {{.*}} <col:8> 'V *' this
 // CHECK-NEXT:    |         `-CompoundStmt {{.*}} <col:15, col:16>
 // CHECK-NEXT:    |-DeclStmt {{.*}} <line:22:3, col:11>
diff --git a/clang/test/CXX/over/over.built/ast.cpp b/clang/test/CXX/over/over.built/ast.cpp
index 56a63431269f30..b95a453de90222 100644
--- a/clang/test/CXX/over/over.built/ast.cpp
+++ b/clang/test/CXX/over/over.built/ast.cpp
@@ -4,15 +4,17 @@ struct A{};
 
 template <typename T, typename U>
 auto Test(T* pt, U* pu) {
-  // CHECK: UnaryOperator {{.*}} '<dependent type>' lvalue prefix '*'
+  // CHECK: UnaryOperator {{.*}} 'T' lvalue prefix '*'
+  // CHECK-NEXT: ImplicitCastExpr {{.*}} 'T *' <LValueToRValue>
   // CHECK-NEXT: DeclRefExpr {{.*}} 'T *' lvalue ParmVar {{.*}} 'pt' 'T *'
   (void)*pt;
 
-  // CHECK: UnaryOperator {{.*}} '<dependent type>' lvalue prefix '++'
+  // CHECK: UnaryOperator {{.*}} 'T *' lvalue prefix '++'
   // CHECK-NEXT: DeclRefExpr {{.*}} 'T *' lvalue ParmVar {{.*}} 'pt' 'T *'
   (void)(++pt);
 
-  // CHECK: UnaryOperator {{.*}} '<dependent type>' prefix '+'
+  // CHECK: UnaryOperator {{.*}} 'T *' prefix '+'
+  // CHECK-NEXT: ImplicitCastExpr {{.*}} 'T *' <LValueToRValue>
   // CHECK-NEXT: DeclRefExpr {{.*}} 'T *' lvalue ParmVar {{.*}} 'pt' 'T *'
   (void)(+pt);
 
diff --git a/clang/test/Frontend/noderef_templates.cpp b/clang/test/Frontend/noderef_templates.cpp
index 5fde6efd87c7fb..9e54cd5d788992 100644
--- a/clang/test/Frontend/noderef_templates.cpp
+++ b/clang/test/Frontend/noderef_templates.cpp
@@ -3,8 +3,8 @@
 #define NODEREF __attribute__((noderef))
 
 template <typename T>
-int func(T NODEREF *a) { // expected-note 2 {{a declared here}}
-  return *a + 1;         // expected-warning 2 {{dereferencing a; was declared with a 'noderef' type}}
+int func(T NODEREF *a) { // expected-note 3 {{a declared here}}
+  return *a + 1;         // expected-warning 3 {{dereferencing a; was declared with a 'noderef' type}}
 }
 
 void func() {
diff --git a/clang/test/SemaCXX/cxx2b-deducing-this.cpp b/clang/test/SemaCXX/cxx2b-deducing-this.cpp
index 5f29a955e053c3..aa64530bd5be3d 100644
--- a/clang/test/SemaCXX/cxx2b-deducing-this.cpp
+++ b/clang/test/SemaCXX/cxx2b-deducing-this.cpp
@@ -19,7 +19,7 @@ struct S {
     // new and delete are implicitly static
     void *operator new(this unsigned long); // expected-error{{an explicit object parameter cannot appear in a static function}}
     void operator delete(this void*); // expected-error{{an explicit object parameter cannot appear in a static function}}
-    
+
     void g(this auto) const; // expected-error{{explicit object member function cannot have 'const' qualifier}}
     void h(this auto) &; // expected-error{{explicit object member function cannot have '&' qualifier}}
     void i(this auto) &&; // expected-error{{explicit object member function cannot have '&&' qualifier}}
@@ -198,9 +198,7 @@ void func(int i) {
 void TestMutationInLambda() {
     [i = 0](this auto &&){ i++; }();
     [i = 0](this auto){ i++; }();
-    [i = 0](this const auto&){ i++; }();
-    // expected-error@-1 {{cannot assign to a variable captured by copy in a non-mutable lambda}}
-    // expected-note@-2 {{in instantiation of}}
+    [i = 0](this const auto&){ i++; }(); // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
 
     int x;
     const auto l1 = [x](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
diff --git a/clang/test/SemaTemplate/class-template-spec.cpp b/clang/test/SemaTemplate/class-template-spec.cpp
index 56b8207bd9a435..faa54c36753831 100644
--- a/clang/test/SemaTemplate/class-template-spec.cpp
+++ b/clang/test/SemaTemplate/class-template-spec.cpp
@@ -18,7 +18,7 @@ int test_specs(A<float, float> *a1, A<float, int> *a2) {
   return a1->x + a2->y;
 }
 
-int test_incomplete_specs(A<double, double> *a1, 
+int test_incomplete_specs(A<double, double> *a1,
                           A<double> *a2)
 {
   (void)a1->x; // expected-error{{member access into incomplete type}}
@@ -39,7 +39,7 @@ template <> struct X<int, int> { int foo(); }; // #1
 template <> struct X<float> { int bar(); };  // #2
 
 typedef int int_type;
-void testme(X<int_type> *x1, X<float, int> *x2) { 
+void testme(X<int_type> *x1, X<float, int> *x2) {
   (void)x1->foo(); // okay: refers to #1
   (void)x2->bar(); // okay: refers to #2
 }
@@ -53,7 +53,7 @@ struct A<char> {
 A<char>::A() { }
 
 // Make sure we can see specializations defined before the primary template.
-namespace N{ 
+namespace N{
   template<typename T> struct A0;
 }
 
@@ -97,7 +97,7 @@ namespace M {
   template<> struct ::A<long double>; // expected-error{{must occur at global scope}}
 }
 
-template<> struct N::B<char> { 
+template<> struct N::B<char> {
   int testf(int x) { return f(x); }
 };
 
@@ -138,9 +138,9 @@ namespace PR18009 {
 
   template <typename T> struct C {
     template <int N, int M> struct S;
-    template <int N> struct S<N, N ? **(T(*)[N])0 : 0> {}; // expected-error {{depends on a template parameter of the partial specialization}}
+    template <int N> struct S<N, N ? **(T(*)[N])0 : 0> {}; // ok
   };
-  C<int> c; // expected-note {{in instantiation of}}
+  C<int> c;
 
   template<int A> struct outer {
     template<int B, int C> struct inner {};

erichkeane

This needs a release note, likely in the 'potentially breaking changes'. Projects are filled with code that is not ever instantiated, and Clang's aggressive instantiation can be a problem.

Else, LGTM.

shafik

So this seems to apply to member pointer types, pointer types, arrays and function types but I don't see coverage for the full set of paths in the tests. Can we please add more testing.

Erich also expressed some concern about breaking existing code due to eager instantiation. Can we make sure try to cover any change in behavior in the tests so reviewers can get a better idea of impact.

sdkrystian · 2024-05-03T14:00:08Z

@erichkeane Release note added

erichkeane

I'm still OK with this, but @shafik : Please ensure this has the test coverage you asked for.

sdkrystian · 2024-05-03T14:02:40Z

Yup, working on it :)

sdkrystian · 2024-05-03T19:49:39Z

@shafik More tests added

sdkrystian · 2024-05-08T10:01:48Z

Ping @shafik

sdkrystian · 2024-05-14T15:03:57Z

@erichkeane Since I addressed @shafik's comments, it this good to be merged?

shafik

Thank you for the additional test coverage, LGTM

erichkeane · 2024-05-14T15:43:49Z

@erichkeane Since I addressed @shafik's comments, it this good to be merged?

Yep, go for it!

sdkrystian · 2024-05-14T17:12:57Z

This seems to break something in LLVM... investigating

erichkeane · 2024-05-14T17:14:38Z

This is unfortunately one of the cases where if you can't fix it 'quickly', a revert is necessary, as it breaks the build.

…rs (llvm#90500)" This reverts commit 8019cbb.

…rs (#90500)" (#92149) This reverts commit 8019cbb.

sdkrystian · 2024-05-14T17:24:42Z

Reverted in #92149

MaskRay · 2024-05-14T18:23:26Z

This seems to break something in LLVM... investigating

I've also noticed a stage-2-build issue.

      llvm::any_of(LHS->users(),
                    [&](auto *U) {
                      return U != I &&
                             !(U->hasOneUser() && *U->users().begin() == I);
                    }) ||

llvm/lib/Transforms/Scalar/NaryReassociate.cpp:610:32: error: comparison of distinct pointer types ('auto *' and 'Instruction *')
  610 |                       return U != I &&
      |                              ~ ^  ~

…ors (llvm#90500)"

sdkrystian · 2024-05-14T19:58:23Z

Well this took forever to reduce:

template<typename T>
struct A;

template<typename T>
bool operator==(const A<T>&, const A<T>&);

template<typename>
void f(int *x)
{
    [&](auto *y) { return x == y; };
}

void g()
{
    f<int>(nullptr);
}

We initially build a CXXOperatorCallExpr during parsing for the comparison, but TreeTransform prematurely rebuilds it as a builtin binary operator when f is instantiated.

erichkeane · 2024-05-14T20:01:12Z

Well this took forever to reduce:
template<typename T>
struct A;

template<typename T>
bool operator==(const A<T>&, const A<T>&);

template<typename>
void f(int *x)
{
    [&](auto *y) { return x == y; };
}

void g()
{
    f<int>(nullptr);
}
We initially build a CXXOperatorCallExpr during parsing for the comparison, but TreeTransform prematurely rebuilds it as a builtin binary operator when f is instantiated.

Yeah, unfortunately that is a limitation of Clang, we still don't correctly implement the instantiation of Lambdas. We do so 'greedily', in a way that the standard doesn't allow. We eventually need to delay ALL instantiation until the actual first full instantiation of the lambda.

That said, in this case I'm surprised that we don't skip that diagnostic based on the RHS (in your repro) being dependent. THAT would probably fix this case at least.

sdkrystian · 2024-05-14T20:03:25Z

The issue seems to be that TreeTransform::RebuildCXXOperatorCallExpr relies on isOverloadableType to always be true for dependent types

…ors (#90500)" (#92283) This patch reapplies #90500, addressing a bug which caused binary operators with dependent operands to be incorrectly rebuilt by `TreeTransform`.

shafik · 2024-07-10T01:52:01Z

You reapplied this here: 1595988

but it has caused a regression: #97646

…y operators (llvm#90500)" (llvm#92283)" This reverts commit 1595988. rdar://131028403 Conflicts: clang/docs/ReleaseNotes.rst

sdkrystian requested review from cor3ntin, Sirraide and erichkeane April 29, 2024 17:40

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 29, 2024

sdkrystian force-pushed the overloadable branch from f292ba9 to 6a2d371 Compare April 29, 2024 17:42

erichkeane approved these changes Apr 29, 2024

View reviewed changes

shafik reviewed Apr 29, 2024

View reviewed changes

sdkrystian force-pushed the overloadable branch from 6a2d371 to 1b3476d Compare May 3, 2024 13:45

erichkeane reviewed May 3, 2024

View reviewed changes

sdkrystian force-pushed the overloadable branch from 953bd3c to c7254a3 Compare May 3, 2024 14:02

sdkrystian force-pushed the overloadable branch from c7254a3 to 249658f Compare May 3, 2024 18:54

sdkrystian requested a review from shafik May 3, 2024 19:49

sdkrystian added 4 commits May 6, 2024 09:09

[Clang][Sema] Earlier type checking for builtin unary operators

68ae8a9

[FOLD] update tests after llvm#90152

6cf946e

[FOLD] add release note

01241d9

[FOLD] more tests & support for more operators

196359f

sdkrystian force-pushed the overloadable branch from 249658f to 196359f Compare May 6, 2024 13:10

shafik approved these changes May 14, 2024

View reviewed changes

sdkrystian merged commit 8019cbb into llvm:main May 14, 2024
5 checks passed

sdkrystian added a commit to sdkrystian/llvm-project that referenced this pull request May 14, 2024

Revert "[Clang][Sema] Earlier type checking for builtin unary operato…

55a5910

…rs (llvm#90500)" This reverts commit 8019cbb.

sdkrystian added a commit that referenced this pull request May 14, 2024

Revert "[Clang][Sema] Earlier type checking for builtin unary operato…

97e35e0

…rs (#90500)" (#92149) This reverts commit 8019cbb.

sdkrystian added a commit to sdkrystian/llvm-project that referenced this pull request May 14, 2024

Reapply "[Clang][Sema] Earlier type checking for builtin unary operat…

365d975

…ors (llvm#90500)"

sdkrystian mentioned this pull request May 15, 2024

Reapply "[Clang][Sema] Earlier type checking for builtin unary operators (#90500)" #92283

Merged

dcci mentioned this pull request May 24, 2024

error: invalid argument type 'T *' to unary expression #93331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][Sema] Earlier type checking for builtin unary operators #90500

[Clang][Sema] Earlier type checking for builtin unary operators #90500

sdkrystian commented Apr 29, 2024

llvmbot commented Apr 29, 2024

erichkeane left a comment

shafik left a comment

sdkrystian commented May 3, 2024

erichkeane left a comment

sdkrystian commented May 3, 2024

sdkrystian commented May 3, 2024

sdkrystian commented May 8, 2024

sdkrystian commented May 14, 2024

shafik left a comment

erichkeane commented May 14, 2024

sdkrystian commented May 14, 2024

erichkeane commented May 14, 2024

sdkrystian commented May 14, 2024

MaskRay commented May 14, 2024

sdkrystian commented May 14, 2024

erichkeane commented May 14, 2024

sdkrystian commented May 14, 2024

shafik commented Jul 10, 2024

[Clang][Sema] Earlier type checking for builtin unary operators #90500

[Clang][Sema] Earlier type checking for builtin unary operators #90500

Conversation

sdkrystian commented Apr 29, 2024

llvmbot commented Apr 29, 2024

erichkeane left a comment

Choose a reason for hiding this comment

shafik left a comment

Choose a reason for hiding this comment

sdkrystian commented May 3, 2024

erichkeane left a comment

Choose a reason for hiding this comment

sdkrystian commented May 3, 2024

sdkrystian commented May 3, 2024

sdkrystian commented May 8, 2024

sdkrystian commented May 14, 2024

shafik left a comment

Choose a reason for hiding this comment

erichkeane commented May 14, 2024

sdkrystian commented May 14, 2024

erichkeane commented May 14, 2024

sdkrystian commented May 14, 2024

MaskRay commented May 14, 2024

sdkrystian commented May 14, 2024

erichkeane commented May 14, 2024

sdkrystian commented May 14, 2024

shafik commented Jul 10, 2024