Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reapply "[Clang][Sema] Diagnose class member access expressions naming non-existent members of the current instantiation prior to instantiation in the absence of dependent base classes (#84050)" #90152

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

sdkrystian
Copy link
Member

Reapplies #84050, addressing a bug which cases a crash when an expression with the type of the current instantiation is used as the postfix-expression in a class member access expression (arrow form).

@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tools-extra clangd clang-tidy clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support clang:openmp OpenMP related changes to Clang labels Apr 26, 2024
@sdkrystian sdkrystian removed the HLSL HLSL Language Support label Apr 26, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 26, 2024

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-clangd
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-tidy

Author: Krystian Stasiowski (sdkrystian)

Changes

Reapplies #84050, addressing a bug which cases a crash when an expression with the type of the current instantiation is used as the postfix-expression in a class member access expression (arrow form).


Patch is 68.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/90152.diff

30 Files Affected:

  • (modified) clang-tools-extra/clangd/unittests/FindTargetTests.cpp (+4-4)
  • (modified) clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp (+1-1)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp (+2)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default-copy.cpp (+12)
  • (modified) clang/docs/ReleaseNotes.rst (+12)
  • (modified) clang/include/clang/Sema/Lookup.h (+3-1)
  • (modified) clang/include/clang/Sema/Sema.h (+8-6)
  • (modified) clang/lib/AST/Expr.cpp (+1-1)
  • (modified) clang/lib/Parse/ParseDecl.cpp (+1-1)
  • (modified) clang/lib/Sema/HLSLExternalSemaSource.cpp (+5-2)
  • (modified) clang/lib/Sema/SemaAttr.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+4-3)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+3-3)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+10-10)
  • (modified) clang/lib/Sema/SemaExprCXX.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaExprMember.cpp (+69-94)
  • (modified) clang/lib/Sema/SemaLookup.cpp (+89-25)
  • (modified) clang/lib/Sema/SemaOpenMP.cpp (+12-5)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+12-20)
  • (modified) clang/lib/Sema/TreeTransform.h (+20)
  • (modified) clang/test/AST/HLSL/this-reference-template.hlsl (+1-1)
  • (modified) clang/test/CXX/drs/dr2xx.cpp (+5-5)
  • (modified) clang/test/CXX/drs/dr3xx.cpp (+8-8)
  • (added) clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp (+456)
  • (modified) clang/test/CXX/temp/temp.res/temp.local/p3.cpp (+1-2)
  • (modified) clang/test/CodeGenCXX/mangle.cpp (-8)
  • (modified) clang/test/Index/annotate-nested-name-specifier.cpp (+2-2)
  • (modified) clang/test/SemaCXX/member-expr.cpp (+2-2)
  • (modified) clang/test/SemaTemplate/instantiate-function-1.cpp (+7-7)
  • (modified) clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (+3-2)
diff --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index 799a549ff0816e..94437857cecca6 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -854,7 +854,7 @@ TEST_F(TargetDeclTest, DependentExprs) {
           }
         };
       )cpp";
-  EXPECT_DECLS("CXXDependentScopeMemberExpr", "void foo()");
+  EXPECT_DECLS("MemberExpr", "void foo()");
 
   // Similar to above but base expression involves a function call.
   Code = R"cpp(
@@ -872,7 +872,7 @@ TEST_F(TargetDeclTest, DependentExprs) {
           }
         };
       )cpp";
-  EXPECT_DECLS("CXXDependentScopeMemberExpr", "void foo()");
+  EXPECT_DECLS("MemberExpr", "void foo()");
 
   // Similar to above but uses a function pointer.
   Code = R"cpp(
@@ -891,7 +891,7 @@ TEST_F(TargetDeclTest, DependentExprs) {
           }
         };
       )cpp";
