Skip to content

[clang] fix NestedNameSpecifier dependency calculation #135067

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

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

mizvekov
Copy link
Contributor

@mizvekov mizvekov commented Apr 9, 2025

A NestedNameSpecifier of TypeSpec kind can be non-dependent even if its prefix is dependent, when for example the prefix is an injected class type but the type itself is a simple alias to a non-dependent type.

This issue was a bit hard to observe because if it is an alias to a class type, then we (for some unknown reason) ignored that the NNS was dependent in the first place, which wouldn't happen with an enum type.

This could have been a workaround for previous dependency bugs, and is not relevant anymore for any of the test cases in the tree, so this patch also removes that.

The other kinds of dependencies are still relevant. If the prefix contains an unexpanded pack, then this NNS is still unexpanded, and likewise for errors.

This fixes a regression reported here: #133610 (comment) which was introduced by #133610

There are no release notes since the regression was never released.

A NestedNameSpecifier of TypeSpec kind can be non-dependent even if its
prefix is dependent, when for example the prefix is an injected class type
but the type itself is a simple alias to a non-dependent type.

This issue was a bit hard to observe because if its an alias to
a class type, then we (for some unknown reason) ignored that the
NNS was dependent in the first place, which wouldn't happen
with an enum type.

This could have been a workaround for previous dependency bugs,
and is not relevant anymore for any of the test cases in the
tree, so this patch also removes that.

The other kinds of dependencies are still relevant.
If the prefix contains an unexpanded pack, then this NNS is still
unexpanded, and likewise for errors.

This fixes a regression reported here: #133610 (comment)
which was introduced by #133610

There are no release notes since the regression was never released.
@mizvekov mizvekov self-assigned this Apr 9, 2025
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 9, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 9, 2025

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

A NestedNameSpecifier of TypeSpec kind can be non-dependent even if its prefix is dependent, when for example the prefix is an injected class type but the type itself is a simple alias to a non-dependent type.

This issue was a bit hard to observe because if its an alias to a class type, then we (for some unknown reason) ignored that the NNS was dependent in the first place, which wouldn't happen with an enum type.

This could have been a workaround for previous dependency bugs, and is not relevant anymore for any of the test cases in the tree, so this patch also removes that.

The other kinds of dependencies are still relevant. If the prefix contains an unexpanded pack, then this NNS is still unexpanded, and likewise for errors.

This fixes a regression reported here: #133610 (comment) which was introduced by #133610

There are no release notes since the regression was never released.


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

3 Files Affected:

  • (modified) clang/lib/AST/NestedNameSpecifier.cpp (+2-1)
  • (modified) clang/lib/Sema/SemaCXXScopeSpec.cpp (+1-2)
  • (modified) clang/test/SemaTemplate/dependent-names.cpp (+14)
diff --git a/clang/lib/AST/NestedNameSpecifier.cpp b/clang/lib/AST/NestedNameSpecifier.cpp
index 51aa2d69d0f0d..bd0fe36d781da 100644
--- a/clang/lib/AST/NestedNameSpecifier.cpp
+++ b/clang/lib/AST/NestedNameSpecifier.cpp
@@ -221,7 +221,8 @@ NestedNameSpecifierDependence NestedNameSpecifier::getDependence() const {
     NestedNameSpecifierDependence Dep =
         toNestedNameSpecifierDependendence(getAsType()->getDependence());
     if (NestedNameSpecifier *Prefix = getPrefix())
-      Dep |= Prefix->getDependence();
+      Dep |=
+          Prefix->getDependence() & ~NestedNameSpecifierDependence::Dependent;
     return Dep;
   }
   }
diff --git a/clang/lib/Sema/SemaCXXScopeSpec.cpp b/clang/lib/Sema/SemaCXXScopeSpec.cpp
index ea7936b0e5b8f..68ee006c6cf00 100644
--- a/clang/lib/Sema/SemaCXXScopeSpec.cpp
+++ b/clang/lib/Sema/SemaCXXScopeSpec.cpp
@@ -31,8 +31,7 @@ static CXXRecordDecl *getCurrentInstantiationOf(QualType T,
   const Type *Ty = T->getCanonicalTypeInternal().getTypePtr();
   if (const RecordType *RecordTy = dyn_cast<RecordType>(Ty)) {
     CXXRecordDecl *Record = cast<CXXRecordDecl>(RecordTy->getDecl());
-    if (!Record->isDependentContext() ||
-        Record->isCurrentInstantiation(CurContext))
+    if (Record->isCurrentInstantiation(CurContext))
       return Record;
 
     return nullptr;
diff --git a/clang/test/SemaTemplate/dependent-names.cpp b/clang/test/SemaTemplate/dependent-names.cpp
index 538abde3eddd5..54ce3760d2d01 100644
--- a/clang/test/SemaTemplate/dependent-names.cpp
+++ b/clang/test/SemaTemplate/dependent-names.cpp
@@ -467,3 +467,17 @@ namespace TransformDependentTemplates {
     void f(Arg<int>);
   };
 } // namespace TransformDependentTemplates
+
+namespace TransformNestedName {
+  enum class S { kA };
+
+  template <class T> struct N {
+    using State = S;
+    template <typename T::template X<State::kA> = 0>
+    void F();
+  };
+
+  template <class T>
+  template <typename T::template X<N<T>::State::kA>>
+  inline void N<T>::F() {}
+} // namespace TransformNestedName

@mizvekov mizvekov merged commit 154507c into main Apr 9, 2025
14 checks passed
@mizvekov mizvekov deleted the users/mizvekov/fix-nns-dependency branch April 9, 2025 22:44
AllinLeeYL pushed a commit to AllinLeeYL/llvm-project that referenced this pull request Apr 10, 2025
A NestedNameSpecifier of TypeSpec kind can be non-dependent even if its
prefix is dependent, when for example the prefix is an injected class
type but the type itself is a simple alias to a non-dependent type.

This issue was a bit hard to observe because if it is an alias to a
class type, then we (for some unknown reason) ignored that the NNS was
dependent in the first place, which wouldn't happen with an enum type.

This could have been a workaround for previous dependency bugs, and is
not relevant anymore for any of the test cases in the tree, so this
patch also removes that.

The other kinds of dependencies are still relevant. If the prefix
contains an unexpanded pack, then this NNS is still unexpanded, and
likewise for errors.

This fixes a regression reported here:
llvm#133610 (comment)
which was introduced by llvm#133610

There are no release notes since the regression was never released.
var-const pushed a commit to ldionne/llvm-project that referenced this pull request Apr 17, 2025
A NestedNameSpecifier of TypeSpec kind can be non-dependent even if its
prefix is dependent, when for example the prefix is an injected class
type but the type itself is a simple alias to a non-dependent type.

This issue was a bit hard to observe because if it is an alias to a
class type, then we (for some unknown reason) ignored that the NNS was
dependent in the first place, which wouldn't happen with an enum type.

This could have been a workaround for previous dependency bugs, and is
not relevant anymore for any of the test cases in the tree, so this
patch also removes that.

The other kinds of dependencies are still relevant. If the prefix
contains an unexpanded pack, then this NNS is still unexpanded, and
likewise for errors.

This fixes a regression reported here:
llvm#133610 (comment)
which was introduced by llvm#133610

There are no release notes since the regression was never released.
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 Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants