Merge pull request #10729 from jakobandersen/cpp-template-args-non-specialized

[C++] Ensure consistent non-specialization template argument representation, rebased and updated

CHANGES

@@ -19,6 +19,10 @@ Features added
 Bugs fixed
 ----------
 
+- #10257: C++, ensure consistent non-specialization template argument
+  representation.
+- #10729: C++, fix parsing of certain non-type template parameter packs.
+
 Testing
 --------
 

sphinx/domains/cpp.py

@@ -2604,6 +2604,10 @@ class ASTDeclaratorPtr(ASTDeclarator):
     def name(self, name: ASTNestedName) -> None:
         self.next.name = name
 
+    @property
+    def isPack(self) -> bool:
+        return self.next.isPack
+
     @property
     def function_params(self) -> List[ASTFunctionParameter]:
         return self.next.function_params
@@ -2707,7 +2711,7 @@ class ASTDeclaratorRef(ASTDeclarator):
 
     @property
     def isPack(self) -> bool:
-        return True
+        return self.next.isPack
 
     @property
     def function_params(self) -> List[ASTFunctionParameter]:
@@ -2779,6 +2783,10 @@ class ASTDeclaratorParamPack(ASTDeclarator):
     def trailingReturn(self) -> "ASTType":
         return self.next.trailingReturn
 
+    @property
+    def isPack(self) -> bool:
+        return True
+
     def require_space_after_declSpecs(self) -> bool:
         return False
 
@@ -2835,6 +2843,10 @@ class ASTDeclaratorMemPtr(ASTDeclarator):
     def name(self, name: ASTNestedName) -> None:
         self.next.name = name
 
+    @property
+    def isPack(self):
+        return self.next.isPack
+
     @property
     def function_params(self) -> List[ASTFunctionParameter]:
         return self.next.function_params
@@ -2932,6 +2944,10 @@ class ASTDeclaratorParen(ASTDeclarator):
     def name(self, name: ASTNestedName) -> None:
         self.inner.name = name
 
+    @property
+    def isPack(self):
+        return self.inner.isPack or self.next.isPack
+
     @property
     def function_params(self) -> List[ASTFunctionParameter]:
         return self.inner.function_params
@@ -3510,6 +3526,14 @@ class ASTTemplateParam(ASTBase):
                            env: "BuildEnvironment", symbol: "Symbol") -> None:
         raise NotImplementedError(repr(self))
 
+    @property
+    def isPack(self) -> bool:
+        raise NotImplementedError(repr(self))
+
+    @property
+    def name(self) -> ASTNestedName:
+        raise NotImplementedError(repr(self))
+
 
 class ASTTemplateKeyParamPackIdDefault(ASTTemplateParam):
     def __init__(self, key: str, identifier: ASTIdentifier,
@@ -3644,9 +3668,11 @@ class ASTTemplateParamTemplateType(ASTTemplateParam):
 class ASTTemplateParamNonType(ASTTemplateParam):
     def __init__(self,
                  param: Union[ASTTypeWithInit,
-                              ASTTemplateParamConstrainedTypeWithInit]) -> None:
+                              ASTTemplateParamConstrainedTypeWithInit],
+                 parameterPack: bool = False) -> None:
         assert param
         self.param = param
+        self.parameterPack = parameterPack
 
     @property
     def name(self) -> ASTNestedName:
@@ -3655,7 +3681,7 @@ class ASTTemplateParamNonType(ASTTemplateParam):
 
     @property
     def isPack(self) -> bool:
-        return self.param.isPack
+        return self.param.isPack or self.parameterPack
 
     def get_identifier(self) -> ASTIdentifier:
         name = self.param.name
@@ -3676,14 +3702,22 @@ class ASTTemplateParamNonType(ASTTemplateParam):
             # the anchor will be our parent
             return symbol.parent.declaration.get_id(version, prefixed=None)
         else:
-            return '_' + self.param.get_id(version)
+            res = '_'
+            if self.parameterPack:
+                res += 'Dp'
+            return res + self.param.get_id(version)
 
     def _stringify(self, transform: StringifyTransform) -> str:
