[clang] Fix a bug with qualified name lookup into current instantiation #73018

Fznamznon · 2023-11-21T17:08:08Z

Due to d0d2ee0 clang doesn't perform qualified name lookup into the current instantiation when it has dependent bases, because of that getTypeName call always returns null for unknown specialization case. When there is a typename keyword, DependentNameType is constructed instead of simply returning null.
This change attempts to do the same in case of typename absence.

Fixes #13826

Due to d0d2ee0 clang doesn't perform qualified name lookup into the current instantiation when it has dependent bases, because of that `getTypeName` call always returns null for unknown specialization case. When there is a `typename` keyword, `DependentNameType` is constructed instead of simply returning null. This change attempts to do the same in case of `typename` absence. Fixes llvm#13826

llvmbot · 2023-11-21T17:10:54Z

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

Due to d0d2ee0 clang doesn't perform qualified name lookup into the current instantiation when it has dependent bases, because of that getTypeName call always returns null for unknown specialization case. When there is a typename keyword, DependentNameType is constructed instead of simply returning null.
This change attempts to do the same in case of typename absence.

Fixes #13826

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/lib/Sema/SemaDecl.cpp (+17-3)
(modified) clang/test/SemaTemplate/dependent-base-classes.cpp (+14)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 157afd9e8629152..09ceb591d06eab5 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -610,6 +610,9 @@ Bug Fixes in This Version
   inside a lambda. (`#61460 <https://github.com/llvm/llvm-project/issues/61460>`_)
 - Fix crash during instantiation of some class template specializations within class
   templates. Fixes (`#70375 <https://github.com/llvm/llvm-project/issues/70375>`_)
