From 5b97ec522a3844022340ae4dd657b8f55adb1329 Mon Sep 17 00:00:00 2001 From: Jeremy Maitin-Shepard Date: Fri, 11 Mar 2022 06:51:30 -0800 Subject: [PATCH 1/6] [C++] Ensure consistent non-specialization template argument representation Previously, in certain cases the template arguments of non-specializations were retained, leading to incorrect merging of symbols. --- sphinx/domains/cpp.py | 109 ++++++++++++++++++++++++++++----------- tests/test_domain_cpp.py | 92 +++++++++++++++++++++++++++++++-- 2 files changed, 167 insertions(+), 34 deletions(-) diff --git a/sphinx/domains/cpp.py b/sphinx/domains/cpp.py index 2534f6239..e9f6264fd 100644 --- a/sphinx/domains/cpp.py +++ b/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, @@ -4129,6 +4153,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 +4226,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 class A + # + # and + # + # .. cpp:function:: template int A::foo() + if (templateArgs is not None and + not _is_specialization(templateParams, templateArgs)): + templateArgs = None self.templateParams = templateParams # template self.templateArgs = templateArgs # identifier self.declaration = declaration @@ -4357,33 +4416,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 int A::var; - if not isSpecialization(): + if not _is_specialization(templateParams, templateArgs): templateArgs = None def matches(s: "Symbol") -> bool: @@ -4839,14 +4877,23 @@ class Symbol: ourChild.declaration.directiveType, name) logger.warning(msg, location=(otherChild.docname, otherChild.line)) else: - # Both have declarations, and in the same docname. - # This can apparently happen, it should be safe to - # just ignore it, right? - # Hmm, only on duplicate declarations, right? - msg = "Internal C++ domain error during symbol merging.\n" - msg += "ourChild:\n" + ourChild.to_string(1) - msg += "\notherChild:\n" + otherChild.to_string(1) - logger.warning(msg, location=otherChild.docname) + if (otherChild.declaration.objectType == + ourChild.declaration.objectType and + otherChild.declaration.objectType in + ('templateParam', 'functionParam')): + # `ourChild` was presumably just created during mergging + # 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 + # just ignore it, right? + # Hmm, only on duplicate declarations, right? + msg = "Internal C++ domain error during symbol merging.\n" + msg += "ourChild:\n" + ourChild.to_string(1) + msg += "\notherChild:\n" + otherChild.to_string(1) + logger.warning(msg, location=otherChild.docname) ourChild.merge_with(otherChild, docnames, env) if Symbol.debug_lookup: Symbol.debug_indent -= 2 diff --git a/tests/test_domain_cpp.py b/tests/test_domain_cpp.py index ee8da3d39..23c496b45 100644 --- a/tests/test_domain_cpp.py +++ b/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', - {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', - {2: 'I00DpEXN3abc2ns3fooEI4id_04id_1sp4id_2EEN3xyz3barI4id_04id_1Dp4id_2EE'}) + {2: 'I00DpEXN3abc2ns3fooEI4id_04id_1sp4id_2EEN3xyz3barE'}) check('class', 'template<> Concept{{U}} {key}A::B', {2: 'IEI0EX7ConceptI1UEEN1AIiE1BE'}) @@ -901,7 +901,7 @@ def test_domain_cpp_ast_requires_clauses(): 'template requires R ' + 'template requires S ' + 'void A::f() requires B', - {4: 'I0EIQ1RI1TEEI0EIQaa1SI1TE1BEN1AI1TE1fEvv'}) + {4: 'I0EIQ1RI1TEEI0EIQaa1SI1TE1BEN1A1fEvv'}) check('function', 'template requires R typename X> ' + 'void f()', @@ -1395,3 +1395,89 @@ 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 A\n") + text2 = (".. cpp:class:: template template A::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 \n' + ' A: template A\t(text1)\n' + ' T: 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 \n' + ' A\n' + ' T\n' + ' template \n' + ' B: template template A::B\t(text2)\n' + ' U: typename U\t(text2)\n' + ) + + root2.merge_with(root1, ['text1'], app2.env) + + assert root2.dump(1) == ( + ' ::\n' + ' template \n' + ' A: template A\t(text1)\n' + ' T: typename T\t(text1)\n' + ' template \n' + ' B: template template A::B\t(text2)\n' + ' U: typename U\t(text2)\n' + ) + warning = app2._warning.getvalue() + assert 'Internal C++ domain error during symbol merging' not in warning + + +def parse_template_parameter(param: str): + ast = parse('type', 'template<' + param + '> X') + return ast.templatePrefix.templates[0].params[0] + + +@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), + # TODO: the following two declarations cannot currently be parsed + # ('int (X::*)(bool)...', True), + # ('int (X::*x...)(bool)', True), + ('template class', False), + ('template class...', True), + ]) +def test_domain_cpp_template_parameters_is_pack(param: str, is_pack: bool): + ast = parse_template_parameter(param) + assert ast.isPack == is_pack From 66f98957b1ff8636d47a2f1e4cfaff4432618cfe Mon Sep 17 00:00:00 2001 From: Jakob Lykke Andersen Date: Fri, 29 Jul 2022 17:22:14 +0200 Subject: [PATCH 2/6] C++, ensure merging case is as assumed --- sphinx/domains/cpp.py | 11 ++++++----- tests/test_domain_cpp.py | 16 ++++++++-------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/sphinx/domains/cpp.py b/sphinx/domains/cpp.py index e9f6264fd..3b11f15e9 100644 --- a/sphinx/domains/cpp.py +++ b/sphinx/domains/cpp.py @@ -4850,7 +4850,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 @@ -4880,10 +4880,10 @@ class Symbol: if (otherChild.declaration.objectType == ourChild.declaration.objectType and otherChild.declaration.objectType in - ('templateParam', 'functionParam')): - # `ourChild` was presumably just created during mergging - # by the call to `_fill_empty` on the parent and can be - # ignored. + ('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. @@ -5153,6 +5153,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(') diff --git a/tests/test_domain_cpp.py b/tests/test_domain_cpp.py index 23c496b45..41ea21e3d 100644 --- a/tests/test_domain_cpp.py +++ b/tests/test_domain_cpp.py @@ -1414,8 +1414,8 @@ def test_domain_cpp_normalize_unspecialized_template_args(make_app, app_params): assert root1.dump(1) == ( ' ::\n' ' template \n' - ' A: template A\t(text1)\n' - ' T: typename T\t(text1)\n' + ' A: {class} template A\t(text1)\n' + ' T: {templateParam} typename T\t(text1)\n' ) app2 = make_app(*args, **kwargs) @@ -1428,8 +1428,8 @@ def test_domain_cpp_normalize_unspecialized_template_args(make_app, app_params): ' A\n' ' T\n' ' template \n' - ' B: template template A::B\t(text2)\n' - ' U: typename U\t(text2)\n' + ' B: {class} template template A::B\t(text2)\n' + ' U: {templateParam} typename U\t(text2)\n' ) root2.merge_with(root1, ['text1'], app2.env) @@ -1437,11 +1437,11 @@ def test_domain_cpp_normalize_unspecialized_template_args(make_app, app_params): assert root2.dump(1) == ( ' ::\n' ' template \n' - ' A: template A\t(text1)\n' - ' T: typename T\t(text1)\n' + ' A: {class} template A\t(text1)\n' + ' T: {templateParam} typename T\t(text1)\n' ' template \n' - ' B: template template A::B\t(text2)\n' - ' U: typename U\t(text2)\n' + ' B: {class} template template A::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 From a95949aa1319e1a7c0b5d2f6a91577b04838c146 Mon Sep 17 00:00:00 2001 From: Jakob Lykke Andersen Date: Fri, 29 Jul 2022 18:04:36 +0200 Subject: [PATCH 3/6] C++, add changes entry PR --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index abe17757f..70ac240f4 100644 --- a/CHANGES +++ b/CHANGES @@ -19,6 +19,9 @@ Features added Bugs fixed ---------- +- #10257: C++, ensure consistent non-specialization template argument + representation. + Testing -------- From aa43a378655a62ecf43199abe5edd0059fb989bc Mon Sep 17 00:00:00 2001 From: Jakob Lykke Andersen Date: Fri, 29 Jul 2022 17:49:01 +0200 Subject: [PATCH 4/6] C++, fix parsing of certain non-type template parameters Specifically 'template' --- CHANGES | 1 + sphinx/domains/cpp.py | 26 +++++++++++++++++++------- tests/test_domain_cpp.py | 7 ++++--- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/CHANGES b/CHANGES index 70ac240f4..f9f689895 100644 --- a/CHANGES +++ b/CHANGES @@ -21,6 +21,7 @@ Bugs fixed - #10257: C++, ensure consistent non-specialization template argument representation. +- #10729: C++, fix parsing of certain non-type template parameter packs. Testing -------- diff --git a/sphinx/domains/cpp.py b/sphinx/domains/cpp.py index 3b11f15e9..ac13978d0 100644 --- a/sphinx/domains/cpp.py +++ b/sphinx/domains/cpp.py @@ -3668,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: @@ -3679,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 @@ -3700,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): @@ -4155,7 +4165,7 @@ class LookupKey: def _is_specialization(templateParams: Union[ASTTemplateParams, ASTTemplateIntroduction], templateArgs: ASTTemplateArgs) -> bool: - """Checks if `templateArgs` does not exactly match `templateParams`.""" + # 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): @@ -6775,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 @@ -6822,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." diff --git a/tests/test_domain_cpp.py b/tests/test_domain_cpp.py index 41ea21e3d..54c4d35ec 100644 --- a/tests/test_domain_cpp.py +++ b/tests/test_domain_cpp.py @@ -876,6 +876,9 @@ def test_domain_cpp_ast_templates(): # defaulted constrained type parameters check('type', 'template {key}A', {2: 'I_1CE1A'}, key='using') + # pack expansion after non-type template parameter + check('type', 'template {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'}) @@ -1472,9 +1475,7 @@ def parse_template_parameter(param: str): ('int X::*...', True), ('int (X::*)(bool)', False), ('int (X::*x)(bool)', False), - # TODO: the following two declarations cannot currently be parsed - # ('int (X::*)(bool)...', True), - # ('int (X::*x...)(bool)', True), + ('int (X::*)(bool)...', True), ('template class', False), ('template class...', True), ]) From 64c1b2be2e7ced003b5ae7d1b6232965d18ec363 Mon Sep 17 00:00:00 2001 From: Jakob Lykke Andersen Date: Fri, 29 Jul 2022 18:21:34 +0200 Subject: [PATCH 5/6] C++, restructure tests --- tests/test_domain_cpp.py | 66 +++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/tests/test_domain_cpp.py b/tests/test_domain_cpp.py index 54c4d35ec..5e9bc6aa3 100644 --- a/tests/test_domain_cpp.py +++ b/tests/test_domain_cpp.py @@ -1043,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 class', False), + ('template 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: @@ -1448,37 +1480,3 @@ def test_domain_cpp_normalize_unspecialized_template_args(make_app, app_params): ) warning = app2._warning.getvalue() assert 'Internal C++ domain error during symbol merging' not in warning - - -def parse_template_parameter(param: str): - ast = parse('type', 'template<' + param + '> X') - return ast.templatePrefix.templates[0].params[0] - - -@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 class', False), - ('template class...', True), - ]) -def test_domain_cpp_template_parameters_is_pack(param: str, is_pack: bool): - ast = parse_template_parameter(param) - assert ast.isPack == is_pack From f5c21a118f58af94bbd636d8defc33f4519a49df Mon Sep 17 00:00:00 2001 From: Jakob Lykke Andersen Date: Fri, 29 Jul 2022 17:55:58 +0200 Subject: [PATCH 6/6] C++, bump env version due to AST and Symbol changes --- sphinx/domains/cpp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sphinx/domains/cpp.py b/sphinx/domains/cpp.py index ac13978d0..3dedb49c1 100644 --- a/sphinx/domains/cpp.py +++ b/sphinx/domains/cpp.py @@ -8149,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, }