-        return transform(self.param)
+        res = transform(self.param)
+        if self.parameterPack:
+            res += '...'
+        return res
 
     def describe_signature(self, signode: TextElement, mode: str,
                            env: "BuildEnvironment", symbol: "Symbol") -> None:
         self.param.describe_signature(signode, mode, env, symbol)
+        if self.parameterPack:
+            signode += addnodes.desc_sig_punctuation('...', '...')
 
 
 class ASTTemplateParams(ASTBase):
@@ -4129,6 +4163,31 @@ class LookupKey:
         self.data = data
 
 
+def _is_specialization(templateParams: Union[ASTTemplateParams, ASTTemplateIntroduction],
+                       templateArgs: ASTTemplateArgs) -> bool:
+    # Checks if `templateArgs` does not exactly match `templateParams`.
+    # the names of the template parameters must be given exactly as args
+    # and params that are packs must in the args be the name expanded
+    if len(templateParams.params) != len(templateArgs.args):
+        return True
+    # having no template params and no arguments is also a specialization
+    if len(templateParams.params) == 0:
+        return True
+    for i in range(len(templateParams.params)):
+        param = templateParams.params[i]
+        arg = templateArgs.args[i]
+        # TODO: doing this by string manipulation is probably not the most efficient
+        paramName = str(param.name)
+        argTxt = str(arg)
+        isArgPackExpansion = argTxt.endswith('...')
+        if param.isPack != isArgPackExpansion:
+            return True
+        argName = argTxt[:-3] if isArgPackExpansion else argTxt
+        if paramName != argName:
+            return True
+    return False
+
+
 class Symbol:
     debug_indent = 0
     debug_indent_string = "  "
@@ -4177,6 +4236,16 @@ class Symbol:
         self.siblingAbove: Symbol = None
         self.siblingBelow: Symbol = None
         self.identOrOp = identOrOp
+        # Ensure the same symbol for `A` is created for:
+        #
+        #     .. cpp:class:: template <typename T> class A
+        #
+        # and
+        #
+        #     .. cpp:function:: template <typename T> int A<T>::foo()
+        if (templateArgs is not None and
+                not _is_specialization(templateParams, templateArgs)):
+            templateArgs = None
         self.templateParams = templateParams  # template<templateParams>
         self.templateArgs = templateArgs  # identifier<templateArgs>
         self.declaration = declaration
@@ -4357,33 +4426,12 @@ class Symbol:
             Symbol.debug_print("correctPrimaryTemplateAargs:", correctPrimaryTemplateArgs)
             Symbol.debug_print("searchInSiblings:           ", searchInSiblings)
 
-        def isSpecialization() -> bool:
-            # the names of the template parameters must be given exactly as args
-            # and params that are packs must in the args be the name expanded
-            if len(templateParams.params) != len(templateArgs.args):
-                return True
-            # having no template params and no arguments is also a specialization
-            if len(templateParams.params) == 0:
-                return True
-            for i in range(len(templateParams.params)):
-                param = templateParams.params[i]
-                arg = templateArgs.args[i]
-                # TODO: doing this by string manipulation is probably not the most efficient
-                paramName = str(param.name)
-                argTxt = str(arg)
-                isArgPackExpansion = argTxt.endswith('...')
-                if param.isPack != isArgPackExpansion:
-                    return True
-                argName = argTxt[:-3] if isArgPackExpansion else argTxt
-                if paramName != argName:
-                    return True
-            return False
         if correctPrimaryTemplateArgs:
             if templateParams is not None and templateArgs is not None:
                 # If both are given, but it's not a specialization, then do lookup as if
                 # there is no argument list.
                 # For example: template<typename T> int A<T>::var;
-                if not isSpecialization():
+                if not _is_specialization(templateParams, templateArgs):
                     templateArgs = None
 
         def matches(s: "Symbol") -> bool:
@@ -4812,7 +4860,7 @@ class Symbol:
                     if symbol.declaration is None:
                         if Symbol.debug_lookup:
                             Symbol.debug_print("empty candidate")
-                        # if in the end we have non matching, but have an empty one,
+                        # if in the end we have non-matching, but have an empty one,
                         # then just continue with that
                         ourChild = symbol
                         continue
