[clang] [Sema] Preserve nested name specifier prefix in MemberPointerType #118236

HighCommander4 · 2024-12-02T00:59:10Z

Fixes #118198

llvmbot · 2024-12-02T00:59:46Z

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

@llvm/pr-subscribers-clang

Author: Nathan Ridge (HighCommander4)

Changes

Fixes #118198

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

2 Files Affected:

(modified) clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp (+7)
(modified) clang/lib/Sema/SemaType.cpp (+10-7)

diff --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
index 30b9b1902aa9c7..1ec51d862d0a6f 100644
--- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -1092,6 +1092,13 @@ sizeof...($TemplateParameter[[Elements]]);
             $Field_dependentName[[waldo]];
           }
         };
+    )cpp",
+      // Pointer-to-member with nested-name-specifiers
+      R"cpp(
+      struct $Class_def[[Outer]] {
+        struct $Class_def[[Inner]] {};
+      };
+      using $Typedef_decl[[Alias]] = void ($Class[[Outer]]::$Class[[Inner]]:: *)();
     )cpp"};
   for (const auto &TestCase : TestCases)
     // Mask off scope modifiers to keep the tests manageable.
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index f32edc5ac06440..73c9362208b14b 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -5347,15 +5347,18 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
 
         case NestedNameSpecifier::TypeSpec:
         case NestedNameSpecifier::TypeSpecWithTemplate:
-          ClsType = QualType(NNS->getAsType(), 0);
-          // Note: if the NNS has a prefix and ClsType is a nondependent
-          // TemplateSpecializationType, then the NNS prefix is NOT included
-          // in ClsType; hence we wrap ClsType into an ElaboratedType.
-          // NOTE: in particular, no wrap occurs if ClsType already is an
-          // Elaborated, DependentName, or DependentTemplateSpecialization.
-          if (isa<TemplateSpecializationType>(NNS->getAsType()))
+          const Type *NNSType = NNS->getAsType();
+          ClsType = QualType(NNSType, 0);
+          // If ClsType is an Elaborated, DependentName, or
+          // DependentTemplateSpecialization, it already stores the NNS prefix.
+          // Otherwise, wrap it in an Elaborated type to have a place to store
+          // the NNS prefix.
+          if (!(isa<ElaboratedType>(NNSType) ||
+                isa<DependentNameType>(NNSType) ||
+                isa<DependentTemplateSpecializationType>(NNSType))) {
             ClsType = Context.getElaboratedType(ElaboratedTypeKeyword::None,
                                                 NNSPrefix, ClsType);
+          }
           break;
         }
       } else {

HighCommander4 · 2024-12-02T01:42:58Z

(Marked as Draft since there are test failures I need to investigate.)

mizvekov · 2024-12-02T01:43:39Z

clang/lib/Sema/SemaType.cpp

+          if (!(isa<ElaboratedType>(NNSType) ||
+                isa<DependentNameType>(NNSType) ||
+                isa<DependentTemplateSpecializationType>(NNSType))) {


This seems to be testing for conditions which seem impossible, the NNS would be malformed otherwise.

For example, what would it mean for the NNS type to be an ElaboratedType? If the NNS has a prefix, which would take precedence, that prefix or the ElaboratedType's prefix? If there is no prefix, what sense would it convey beyond using the Elaborated prefix as its prefix instead?

For DependentNameType, that would be redundant with how we represent these as Identifier + prefix, so we should prefer that over forming a NNS with such type.

The DTST is an exception here, as we lack a NNS kind for an identifier + list of template arguments, but then such DTST should not store a NNS itself, otherwise again this would be a bug somewhere else, as that would be redundant / confusing with the NNS' own prefix as explained above.

So if we are getting either an ElaboratedType or a DependentNameType here, there seems to be a bug elsewhere.

Regarding the explanation for DependentTemplateSpecialization: even if the type node itself can have a nested name, it should not be present when the node is used as the type for a NNS node itself.

We should handle a DTST here by rebuilding it, adding the NNSPrefix as the DTST's nested name.

@mizvekov Thanks for having a look!

I don't have any evidence of us actually getting an ElaboratedType or DependentNameType here, I was just basing this on the comment which mentioned these types.

I revised the patch to make a more targeted change (wrap RecordType when there is a qualifier present), which still addresses the original issue I was having in clangd, and (hopefully) passes tests. I would be grateful of your opinion about whether there is a more appropriate condition we should be using.

Yeah I think the existing comment is wrong / misleading.

mizvekov · 2024-12-02T03:23:53Z

clang/lib/Sema/SemaType.cpp

+          // already is an Elaborated, DependentName, or
+          // DependentTemplateSpecialization.
+          if (isa<TemplateSpecializationType>(NNSType) ||
+              (NNSPrefix && isa<RecordType>(NNSType)))


Suggested change

(NNSPrefix && isa<RecordType>(NNSType)))

isa<RecordType>(NNSType))

We use an ElaboratedType with null prefix to represent a type which was written without qualifiers, so it seems arbitrary to not add the elaboration in this case.

Also, can you at least add a FIXME / TODO explaining how a DependentTemplateSpecializationType needs to be handled here, as I explained in the other thread?

It would also be cool to have an assertion here, that we are handling all cases.