+- Fixed false positive error emitted by clang when performing qualified name
+  lookup and the current class instantiation has dependent bases.
+  Fixes (`#13826 <https://github.com/llvm/llvm-project/issues/13826>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 4e1857b931cc868..71003c1815c90e4 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -442,7 +442,6 @@ ParsedType Sema::getTypeName(const IdentifierInfo &II, SourceLocation NameLoc,
   UsingShadowDecl *FoundUsingShadow = nullptr;
   switch (Result.getResultKind()) {
   case LookupResult::NotFound:
-  case LookupResult::NotFoundInCurrentInstantiation:
     if (CorrectedII) {
       TypeNameValidatorCCC CCC(/*AllowInvalid=*/true, isClassName,
                                AllowDeducedTemplate);
@@ -482,8 +481,23 @@ ParsedType Sema::getTypeName(const IdentifierInfo &II, SourceLocation NameLoc,
         }
       }
     }
-    // If typo correction failed or was not performed, fall through
-    [[fallthrough]];
+    Result.suppressDiagnostics();
+    return nullptr;
+  case LookupResult::NotFoundInCurrentInstantiation: {
+    if (AllowImplicitTypename == ImplicitTypenameContext::Yes) {
+      QualType T;
+      T = Context.getDependentNameType(ElaboratedTypeKeyword::None,
+                                       SS->getScopeRep(), &II);
+      TypeLocBuilder TLB;
+      DependentNameTypeLoc TL = TLB.push<DependentNameTypeLoc>(T);
+      TL.setElaboratedKeywordLoc(SourceLocation());
+      TL.setQualifierLoc(SS->getWithLocInContext(Context));
+      TL.setNameLoc(NameLoc);
+      return CreateParsedType(T, TLB.getTypeSourceInfo(Context, T));
+    }
+    Result.suppressDiagnostics();
+    return nullptr;
+  }
   case LookupResult::FoundOverloaded:
   case LookupResult::FoundUnresolvedValue:
     Result.suppressDiagnostics();
diff --git a/clang/test/SemaTemplate/dependent-base-classes.cpp b/clang/test/SemaTemplate/dependent-base-classes.cpp
index 09f475f8bde9183..92a37efaa7e73f6 100644
--- a/clang/test/SemaTemplate/dependent-base-classes.cpp
+++ b/clang/test/SemaTemplate/dependent-base-classes.cpp
@@ -130,3 +130,17 @@ namespace PR5812 {
 
   Derived<int> di;
 }
+
+namespace GH13826 {
+template <typename T> struct A {
+  typedef int type;
+  struct B;
+};
+
+template <typename T> struct A<T>::B : A<T> {
+  B::type t;
+};
+
+A<int> a;
+A<int>::B b;
+}

erichkeane

The patch makes sense to me, though my knowledge of this function is limited. Please give @cor3ntin a chance to look at it too in case he sees something I don't.

clang/lib/Sema/SemaDecl.cpp

cor3ntin

That looks reasonable to me modulo nit and @shafik comments.
Thanks for fixing this!

cor3ntin · 2023-11-29T04:28:26Z

clang/lib/Sema/SemaDecl.cpp

+                                       SS->getScopeRep(), &II);
+      TypeLocBuilder TLB;
+      DependentNameTypeLoc TL = TLB.push<DependentNameTypeLoc>(T);
+      TL.setElaboratedKeywordLoc(SourceLocation());


I don't think that's actually needed even if we do it in a bunch of places.

Oh, we probably need this. Sanitizer build failed due to elaborated keyword loc not initialized https://lab.llvm.org/buildbot/#/builders/5/builds/38796/steps/2/logs/stdio .

Fznamznon · 2023-11-30T12:13:46Z

Fix for buildbot #73928
I'm not able to verify locally though

john-brawn-arm · 2023-12-01T11:47:56Z

As a result of this commit clang no longer gives an error for

template <int A> struct X {
  enum E { a };
};

template <int A> struct Y : X<A> {
  Y::E m;
};

where gcc (though not msvc) gives an error: https://godbolt.org/z/qGfnzhfsK

john-brawn-arm · 2023-12-01T16:53:48Z

I'm not completely sure if we should be giving an error for the example I gave above. Looking through the C++20 standard I see temp.res paragraph 2:

A name used in a template declaration or definition and that is dependent on a template-parameter is assumed
not to name a type unless the applicable name lookup finds a type name or the name is qualified by the
keyword typename

The error that gcc is giving is

<source>:6:6: error: invalid use of incomplete type 'struct Y<A>'
    6 |   Y::E m;
      |      ^
<source>:5:25: note: definition of 'struct Y<A>' is not complete until the closing brace
    5 | template <int A> struct Y : X<A> {
      |                         ^

so I'm guessing that gcc thinks that the name lookup for Y::E doesn't find a type name because Y is incomplete, though I don't understand why the same isn't true for the example in #13826

template <typename T> struct A {
  typedef int type;
  struct B;
};

template <typename T> struct A<T>::B : A<T> {
  B::type t;
};

Maybe the rule in temp.res doesn't apply because this is a member of the template and not the template itself?

Fznamznon · 2023-12-04T09:51:34Z

I'm not sure I understand why gcc is giving an error in https://godbolt.org/z/qGfnzhfsK. It doesn't give an error with typename keyword and I think there shouldn't be any difference since Y::E refers to current instantiation and is being used by class member declaration so no typename required.
cc @shafik @cor3ntin @zygoloid for an opinion.

perry-ca · 2024-01-29T19:30:44Z

Was this PR meant to change the behaviour for c++11/14?

zygoloid · 2024-01-29T20:28:28Z

I'm not sure I understand why gcc is giving an error in https://godbolt.org/z/qGfnzhfsK. It doesn't give an error with typename keyword and I think there shouldn't be any difference since Y::E refers to current instantiation and is being used by class member declaration so no typename required. cc @shafik @cor3ntin @zygoloid for an opinion.

This looks like a GCC bug to me. I think the presence or absence of typename isn't supposed to make a difference in an implicit typename context.

Fznamznon requested review from shafik and erichkeane November 21, 2023 17:08

Fznamznon mentioned this pull request Nov 21, 2023

Ignores dependent base classes using qualified lookup in definition context #13826

Closed

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 21, 2023

erichkeane approved these changes Nov 21, 2023

View reviewed changes

Fznamznon requested a review from cor3ntin November 21, 2023 17:22

shafik reviewed Nov 28, 2023

View reviewed changes

clang/lib/Sema/SemaDecl.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaDecl.cpp Outdated Show resolved Hide resolved

cor3ntin approved these changes Nov 29, 2023

View reviewed changes

Fznamznon added 3 commits November 29, 2023 10:47

Apply comments

5f4b4b9

Merge branch 'main' into 13826

fa03150

Merge branch 'main' into 13826

6b2fc2f

Fznamznon merged commit 14ca8d4 into llvm:main Nov 30, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Fix a bug with qualified name lookup into current instantiation #73018

[clang] Fix a bug with qualified name lookup into current instantiation #73018

Fznamznon commented Nov 21, 2023

llvmbot commented Nov 21, 2023

erichkeane left a comment

cor3ntin left a comment

cor3ntin Nov 29, 2023

Fznamznon Nov 30, 2023

Fznamznon commented Nov 30, 2023

john-brawn-arm commented Dec 1, 2023

john-brawn-arm commented Dec 1, 2023 •

edited

Fznamznon commented Dec 4, 2023

perry-ca commented Jan 29, 2024

zygoloid commented Jan 29, 2024

[clang] Fix a bug with qualified name lookup into current instantiation #73018

[clang] Fix a bug with qualified name lookup into current instantiation #73018

Conversation

Fznamznon commented Nov 21, 2023

llvmbot commented Nov 21, 2023

erichkeane left a comment

Choose a reason for hiding this comment

cor3ntin left a comment

Choose a reason for hiding this comment

cor3ntin Nov 29, 2023

Choose a reason for hiding this comment

Fznamznon Nov 30, 2023

Choose a reason for hiding this comment

Fznamznon commented Nov 30, 2023

john-brawn-arm commented Dec 1, 2023

john-brawn-arm commented Dec 1, 2023 • edited

Fznamznon commented Dec 4, 2023

perry-ca commented Jan 29, 2024

zygoloid commented Jan 29, 2024

john-brawn-arm commented Dec 1, 2023 •

edited