-  EXPECT_DECLS("CXXDependentScopeMemberExpr", "void foo()");
+  EXPECT_DECLS("MemberExpr", "void foo()");
 
   // Base expression involves a member access into this.
   Code = R"cpp(
@@ -962,7 +962,7 @@ TEST_F(TargetDeclTest, DependentExprs) {
           void Foo() { this->[[find]](); }
         };
   )cpp";
-  EXPECT_DECLS("CXXDependentScopeMemberExpr", "void find()");
+  EXPECT_DECLS("MemberExpr", "void find()");
 }
 
 TEST_F(TargetDeclTest, DependentTypes) {
diff --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
index 4156921d83edf8..30b9b1902aa9c7 100644
--- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -621,7 +621,7 @@ sizeof...($TemplateParameter[[Elements]]);
       struct $Class_def[[Foo]] {
         int $Field_decl[[Waldo]];
         void $Method_def[[bar]]() {
-          $Class[[Foo]]().$Field_dependentName[[Waldo]];
+          $Class[[Foo]]().$Field[[Waldo]];
         }
         template $Bracket[[<]]typename $TemplateParameter_def[[U]]$Bracket[[>]]
         void $Method_def[[bar1]]() {
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp
index 574efe7bd91478..ae61b17ca14d20 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp
@@ -309,6 +309,8 @@ struct HeapArray {                                          // Ok, since destruc
 
   HeapArray(HeapArray &&other) : _data(other._data), size(other.size) { // Ok
     other._data = nullptr;                                              // Ok
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: expected assignment source to be of type 'gsl::owner<>'; got 'std::nullptr_t'
+    // FIXME: This warning is emitted because an ImplicitCastExpr for the NullToPointer conversion isn't created for dependent types.
     other.size = 0;
   }
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default-copy.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default-copy.cpp
index 559031cf4d9bda..4abb9c8555970e 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default-copy.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-equals-default-copy.cpp
@@ -260,6 +260,8 @@ template <class T>
 struct Template {
   Template() = default;
   Template(const Template &Other) : Field(Other.Field) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
+  // CHECK-FIXES: Template(const Template &Other)  = default;
   Template &operator=(const Template &Other);
   void foo(const T &t);
   int Field;
@@ -269,8 +271,12 @@ Template<T> &Template<T>::operator=(const Template<T> &Other) {
   Field = Other.Field;
   return *this;
 }
+// CHECK-MESSAGES: :[[@LINE-4]]:27: warning: use '= default'
+// CHECK-FIXES: Template<T> &Template<T>::operator=(const Template<T> &Other) = default;
+
 Template<int> T1;
 
+
 // Dependent types.
 template <class T>
 struct DT1 {
@@ -284,6 +290,9 @@ DT1<T> &DT1<T>::operator=(const DT1<T> &Other) {
   Field = Other.Field;
   return *this;
 }
+// CHECK-MESSAGES: :[[@LINE-4]]:17: warning: use '= default'
+// CHECK-FIXES: DT1<T> &DT1<T>::operator=(const DT1<T> &Other) = default;
+
 DT1<int> Dt1;
 
 template <class T>
@@ -303,6 +312,9 @@ DT2<T> &DT2<T>::operator=(const DT2<T> &Other) {
 struct T {
   typedef int TT;
 };
+// CHECK-MESSAGES: :[[@LINE-8]]:17: warning: use '= default'
+// CHECK-FIXES: DT2<T> &DT2<T>::operator=(const DT2<T> &Other) = default;
+
 DT2<T> Dt2;
 
 // Default arguments.
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f5e5d3a2e6ea36..00c684e773a2e0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -385,6 +385,18 @@ Improvements to Clang's diagnostics
 
 - Clang now diagnoses requires expressions with explicit object parameters.
 
+- Clang now looks up members of the current instantiation in the template definition context
+  if the current instantiation has no dependent base classes.
+
+  .. code-block:: c++
+
+     template<typename T>
+     struct A {
+       int f() {
+         return this->x; // error: no member named 'x' in 'A<T>'
+       }
+     };
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/Sema/Lookup.h b/clang/include/clang/Sema/Lookup.h
index 0db5b847038ffd..b0a08a05ac6a0a 100644
--- a/clang/include/clang/Sema/Lookup.h
+++ b/clang/include/clang/Sema/Lookup.h
@@ -499,7 +499,9 @@ class LookupResult {
   /// Note that while no result was found in the current instantiation,
   /// there were dependent base classes that could not be searched.
   void setNotFoundInCurrentInstantiation() {
-    assert(ResultKind == NotFound && Decls.empty());
+    assert((ResultKind == NotFound ||
+            ResultKind == NotFoundInCurrentInstantiation) &&
+           Decls.empty());
     ResultKind = NotFoundInCurrentInstantiation;
   }
 
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 1ca523ec88c2f9..aa182b15e66ecc 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -7472,7 +7472,7 @@ class Sema final : public SemaBase {
   bool LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx,
                            CXXScopeSpec &SS);
   bool LookupParsedName(LookupResult &R, Scope *S, CXXScopeSpec *SS,
-                        bool AllowBuiltinCreation = false,
+                        QualType ObjectType, bool AllowBuiltinCreation = false,
                         bool EnteringContext = false);
   ObjCProtocolDecl *LookupProtocol(
       IdentifierInfo *II, SourceLocation IdLoc,
@@ -8881,11 +8881,13 @@ class Sema final : public SemaBase {
     /// functions (but no function templates).
     FoundFunctions,
   };
-  bool LookupTemplateName(
-      LookupResult &R, Scope *S, CXXScopeSpec &SS, QualType ObjectType,
-      bool EnteringContext, bool &MemberOfUnknownSpecialization,
-      RequiredTemplateKind RequiredTemplate = SourceLocation(),
-      AssumedTemplateKind *ATK = nullptr, bool AllowTypoCorrection = true);
+
+  bool
+  LookupTemplateName(LookupResult &R, Scope *S, CXXScopeSpec &SS,
+                     QualType ObjectType, bool EnteringContext,
+                     RequiredTemplateKind RequiredTemplate = SourceLocation(),
+                     AssumedTemplateKind *ATK = nullptr,
+                     bool AllowTypoCorrection = true);
 
   TemplateNameKind isTemplateName(Scope *S, CXXScopeSpec &SS,
                                   bool hasTemplateKeyword,
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 63dcdb919c7117..d2e40be59d6f3b 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -103,7 +103,7 @@ const Expr *Expr::skipRValueSubobjectAdjustments(
       }
     } else if (const auto *ME = dyn_cast<MemberExpr>(E)) {
       if (!ME->isArrow()) {
-        assert(ME->getBase()->getType()->isRecordType());
+        assert(ME->getBase()->getType()->getAsRecordDecl());
         if (const auto *Field = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
           if (!Field->isBitField() && !Field->getType()->isReferenceType()) {
             E = ME->getBase();
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 05ad5ecbfaa0cf..53a33fa4add54e 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -2998,7 +2998,7 @@ bool Parser::ParseImplicitInt(DeclSpec &DS, CXXScopeSpec *SS,
         << TokenName << TagName << getLangOpts().CPlusPlus
         << FixItHint::CreateInsertion(Tok.getLocation(), FixitTagName);
 
-      if (Actions.LookupParsedName(R, getCurScope(), SS)) {
+      if (Actions.LookupName(R, getCurScope())) {
         for (LookupResult::iterator I = R.begin(), IEnd = R.end();
              I != IEnd; ++I)
           Diag((*I)->getLocation(), diag::note_decl_hiding_tag_type)
diff --git a/clang/lib/Sema/HLSLExternalSemaSource.cpp b/clang/lib/Sema/HLSLExternalSemaSource.cpp
index 1a1febf7a35241..bb283c54b3d29c 100644
--- a/clang/lib/Sema/HLSLExternalSemaSource.cpp
+++ b/clang/lib/Sema/HLSLExternalSemaSource.cpp
@@ -126,12 +126,15 @@ struct BuiltinTypeDeclBuilder {
 
   static DeclRefExpr *lookupBuiltinFunction(ASTContext &AST, Sema &S,
                                             StringRef Name) {
-    CXXScopeSpec SS;
     IdentifierInfo &II = AST.Idents.get(Name, tok::TokenKind::identifier);
     DeclarationNameInfo NameInfo =
         DeclarationNameInfo(DeclarationName(&II), SourceLocation());
     LookupResult R(S, NameInfo, Sema::LookupOrdinaryName);
-    S.LookupParsedName(R, S.getCurScope(), &SS, false);
+    // AllowBuiltinCreation is false but LookupDirect will create
+    // the builtin when searching the global scope anyways...
+    S.LookupName(R, S.getCurScope());
+    // FIXME: If the builtin function was user-declared in global scope,
+    // this assert *will* fail. Should this call LookupBuiltin instead?
     assert(R.isSingleResult() &&
            "Since this is a builtin it should always resolve!");
     auto *VD = cast<ValueDecl>(R.getFoundDecl());
diff --git a/clang/lib/Sema/SemaAttr.cpp b/clang/lib/Sema/SemaAttr.cpp
index a5dd158808f26b..a83b1e8afadbc6 100644
--- a/clang/lib/Sema/SemaAttr.cpp
+++ b/clang/lib/Sema/SemaAttr.cpp
@@ -837,7 +837,7 @@ void Sema::ActOnPragmaUnused(const Token &IdTok, Scope *curScope,
 
   IdentifierInfo *Name = IdTok.getIdentifierInfo();
   LookupResult Lookup(*this, Name, IdTok.getLocation(), LookupOrdinaryName);
-  LookupParsedName(Lookup, curScope, nullptr, true);
+  LookupName(Lookup, curScope, /*AllowBuiltinCreation=*/true);
 
   if (Lookup.empty()) {
     Diag(PragmaLoc, diag::warn_pragma_unused_undeclared_var)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index e0745fe9a45367..4e275dc15fbb4e 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -832,7 +832,7 @@ static bool isTagTypeWithMissingTag(Sema &SemaRef, LookupResult &Result,
                                     IdentifierInfo *&Name,
                                     SourceLocation NameLoc) {
   LookupResult R(SemaRef, Name, NameLoc, Sema::LookupTagName);
-  SemaRef.LookupParsedName(R, S, &SS);
+  SemaRef.LookupParsedName(R, S, &SS, /*ObjectType=*/QualType());
   if (TagDecl *Tag = R.getAsSingle<TagDecl>()) {
     StringRef FixItTagName;
     switch (Tag->getTagKind()) {
@@ -869,7 +869,7 @@ static bool isTagTypeWithMissingTag(Sema &SemaRef, LookupResult &Result,
 
     // Replace lookup results with just the tag decl.
     Result.clear(Sema::LookupTagName);
-    SemaRef.LookupParsedName(Result, S, &SS);
+    SemaRef.LookupParsedName(Result, S, &SS, /*ObjectType=*/QualType());
     return true;
   }
 
@@ -896,7 +896,8 @@ Sema::NameClassification Sema::ClassifyName(Scope *S, CXXScopeSpec &SS,
   }
 
   LookupResult Result(*this, Name, NameLoc, LookupOrdinaryName);
-  LookupParsedName(Result, S, &SS, !CurMethod);
+  LookupParsedName(Result, S, &SS, /*ObjectType=*/QualType(),
+                   /*AllowBuiltinCreation=*/!CurMethod);
 
   if (SS.isInvalid())
     return NameClassification::Error();
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index abdbc9d8830c03..4d5836720a651f 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -4517,7 +4517,7 @@ Sema::BuildMemInitializer(Decl *ConstructorD,
                               DS.getBeginLoc(), DS.getEllipsisLoc());
   } else {
     LookupResult R(*this, MemberOrBase, IdLoc, LookupOrdinaryName);
-    LookupParsedName(R, S, &SS);
+    LookupParsedName(R, S, &SS, /*ObjectType=*/QualType());
 
     TypeDecl *TyD = R.getAsSingle<TypeDecl>();
     if (!TyD) {
@@ -12262,7 +12262,7 @@ Decl *Sema::ActOnUsingDirective(Scope *S, SourceLocation UsingLoc,
 
   // Lookup namespace name.
   LookupResult R(*this, NamespcName, IdentLoc, LookupNamespaceName);
-  LookupParsedName(R, S, &SS);
+  LookupParsedName(R, S, &SS, /*ObjectType=*/QualType());
   if (R.isAmbiguous())
     return nullptr;
 
@@ -13721,7 +13721,7 @@ Decl *Sema::ActOnNamespaceAliasDef(Scope *S, SourceLocation NamespaceLoc,
 
   // Lookup the namespace name.
   LookupResult R(*this, Ident, IdentLoc, LookupNamespaceName);
-  LookupParsedName(R, S, &SS);
+  LookupParsedName(R, S, &SS, /*ObjectType=*/QualType());
 
   if (R.isAmbiguous())
     return nullptr;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 50f92c496a539a..0c37f43f75401b 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -673,8 +673,9 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E) {
   // expressions of certain types in C++.
   if (getLangOpts().CPlusPlus &&
       (E->getType() == Context.OverloadTy ||
-       T->isDependentType() ||
-       T->isRecordType()))
+       // FIXME: This is a hack! We want the lvalue-to-rvalue conversion applied
+       // to pointer types even if the pointee type is dependent.
+       (T->isDependentType() && !T->isPointerType()) || T->isRecordType()))
     return E;
 
   // The C standard is actually really unclear on this point, and
@@ -2751,8 +2752,8 @@ Sema::ActOnIdExpression(Scope *S, CXXScopeSpec &SS,
   if (isBoundsAttrContext() && !getLangOpts().CPlusPlus && S->isClassScope()) {
     // See if this is reference to a field of struct.
     LookupResult R(*this, NameInfo, LookupMemberName);
-    // LookupParsedName handles a name lookup from within anonymous struct.
-    if (LookupParsedName(R, S, &SS)) {
+    // LookupName handles a name lookup from within anonymous struct.
+    if (LookupName(R, S)) {
       if (auto *VD = dyn_cast<ValueDecl>(R.getFoundDecl())) {
         QualType type = VD->getType().getNonReferenceType();
         // This will eventually be translated into MemberExpr upon
@@ -2773,20 +2774,19 @@ Sema::ActOnIdExpression(Scope *S, CXXScopeSpec &SS,
     // lookup to determine that it was a template name in the first place. If
     // this becomes a performance hit, we can work harder to preserve those
     // results until we get here but it's likely not worth it.
-    bool MemberOfUnknownSpecialization;
     AssumedTemplateKind AssumedTemplate;
-    if (LookupTemplateName(R, S, SS, QualType(), /*EnteringContext=*/false,
-                           MemberOfUnknownSpecialization, TemplateKWLoc,
+    if (LookupTemplateName(R, S, SS, /*ObjectType=*/QualType(),
+                           /*EnteringContext=*/false, TemplateKWLoc,
                            &AssumedTemplate))
       return ExprError();
 
-    if (MemberOfUnknownSpecialization ||
-        (R.getResultKind() == LookupResult::NotFoundInCurrentInstantiation))
+    if (R.wasNotFoundInCurrentInstantiation())
       return ActOnDependentIdExpression(SS, TemplateKWLoc, NameInfo,
                                         IsAddressOfOperand, TemplateArgs);
   } else {
     bool IvarLookupFollowUp = II && !SS.isSet() && getCurMethodDecl();
-    LookupParsedName(R, S, &SS, !IvarLookupFollowUp);
+    LookupParsedName(R, S, &SS, /*ObjectType=*/QualType(),
+                     /*AllowBuiltinCreation=*/!IvarLookupFollowUp);
 
     // If the result might be in a dependent base class, this is a dependent
     // id-expression.
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 779a41620033dc..c1cb03e4ec7ae2 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -9157,7 +9157,7 @@ Sema::CheckMicrosoftIfExistsSymbol(Scope *S,
   // Do the redeclaration lookup in the current scope.
   LookupResult R(*this, TargetNameInfo, Sema::LookupAnyName,
                  RedeclarationKind::NotForRedeclaration);
-  LookupParsedName(R, S, &SS);
+  LookupParsedName(R, S, &SS, /*ObjectType=*/QualType());
   R.suppressDiagnostics();
 
   switch (R.getResultKind()) {
diff --git a/clang/lib/Sema/SemaExprMember.cpp b/clang/lib/Sema/SemaExprMember.cpp
index 6e30716b9ae436..87fa2518475d5a 100644
--- a/clang/lib/Sema/SemaExprMember.cpp
+++ b/clang/lib/Sema/SemaExprMember.cpp
@@ -667,8 +667,8 @@ namespace {
 // classes, one of its base classes.
 class RecordMemberExprValidatorCCC final : public CorrectionCandidateCallback {
 public:
-  explicit RecordMemberExprValidatorCCC(const RecordType *RTy)
-      : Record(RTy->getDecl()) {
+  explicit RecordMemberExprValidatorCCC(QualType RTy)
+      : Record(RTy->getAsRecordDecl()) {
     // Don't add bare keywords to the consumer since they will always fail
     // validation by virtue of not being associated with any decls.
     WantTypeSpecifiers = false;
@@ -713,58 +713,36 @@ class RecordMemberExprValidatorCCC final : public CorrectionCandidateCallback {
 }
 
 static bool LookupMemberExprInRecord(Sema &SemaRef, LookupResult &R,
-                                     Expr *BaseExpr,
-                                     const RecordType *RTy,
+                                     Expr *BaseExpr, QualType RTy,
                                      SourceLocation OpLoc, bool IsArrow,
                                      CXXScopeSpec &SS, bool HasTemplateArgs,
                                      SourceLocation TemplateKWLoc,
                                      TypoExpr *&TE) {
   SourceRange BaseRange = BaseExpr ? BaseExpr->getSourceRange() : SourceRange();
-  RecordDecl *RDecl = RTy->getDecl();
-  if (!SemaRef.isThisOutsideMemberFunctionBody(QualType(RTy, 0)) &&
-      SemaRef.RequireCompleteType(OpLoc, QualType(RTy, 0),
-                                  diag::err_typecheck_incomplete_tag,
-                                  BaseRange))
+  if (!RTy->isDependentType() &&
+      !SemaRef.isThisOutsideMemberFunctionBody(RTy) &&
+      SemaRef.RequireCompleteType(
+          OpLoc, RTy, diag::err_typecheck_incomplete_tag, BaseRange))
     return true;
 
-  if (HasTemplateArgs || TemplateKWLoc.isValid()) {
-    // LookupTemplateName doesn't expect these both to exist simultaneously.
-    QualType ObjectType = SS.isSet() ? QualType() : QualType(RTy, 0);
+  // LookupTemplateName/LookupParsedName don't expect these both to exist
+  // simultaneously.
+  QualType ObjectType = SS.isSet() ? QualType() : RTy;
+  if (HasTemplateArgs || TemplateKWLoc.isValid())
+    return SemaRef.LookupTemplateName(R,
+                                      /*S=*/nullptr, SS, ObjectType,
+                                      /*EnteringContext=*/false, TemplateKWLoc);
 
-    bool MOUS;
-    return SemaRef.LookupTemplateName(R, nullptr, SS, ObjectType, false, MOUS,
-                                      TemplateKWLoc);
-  }
-
-  DeclContext *DC = RDecl;
-  if (SS.isSet()) {
-    // If the member name was a qualified-id, look into the
-    // nested-name-specifier.
-    DC = SemaRef.computeDeclContext(SS, false);
-
-    if (SemaRef.RequireCompleteDeclContext(SS, DC)) {
-      SemaRef.Diag(SS.getRange().getEnd(), diag::err_typecheck_incomplete_tag)
-          << SS.getRange() << DC;
-      return true;
-    }
-
-    assert(DC && "Cannot handle non-computable dependent contexts in lookup");
-
-    if (!isa<TypeDecl>(DC)) {
-      SemaRef.Diag(R.getNameLoc(), diag::err_qualified_member_nonclass)
-          << DC << SS.getRange();
-      return true;
-    }
-  }
+  SemaRef.LookupParsedName(R, /*S=*/nullp...
[truncated]

@sdkrystian
Copy link
Member Author

Will add tests tomorrow...

@llvmbot llvmbot added the HLSL HLSL Language Support label Apr 26, 2024
@sdkrystian sdkrystian force-pushed the reapply-mem-expr-curr-inst branch 5 times, most recently from da7472e to b8fda81 Compare April 26, 2024 16:02
@erichkeane
Copy link
Collaborator

Please also identify the changes you made vs the previous patch.

@sdkrystian
Copy link
Member Author

sdkrystian commented Apr 26, 2024

@erichkeane I placed all the new changes into a single commit (unless you mean "reference the changes since the previous patch in the commit message", which I can do).

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM, please add the tests, and I'll do another run at this.

@sdkrystian
Copy link
Member Author

sdkrystian commented Apr 26, 2024

Tests have already been added :) @erichkeane (see tests added by b8fda81)

Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to Sema.h and DR tests look good to me.

…g non-existent members of the current instantiation prior to instantiation in the absence of dependent base classes (llvm#84050)"

Consider the following:
```cpp
template<typename T>
struct A
{
    auto f()
    {
        return this->x;
    }
};
```
Although `A` has no dependent base classes and the lookup context for
`x` is the current instantiation, we currently do not diagnose the
absence of a member `x` until `A<T>::f` is instantiated. This patch
moves the point of diagnosis for such expressions to occur at the point
of definition (i.e. prior to instantiation).
@sdkrystian
Copy link
Member Author

@alexfh see #91503

@dyung
Copy link
Collaborator

dyung commented May 9, 2024

Hi @sdkrystian, we were fixing up some internal codebases which this change exposed problems with but noticed that this slightly tweaked example of one doesn't seem to trigger an error even though it seems like it should with your change:

enum BK {};
template <class > class COneSet {
  typedef COneSet self;
  void Swap();
  BK m_bk;
};
template <class T> void COneSet<T>::Swap() {
  self pSetOther;
  SWAP(pSetOther); // Still calls the undeclared function SWAP
}

sdkrystian added a commit that referenced this pull request May 9, 2024
… from #91498, #90999, and #90152 (#91620)

This reverts changes in #91498, #90999, and #90152 which make
`operator=` dependent whenever the current class is templated.
@sdkrystian
Copy link
Member Author

sdkrystian commented May 9, 2024

@dyung Per [temp.dep.type] p10.11:

A type is dependent if it is

  • [...]
  • denoted by a simple-template-id in which either the template name is a template parameter or any of the template arguments is a dependent type or an expression that is type-dependent or value-dependent or is a pack expansion,
  • [...]

In this case, self is a typedef for COneSet (the injected-class-name of the class template). When used as a type-name, the injected-class-name of a class template is equivalent to a simple-template-id with the template arguments of the template parameters of that template. Despite that type being the current instantiation, it is still considered to be a dependent type (name lookup has special rules when the lookup context is dependent and is the current instantiation).

Since pSetOther names a declaration that has a dependent type, SWAP(pSetOther) is a dependent call per [temp.dep.general] p2.

@eaeltsin
Copy link
Contributor

Hi,

This seems to break the following combination with overloaded -> operator - https://godbolt.org/z/jc6chKTdv

    template <class T>
    class Clone {
      public:
        Clone(const Clone<T>&);
        T* operator->() const;
        T* ptr_;
    };

    // Assume T* T::clone()
    template <class T>
    inline Clone<T>::Clone(const Clone<T>& t)
    : ptr_(t->clone()) {}

    template <class T>
    inline T* Clone<T>::operator->() const {
        return ptr_;
    }

@ilya-biryukov
Copy link
Contributor

🤯 amazingly, this only happens when the identifier being called is a lowercase version of the class name:
https://gcc.godbolt.org/z/8aoaoPKnT:

template <class T>
class Clone {
   public:
    Clone(const Clone<T>&);

    T* operator->() const;
    T* ptr_;
};

template <class T>
struct Foo {
    Foo(const Foo<T>&);
    T* operator->() const;
    T* ptr_;
};

// Assume T* T::clone()
template <class T>
inline Clone<T>::Clone(const Clone<T>& t) {
    t->foo(); // ok 
    t->clone(); // error
    t->bar(); // ok
}

// Assume T* T::clone()
template <class T>
inline Foo<T>::Foo(const Foo<T>& t) {
    t->foo(); // error
    t->clone(); // ok
    t->bar(); // ok
}

I am looking forward to learning what is special about the matching names.

@sdkrystian
Copy link
Member Author

sdkrystian commented May 13, 2024

@ilya-biryukov Actually, that is an incredibly useful piece of information :) I think this is an issue with typo correction...

@sdkrystian
Copy link
Member Author

Repro:

template<typename T>
struct Typo {
  Typo(const Typo& t) {   
    t->typo; // error
    t->Typp; // error
    t->Tzpo; // error
    t->ty; // ok
  }
};

@ilya-biryukov
Copy link
Contributor

Heh, of course! Now that you say it, that's quite obvious.

Do you think is fix is just around the corner or will it take a long time?
This blocks our internal compiler release and there is no workaround we could easily employ.

@sdkrystian
Copy link
Member Author

@ilya-biryukov I can fix this quickly (less than an hour).

@ilya-biryukov
Copy link
Contributor

Awesome, thanks a lot for looking into it!

@sdkrystian
Copy link
Member Author

@ilya-biryukov See #91972

sdkrystian added a commit that referenced this pull request May 13, 2024
…nstantiation (#91972)

#90152 introduced a bug that occurs when typo-correction attempts to fix a reference to a
non-existent member of the current instantiation (even though
`operator->` may return a different type than the object type). This
patch fixes it by simply considering the object expression to be of type
`ASTContext::DependentTy` when the arrow operator is used with a
dependent non-pointer non-function operand (after any implicit
conversions).
@sdkrystian
Copy link
Member Author

@eaeltsin @ilya-biryukov Fixed in 596a9c1

@eaeltsin
Copy link
Contributor

Thanks for the quick fix!

sdkrystian added a commit that referenced this pull request May 14, 2024
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.
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Jul 11, 2024
…g non-existent members of the current instantiation prior to instantiation in the absence of dependent base classes (llvm#84050)" (llvm#90152)

Reapplies llvm#84050, addressing a bug which cases a crash when an
expression with the type of the current instantiation is used as the
_postfix-expression_ in a class member access expression (arrow form).

Change-Id: I17501886f0d2fc833749b4804f339bacbb85753c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category clang-tidy clang-tools-extra clangd HLSL HLSL Language Support
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet