Skip to content

Commit

Permalink
[C++] Ensure consistent non-specialization template argument represen…
Browse files Browse the repository at this point in the history 
…tation

Previously, in certain cases the template arguments of
non-specializations were retained, leading to incorrect merging of symbols.
  • Loading branch information
@jbms
jbms committed Mar 11, 2022
1 parent b3812f7 commit 162963b
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 32 deletions.
95 changes: 65 additions & 30 deletions sphinx/domains/cpp.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2756,6 +2756,10 @@ def function_params(self) -> List[ASTFunctionParameter]:
def trailingReturn(self) -> "ASTType":
return self.next.trailingReturn

@property
def isPack(self) -> bool:
return True

def require_space_after_declSpecs(self) -> bool:
return False

Expand Down Expand Up @@ -3457,6 +3461,14 @@ def describe_signature(self, parentNode: TextElement, mode: str,
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,
Expand Down Expand Up @@ -4041,6 +4053,31 @@ def __init__(self, data: List[Tuple[ASTNestedNameElement,
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 = " "
Expand Down Expand Up @@ -4089,6 +4126,16 @@ def __init__(self, parent: "Symbol", identOrOp: Union[ASTIdentifier, ASTOperator
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
Expand Down Expand Up @@ -4269,33 +4316,12 @@ def _find_named_symbols(self, identOrOp: Union[ASTIdentifier, ASTOperator],
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:
Expand Down Expand Up @@ -4751,14 +4777,23 @@ def unconditionalAdd(self, otherChild):
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
Expand Down
54 changes: 52 additions & 2 deletions tests/test_domain_cpp.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -846,9 +846,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'})

Expand Down Expand Up @@ -1367,3 +1367,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: template<typename T> 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<typename T> \n'
' A\n'
' T\n'
' template<typename U> \n'
' B: template<typename T> template<typename U> A<T>::B\t(text2)\n'
' U: typename U\t(text2)\n'
)

root2.merge_with(root1, ['text1'], app2.env)

assert root2.dump(1) == (
' ::\n'
' template<typename T> \n'
' A: template<typename T> A\t(text1)\n'
' T: typename T\t(text1)\n'
' template<typename U> \n'
' B: template<typename T> template<typename U> A<T>::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

0 comments on commit 162963b

Please sign in to comment.