diff --git a/changelog/15137.bugfix.rst b/changelog/15137.bugfix.rst new file mode 100644 index 00000000000..25d66cae6c6 --- /dev/null +++ b/changelog/15137.bugfix.rst @@ -0,0 +1 @@ +The deprecation warning for passing a non-string value to a ``string``-typed ini option no longer reuses one warning instance, so it no longer accumulates traceback frames when warnings are turned into errors -- the case :issue:`14912` fixed for the other deprecations. diff --git a/src/_pytest/config/__init__.py b/src/_pytest/config/__init__.py index 60ba5f54a89..feeca449fc4 100644 --- a/src/_pytest/config/__init__.py +++ b/src/_pytest/config/__init__.py @@ -301,11 +301,9 @@ def console_main() -> int: This function is slated for removal in pytest 10. It is not meant for programmable use; use :func:`pytest.main` instead. """ - import warnings - from _pytest.deprecated import CONSOLE_MAIN - warnings.warn(CONSOLE_MAIN.format(), stacklevel=2) + CONSOLE_MAIN.warn(stacklevel=2) return _console_main() @@ -1283,10 +1281,7 @@ def inicfg(self) -> _DeprecatedInicfgProxy: @property def inicfg(self) -> _DeprecatedInicfgProxy: - warnings.warn( - _pytest.deprecated.CONFIG_INICFG.format(), - stacklevel=2, - ) + _pytest.deprecated.CONFIG_INICFG.warn(stacklevel=2) return _DeprecatedInicfgProxy(self) @property @@ -2052,10 +2047,7 @@ def _getini_ini( return _strtobool(str(value).strip()) elif type == "string": if not isinstance(value, str): - warnings.warn( - _pytest.deprecated.INI_STRING_TYPE_NON_STR_VALUE, - stacklevel=2, - ) + _pytest.deprecated.INI_STRING_TYPE_NON_STR_VALUE.warn(stacklevel=2) return value elif type == "int": if not isinstance(value, str): diff --git a/src/_pytest/deprecated.py b/src/_pytest/deprecated.py index a94aeb735e5..a66e9c08764 100644 --- a/src/_pytest/deprecated.py +++ b/src/_pytest/deprecated.py @@ -5,19 +5,16 @@ be removed when the time comes. All constants defined in this module should be instances of -:class:`UnformattedWarning`, whose ``format`` method returns a fresh warning -instance each time. Do not warn with shared warning instances: if a warning is -raised as an error, CPython appends to the exception's existing ``__traceback__``, -so a process-lifetime instance would accumulate stale frames from previous raises. +:class:`WarningTemplate`. Emit them with its ``warn`` method, or pass the fresh +instance from ``format`` to an API that takes a warning; see its docstring for +why a warning instance is never kept around. """ from __future__ import annotations -from warnings import warn - from _pytest.warning_types import PytestDeprecationWarning from _pytest.warning_types import PytestRemovedIn10Warning -from _pytest.warning_types import UnformattedWarning +from _pytest.warning_types import WarningTemplate # set of plugins which have been integrated into the core; we use this list to ignore @@ -31,13 +28,13 @@ # This could have been removed pytest 8, but it's harmless and common, so no rush to remove. -YIELD_FIXTURE = UnformattedWarning( +YIELD_FIXTURE = WarningTemplate( PytestRemovedIn10Warning, "@pytest.yield_fixture is deprecated.\n" "Use @pytest.fixture instead; they are the same.", ) -CLASS_FIXTURE_INSTANCE_METHOD = UnformattedWarning( +CLASS_FIXTURE_INSTANCE_METHOD = WarningTemplate( PytestRemovedIn10Warning, "{scope}-scoped fixtures defined as instance methods are deprecated.\n" "Instance attributes set in the {fixturename!r} fixture will NOT be visible to test methods,\n" @@ -47,13 +44,13 @@ ) # This deprecation is never really meant to be removed. -PRIVATE = UnformattedWarning( +PRIVATE = WarningTemplate( PytestDeprecationWarning, "A private pytest class or function was used.", ) -HOOK_LEGACY_MARKING = UnformattedWarning( +HOOK_LEGACY_MARKING = WarningTemplate( PytestRemovedIn10Warning, "The hook{type} {fullname} uses old-style configuration options (marks or attributes).\n" "Please use the pytest.hook{type}({hook_opts}) decorator instead\n" @@ -62,7 +59,7 @@ "#configuring-hook-specs-impls-using-markers", ) -MONKEYPATCH_LEGACY_NAMESPACE_PACKAGES = UnformattedWarning( +MONKEYPATCH_LEGACY_NAMESPACE_PACKAGES = WarningTemplate( PytestRemovedIn10Warning, "monkeypatch.syspath_prepend() called with pkg_resources legacy namespace packages detected.\n" "Legacy namespace packages (using pkg_resources.declare_namespace) are deprecated.\n" @@ -70,7 +67,7 @@ "See https://docs.pytest.org/en/stable/deprecations.html#monkeypatch-fixup-namespace-packages", ) -PARAMETRIZE_NON_COLLECTION_ITERABLE = UnformattedWarning( +PARAMETRIZE_NON_COLLECTION_ITERABLE = WarningTemplate( PytestRemovedIn10Warning, "Passing a non-Collection iterable to parametrize is deprecated.\n" "Test: {nodeid}, argvalues type: {type_name}\n" @@ -78,20 +75,20 @@ "See https://docs.pytest.org/en/stable/deprecations.html#parametrize-iterators", ) -CONSOLE_MAIN = UnformattedWarning( +CONSOLE_MAIN = WarningTemplate( PytestRemovedIn10Warning, "pytest.console_main() is deprecated and will be removed in pytest 10.\n" "It was never intended for programmatic use; use pytest.main() instead.\n" "See https://docs.pytest.org/en/stable/deprecations.html#console-main", ) -CONFIG_INICFG = UnformattedWarning( +CONFIG_INICFG = WarningTemplate( PytestRemovedIn10Warning, "config.inicfg is deprecated, use config.getini() to access configuration values instead.\n" "See https://docs.pytest.org/en/stable/deprecations.html#config-inicfg", ) -FIXTURE_GETFIXTUREVALUE_DURING_TEARDOWN = UnformattedWarning( +FIXTURE_GETFIXTUREVALUE_DURING_TEARDOWN = WarningTemplate( PytestRemovedIn10Warning, 'Calling request.getfixturevalue("{argname}") during teardown is deprecated.\n' "Please request the fixture before teardown begins, either by declaring it in the fixture signature " @@ -99,19 +96,20 @@ "See https://docs.pytest.org/en/stable/deprecations.html#dynamic-fixture-request-during-teardown", ) -PASTEBIN = UnformattedWarning( +PASTEBIN = WarningTemplate( PytestRemovedIn10Warning, "The --pastebin option is deprecated. " "The functionality is now available in an external plugin package, pytest-pastebin.\n" "See https://docs.pytest.org/en/stable/deprecations.html#the-pastebin-option", ) -INI_STRING_TYPE_NON_STR_VALUE = PytestRemovedIn10Warning( +INI_STRING_TYPE_NON_STR_VALUE = WarningTemplate( + PytestRemovedIn10Warning, "Passing a value that is not a string to a 'string'-typed ini option is deprecated.\n" "In a future version this will raise a TypeError, matching the behavior of the " "corresponding TOML config path.\n" "If your plugin intentionally accepts non-string values, declare an explicit type " - '(e.g. type="args") instead of relying on the implicit string default.' + '(e.g. type="args") instead of relying on the implicit string default.', ) # You want to make some `__init__` or function "private". @@ -133,30 +131,30 @@ # the warning (possibly error in the future). -FIXTURE_BASEID_DEPRECATED = UnformattedWarning( +FIXTURE_BASEID_DEPRECATED = WarningTemplate( PytestRemovedIn10Warning, "Passing baseid to FixtureDef is deprecated. Pass node instead for fixture scoping.", ) -FIXTURE_NODEID_DEPRECATED = UnformattedWarning( +FIXTURE_NODEID_DEPRECATED = WarningTemplate( PytestRemovedIn10Warning, "Passing nodeid to _register_fixture is deprecated. " "Pass node instead for fixture scoping.", ) -FIXTUREDEF_HAS_LOCATION_DEPRECATED = UnformattedWarning( +FIXTUREDEF_HAS_LOCATION_DEPRECATED = WarningTemplate( PytestRemovedIn10Warning, "FixtureDef.has_location is deprecated and will be removed in pytest 10. " "See https://docs.pytest.org/en/stable/deprecations.html#fixturedef-has-location-deprecated", ) -PARSEFACTORIES_NODEID_DEPRECATED = UnformattedWarning( +PARSEFACTORIES_NODEID_DEPRECATED = WarningTemplate( PytestRemovedIn10Warning, "Passing nodeid string to parsefactories is deprecated. " "Use parsefactories(holder=obj, node=node) instead.", ) -CALLSPEC2_RENAMED = UnformattedWarning( +CALLSPEC2_RENAMED = WarningTemplate( PytestRemovedIn10Warning, "_pytest.python.CallSpec2 has been renamed to CallSpec.\n" "The CallSpec2 alias will be removed in pytest 10.\n" @@ -167,4 +165,4 @@ def check_ispytest(ispytest: bool) -> None: if not ispytest: - warn(PRIVATE.format(), stacklevel=3) + PRIVATE.warn(stacklevel=3) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 20e43f89132..bdbadddaa4d 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -30,7 +30,6 @@ from typing import overload from typing import TYPE_CHECKING from typing import TypeVar -import warnings import _pytest from _pytest import nodes @@ -777,10 +776,7 @@ def _get_active_fixturedef(self, argname: str) -> FixtureDef[object]: if not self.session._setupstate.is_node_active(self.node): # TODO(pytest10.1): Remove the `warn` and `if` and call # _raise_teardown_lookup_error unconditionally. - warnings.warn( - FIXTURE_GETFIXTUREVALUE_DURING_TEARDOWN.format(argname=argname), - stacklevel=3, - ) + FIXTURE_GETFIXTUREVALUE_DURING_TEARDOWN.warn(stacklevel=3, argname=argname) if subrequest.node not in self.session._setupstate.stack: self._raise_teardown_lookup_error(argname) @@ -1132,7 +1128,7 @@ def __init__( check_ispytest(_ispytest) # Emit deprecation warning if deprecated baseid string is used. if node is NOTSET: - warnings.warn(FIXTURE_BASEID_DEPRECATED.format(), stacklevel=2) + FIXTURE_BASEID_DEPRECATED.warn(stacklevel=2) if baseid is NOTSET: baseid = None # The node where this fixture was defined, if available. @@ -1197,7 +1193,7 @@ def scope(self) -> ScopeName: @property def has_location(self) -> bool: - warnings.warn(FIXTUREDEF_HAS_LOCATION_DEPRECATED.format(), stacklevel=2) + FIXTUREDEF_HAS_LOCATION_DEPRECATED.warn(stacklevel=2) return self._has_location def addfinalizer(self, finalizer: Callable[[], object]) -> None: @@ -1350,12 +1346,10 @@ def __tracebackhide__(e): # classmethod: bound_to is the class itself (a type) # instance method: bound_to is an instance (not a type) if not isinstance(bound_to, type): - warnings.warn( - CLASS_FIXTURE_INSTANCE_METHOD.format( - scope=fixturedef._scope.name.capitalize(), - fixturename=request.fixturename, - ), + CLASS_FIXTURE_INSTANCE_METHOD.warn( stacklevel=2, + scope=fixturedef._scope.name.capitalize(), + fixturename=request.fixturename, ) if instance is not None: @@ -1620,7 +1614,7 @@ def yield_fixture( .. deprecated:: 3.0 Use :py:func:`pytest.fixture` directly instead. """ - warnings.warn(YIELD_FIXTURE.format(), stacklevel=2) + YIELD_FIXTURE.warn(stacklevel=2) return fixture( fixture_function, *args, @@ -2091,7 +2085,7 @@ def _register_fixture( """ # Emit deprecation warning if nodeid string. if nodeid is not NOTSET or node is NOTSET: - warnings.warn(FIXTURE_NODEID_DEPRECATED.format(), stacklevel=2) + FIXTURE_NODEID_DEPRECATED.warn(stacklevel=2) fixture_def = FixtureDef( config=self.config, baseid=nodeid, @@ -2280,7 +2274,7 @@ def parsefactories( raise TypeError("parsefactories() requires holder or node_or_obj") elif nodeid is not NOTSET: # Legacy: parsefactories(obj, nodeid) - string-based scoping only. - warnings.warn(PARSEFACTORIES_NODEID_DEPRECATED.format(), stacklevel=2) + PARSEFACTORIES_NODEID_DEPRECATED.warn(stacklevel=2) holderobj = node_or_obj effective_nodeid = nodeid else: diff --git a/src/_pytest/mark/structures.py b/src/_pytest/mark/structures.py index 34a004f4881..572fac99fa0 100644 --- a/src/_pytest/mark/structures.py +++ b/src/_pytest/mark/structures.py @@ -202,12 +202,10 @@ def _for_parametrize( nodeid: str, ) -> tuple[Sequence[str], list[ParameterSet]]: if not isinstance(argvalues, Collection): - warnings.warn( - PARAMETRIZE_NON_COLLECTION_ITERABLE.format( - nodeid=nodeid, - type_name=type(argvalues).__name__, - ), + PARAMETRIZE_NON_COLLECTION_ITERABLE.warn( stacklevel=3, + nodeid=nodeid, + type_name=type(argvalues).__name__, ) argnames, force_tuple = cls._parse_parametrize_args(argnames, argvalues) diff --git a/src/_pytest/monkeypatch.py b/src/_pytest/monkeypatch.py index 9b9bcd3fb95..52e4ca55145 100644 --- a/src/_pytest/monkeypatch.py +++ b/src/_pytest/monkeypatch.py @@ -384,9 +384,7 @@ def syspath_prepend(self, path) -> None: continue ns_pkg_path = path_obj / ns_pkg.replace(".", os.sep) if ns_pkg_path.is_dir(): - warnings.warn( - MONKEYPATCH_LEGACY_NAMESPACE_PACKAGES.format(), stacklevel=2 - ) + MONKEYPATCH_LEGACY_NAMESPACE_PACKAGES.warn(stacklevel=2) break fixup_namespace_packages(str(path)) diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 1ce7f71a7bd..b8ec09a8b9c 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -1852,6 +1852,6 @@ def runtest(self) -> None: def __getattr__(name: str) -> object: if name == "CallSpec2": - warnings.warn(CALLSPEC2_RENAMED.format(), stacklevel=2) + CALLSPEC2_RENAMED.warn(stacklevel=2) return CallSpec raise AttributeError(f"module {__name__!r} has no attribute {name!r}") diff --git a/src/_pytest/warning_types.py b/src/_pytest/warning_types.py index c5b339dec4c..29eb44e2c85 100644 --- a/src/_pytest/warning_types.py +++ b/src/_pytest/warning_types.py @@ -118,21 +118,48 @@ class PytestUnhandledThreadExceptionWarning(PytestWarning): @final -@dataclasses.dataclass -class UnformattedWarning(Generic[_W]): - """A warning meant to be formatted during runtime. - - This is used to hold warnings that need to format their message at runtime, - as opposed to a direct message. +@dataclasses.dataclass(frozen=True) +class WarningTemplate(Generic[_W]): + """A warning category paired with its message template. + + Emitting a warning goes through this type so that a *fresh* warning instance + is built every time. + + Warning instances must never be kept around and reused: under an ``error`` + filter ``warnings.warn(instance)`` raises that very object, so CPython + appends to its existing ``__traceback__`` instead of replacing it, and + ``__context__`` picks up whatever exception happened to be in flight. A + shared instance therefore grows a traceback without bound, pins the locals + of unrelated frames, and shows frames of earlier, unrelated code in failure + reports (#14912). """ category: type[_W] template: str def format(self, **kwargs: Any) -> _W: - """Return an instance of the warning category, formatted with given kwargs.""" + """Return a new instance of the warning category. + + The template is only run through :meth:`str.format` when ``kwargs`` are + given, so constant messages may contain literal braces unescaped. + """ + if not kwargs: + return self.category(self.template) return self.category(self.template.format(**kwargs)) + def warn(self, *, stacklevel: int = 1, **kwargs: Any) -> None: + """Emit the warning, with ``stacklevel`` relative to the caller. + + The extra level compensates for this method's own frame, so callers pass + the same ``stacklevel`` they would pass to :func:`warnings.warn`. + """ + __tracebackhide__ = True + warnings.warn(self.format(**kwargs), stacklevel=stacklevel + 1) + + +#: Kept for third-party code that reached into this private module. +UnformattedWarning = WarningTemplate + @final class PytestApproxDecimalToleranceWarning(PytestWarning): diff --git a/testing/deprecated_test.py b/testing/deprecated_test.py index 5de659a481f..bdfe73247c8 100644 --- a/testing/deprecated_test.py +++ b/testing/deprecated_test.py @@ -1,11 +1,16 @@ # mypy: allow-untyped-defs from __future__ import annotations +import importlib +import pkgutil +import string from types import TracebackType +import _pytest from _pytest import deprecated from _pytest.pytester import Pytester from _pytest.scope import Scope +from _pytest.warning_types import WarningTemplate import pytest from pytest import PytestDeprecationWarning from pytest import PytestRemovedIn10Warning @@ -401,3 +406,75 @@ def raise_and_count() -> int: # The traceback of a shared instance would grow with every raise. assert second <= first + + +class TestNoSharedWarningInstances: + """Deprecations must not be emitted from shared ``Warning`` instances (#14912). + + Under an ``error`` filter ``warnings.warn(instance)`` raises that very object, + so a module-level instance accumulates traceback frames and ``__context__`` + across unrelated emissions. + """ + + @staticmethod + def _raise_private() -> PytestDeprecationWarning: + with pytest.raises(PytestDeprecationWarning) as excinfo: + deprecated.check_ispytest(False) + return excinfo.value + + @pytest.mark.filterwarnings("error") + def test_unrelated_context_does_not_leak_into_later_emissions(self) -> None: + try: + raise ValueError("unrelated") + except ValueError: + first = self._raise_private() + assert isinstance(first.__context__, ValueError) + + second = self._raise_private() + + assert second.__context__ is None + first_frames = {tb.tb_frame for tb in _walk_traceback(first.__traceback__)} + second_frames = {tb.tb_frame for tb in _walk_traceback(second.__traceback__)} + assert not first_frames & second_frames + + def test_no_module_level_warning_instances(self) -> None: + """Guard against reintroducing shared instances anywhere in ``_pytest``.""" + offenders = [] + for module_info in pkgutil.walk_packages(_pytest.__path__, prefix="_pytest."): + module = importlib.import_module(module_info.name) + for name, value in vars(module).items(): + if isinstance(value, Warning): + offenders.append(f"{module_info.name}.{name}") + assert sorted(offenders) == [] + + @pytest.mark.parametrize( + ("name", "template"), + sorted( + (name, value) + for name, value in vars(deprecated).items() + if isinstance(value, WarningTemplate) + ), + ) + def test_templates_are_well_formed(self, name: str, template) -> None: + fields = { + field + for _, field, _, _ in string.Formatter().parse(template.template) + if field is not None + } + # ``{}`` would silently consume a positional argument that never arrives. + assert "" not in fields, name + # ``warn()`` takes ``stacklevel`` as a keyword, so a field of that name + # could never be filled in. + assert "stacklevel" not in fields, name + + warning = template.format(**dict.fromkeys(fields, "x")) + + assert isinstance(warning, template.category) + if not fields: + assert str(warning) == template.template + + +def _walk_traceback(tb): + while tb is not None: + yield tb + tb = tb.tb_next diff --git a/testing/test_warning_types.py b/testing/test_warning_types.py index 81d8785733c..b24bff8211e 100644 --- a/testing/test_warning_types.py +++ b/testing/test_warning_types.py @@ -2,6 +2,7 @@ from __future__ import annotations import inspect +import warnings from _pytest import warning_types from _pytest.pytester import Pytester @@ -48,3 +49,57 @@ def test_warn_explicit_for_annotates_errors_with_location(): pytest.raises, # type: ignore[arg-type] warning_types.PytestWarning("test"), ) + + +class TestWarningTemplate: + """A template must build a fresh warning per emission (#14912).""" + + template: warning_types.WarningTemplate[warning_types.PytestWarning] = ( + warning_types.WarningTemplate(warning_types.PytestWarning, "a message") + ) + + def test_format_returns_a_new_instance_each_call(self) -> None: + first = self.template.format() + second = self.template.format() + + assert first is not second + assert isinstance(first, warning_types.PytestWarning) + assert str(first) == str(second) == "a message" + + def test_constant_message_keeps_literal_braces(self) -> None: + """Messages without placeholders are not run through str.format().""" + template = warning_types.WarningTemplate( + warning_types.PytestWarning, "a {literal} brace and a }} one" + ) + + assert str(template.format()) == "a {literal} brace and a }} one" + + def test_format_substitutes_kwargs(self) -> None: + template = warning_types.WarningTemplate( + warning_types.PytestWarning, "hello {name}" + ) + + assert str(template.format(name="world")) == "hello world" + + def test_warn_reports_the_callers_location(self) -> None: + with warnings.catch_warnings(record=True) as records: + warnings.simplefilter("always") + lineno = inspect.currentframe().f_lineno + 1 # type: ignore[union-attr] + self.template.warn(stacklevel=1) + + [record] = records + assert record.filename == __file__ + assert record.lineno == lineno + + def test_warn_stacklevel_skips_the_helper(self) -> None: + def helper() -> None: + self.template.warn(stacklevel=2) + + with warnings.catch_warnings(record=True) as records: + warnings.simplefilter("always") + lineno = inspect.currentframe().f_lineno + 1 # type: ignore[union-attr] + helper() + + [record] = records + assert record.filename == __file__ + assert record.lineno == lineno