@@ -4838,6 +4886,15 @@ class Symbol:
                     msg = msg % (ourChild.docname, ourChild.line,
                                  ourChild.declaration.directiveType, name)
                     logger.warning(msg, location=(otherChild.docname, otherChild.line))
+                else:
+                    if (otherChild.declaration.objectType ==
+                            ourChild.declaration.objectType and
+                            otherChild.declaration.objectType in
+                            ('templateParam', 'functionParam') and
+                            ourChild.parent.declaration == otherChild.parent.declaration):
+                        # `ourChild` was just created during merging by the call
+                        # to `_fill_empty` on the parent and can be ignored.
+                        pass
                     else:
                         # Both have declarations, and in the same docname.
                         # This can apparently happen, it should be safe to
@@ -5106,6 +5163,7 @@ class Symbol:
                 res.append(": ")
                 if self.isRedeclaration:
                     res.append('!!duplicate!! ')
+                res.append("{" + self.declaration.objectType + "} ")
                 res.append(str(self.declaration))
         if self.docname:
             res.append('\t(')
@@ -6727,7 +6785,7 @@ class DefinitionParser(BaseParser):
     def _parse_template_parameter(self) -> ASTTemplateParam:
         self.skip_ws()
         if self.skip_word('template'):
-            # declare a tenplate template parameter
+            # declare a template template parameter
             nestedParams = self._parse_template_parameter_list()
         else:
             nestedParams = None
@@ -6774,7 +6832,9 @@ class DefinitionParser(BaseParser):
                 # non-type parameter or constrained type parameter
                 self.pos = pos
                 param = self._parse_type_with_init('maybe', 'templateParam')
-                return ASTTemplateParamNonType(param)
+                self.skip_ws()
+                parameterPack = self.skip_string('...')
+                return ASTTemplateParamNonType(param, parameterPack)
             except DefinitionError as eNonType:
                 self.pos = pos
                 header = "Error when parsing template parameter."
@@ -8089,7 +8149,7 @@ def setup(app: Sphinx) -> Dict[str, Any]:
 
     return {
         'version': 'builtin',
-        'env_version': 7,
+        'env_version': 8,
         'parallel_read_safe': True,
         'parallel_write_safe': True,
     }

tests/test_domain_cpp.py

@@ -841,9 +841,9 @@ def test_domain_cpp_ast_templates():
     check('class', 'abc::ns::foo{{id_0, id_1, ...id_2}} {key}xyz::bar',
           {2: 'I00DpEXN3abc2ns3fooEI4id_04id_1sp4id_2EEN3xyz3barE'})
     check('class', 'abc::ns::foo{{id_0, id_1, id_2}} {key}xyz::bar<id_0, id_1, id_2>',
-          {2: 'I000EXN3abc2ns3fooEI4id_04id_14id_2EEN3xyz3barI4id_04id_14id_2EE'})
+          {2: 'I000EXN3abc2ns3fooEI4id_04id_14id_2EEN3xyz3barE'})
     check('class', 'abc::ns::foo{{id_0, id_1, ...id_2}} {key}xyz::bar<id_0, id_1, id_2...>',
-          {2: 'I00DpEXN3abc2ns3fooEI4id_04id_1sp4id_2EEN3xyz3barI4id_04id_1Dp4id_2EE'})
+          {2: 'I00DpEXN3abc2ns3fooEI4id_04id_1sp4id_2EEN3xyz3barE'})
 
     check('class', 'template<> Concept{{U}} {key}A<int>::B', {2: 'IEI0EX7ConceptI1UEEN1AIiE1BE'})
 
@@ -876,6 +876,9 @@ def test_domain_cpp_ast_templates():
     # defaulted constrained type parameters
     check('type', 'template<C T = int&> {key}A', {2: 'I_1CE1A'}, key='using')
 
+    # pack expansion after non-type template parameter
+    check('type', 'template<int (X::*)(bool)...> {key}A', {2: 'I_DpM1XFibEE1A'}, key='using')
+
 
 def test_domain_cpp_ast_placeholder_types():
     check('function', 'void f(Sortable auto &v)', {1: 'f__SortableR', 2: '1fR8Sortable'})
@@ -901,7 +904,7 @@ def test_domain_cpp_ast_requires_clauses():
           'template<typename T> requires R<T> ' +
           'template<typename U> requires S<T> ' +
           'void A<T>::f() requires B',
