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

[Clang][Sema] Fix lookup of dependent operator= named by using-declaration #91503

Closed
wants to merge 3 commits into from

Conversation

sdkrystian
Copy link
Member

Fixes this bug caused by #90152.

Will add tests shortly.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 8, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 8, 2024

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Fixes this bug caused by #90152.

Will add tests shortly.


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

1 Files Affected:

  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+12-5)
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 157d42c09cfcd..91c83564b567e 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -12963,14 +12963,15 @@ NamedDecl *Sema::BuildUsingDeclaration(
     return nullptr;
   }
 
-  DeclContext *LookupContext = computeDeclContext(SS);
   NestedNameSpecifierLoc QualifierLoc = SS.getWithLocInContext(Context);
-  if (!LookupContext || EllipsisLoc.isValid()) {
-    NamedDecl *D;
+  DeclContext *LookupContext = computeDeclContext(SS);
+
+  auto BuildDependent = [&] {
+    NamedDecl *D = nullptr;
     // Dependent scope, or an unexpanded pack
     if (!LookupContext && CheckUsingDeclQualifier(UsingLoc, HasTypenameKeyword,
                                                   SS, NameInfo, IdentLoc))
-      return nullptr;
+      return D;
 
     if (HasTypenameKeyword) {
       // FIXME: not all declaration name kinds are legal here
@@ -12987,7 +12988,7 @@ NamedDecl *Sema::BuildUsingDeclaration(
     CurContext->addDecl(D);
     ProcessDeclAttributeList(S, D, AttrList);
     return D;
-  }
+  };
 
   auto Build = [&](bool Invalid) {
     UsingDecl *UD =
@@ -13002,6 +13003,9 @@ NamedDecl *Sema::BuildUsingDeclaration(
   auto BuildInvalid = [&]{ return Build(true); };
   auto BuildValid = [&]{ return Build(false); };
 
+  if (!LookupContext || EllipsisLoc.isValid())
+    return BuildDependent();
+
   if (RequireCompleteDeclContext(SS, LookupContext))
     return BuildInvalid();
 
@@ -13024,6 +13028,9 @@ NamedDecl *Sema::BuildUsingDeclaration(
 
   LookupQualifiedName(R, LookupContext);
 
+  if (R.wasNotFoundInCurrentInstantiation())
+    return BuildDependent();
+
   // Validate the context, now we have a lookup
   if (CheckUsingDeclQualifier(UsingLoc, HasTypenameKeyword, SS, NameInfo,
                               IdentLoc, &R))

template<typename T>
struct S : T {
struct U : S { // expected-note 6{{candidate}}
// FIXME: S is incomplete here and we should diagnose this!
struct U : S {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did the FIXME ever get a core issue? Perhaps we need to run that down a bit? @Endilll or @shafik , any chance the 'FIXME" that we just 'fixed' looks familiar?

Copy link
Contributor

Choose a reason for hiding this comment

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

This rings a bell. I've put something similar in our DR tests, probably in a PR about complete-class context. I'm running out of time today, but I'll look into this tomorrow if Shafik doesn't beat me to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The FIXME wasn't fixed, it's just broken in a different way that's consistent with other "current instantiation broken-ness" related things. I'm going to submit a patch (probably this week) which diagnoses cases where the current instantiation is used in a context which requires a complete type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, this is githubs fault :) I mean the OLD 'FIXME' that you removed. I was wondering if it has a CWG issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are somewhat related CWG issues 45, 374, and 500, but I can't find one arguing that base specifiers should become a complete-class context. http://eel.is/c++draft/class#mem.general-8 seems quite clear on this.

@sdkrystian
Copy link
Member Author

Closing this for now; I'll return to this once we correctly handle dependent operator=.

@sdkrystian sdkrystian closed this May 17, 2024
@pawosm-arm
Copy link
Contributor

pawosm-arm commented Jun 28, 2024

The number of C++ workloads failing to build after #90152 is rather impressive, even this PR does not help. Is there a plan to address that?

@sdkrystian
Copy link
Member Author

@pawosm-arm Could you provide some examples? Barring an issue with operator= which has been addressed, builds should only fail in the presence of accesses to non-existent members. Addressing such issues is the responsibility of users.

@pawosm-arm
Copy link
Contributor

pawosm-arm commented Jun 28, 2024

I'm sorry, I didn't notice that there is more discussion after #90152 covering also my issue at hand. The obvious advice I can get from those is the 'this is not valid c++, fix your code' suggestion. Trouble starts when there is a lot of it to fix, even if it mostly involve deletion of the functions that aren't instantiated.

NB, suggesting use of -fdelayed-template-parsing isn't always working, unless we start using something like -fdelayed-template-parsing -Wno-error=delayed-template-parsing-in-cxx20

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.

5 participants