From a7c4be2fdccbda80517b49fc40fb1f1bf53fc3f8 Mon Sep 17 00:00:00 2001 From: John Vandenberg Date: Sat, 24 Oct 2020 19:22:05 +0700 Subject: [PATCH] Improve substitution recursion check Creates SubstitutionStackError and trace recursion from the beginning of substitution, allowing prevention of the recursion to halt sooner, and avoid None in error message. Related to https://github.com/tox-dev/tox/issues/1716 --- src/tox/config/__init__.py | 23 ++++++++++++----------- src/tox/exception.py | 4 ++++ tests/unit/config/test_config.py | 13 +++++-------- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/tox/config/__init__.py b/src/tox/config/__init__.py index 8d8e559cc..6e80d4061 100644 --- a/src/tox/config/__init__.py +++ b/src/tox/config/__init__.py @@ -391,7 +391,7 @@ def get(self, name, default=None): return os.environ.get(name, default) self._lookupstack.append(name) try: - self.resolved[name] = res = self.reader._replace(val) + self.resolved[name] = res = self.reader._replace(val, name="setenv") finally: self._lookupstack.pop() return res @@ -1693,7 +1693,7 @@ def getbool(self, name, default=None, replace=True): def getargvlist(self, name, default="", replace=True): s = self.getstring(name, default, replace=False) - return _ArgvlistReader.getargvlist(self, s, replace=replace) + return _ArgvlistReader.getargvlist(self, s, replace=replace, name=name) def getargv(self, name, default="", replace=True): return self.getargvlist(name, default, replace=replace)[0] @@ -1712,7 +1712,7 @@ def getargv_install_command(self, name, default="", replace=True): if "{opts}" in s: s = s.replace("{opts}", r"\{opts\}") - return _ArgvlistReader.getargvlist(self, s, replace=replace)[0] + return _ArgvlistReader.getargvlist(self, s, replace=replace, name=name)[0] def getstring(self, name, default=None, replace=True, crossonly=False, no_fallback=False): x = None @@ -1775,6 +1775,7 @@ def _replace(self, value, name=None, section_name=None, crossonly=False): return value section_name = section_name if section_name else self.section_name + assert name self._subststack.append((section_name, name)) try: replaced = Replacer(self, crossonly=crossonly).do_replace(value) @@ -1890,7 +1891,7 @@ def _substitute_from_other_section(self, key): cfg = self.reader._cfg if section in cfg and item in cfg[section]: if (section, item) in self.reader._subststack: - raise ValueError( + raise tox.exception.SubstitutionStackError( "{} already in {}".format((section, item), self.reader._subststack), ) x = str(cfg[section][item]) @@ -1918,7 +1919,7 @@ def is_interactive(): class _ArgvlistReader: @classmethod - def getargvlist(cls, reader, value, replace=True): + def getargvlist(cls, reader, value, replace=True, name=None): """Parse ``commands`` argvlist multiline string. :param SectionReader reader: reader to be used. @@ -1940,10 +1941,10 @@ def getargvlist(cls, reader, value, replace=True): current_command += line if is_section_substitution(current_command): - replaced = reader._replace(current_command, crossonly=True) - commands.extend(cls.getargvlist(reader, replaced)) + replaced = reader._replace(current_command, crossonly=True, name=name) + commands.extend(cls.getargvlist(reader, replaced, name=name)) else: - commands.append(cls.processcommand(reader, current_command, replace)) + commands.append(cls.processcommand(reader, current_command, replace, name=name)) current_command = "" else: if current_command: @@ -1956,7 +1957,7 @@ def getargvlist(cls, reader, value, replace=True): return commands @classmethod - def processcommand(cls, reader, command, replace=True): + def processcommand(cls, reader, command, replace=True, name=None): # Iterate through each word of the command substituting as # appropriate to construct the new command string. This # string is then broken up into exec argv components using @@ -1969,8 +1970,8 @@ def processcommand(cls, reader, command, replace=True): continue new_arg = "" - new_word = reader._replace(word) - new_word = reader._replace(new_word) + new_word = reader._replace(word, name=name) + new_word = reader._replace(new_word, name=name) new_word = Replacer._unescape(new_word) new_arg += new_word newcommand += new_arg diff --git a/src/tox/exception.py b/src/tox/exception.py index b2fba4d16..c5f842acf 100644 --- a/src/tox/exception.py +++ b/src/tox/exception.py @@ -56,6 +56,10 @@ class ConfigError(Error): """Error in tox configuration.""" +class SubstitutionStackError(ConfigError, ValueError): + """Error in tox configuration recursive substitution.""" + + class UnsupportedInterpreter(Error): """Signals an unsupported Interpreter.""" diff --git a/tests/unit/config/test_config.py b/tests/unit/config/test_config.py index 548d762c4..d53f9cf86 100644 --- a/tests/unit/config/test_config.py +++ b/tests/unit/config/test_config.py @@ -438,10 +438,7 @@ class TestIniParserAgainstCommandsKey: """Test parsing commands with substitutions""" def test_command_substitution_recursion_error_same_section(self, newconfig): - expected = ( - r"\('testenv:a', 'commands'\) already in " - r"\[\('testenv:a', None\), \('testenv:a', 'commands'\)\]" - ) + expected = r"\('testenv:a', 'commands'\) already in \[\('testenv:a', 'commands'\)\]" with pytest.raises(tox.exception.ConfigError, match=expected): newconfig( """ @@ -452,9 +449,9 @@ def test_command_substitution_recursion_error_same_section(self, newconfig): def test_command_substitution_recursion_error_other_section(self, newconfig): expected = ( - r"\('testenv:base', 'foo'\) already in " - r"\[\('testenv:py27', None\), \('testenv:base', 'foo'\), " - r"\('testenv:py27', 'commands'\)\]" + r"\('testenv:py27', 'commands'\) already in " + r"\[\('testenv:py27', 'commands'\), " + r"\('testenv:base', 'foo'\)\]" ) with pytest.raises(tox.exception.ConfigError, match=expected): newconfig( @@ -472,7 +469,7 @@ def test_command_substitution_recursion_error_unnecessary(self, newconfig): # could be optimised away, or emit a warning, or give a custom error expected = ( r"\('testenv:base', 'foo'\) already in " - r"\[\('testenv:py27', None\), \('testenv:base', 'foo'\)\]" + r"\[\('testenv:py27', 'commands'\), \('testenv:base', 'foo'\)\]" ) with pytest.raises(tox.exception.ConfigError, match=expected): newconfig(