-          {4: 'I0EIQ1RI1TEEI0EIQaa1SI1TE1BEN1AI1TE1fEvv'})
+          {4: 'I0EIQ1RI1TEEI0EIQaa1SI1TE1BEN1A1fEvv'})
     check('function',
           'template<template<typename T> requires R<T> typename X> ' +
           'void f()',
@@ -1040,6 +1043,38 @@ def test_domain_cpp_ast_xref_parsing():
     check('T f()')
 
 
+@pytest.mark.parametrize(
+    'param,is_pack',
+    [('typename', False),
+     ('typename T', False),
+     ('typename...', True),
+     ('typename... T', True),
+     ('int', False),
+     ('int N', False),
+     ('int* N', False),
+     ('int& N', False),
+     ('int&... N', True),
+     ('int*... N', True),
+     ('int...', True),
+     ('int... N', True),
+     ('auto', False),
+     ('auto...', True),
+     ('int X::*', False),
+     ('int X::*...', True),
+     ('int (X::*)(bool)', False),
+     ('int (X::*x)(bool)', False),
+     ('int (X::*)(bool)...', True),
+     ('template<typename> class', False),
+     ('template<typename> class...', True),
+     ])
+def test_domain_cpp_template_parameters_is_pack(param: str, is_pack: bool):
+    def parse_template_parameter(param: str):
+        ast = parse('type', 'template<' + param + '> X')
+        return ast.templatePrefix.templates[0].params[0]
+    ast = parse_template_parameter(param)
+    assert ast.isPack == is_pack
+
+
 # def test_print():
 #     # used for getting all the ids out for checking
 #     for a in ids:
@@ -1395,3 +1430,53 @@ def test_domain_cpp_parse_mix_decl_duplicate(app, warning):
     assert "index.rst:3: WARNING: Duplicate C++ declaration, also defined at index:1." in ws[2]
     assert "Declaration is '.. cpp:struct:: A'." in ws[3]
     assert ws[4] == ""
+
+
+# For some reason, using the default testroot of "root" leads to the contents of
+# `test-root/objects.txt` polluting the symbol table depending on the test
+# execution order.  Using a testroot of "config" seems to avoid that problem.
+@pytest.mark.sphinx(testroot='config')
+def test_domain_cpp_normalize_unspecialized_template_args(make_app, app_params):
+    args, kwargs = app_params
+
+    text1 = (".. cpp:class:: template <typename T> A\n")
+    text2 = (".. cpp:class:: template <typename T> template <typename U> A<T>::B\n")
+
+    app1 = make_app(*args, **kwargs)
+    restructuredtext.parse(app=app1, text=text1, docname='text1')
+    root1 = app1.env.domaindata['cpp']['root_symbol']
+
+    assert root1.dump(1) == (
+        '  ::\n'
+        '    template<typename T> \n'
+        '    A: {class} template<typename T> A\t(text1)\n'
+        '      T: {templateParam} typename T\t(text1)\n'
+    )
+
+    app2 = make_app(*args, **kwargs)
+    restructuredtext.parse(app=app2, text=text2, docname='text2')
+    root2 = app2.env.domaindata['cpp']['root_symbol']
+
+    assert root2.dump(1) == (
+        '  ::\n'
+        '    template<typename T> \n'
+        '    A\n'
+        '      T\n'
+        '      template<typename U> \n'
+        '      B: {class} template<typename T> template<typename U> A<T>::B\t(text2)\n'
+        '        U: {templateParam} typename U\t(text2)\n'
+    )
+
+    root2.merge_with(root1, ['text1'], app2.env)
+
+    assert root2.dump(1) == (
+        '  ::\n'
+        '    template<typename T> \n'
+        '    A: {class} template<typename T> A\t(text1)\n'
+        '      T: {templateParam} typename T\t(text1)\n'
+        '      template<typename U> \n'
+        '      B: {class} template<typename T> template<typename U> A<T>::B\t(text2)\n'
+        '        U: {templateParam} typename U\t(text2)\n'
+    )
+    warning = app2._warning.getvalue()
+    assert 'Internal C++ domain error during symbol merging' not in warning