I think the logic here would be something like:

if (isa<DependentTemplateSpecializationType>...) // Rebuild DependentTemplateSpecializationType, adding the Prefix. else // either the dependent case (TemplateSpecializationType), or the non-dependent one (RecordType). // assert(isa<TemplateSpecializationType, RecordType>...)

We use an ElaboratedType with null prefix to represent a type which was written without qualifiers, so it seems arbitrary to not add the elaboration in this case.

Wrapping RecordTypes into ElaboratedTypes with a null prefix was causing a bunch of failures on the first draft of the patch.

For example, on this line:

llvm-project/clang/test/CXX/conv/conv.mem/p4.cpp

Line 22 in 8cb4485

int (Derived::*d) = data_ptr; // expected-error {{cannot cast private base class 'Base' to 'test1::Derived'}}

the change has the effect of changing the way the derived type is printed from test1::Derived to just Derived, I guess because when the diagnostic formatter encounters an ElaboratedType it says "I know how the type was printed in the source, I will print it the same way in the diagnostic".

Should we accept changes like this to diagnostic output?

Yes exactly, this test change is correct, because the type was written as 'Derived' and not 'test1::Derived', and the diagnostics are supposed to print the type as written. It would be inconsistent to print the type as written when it has any qualifier, but print it fully qualified when it does not have any.

We started doing this consistently since this patch: https://reviews.llvm.org/D112374

It would also be cool to have an assertion here, that we are handling all cases.

I think the logic here would be something like:

if (isa<DependentTemplateSpecializationType>...) // Rebuild DependentTemplateSpecializationType, adding the Prefix. else // either the dependent case (TemplateSpecializationType), or the non-dependent one (RecordType). // assert(isa<TemplateSpecializationType, RecordType>...)

I tried this, but SemaCXX/err_init_conversion_failed.cpp trips the assertion; we get a TemplateTypeParmType here.

I see, well this is one case I left out :) The assert is still viable and would be helpful, but it's not critical for merging this.

The assert is still viable and would be helpful

FWIW, I did try adding back the assert to see what other sorts of types we get in this codepath. I found the following:

TemplateTypeParmType (seen in SemaCXX/err_init_conversion_failed.cpp)

TypedefType (seen in CXX/drs/cwg3xx.cpp)

DecltypeType (seen in CXX/expr/expr_prim/expr_prim_general/p8-0x.cpp)

InjectedClassNameType (seen in CXX/temp/temp_decls/temp_variadic/p5.cpp)

UsingType (seen in Modules/using-decl.cpp)

It's not clear to me what sort of handling we'd want for these various types.

HighCommander4 · 2024-12-06T02:20:19Z

I updated the patch to do the wrapping into ElaboratedType even if a prefix isn't present, and updated the various failing tests accordingly.

I haven't added an assert, as explained in the previous comment.

…Type

HighCommander4 · 2024-12-06T02:47:32Z

Fixed one additional test failure.

HighCommander4 · 2024-12-06T04:05:50Z

CI is green. Requesting review.

mizvekov

Thanks, LGTM!

llvmbot added clang Clang issues not falling into any other category clang-tools-extra clangd clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 2, 2024

HighCommander4 mentioned this pull request Dec 2, 2024

No TypeLoc node for outer nested-name-specifier in pointer-to-member type #118198

Closed

HighCommander4 marked this pull request as draft December 2, 2024 01:28

mizvekov reviewed Dec 2, 2024

View reviewed changes

HighCommander4 force-pushed the issue-2235 branch from 675aec8 to 37eb30a Compare December 2, 2024 03:03

mizvekov reviewed Dec 2, 2024

View reviewed changes

HighCommander4 force-pushed the issue-2235 branch from 37eb30a to 4594891 Compare December 6, 2024 02:18

[clang] [Sema] Preserve nested name specifier prefix in MemberPointer…

9fe707f

…Type

HighCommander4 force-pushed the issue-2235 branch from 4594891 to 9fe707f Compare December 6, 2024 02:47

HighCommander4 marked this pull request as ready for review December 6, 2024 04:05

HighCommander4 requested a review from Endilll as a code owner December 6, 2024 04:05

HighCommander4 requested a review from mizvekov December 6, 2024 04:06

mizvekov approved these changes Dec 6, 2024

View reviewed changes

HighCommander4 merged commit 70c1764 into llvm:main Dec 8, 2024
10 checks passed

HighCommander4 mentioned this pull request Dec 9, 2024

[clang] [Sema] Add assertion about expected type classes when building MemberPointerType #119105

Closed

	(NNSPrefix && isa<RecordType>(NNSType)))
	isa<RecordType>(NNSType))

[clang] [Sema] Preserve nested name specifier prefix in MemberPointerType #118236

[clang] [Sema] Preserve nested name specifier prefix in MemberPointerType #118236

Uh oh!

Conversation

HighCommander4 commented Dec 2, 2024

Uh oh!

llvmbot commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HighCommander4 commented Dec 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HighCommander4 commented Dec 6, 2024

Uh oh!

HighCommander4 commented Dec 6, 2024

Uh oh!

HighCommander4 commented Dec 6, 2024

Uh oh!

mizvekov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Dec 2, 2024 •

edited

Loading