diff --git a/docs/docs/in_depth/property-mapping.md b/docs/docs/in_depth/property-mapping.md index 88ef5123..3d400061 100644 --- a/docs/docs/in_depth/property-mapping.md +++ b/docs/docs/in_depth/property-mapping.md @@ -558,6 +558,9 @@ The matcher must be a `classmethod`; a `staticmethod` matcher on a class that de The guard is installed at class definition, so mutating `PROPERTY_MAPPING` or replacing `match_feature_group_criteria` after the class body escapes it. +This guard, the name-path presence guard, and the class-definition diagnostics below live in +`feature_chain_author_guards.py`, which imports `feature_chain_parser` and never the reverse. + ## Guarding against a universal configuration matcher A key is unconditionally required only when it declares no `default` and no `required_when`. A diff --git a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_author_guards.py b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_author_guards.py new file mode 100644 index 00000000..6fff743c --- /dev/null +++ b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_author_guards.py @@ -0,0 +1,470 @@ +"""Class-definition-time guards for feature chaining: author validation, authoring diagnostics, and the +matcher guards the two ``__init_subclass__`` hooks install. + +Imports ``feature_chain_parser``; the parser never imports this module, which keeps the split acyclic. + +Depends on these parser-private names, so renaming one of them is a cross-module break: +``FeatureChainParser._can_skip_required_check``, ``._check_name_path_required_presence``, ``._merge_bindings``, +``._name_identifies_group``, ``._name_path_missing_required_keys``, and module-private ``_contained_raise_log_level``. +""" + +from __future__ import annotations + +import contextvars +import functools +import logging +import re +from typing import Any + +from mloda.core.abstract_plugins.components.feature_name import FeatureName +from mloda.core.abstract_plugins.components.options import Options +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import ( + CHAIN_SEPARATOR, + FeatureChainParser, + _contained_raise_log_level, + option_key_is_present, + record_match_rejection, +) +from mloda.core.abstract_plugins.components.feature_chainer.parsed_feature_name import ParsedFeatureName +from mloda.core.abstract_plugins.components.feature_chainer.property_spec import PropertySpec +from mloda.core.abstract_plugins.components.utils import safe_field + +logger = logging.getLogger(__name__) + +# Marks a matcher that already carries the required_when guard, so it is never wrapped twice. +REQUIRED_WHEN_GUARD_FLAG = "_mloda_required_when_guard" + +# Marks a matcher that already carries the name-path presence guard, so it is never wrapped twice. +NAME_PATH_PRESENCE_GUARD_FLAG = "_mloda_name_path_presence_guard" + +# Marks a class whose captureless diagnostic already ran, so the two __init_subclass__ hooks +# emit it at most once. Checked on the class's OWN dict so a subclass still evaluates fresh. +CAPTURELESS_DIAGNOSTIC_FLAG = "_mloda_captureless_diagnostic_emitted" + +# An unrelated feature name used to probe whether a matcher is universal: does it accept a name it +# has no business matching, with empty options? It carries NO chain separator, so no +# PREFIX_PATTERN/SUFFIX_PATTERN can capture it and the resolved matcher falls through to the +# configuration path, where the universal-matcher problem actually lives. +_UNIVERSAL_MATCHER_PROBE_NAME = "mloda_universal_matcher_probe" + +# How many guards the current match call is nested in. A guarded matcher that delegates via super() +# reaches the guard of its parent, and only the outermost one may evaluate the predicates. +# A ContextVar (not a plain global) keeps the count per thread and per async task. +REQUIRED_WHEN_GUARD_DEPTH: contextvars.ContextVar[int] = contextvars.ContextVar( + "mloda_required_when_guard_depth", default=0 +) + +# Same nesting rule for the name-path presence guard, tracked independently so the two guards compose. +NAME_PATH_PRESENCE_GUARD_DEPTH: contextvars.ContextVar[int] = contextvars.ContextVar( + "mloda_name_path_presence_guard_depth", default=0 +) + + +def _pattern_named_and_total_groups(pattern: Any) -> tuple[frozenset[str], int]: + """The named group names and total group count of a pattern; an uncompilable pattern reports neither.""" + if isinstance(pattern, re.Pattern): + return frozenset(pattern.groupindex), pattern.groups + compiled: re.Pattern[str] | None = safe_field(lambda: re.compile(pattern), None, catching=(re.error, TypeError)) + if compiled is None: + return frozenset(), 0 + return frozenset(compiled.groupindex), compiled.groups + + +def _flatten_patterns(patterns: list[Any]) -> list[Any]: + """Flatten one level so a list/tuple pattern attribute contributes its elements as concrete patterns. + + Mirrors how a ``SUFFIX_PATTERN = [regex]`` fixture passes the list straight to ``parse_name`` at + runtime. A compiled ``re.Pattern`` and a ``str`` stay as-is. + """ + flattened: list[Any] = [] + for pattern in patterns: + if isinstance(pattern, (list, tuple)): + flattened.extend(pattern) + else: + flattened.append(pattern) + return flattened + + +def _str_reachable_values(spec: PropertySpec) -> set[str]: + """The str members of a spec's value space; only a str can be reverse-looked-up from a capture.""" + reachable: set[str] = set() + for value in FeatureChainParser.extract_property_values(spec): + if isinstance(value, str): + reachable.add(value) + return reachable + + +def validate_name_binding(owner: type[Any]) -> None: + """Reject an order-dependent legacy positional binding at class-definition time. + + The check is PER CONCRETE PATTERN: a list/tuple pattern attribute is flattened to its + elements first. A pattern needs the overlap check only when it declares a capture group + (``total >= 1``) AND no named group, so it relies on the legacy positional fallback. If any + such positional-only pattern exists and two keys share a reachable (str) allowed value, the + binding is order-dependent and rejected. A named-capture pattern is exempt (binding is + explicit for it); a captureless one has nothing to misbind. Called from both FeatureGroup and + FeatureChainParserMixin at class definition. + """ + property_mapping = getattr(owner, "PROPERTY_MAPPING", None) + if not isinstance(property_mapping, dict): + return + + patterns = FeatureChainParser.prefix_patterns_of(owner) + if not patterns: + return + + needs_overlap_check = False + for pattern in _flatten_patterns(patterns): + named, total = _pattern_named_and_total_groups(pattern) + if total >= 1 and not named: + needs_overlap_check = True + + if not needs_overlap_check: + return + + reachable = { + key: _str_reachable_values(spec) for key, spec in property_mapping.items() if isinstance(spec, PropertySpec) + } + keys = list(reachable) + for i, left in enumerate(keys): + for right in keys[i + 1 :]: + overlap = reachable[left] & reachable[right] + if overlap: + raise ValueError( + f"{owner.__name__}: PROPERTY_MAPPING keys '{left}' and '{right}' share reachable " + f"allowed value(s) {sorted(overlap)}, so a legacy positional capture cannot bind " + f"unambiguously. Use named capture groups (?P...) so binding is explicit." + ) + + +def warn_captureless_without_binding(owner: type[Any]) -> None: + """Nudge authors of a captureless pattern that carries a PROPERTY_MAPPING (#772). + + A captureless pattern binds no key from the name. If a key must come from the name, add a + named capture (?P...); if the pattern is only a recognition predicate, set + RECOGNITION_ONLY_PATTERN = True to declare that intent and silence this diagnostic. + """ + if owner.__dict__.get(CAPTURELESS_DIAGNOSTIC_FLAG, False): + return + if getattr(owner, "RECOGNITION_ONLY_PATTERN", False): + return + property_mapping = getattr(owner, "PROPERTY_MAPPING", None) + if not isinstance(property_mapping, dict) or not property_mapping: + return + patterns = FeatureChainParser.prefix_patterns_of(owner) + if not patterns: + return + for pattern in _flatten_patterns(patterns): + _named, total = _pattern_named_and_total_groups(pattern) + if total == 0: + setattr(owner, CAPTURELESS_DIAGNOSTIC_FLAG, True) + logger.warning( + "%s declares a captureless PREFIX_PATTERN/SUFFIX_PATTERN together with a PROPERTY_MAPPING. " + "A captureless pattern binds no key from the feature name. Add a named capture " + "(?P...) if a mapping key must be populated from the name, or set " + "RECOGNITION_ONLY_PATTERN = True to declare the pattern a recognition-only predicate " + "and silence this warning.", + owner.__name__, + ) + return + + +def warn_universal_optional_matcher(owner: type[Any]) -> None: + """Nudge authors whose all-optional PROPERTY_MAPPING inherits the universal configuration matcher (#771). + + With zero unconditionally required keys, the configuration path matches any feature name given + empty options. Warn unless the class opts in with ALLOW_UNIVERSAL_MATCHER = True. A key that is + unconditionally required, or conditionally required via required_when, gates the match, so the + mapping is not warned. Universality is confirmed behaviorally: the resolved matcher is called + with an unrelated, separator-free name and empty options, which exempts a genuine custom matcher + while still catching a pass-through override that delegates to the universal base. + """ + if getattr(owner, "ALLOW_UNIVERSAL_MATCHER", False): + return + property_mapping = getattr(owner, "PROPERTY_MAPPING", None) + # A None mapping is not a configuration matcher; an EMPTY dict is the strongest universal + # matcher (it validates vacuously), so it stays in scope. + if not isinstance(property_mapping, dict): + return + for spec in property_mapping.values(): + if not isinstance(spec, PropertySpec): + continue + # A required_when key gates the match with a runtime predicate, so the mapping is not a + # blanket universal matcher. It is also left unprobed: the predicate may reference the + # class being defined, which is not yet bound to its name during __init_subclass__, so + # probing it would raise (#771). + if spec.required_when is not None: + return + # A key that declares no default (and, per the check above, no required_when) is + # unconditionally required and already discriminates on the configuration path. + if not FeatureChainParser._can_skip_required_check(spec): + return + matcher = getattr(owner, "match_feature_group_criteria", None) + if matcher is None: + return + # A matcher that raises on the probe is doing custom work, so it is not treated as universal. + try: + universal = bool(matcher(_UNIVERSAL_MATCHER_PROBE_NAME, Options())) + except Exception as exc: + # rebind: Python clears the "except ... as exc" name at block exit, so the closure needs a stable local + err = exc + logger.debug( + "universal-matcher probe for %s raised %s; treating it as non-universal.", + owner.__name__, + safe_field(lambda: str(err), type(err).__name__), + ) + return + if not universal: + return + logger.warning( + "%s declares a PROPERTY_MAPPING with no unconditionally required key and inherits the " + "universal configuration matcher: with empty options it matches any feature name. Add a " + "required key (a PropertySpec with no default, or a required_when predicate that fires), or " + "set ALLOW_UNIVERSAL_MATCHER = True to declare the universal match intentional.", + owner.__name__, + ) + + +def check_required_when( + owner_name: str, + feature_name: str | FeatureName, + prefix_patterns: list[Any], + property_mapping: dict[str, Any] | None, + options: Options, +) -> bool: + """Evaluate every required_when predicate of a mapping. False means the feature is not a match.""" + if property_mapping is None or not FeatureChainParser.has_required_when_predicates(property_mapping): + return True + + # build_effective_options runs no user callback, so a raise from it is a framework defect (or a user + # configuration error carrying actionable guidance) and must surface, not read as a non-match (#763). + effective_options = FeatureChainParser.build_effective_options( + feature_name, prefix_patterns, property_mapping, options + ) + for key, spec in property_mapping.items(): + if not isinstance(spec, PropertySpec): + continue + # Callability is enforced at PropertySpec construction, so a present predicate is callable. + predicate = spec.required_when + if predicate is None: + continue + # A predicate that raises cannot judge the value, so the feature group is a non-match, not the run. + try: + is_required = bool(predicate(effective_options)) + except Exception as exc: + logger.log( + _contained_raise_log_level(exc), + "required_when predicate %s for '%s' raised %s; treating feature group %s as a non-match.", + getattr(predicate, "__name__", repr(predicate)), + key, + exc, + owner_name, + ) + return False + # An opted-in key present as an explicit None counts as present, so the requirement is met (#768). + if is_required and not option_key_is_present(spec, key, effective_options): + predicate_name = getattr(predicate, "__name__", repr(predicate)) + logger.debug( + "Feature group %s requires option '%s' (predicate %s is satisfied) but it was not provided.", + owner_name, + key, + predicate_name, + ) + # Same diagnostic seam as the sibling presence rules, so the resolution-failure report can + # explain this non-match. The engine re-keys the harvest by candidate, so the reason itself + # names the class that declared the requirement. + record_match_rejection( + owner_name, + f"required option '{key}' is absent, but {owner_name} declares it required " + f"(required_when predicate {predicate_name} is satisfied)", + ) + return False + return True + + +def _resolve_match_arguments(args: tuple[Any, ...], kwargs: dict[str, Any]) -> tuple[str | FeatureName, Any]: + """Recover (feature_name, options) from a matcher call without assuming an override's parameter names.""" + values = list(args) + list(kwargs.values()) + + feature_name = kwargs.get("feature_name", args[0] if args else None) + if not isinstance(feature_name, str): + feature_name = next((value for value in values if isinstance(value, str)), "") + + options = kwargs.get("options") + if not isinstance(options, Options): + options = next((value for value in values if isinstance(value, Options)), None) + + return feature_name, options + + +def _matcher_is_staticmethod(owner: type[Any]) -> bool: + """True when the class's resolved matcher is a staticmethod descriptor.""" + for klass in owner.__mro__: + descriptor = klass.__dict__.get("match_feature_group_criteria") + if descriptor is not None: + return isinstance(descriptor, staticmethod) + return False + + +def _reject_staticmethod_matcher(owner: type[Any]) -> None: + """Reject a staticmethod matcher on a class that declares required_when. + + The guard is reinstalled as a classmethod, so the class would be passed as the first + positional argument: a staticmethod matcher would read ``cls`` as its ``feature_name`` and the + feature name as its ``options``, and answer a silently wrong verdict. Fail at class definition. + """ + for klass in owner.__mro__: + descriptor = klass.__dict__.get("match_feature_group_criteria") + if descriptor is None: + continue + if isinstance(descriptor, staticmethod): + raise ValueError( + f"{owner.__name__} declares required_when in its PROPERTY_MAPPING, but its " + f"match_feature_group_criteria is a staticmethod. It must be a classmethod: the " + f"required_when guard is installed as a classmethod and passes the class as the first " + f"argument, which a staticmethod would misread as the feature name." + ) + return + + +def install_required_when_guard(owner: type[Any]) -> None: + """Wrap a class's RESOLVED matcher so its required_when predicates run whatever matcher it kept. + + Called at class-definition time from ``FeatureGroup.__init_subclass__`` and + ``FeatureChainParserMixin.__init_subclass__``. The predicates cannot live inside one + matcher: overriding ``match_feature_group_criteria`` is supported, and an override that + never delegates would silently drop the declared contract. The wrapper stays a + classmethod, so it reads the PROPERTY_MAPPING and patterns of the class it is called on. + + A class that declares no required_when is left untouched, and an already guarded matcher + is never wrapped again. Guards do nest (an override may delegate into a guarded parent), so + only the outermost one evaluates the predicates: exactly once per match call. + + Class definition is the install site, so a PROPERTY_MAPPING mutated, or a matcher replaced, + AFTER the class body is not seen by the guard. + """ + property_mapping = getattr(owner, "PROPERTY_MAPPING", None) + if not isinstance(property_mapping, dict) or not FeatureChainParser.has_required_when_predicates(property_mapping): + return + + _reject_staticmethod_matcher(owner) + + matcher = getattr(owner, "match_feature_group_criteria", None) + if matcher is None: + return + + inner: Any = getattr(matcher, "__func__", matcher) + if getattr(inner, REQUIRED_WHEN_GUARD_FLAG, False): + return + + @functools.wraps(inner) + def guarded(guarded_cls: type[Any], *args: Any, **kwargs: Any) -> bool: + # The outermost guard is the one whose class the matcher was called on, so it is the one + # whose PROPERTY_MAPPING decides. An inner guard, reached through a delegating super() + # call, only answers with its matcher's verdict. + outermost = REQUIRED_WHEN_GUARD_DEPTH.get() == 0 + token = REQUIRED_WHEN_GUARD_DEPTH.set(REQUIRED_WHEN_GUARD_DEPTH.get() + 1) + try: + if not inner(guarded_cls, *args, **kwargs): + return False + + if not outermost: + return True + + feature_name, options = _resolve_match_arguments(args, kwargs) + if options is None: + return True + + return check_required_when( + guarded_cls.__name__, + feature_name, + FeatureChainParser.prefix_patterns_of(guarded_cls), + getattr(guarded_cls, "PROPERTY_MAPPING", None), + options, + ) + finally: + REQUIRED_WHEN_GUARD_DEPTH.reset(token) + + setattr(guarded, REQUIRED_WHEN_GUARD_FLAG, True) + setattr(owner, "match_feature_group_criteria", classmethod(guarded)) + + +def install_name_path_presence_guard(owner: type[Any]) -> None: + """Wrap a class's RESOLVED matcher so the name-path required-presence rule (#769) survives an override. + + Mirrors ``install_required_when_guard``: installed at class definition from both + ``__init_subclass__`` hooks, never wrapped twice, and guards nest so only the outermost one + evaluates. An inner False stands untouched, so the inner path's own presence warning is never + duplicated. Nesting order relative to the required_when guard is behaviorally irrelevant: + each guard ANDs its own predicate onto the inner verdict and passes False through unchanged. + """ + property_mapping = getattr(owner, "PROPERTY_MAPPING", None) + if not isinstance(property_mapping, dict): + return + if not FeatureChainParser.prefix_patterns_of(owner): + return + # Same exemptions as the inner rule: with empty options, the missing keys ARE the flaggable ones. + if not FeatureChainParser._name_path_missing_required_keys(Options(), property_mapping): + return + + # Wrapping a staticmethod matcher would hide it from _reject_staticmethod_matcher, so the + # required_when installer's existing definition-time ValueError keeps precedence. + is_static = _matcher_is_staticmethod(owner) + if is_static and FeatureChainParser.has_required_when_predicates(property_mapping): + return + + # getattr on a staticmethod returns the plain function, so the __func__ fetch below is a no-op + # for it and the flag check covers both shapes. + matcher = getattr(owner, "match_feature_group_criteria", None) + if matcher is None: + return + + inner: Any = getattr(matcher, "__func__", matcher) + if getattr(inner, NAME_PATH_PRESENCE_GUARD_FLAG, False): + return + + @functools.wraps(inner) + def guarded(guarded_cls: type[Any], *args: Any, **kwargs: Any) -> bool: + outermost = NAME_PATH_PRESENCE_GUARD_DEPTH.get() == 0 + token = NAME_PATH_PRESENCE_GUARD_DEPTH.set(NAME_PATH_PRESENCE_GUARD_DEPTH.get() + 1) + try: + # A staticmethod inner keeps its calling convention: no cls injected. An inner False + # stands: the inner default path already warned on its own presence non-match, so + # passing it through keeps one warning per match call. + inner_verdict = inner(*args, **kwargs) if is_static else inner(guarded_cls, *args, **kwargs) + if not inner_verdict: + return False + + if not outermost: + return True + + feature_name, options = _resolve_match_arguments(args, kwargs) + if options is None: + return True + + mapping = getattr(guarded_cls, "PROPERTY_MAPPING", None) + if not isinstance(mapping, dict): + return True + # Flattened, because a matcher passes a list-valued pattern attribute straight to + # parse_name, so its ELEMENTS are the concrete patterns. A matcher must never leak + # an exception, so the parse is contained as in build_effective_options; re.error is + # additionally caught here because a malformed pattern must degrade to a non-match + # of the name path, never veto the inner verdict (#868). + patterns = _flatten_patterns(FeatureChainParser.prefix_patterns_of(guarded_cls)) + parsed = safe_field( + lambda: FeatureChainParser.parse_name(feature_name, patterns, CHAIN_SEPARATOR), + ParsedFeatureName.no_match(), + catching=(ValueError, re.error), + ) + if not FeatureChainParser._name_identifies_group(parsed, mapping): + return True + bindings = FeatureChainParser.bind_name_captures(parsed, mapping) + effective_options = FeatureChainParser._merge_bindings(options, bindings, mapping) + return FeatureChainParser._check_name_path_required_presence( + guarded_cls.__name__, feature_name, effective_options, mapping + ) + finally: + NAME_PATH_PRESENCE_GUARD_DEPTH.reset(token) + + setattr(guarded, NAME_PATH_PRESENCE_GUARD_FLAG, True) + setattr(owner, "match_feature_group_criteria", classmethod(guarded)) diff --git a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser.py b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser.py index 619d1864..e50afe26 100644 --- a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser.py +++ b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser.py @@ -5,7 +5,6 @@ from __future__ import annotations import contextvars -import functools import logging import re from typing import Any, Optional @@ -25,34 +24,6 @@ COLUMN_SEPARATOR = "~" # Separates multi-column output index INPUT_SEPARATOR = "&" # Separates multiple input features -# Marks a matcher that already carries the required_when guard, so it is never wrapped twice. -REQUIRED_WHEN_GUARD_FLAG = "_mloda_required_when_guard" - -# Marks a matcher that already carries the name-path presence guard, so it is never wrapped twice. -NAME_PATH_PRESENCE_GUARD_FLAG = "_mloda_name_path_presence_guard" - -# Marks a class whose captureless diagnostic already ran, so the two __init_subclass__ hooks -# emit it at most once. Checked on the class's OWN dict so a subclass still evaluates fresh. -CAPTURELESS_DIAGNOSTIC_FLAG = "_mloda_captureless_diagnostic_emitted" - -# An unrelated feature name used to probe whether a matcher is universal: does it accept a name it -# has no business matching, with empty options? It carries NO chain separator, so no -# PREFIX_PATTERN/SUFFIX_PATTERN can capture it and the resolved matcher falls through to the -# configuration path, where the universal-matcher problem actually lives. -_UNIVERSAL_MATCHER_PROBE_NAME = "mloda_universal_matcher_probe" - -# How many guards the current match call is nested in. A guarded matcher that delegates via super() -# reaches the guard of its parent, and only the outermost one may evaluate the predicates. -# A ContextVar (not a plain global) keeps the count per thread and per async task. -REQUIRED_WHEN_GUARD_DEPTH: contextvars.ContextVar[int] = contextvars.ContextVar( - "mloda_required_when_guard_depth", default=0 -) - -# Same nesting rule for the name-path presence guard, tracked independently so the two guards compose. -NAME_PATH_PRESENCE_GUARD_DEPTH: contextvars.ContextVar[int] = contextvars.ContextVar( - "mloda_name_path_presence_guard_depth", default=0 -) - # Active for one candidate's match call: the engine opens a window per candidate. Maps the recording # site's owner name to the first structured rejection reason the real match pass produced, and the # engine attributes the harvest to the candidate class object it called (os-005 replaces the replay). @@ -427,7 +398,7 @@ def _name_path_missing_required_keys( The source key is name-provided (its count is enforced by MIN/MAX_IN_FEATURES), so ``in_features`` is excluded. A declared-default or ``required_when`` key is skippable (``_can_skip_required_check``); ``deferred_binding`` is the #769 opt-out. A key is absent - exactly as ``_collect_option_value`` / ``check_required_when`` read absence. + exactly as ``_collect_option_value`` / ``feature_chain_author_guards.check_required_when`` read absence. """ missing: list[str] = [] for key, spec in property_mapping.items(): @@ -523,8 +494,8 @@ def match_configuration_feature_chain_parser( # string based matching. parse_name raises the no-source ValueError exactly as before, contained by # match_parser_criteria. Effective options are built from the parse facts here, keeping the matcher's - # own parse containment; a raise out of build_effective_options in check_required_when now surfaces - # as a framework defect (os-005, see TestBuildEffectiveOptionsRaiseSurfaces). + # own parse containment; a raise out of build_effective_options in the author guards' + # check_required_when now surfaces as a framework defect (see TestBuildEffectiveOptionsRaiseSurfaces). if prefix_patterns is not None: parsed = cls.parse_name(feature_name, prefix_patterns, pattern) if cls._name_identifies_group(parsed, property_mapping): @@ -690,416 +661,6 @@ def build_effective_options( bindings = cls.bind_name_captures(parsed, property_mapping) return cls._merge_bindings(options, bindings, property_mapping) - @classmethod - def _pattern_named_and_total_groups(cls, pattern: Any) -> tuple[frozenset[str], int]: - """The named group names and total group count of a pattern; an uncompilable pattern reports neither.""" - if isinstance(pattern, re.Pattern): - return frozenset(pattern.groupindex), pattern.groups - compiled: re.Pattern[str] | None = safe_field(lambda: re.compile(pattern), None, catching=(re.error, TypeError)) - if compiled is None: - return frozenset(), 0 - return frozenset(compiled.groupindex), compiled.groups - - @classmethod - def _flatten_patterns(cls, patterns: list[Any]) -> list[Any]: - """Flatten one level so a list/tuple pattern attribute contributes its elements as concrete patterns. - - Mirrors how a ``SUFFIX_PATTERN = [regex]`` fixture passes the list straight to ``parse_name`` at - runtime. A compiled ``re.Pattern`` and a ``str`` stay as-is. - """ - flattened: list[Any] = [] - for pattern in patterns: - if isinstance(pattern, (list, tuple)): - flattened.extend(pattern) - else: - flattened.append(pattern) - return flattened - - @classmethod - def _str_reachable_values(cls, spec: PropertySpec) -> set[str]: - """The str members of a spec's value space; only a str can be reverse-looked-up from a capture.""" - reachable: set[str] = set() - for value in cls.extract_property_values(spec): - if isinstance(value, str): - reachable.add(value) - return reachable - - @classmethod - def validate_name_binding(cls, owner: type[Any]) -> None: - """Reject an order-dependent legacy positional binding at class-definition time. - - The check is PER CONCRETE PATTERN: a list/tuple pattern attribute is flattened to its - elements first. A pattern needs the overlap check only when it declares a capture group - (``total >= 1``) AND no named group, so it relies on the legacy positional fallback. If any - such positional-only pattern exists and two keys share a reachable (str) allowed value, the - binding is order-dependent and rejected. A named-capture pattern is exempt (binding is - explicit for it); a captureless one has nothing to misbind. Called from both FeatureGroup and - FeatureChainParserMixin at class definition. - """ - property_mapping = getattr(owner, "PROPERTY_MAPPING", None) - if not isinstance(property_mapping, dict): - return - - patterns = cls.prefix_patterns_of(owner) - if not patterns: - return - - needs_overlap_check = False - for pattern in cls._flatten_patterns(patterns): - named, total = cls._pattern_named_and_total_groups(pattern) - if total >= 1 and not named: - needs_overlap_check = True - - if not needs_overlap_check: - return - - reachable = { - key: cls._str_reachable_values(spec) - for key, spec in property_mapping.items() - if isinstance(spec, PropertySpec) - } - keys = list(reachable) - for i, left in enumerate(keys): - for right in keys[i + 1 :]: - overlap = reachable[left] & reachable[right] - if overlap: - raise ValueError( - f"{owner.__name__}: PROPERTY_MAPPING keys '{left}' and '{right}' share reachable " - f"allowed value(s) {sorted(overlap)}, so a legacy positional capture cannot bind " - f"unambiguously. Use named capture groups (?P...) so binding is explicit." - ) - - @classmethod - def warn_captureless_without_binding(cls, owner: type[Any]) -> None: - """Nudge authors of a captureless pattern that carries a PROPERTY_MAPPING (#772). - - A captureless pattern binds no key from the name. If a key must come from the name, add a - named capture (?P...); if the pattern is only a recognition predicate, set - RECOGNITION_ONLY_PATTERN = True to declare that intent and silence this diagnostic. - """ - if owner.__dict__.get(CAPTURELESS_DIAGNOSTIC_FLAG, False): - return - if getattr(owner, "RECOGNITION_ONLY_PATTERN", False): - return - property_mapping = getattr(owner, "PROPERTY_MAPPING", None) - if not isinstance(property_mapping, dict) or not property_mapping: - return - patterns = cls.prefix_patterns_of(owner) - if not patterns: - return - for pattern in cls._flatten_patterns(patterns): - _named, total = cls._pattern_named_and_total_groups(pattern) - if total == 0: - setattr(owner, CAPTURELESS_DIAGNOSTIC_FLAG, True) - logger.warning( - "%s declares a captureless PREFIX_PATTERN/SUFFIX_PATTERN together with a PROPERTY_MAPPING. " - "A captureless pattern binds no key from the feature name. Add a named capture " - "(?P...) if a mapping key must be populated from the name, or set " - "RECOGNITION_ONLY_PATTERN = True to declare the pattern a recognition-only predicate " - "and silence this warning.", - owner.__name__, - ) - return - - @classmethod - def warn_universal_optional_matcher(cls, owner: type[Any]) -> None: - """Nudge authors whose all-optional PROPERTY_MAPPING inherits the universal configuration matcher (#771). - - With zero unconditionally required keys, the configuration path matches any feature name given - empty options. Warn unless the class opts in with ALLOW_UNIVERSAL_MATCHER = True. A key that is - unconditionally required, or conditionally required via required_when, gates the match, so the - mapping is not warned. Universality is confirmed behaviorally: the resolved matcher is called - with an unrelated, separator-free name and empty options, which exempts a genuine custom matcher - while still catching a pass-through override that delegates to the universal base. - """ - if getattr(owner, "ALLOW_UNIVERSAL_MATCHER", False): - return - property_mapping = getattr(owner, "PROPERTY_MAPPING", None) - # A None mapping is not a configuration matcher; an EMPTY dict is the strongest universal - # matcher (it validates vacuously), so it stays in scope. - if not isinstance(property_mapping, dict): - return - for spec in property_mapping.values(): - if not isinstance(spec, PropertySpec): - continue - # A required_when key gates the match with a runtime predicate, so the mapping is not a - # blanket universal matcher. It is also left unprobed: the predicate may reference the - # class being defined, which is not yet bound to its name during __init_subclass__, so - # probing it would raise (#771). - if spec.required_when is not None: - return - # A key that declares no default (and, per the check above, no required_when) is - # unconditionally required and already discriminates on the configuration path. - if not cls._can_skip_required_check(spec): - return - matcher = getattr(owner, "match_feature_group_criteria", None) - if matcher is None: - return - # A matcher that raises on the probe is doing custom work, so it is not treated as universal. - try: - universal = bool(matcher(_UNIVERSAL_MATCHER_PROBE_NAME, Options())) - except Exception as exc: - err = exc # rebind: Python clears the "except ... as exc" name at block exit, so the closure needs a stable local - logger.debug( - "universal-matcher probe for %s raised %s; treating it as non-universal.", - owner.__name__, - safe_field(lambda: str(err), type(err).__name__), - ) - return - if not universal: - return - logger.warning( - "%s declares a PROPERTY_MAPPING with no unconditionally required key and inherits the " - "universal configuration matcher: with empty options it matches any feature name. Add a " - "required key (a PropertySpec with no default, or a required_when predicate that fires), or " - "set ALLOW_UNIVERSAL_MATCHER = True to declare the universal match intentional.", - owner.__name__, - ) - - @classmethod - def check_required_when( - cls, - owner_name: str, - feature_name: str | FeatureName, - prefix_patterns: list[Any], - property_mapping: dict[str, Any] | None, - options: Options, - ) -> bool: - """Evaluate every required_when predicate of a mapping. False means the feature is not a match.""" - if property_mapping is None or not cls.has_required_when_predicates(property_mapping): - return True - - # build_effective_options runs no user callback, so a raise from it is a framework defect (or a user - # configuration error carrying actionable guidance) and must surface, not read as a non-match (os-005, #763). - effective_options = cls.build_effective_options(feature_name, prefix_patterns, property_mapping, options) - for key, spec in property_mapping.items(): - if not isinstance(spec, PropertySpec): - continue - # Callability is enforced at PropertySpec construction, so a present predicate is callable. - predicate = spec.required_when - if predicate is None: - continue - # A predicate that raises cannot judge the value, so the feature group is a non-match, not the run. - try: - is_required = bool(predicate(effective_options)) - except Exception as exc: - logger.log( - _contained_raise_log_level(exc), - "required_when predicate %s for '%s' raised %s; treating feature group %s as a non-match.", - getattr(predicate, "__name__", repr(predicate)), - key, - exc, - owner_name, - ) - return False - # An opted-in key present as an explicit None counts as present, so the requirement is met (#768). - if is_required and not option_key_is_present(spec, key, effective_options): - predicate_name = getattr(predicate, "__name__", repr(predicate)) - logger.debug( - "Feature group %s requires option '%s' (predicate %s is satisfied) but it was not provided.", - owner_name, - key, - predicate_name, - ) - # Same diagnostic seam as the sibling presence rules, so the resolution-failure report can - # explain this non-match. The engine re-keys the harvest by candidate, so the reason itself - # names the class that declared the requirement. - record_match_rejection( - owner_name, - f"required option '{key}' is absent, but {owner_name} declares it required " - f"(required_when predicate {predicate_name} is satisfied)", - ) - return False - return True - - @staticmethod - def _resolve_match_arguments(args: tuple[Any, ...], kwargs: dict[str, Any]) -> tuple[str | FeatureName, Any]: - """Recover (feature_name, options) from a matcher call without assuming an override's parameter names.""" - values = list(args) + list(kwargs.values()) - - feature_name = kwargs.get("feature_name", args[0] if args else None) - if not isinstance(feature_name, str): - feature_name = next((value for value in values if isinstance(value, str)), "") - - options = kwargs.get("options") - if not isinstance(options, Options): - options = next((value for value in values if isinstance(value, Options)), None) - - return feature_name, options - - @classmethod - def _matcher_is_staticmethod(cls, owner: type[Any]) -> bool: - """True when the class's resolved matcher is a staticmethod descriptor.""" - for klass in owner.__mro__: - descriptor = klass.__dict__.get("match_feature_group_criteria") - if descriptor is not None: - return isinstance(descriptor, staticmethod) - return False - - @classmethod - def _reject_staticmethod_matcher(cls, owner: type[Any]) -> None: - """Reject a staticmethod matcher on a class that declares required_when. - - The guard is reinstalled as a classmethod, so the class would be passed as the first - positional argument: a staticmethod matcher would read ``cls`` as its ``feature_name`` and the - feature name as its ``options``, and answer a silently wrong verdict. Fail at class definition. - """ - for klass in owner.__mro__: - descriptor = klass.__dict__.get("match_feature_group_criteria") - if descriptor is None: - continue - if isinstance(descriptor, staticmethod): - raise ValueError( - f"{owner.__name__} declares required_when in its PROPERTY_MAPPING, but its " - f"match_feature_group_criteria is a staticmethod. It must be a classmethod: the " - f"required_when guard is installed as a classmethod and passes the class as the first " - f"argument, which a staticmethod would misread as the feature name." - ) - return - - @classmethod - def install_required_when_guard(cls, owner: type[Any]) -> None: - """Wrap a class's RESOLVED matcher so its required_when predicates run whatever matcher it kept. - - Called at class-definition time from ``FeatureGroup.__init_subclass__`` and - ``FeatureChainParserMixin.__init_subclass__``. The predicates cannot live inside one - matcher: overriding ``match_feature_group_criteria`` is supported, and an override that - never delegates would silently drop the declared contract. The wrapper stays a - classmethod, so it reads the PROPERTY_MAPPING and patterns of the class it is called on. - - A class that declares no required_when is left untouched, and an already guarded matcher - is never wrapped again. Guards do nest (an override may delegate into a guarded parent), so - only the outermost one evaluates the predicates: exactly once per match call. - - Class definition is the install site, so a PROPERTY_MAPPING mutated, or a matcher replaced, - AFTER the class body is not seen by the guard. - """ - property_mapping = getattr(owner, "PROPERTY_MAPPING", None) - if not isinstance(property_mapping, dict) or not cls.has_required_when_predicates(property_mapping): - return - - cls._reject_staticmethod_matcher(owner) - - matcher = getattr(owner, "match_feature_group_criteria", None) - if matcher is None: - return - - inner: Any = getattr(matcher, "__func__", matcher) - if getattr(inner, REQUIRED_WHEN_GUARD_FLAG, False): - return - - @functools.wraps(inner) - def guarded(guarded_cls: type[Any], *args: Any, **kwargs: Any) -> bool: - # The outermost guard is the one whose class the matcher was called on, so it is the one - # whose PROPERTY_MAPPING decides. An inner guard, reached through a delegating super() - # call, only answers with its matcher's verdict. - outermost = REQUIRED_WHEN_GUARD_DEPTH.get() == 0 - token = REQUIRED_WHEN_GUARD_DEPTH.set(REQUIRED_WHEN_GUARD_DEPTH.get() + 1) - try: - if not inner(guarded_cls, *args, **kwargs): - return False - - if not outermost: - return True - - feature_name, options = FeatureChainParser._resolve_match_arguments(args, kwargs) - if options is None: - return True - - return FeatureChainParser.check_required_when( - guarded_cls.__name__, - feature_name, - FeatureChainParser.prefix_patterns_of(guarded_cls), - getattr(guarded_cls, "PROPERTY_MAPPING", None), - options, - ) - finally: - REQUIRED_WHEN_GUARD_DEPTH.reset(token) - - setattr(guarded, REQUIRED_WHEN_GUARD_FLAG, True) - setattr(owner, "match_feature_group_criteria", classmethod(guarded)) - - @classmethod - def install_name_path_presence_guard(cls, owner: type[Any]) -> None: - """Wrap a class's RESOLVED matcher so the name-path required-presence rule (#769) survives an override. - - Mirrors ``install_required_when_guard``: installed at class definition from both - ``__init_subclass__`` hooks, never wrapped twice, and guards nest so only the outermost one - evaluates. An inner False stands untouched, so the inner path's own presence warning is never - duplicated. Nesting order relative to the required_when guard is behaviorally irrelevant: - each guard ANDs its own predicate onto the inner verdict and passes False through unchanged. - """ - property_mapping = getattr(owner, "PROPERTY_MAPPING", None) - if not isinstance(property_mapping, dict): - return - if not cls.prefix_patterns_of(owner): - return - # Same exemptions as the inner rule: with empty options, the missing keys ARE the flaggable ones. - if not cls._name_path_missing_required_keys(Options(), property_mapping): - return - - # Wrapping a staticmethod matcher would hide it from _reject_staticmethod_matcher, so the - # required_when installer's existing definition-time ValueError keeps precedence. - is_static = cls._matcher_is_staticmethod(owner) - if is_static and cls.has_required_when_predicates(property_mapping): - return - - # getattr on a staticmethod returns the plain function, so the __func__ fetch below is a no-op - # for it and the flag check covers both shapes. - matcher = getattr(owner, "match_feature_group_criteria", None) - if matcher is None: - return - - inner: Any = getattr(matcher, "__func__", matcher) - if getattr(inner, NAME_PATH_PRESENCE_GUARD_FLAG, False): - return - - @functools.wraps(inner) - def guarded(guarded_cls: type[Any], *args: Any, **kwargs: Any) -> bool: - outermost = NAME_PATH_PRESENCE_GUARD_DEPTH.get() == 0 - token = NAME_PATH_PRESENCE_GUARD_DEPTH.set(NAME_PATH_PRESENCE_GUARD_DEPTH.get() + 1) - try: - # A staticmethod inner keeps its calling convention: no cls injected. An inner False - # stands: the inner default path already warned on its own presence non-match, so - # passing it through keeps one warning per match call. - inner_verdict = inner(*args, **kwargs) if is_static else inner(guarded_cls, *args, **kwargs) - if not inner_verdict: - return False - - if not outermost: - return True - - feature_name, options = FeatureChainParser._resolve_match_arguments(args, kwargs) - if options is None: - return True - - mapping = getattr(guarded_cls, "PROPERTY_MAPPING", None) - if not isinstance(mapping, dict): - return True - # Flattened, because a matcher passes a list-valued pattern attribute straight to - # parse_name, so its ELEMENTS are the concrete patterns. A matcher must never leak - # an exception, so the parse is contained as in build_effective_options; re.error is - # additionally caught here because a malformed pattern must degrade to a non-match - # of the name path, never veto the inner verdict (#868). - patterns = FeatureChainParser._flatten_patterns(FeatureChainParser.prefix_patterns_of(guarded_cls)) - parsed = safe_field( - lambda: FeatureChainParser.parse_name(feature_name, patterns, CHAIN_SEPARATOR), - ParsedFeatureName.no_match(), - catching=(ValueError, re.error), - ) - if not FeatureChainParser._name_identifies_group(parsed, mapping): - return True - bindings = FeatureChainParser.bind_name_captures(parsed, mapping) - effective_options = FeatureChainParser._merge_bindings(options, bindings, mapping) - return FeatureChainParser._check_name_path_required_presence( - guarded_cls.__name__, feature_name, effective_options, mapping - ) - finally: - NAME_PATH_PRESENCE_GUARD_DEPTH.reset(token) - - setattr(guarded, NAME_PATH_PRESENCE_GUARD_FLAG, True) - setattr(owner, "match_feature_group_criteria", classmethod(guarded)) - @classmethod def extract_in_feature(cls, feature_name: str, suffix_pattern: str) -> str: """ diff --git a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser_mixin.py b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser_mixin.py index 71b4f370..c7f28749 100644 --- a/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser_mixin.py +++ b/mloda/core/abstract_plugins/components/feature_chainer/feature_chain_parser_mixin.py @@ -61,6 +61,13 @@ from mloda.core.abstract_plugins.components.feature import Feature from mloda.core.abstract_plugins.components.feature_name import FeatureName from mloda.core.abstract_plugins.components.options import Options +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_author_guards import ( + install_name_path_presence_guard, + install_required_when_guard, + validate_name_binding, + warn_captureless_without_binding, + warn_universal_optional_matcher, +) from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import ( FeatureChainParser, CHAIN_SEPARATOR, @@ -100,7 +107,7 @@ class FeatureChainParserMixin: True and the option value is absent, ``match_feature_group_criteria`` rejects the match. When the predicate returns False, the option is treated as optional. The predicates are enforced by a guard installed on the class at definition time (see - ``FeatureChainParser.install_required_when_guard``), so overriding + ``feature_chain_author_guards.install_required_when_guard``), so overriding ``match_feature_group_criteria`` keeps the contract. This works for both string-based and configuration-based feature creation. For @@ -132,11 +139,11 @@ def __init_subclass__(cls, **kwargs: Any) -> None: # The mixin sits first in the MRO of ``class X(FeatureChainParserMixin, FeatureGroup)``, # so super() is what lets FeatureGroup's own class-definition validation still run. super().__init_subclass__(**kwargs) - FeatureChainParser.validate_name_binding(cls) - FeatureChainParser.warn_captureless_without_binding(cls) - FeatureChainParser.install_name_path_presence_guard(cls) - FeatureChainParser.install_required_when_guard(cls) - FeatureChainParser.warn_universal_optional_matcher(cls) + validate_name_binding(cls) + warn_captureless_without_binding(cls) + install_name_path_presence_guard(cls) + install_required_when_guard(cls) + warn_universal_optional_matcher(cls) @classmethod def _validate_string_match(cls, _feature_name: str, _operation_config: str, _in_feature: str) -> bool: diff --git a/mloda/core/abstract_plugins/feature_group.py b/mloda/core/abstract_plugins/feature_group.py index 8946550d..e58808ff 100644 --- a/mloda/core/abstract_plugins/feature_group.py +++ b/mloda/core/abstract_plugins/feature_group.py @@ -12,6 +12,12 @@ from mloda.core.abstract_plugins.components.domain import Domain from mloda.core.abstract_plugins.components.base_feature_group_version import BaseFeatureGroupVersion +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_author_guards import ( + install_name_path_presence_guard, + install_required_when_guard, + validate_name_binding, + warn_captureless_without_binding, +) from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import ( CHAIN_SEPARATOR, COLUMN_SEPARATOR, @@ -92,10 +98,10 @@ def calculate_feature(cls, data, features): def __init_subclass__(cls, **kwargs: Any) -> None: super().__init_subclass__(**kwargs) FeatureChainParser.validate_property_mapping_defaults(cls.__name__, cls.PROPERTY_MAPPING) - FeatureChainParser.validate_name_binding(cls) - FeatureChainParser.warn_captureless_without_binding(cls) - FeatureChainParser.install_name_path_presence_guard(cls) - FeatureChainParser.install_required_when_guard(cls) + validate_name_binding(cls) + warn_captureless_without_binding(cls) + install_name_path_presence_guard(cls) + install_required_when_guard(cls) cls._validate_subtype_declaration() @classmethod diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_author_guards_module_split.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_author_guards_module_split.py new file mode 100644 index 00000000..78302344 --- /dev/null +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_author_guards_module_split.py @@ -0,0 +1,210 @@ +"""Pins the split of the author-time guard subsystem out of feature_chain_parser. + +Ownership of the moved names, the runtime match path staying put, the acyclic direction +feature_chain_parser <- feature_chain_author_guards, and the absence of re-exports back +onto FeatureChainParser. +""" + +from __future__ import annotations + +import ast +import importlib +import inspect +import subprocess # nosec B404 +import sys +from collections.abc import Sequence +from pathlib import Path +from types import ModuleType + +from mloda.core.abstract_plugins.components.feature_chainer import feature_chain_parser +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import FeatureChainParser + +PARSER_MODULE = "mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser" +GUARDS_MODULE = "mloda.core.abstract_plugins.components.feature_chainer.feature_chain_author_guards" + +# Author-time constants that move with the guards. +GUARDS_CONSTANTS = ( + "REQUIRED_WHEN_GUARD_FLAG", + "NAME_PATH_PRESENCE_GUARD_FLAG", + "CAPTURELESS_DIAGNOSTIC_FLAG", + "_UNIVERSAL_MATCHER_PROBE_NAME", + "REQUIRED_WHEN_GUARD_DEPTH", + "NAME_PATH_PRESENCE_GUARD_DEPTH", +) + +# Class-definition-time validation and guard installation, as module-level functions. +GUARDS_FUNCTIONS = ( + "validate_name_binding", + "warn_captureless_without_binding", + "warn_universal_optional_matcher", + "check_required_when", + "install_required_when_guard", + "install_name_path_presence_guard", + "_matcher_is_staticmethod", + "_reject_staticmethod_matcher", + "_resolve_match_arguments", + "_pattern_named_and_total_groups", + "_flatten_patterns", + "_str_reachable_values", +) + +# The public moved names, which must NOT come back as FeatureChainParser attributes. +NO_REEXPORT_NAMES = ( + "validate_name_binding", + "warn_captureless_without_binding", + "warn_universal_optional_matcher", + "install_required_when_guard", + "install_name_path_presence_guard", + "check_required_when", +) + +# The runtime match path, which stays on FeatureChainParser, including the members the guards call back into. +PARSER_KEPT_METHODS = ( + "parse_name", + "parse_feature_name", + "match_configuration_feature_chain_parser", + "build_effective_options", + "bind_name_captures", + "prefix_patterns_of", + "has_required_when_predicates", + "_name_identifies_group", + "_merge_bindings", + "_check_name_path_required_presence", + "_can_skip_required_check", + "_name_path_missing_required_keys", + "extract_property_values", + "name_path_presence_rejection_reason", + "extract_in_feature", + "validate_property_mapping_defaults", +) + +# Module-level names of the parser that carry a __module__ to check. +PARSER_KEPT_MODULE_LEVEL = ( + "record_match_rejection", + "option_key_is_present", + "_contained_raise_log_level", + "PropertyValueRejection", +) + +# Module-level parser constants, which have no __module__ to check. +PARSER_KEPT_CONSTANTS = ( + "MATCH_REJECTION_REASONS", + "CHAIN_SEPARATOR", + "COLUMN_SEPARATOR", + "INPUT_SEPARATOR", +) + +_SUBPROCESS_TIMEOUT = 8.0 + +# Import the parser first, record whether the guards came along, then prove the guards module exists. +_CLEAN_IMPORT_SNIPPET = ( + "import importlib\n" + "import sys\n" + f"importlib.import_module({PARSER_MODULE!r})\n" + f"pulled_in = {GUARDS_MODULE!r} in sys.modules\n" + f"importlib.import_module({GUARDS_MODULE!r})\n" + "print('present' if pulled_in else 'absent')\n" +) + + +def _chainer_dir() -> Path: + """Directory of the feature_chainer package, read off the parser module that already exists.""" + parser_file = feature_chain_parser.__file__ + assert parser_file is not None + return Path(parser_file).parent + + +def _repo_root() -> Path: + """Repo root, four levels above mloda/core/abstract_plugins/components/feature_chainer.""" + return _chainer_dir().parents[4] + + +def _missing_names(module: ModuleType, names: Sequence[str]) -> list[str]: + return [name for name in names if not hasattr(module, name)] + + +def _imported_modules(path: Path) -> set[str]: + """Module names every import statement in one file references. + + Alias names are included so the `from . import sibling` form is covered too. + """ + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + modules: set[str] = set() + for node in ast.walk(tree): + if isinstance(node, ast.Import): + modules.update(alias.name for alias in node.names) + elif isinstance(node, ast.ImportFrom): + if node.module is not None: + modules.add(node.module) + modules.update(alias.name for alias in node.names) + return modules + + +def _references(modules: set[str], target: str) -> list[str]: + return sorted(module for module in modules if module == target or module.endswith(f".{target}")) + + +def test_author_guards_module_defines_the_moved_constants() -> None: + guards = importlib.import_module(GUARDS_MODULE) + missing = _missing_names(guards, GUARDS_CONSTANTS) + assert missing == [], f"{GUARDS_MODULE} does not define {missing}" + + +def test_author_guards_module_defines_the_moved_functions() -> None: + guards = importlib.import_module(GUARDS_MODULE) + missing = _missing_names(guards, GUARDS_FUNCTIONS) + assert missing == [], f"{GUARDS_MODULE} does not define {missing}" + + +def test_moved_functions_are_module_level_functions() -> None: + """The moved guards are plain functions taking the owner class, not classmethods of some new class.""" + guards = importlib.import_module(GUARDS_MODULE) + for name in GUARDS_FUNCTIONS: + member = getattr(guards, name) + assert inspect.isfunction(member), f"{GUARDS_MODULE}.{name} is {type(member)!r}, not a module-level function" + assert member.__module__ == GUARDS_MODULE, f"{name} is defined in {member.__module__}, not {GUARDS_MODULE}" + + +def test_moved_names_are_not_reexported_on_the_parser_class() -> None: + """No compatibility shim: a re-export would let call sites keep the old spelling and undo the split.""" + reexported = [name for name in NO_REEXPORT_NAMES if hasattr(FeatureChainParser, name)] + assert reexported == [], f"FeatureChainParser still exposes {reexported}, so the split is only cosmetic" + + +def test_runtime_match_path_stays_on_the_parser_class() -> None: + for name in PARSER_KEPT_METHODS: + assert hasattr(FeatureChainParser, name), f"FeatureChainParser no longer defines {name}" + owner = getattr(FeatureChainParser, name).__module__ + assert owner == PARSER_MODULE, f"{name} moved out of {PARSER_MODULE} to {owner}" + + +def test_parser_module_level_names_stay_defined_in_the_parser() -> None: + for name in PARSER_KEPT_MODULE_LEVEL: + assert hasattr(feature_chain_parser, name), f"{PARSER_MODULE} no longer defines {name}" + owner = getattr(feature_chain_parser, name).__module__ + assert owner == PARSER_MODULE, f"{name} moved out of {PARSER_MODULE} to {owner}" + missing = _missing_names(feature_chain_parser, PARSER_KEPT_CONSTANTS) + assert missing == [], f"{PARSER_MODULE} no longer defines {missing}" + + +def test_parser_does_not_import_the_author_guards() -> None: + """The guards import the parser, never the reverse; that one direction is what keeps the split acyclic.""" + guards_path = _chainer_dir() / "feature_chain_author_guards.py" + assert guards_path.is_file(), f"{guards_path} does not exist" + parser_path = _chainer_dir() / "feature_chain_parser.py" + offenders = _references(_imported_modules(parser_path), "feature_chain_author_guards") + assert offenders == [], f"{parser_path.name} imports {offenders}, which closes the cycle" + + +def test_parser_imports_in_a_clean_interpreter_without_the_author_guards() -> None: + """Runtime counterpart of the static check: a fresh interpreter must not pull the guards in transitively.""" + # Safe: fixed argv (sys.executable plus a literal snippet), no shell, no user input. + result = subprocess.run( # nosec B603 + [sys.executable, "-c", _CLEAN_IMPORT_SNIPPET], + capture_output=True, + text=True, + timeout=_SUBPROCESS_TIMEOUT, + cwd=_repo_root(), + ) + assert result.returncode == 0, f"importing {PARSER_MODULE} in a fresh interpreter failed:\n{result.stderr}" + assert result.stdout.strip() == "absent", f"{GUARDS_MODULE} was imported transitively by {PARSER_MODULE}" diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_captureless_no_binding.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_captureless_no_binding.py index 2165423b..ae131f91 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_captureless_no_binding.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_captureless_no_binding.py @@ -30,7 +30,8 @@ CLEANED_TEXT_NAME = "x__cleaned_text" CLEANED_TEXT_PATTERN = r".*__cleaned_text$" -FEATURE_CHAIN_PARSER_LOGGER = "mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser" +# The captureless diagnostic is emitted by feature_chain_author_guards.warn_captureless_without_binding. +AUTHOR_GUARDS_LOGGER = "mloda.core.abstract_plugins.components.feature_chainer.feature_chain_author_guards" # "cleaned" is exactly the token the retired code fabricated from ``__cleaned_text``. FABRICATED_TOKEN = "cleaned" # nosec B105 @@ -162,7 +163,7 @@ class _CapturelessNoMarkerC772(FeatureChainParserMixin): record for record in caplog.records if record.levelno == logging.WARNING - and record.name == FEATURE_CHAIN_PARSER_LOGGER + and record.name == AUTHOR_GUARDS_LOGGER and "RECOGNITION_ONLY_PATTERN" in record.getMessage() ] assert warnings @@ -186,7 +187,7 @@ class _CapturelessWithMarkerC772(FeatureChainParserMixin): record for record in caplog.records if record.levelno == logging.WARNING - and record.name == FEATURE_CHAIN_PARSER_LOGGER + and record.name == AUTHOR_GUARDS_LOGGER and "RECOGNITION_ONLY_PATTERN" in record.getMessage() ] assert not warnings @@ -303,7 +304,7 @@ class _X_rf772b(FeatureChainParserMixin, FeatureGroup): warnings = [ record for record in caplog.records - if record.name == FEATURE_CHAIN_PARSER_LOGGER and "RECOGNITION_ONLY_PATTERN" in record.getMessage() + if record.name == AUTHOR_GUARDS_LOGGER and "RECOGNITION_ONLY_PATTERN" in record.getMessage() ] assert len(warnings) == 1 @@ -324,6 +325,6 @@ class _Y_rf772b(FeatureChainParserMixin): warnings = [ record for record in caplog.records - if record.name == FEATURE_CHAIN_PARSER_LOGGER and "RECOGNITION_ONLY_PATTERN" in record.getMessage() + if record.name == AUTHOR_GUARDS_LOGGER and "RECOGNITION_ONLY_PATTERN" in record.getMessage() ] assert len(warnings) == 1 diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_explicit_none_opt_in.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_explicit_none_opt_in.py index 50fb2c36..3aede4db 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_explicit_none_opt_in.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_explicit_none_opt_in.py @@ -18,6 +18,7 @@ import pytest +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_author_guards import check_required_when from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import ( FeatureChainParser, PropertyValueRejection, @@ -129,32 +130,19 @@ def test_opted_in_explicit_none_satisfies_required_when(self) -> None: """D1: an opted-in satisfied requirement is met by an explicit None, which now counts as present.""" property_mapping = {"k": PropertySpec("k", required_when=_always_required, allow_explicit_none=True)} - assert ( - FeatureChainParser.check_required_when( - "Owner", "any_feature", [], property_mapping, Options(context={"k": None}) - ) - is True - ) + assert check_required_when("Owner", "any_feature", [], property_mapping, Options(context={"k": None})) is True def test_flagless_explicit_none_leaves_requirement_unmet(self) -> None: """D2 contrast (DoD 3): without the flag the explicit None reads as absent, so the requirement is unmet.""" property_mapping = {"k": PropertySpec("k", required_when=_always_required)} - assert ( - FeatureChainParser.check_required_when( - "Owner", "any_feature", [], property_mapping, Options(context={"k": None}) - ) - is False - ) + assert check_required_when("Owner", "any_feature", [], property_mapping, Options(context={"k": None})) is False def test_opted_in_absent_key_leaves_requirement_unmet(self) -> None: """D3: absent stays absent; an opted-in required key that is truly absent is still unmet.""" property_mapping = {"k": PropertySpec("k", required_when=_always_required, allow_explicit_none=True)} - assert ( - FeatureChainParser.check_required_when("Owner", "any_feature", [], property_mapping, Options(context={})) - is False - ) + assert check_required_when("Owner", "any_feature", [], property_mapping, Options(context={})) is False class TestMatchGuardHonorsExplicitNone: diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_name_path_presence_guard.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_name_path_presence_guard.py index da2a7f4d..9e0287f7 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_name_path_presence_guard.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_name_path_presence_guard.py @@ -2,7 +2,7 @@ The rule is enforced inside ``FeatureChainParserMixin.match_parser_criteria``, so a provider that overrides ``match_feature_group_criteria`` and returns True WITHOUT delegating bypasses it entirely. -Like ``required_when`` (``FeatureChainParser.install_required_when_guard``), the enforcement must be +Like ``required_when`` (``feature_chain_author_guards.install_required_when_guard``), the enforcement must be a definition-time wrapper around the matcher: it turns the bypass into a warned non-match, honors the same exemptions as the inner rule, applies only when the name identifies the group, and never duplicates the inner path's warning. diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_option_value_rejection_never_escapes.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_option_value_rejection_never_escapes.py index 42e6d5fb..624e6758 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_option_value_rejection_never_escapes.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_option_value_rejection_never_escapes.py @@ -16,6 +16,7 @@ from mloda.core.abstract_plugins.components.data_access_collection import DataAccessCollection from mloda.core.abstract_plugins.components.default_options_key import DefaultOptionKeys from mloda.core.abstract_plugins.components.feature import Feature +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_author_guards import check_required_when from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import ( FeatureChainParser, PropertyValueRejection, @@ -76,9 +77,11 @@ TYPE_POISON_VALUE_TIER763 = "type_poison_tier763" CAUSE_KEY_TIER763 = "cause_key_tier763" -# Both modules create their logger via logging.getLogger(__name__), so the class's module IS the logger name. +# Every module creates its logger via logging.getLogger(__name__), so the class's module IS the logger name. PARSER_LOGGER_NAME = FeatureChainParser.__module__ MIXIN_LOGGER_NAME = FeatureChainParserMixin.__module__ +# check_required_when, and with it the required_when containment record, lives in the author-guards module. +GUARDS_LOGGER_NAME = check_required_when.__module__ def _keyerror_element_validator_esc763(value: Any) -> bool: @@ -330,6 +333,11 @@ def _records_at_exactly(caplog: pytest.LogCaptureFixture, logger_name: str, leve return [record for record in caplog.records if record.name == logger_name and record.levelno == levelno] +def _any_logger_records_at_or_above(caplog: pytest.LogCaptureFixture, levelno: int) -> list[logging.LogRecord]: + """Records any logger emitted at or above the given level.""" + return [record for record in caplog.records if record.levelno >= levelno] + + class TestRaisingElementValidatorIsARejection: """A validator that raises rejects the value; it does not blow up the match.""" @@ -721,8 +729,8 @@ def test_build_effective_options_raise_propagates_out_of_the_engine(self, monkey def test_keyerror_build_effective_options_raise_propagates_without_warning( self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture ) -> None: - """A KeyError from build_effective_options propagates unchanged; the parser logs no WARNING containment.""" - caplog.set_level(logging.DEBUG, logger=PARSER_LOGGER_NAME) + """A KeyError from build_effective_options propagates unchanged; no module logs a WARNING containment.""" + caplog.set_level(logging.DEBUG) monkeypatch.setattr( FeatureChainParser, "build_effective_options", @@ -735,7 +743,7 @@ def test_keyerror_build_effective_options_raise_propagates_without_warning( REQUIRED_WHEN_FEATURE_ESC763, options ) - assert _records_at_or_above(caplog, PARSER_LOGGER_NAME, logging.WARNING) == [], ( + assert _any_logger_records_at_or_above(caplog, logging.WARNING) == [], ( "a propagating build_effective_options raise must not leave a WARNING-tier containment record" ) @@ -743,7 +751,7 @@ def test_valueerror_build_effective_options_raise_leaves_no_containment_record( self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture ) -> None: """A ValueError from build_effective_options propagates unchanged and leaves no containment record at all.""" - caplog.set_level(logging.DEBUG, logger=PARSER_LOGGER_NAME) + caplog.set_level(logging.DEBUG) monkeypatch.setattr( FeatureChainParser, "build_effective_options", @@ -756,8 +764,8 @@ def test_valueerror_build_effective_options_raise_leaves_no_containment_record( REQUIRED_WHEN_FEATURE_ESC763, options ) - assert _records_at_or_above(caplog, PARSER_LOGGER_NAME, logging.WARNING) == [] - debug_records = _records_at_exactly(caplog, PARSER_LOGGER_NAME, logging.DEBUG) + assert _any_logger_records_at_or_above(caplog, logging.WARNING) == [] + debug_records = _records_at_exactly(caplog, GUARDS_LOGGER_NAME, logging.DEBUG) assert all("non-match" not in record.getMessage() for record in debug_records), ( "a propagating build_effective_options raise must not leave a DEBUG non-match containment record" ) @@ -850,7 +858,7 @@ def test_typeerror_guard_raise_logs_at_debug_only(self, caplog: pytest.LogCaptur def test_keyerror_required_when_raise_logs_at_warning(self, caplog: pytest.LogCaptureFixture) -> None: """A predicate raising KeyError is still a non-match, but the containment surfaces at WARNING.""" - caplog.set_level(logging.DEBUG, logger=PARSER_LOGGER_NAME) + caplog.set_level(logging.DEBUG, logger=GUARDS_LOGGER_NAME) options = Options(context={DRIVER_KEY_ESC763: POISON_VALUE_ESC763}) result = RaisingKeyErrorRequiredWhenFeatureGroupEsc763.match_feature_group_criteria( @@ -858,7 +866,7 @@ def test_keyerror_required_when_raise_logs_at_warning(self, caplog: pytest.LogCa ) assert result is False - warnings = _records_at_or_above(caplog, PARSER_LOGGER_NAME, logging.WARNING) + warnings = _records_at_or_above(caplog, GUARDS_LOGGER_NAME, logging.WARNING) assert warnings, "a KeyError-raising predicate looks broken and must log at WARNING" owner = RaisingKeyErrorRequiredWhenFeatureGroupEsc763.__name__ assert any( @@ -867,7 +875,7 @@ def test_keyerror_required_when_raise_logs_at_warning(self, caplog: pytest.LogCa def test_typeerror_required_when_raise_logs_at_debug_only(self, caplog: pytest.LogCaptureFixture) -> None: """A predicate raising TypeError is an expected judgment failure: contained at DEBUG, never WARNING.""" - caplog.set_level(logging.DEBUG, logger=PARSER_LOGGER_NAME) + caplog.set_level(logging.DEBUG, logger=GUARDS_LOGGER_NAME) options = Options(context={DRIVER_KEY_ESC763: TYPE_POISON_VALUE_TIER763}) result = RaisingKeyErrorRequiredWhenFeatureGroupEsc763.match_feature_group_criteria( @@ -875,8 +883,8 @@ def test_typeerror_required_when_raise_logs_at_debug_only(self, caplog: pytest.L ) assert result is False - assert _records_at_or_above(caplog, PARSER_LOGGER_NAME, logging.WARNING) == [] - debug_records = _records_at_exactly(caplog, PARSER_LOGGER_NAME, logging.DEBUG) + assert _records_at_or_above(caplog, GUARDS_LOGGER_NAME, logging.WARNING) == [] + debug_records = _records_at_exactly(caplog, GUARDS_LOGGER_NAME, logging.DEBUG) assert debug_records, "the contained TypeError must still log its rejection at DEBUG" owner = RaisingKeyErrorRequiredWhenFeatureGroupEsc763.__name__ assert any( diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_required_when_enforced_on_override.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_required_when_enforced_on_override.py index 83b5f101..f25d37b6 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_required_when_enforced_on_override.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_required_when_enforced_on_override.py @@ -15,11 +15,11 @@ import pytest from mloda.core.abstract_plugins.components.default_options_key import DefaultOptionKeys -from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import ( +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_author_guards import ( NAME_PATH_PRESENCE_GUARD_FLAG, REQUIRED_WHEN_GUARD_FLAG, - FeatureChainParser, ) +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import FeatureChainParser from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser_mixin import ( FeatureChainParserMixin, ) diff --git a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_universal_optional_matcher.py b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_universal_optional_matcher.py index 51ffa96a..be496e88 100644 --- a/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_universal_optional_matcher.py +++ b/tests/test_core/test_abstract_plugins/test_components/feature_chainer/test_universal_optional_matcher.py @@ -30,10 +30,10 @@ import pytest -from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import ( - FeatureChainParser, +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_author_guards import ( _UNIVERSAL_MATCHER_PROBE_NAME, ) +from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser import FeatureChainParser from mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser_mixin import FeatureChainParserMixin from mloda.core.abstract_plugins.components.feature_name import FeatureName from mloda.core.abstract_plugins.components.options import Options @@ -42,7 +42,8 @@ from mloda.provider import PropertySpec from mloda_plugins.feature_group.experimental.aggregated_feature_group.base import AggregatedFeatureGroup -FEATURE_CHAIN_PARSER_LOGGER = "mloda.core.abstract_plugins.components.feature_chainer.feature_chain_parser" +# Both the universal-matcher diagnostic and check_required_when live in the author-guards module. +AUTHOR_GUARDS_LOGGER = "mloda.core.abstract_plugins.components.feature_chainer.feature_chain_author_guards" # A name that no fixture pattern would ever recognize, used as the "unrelated probe". UNRELATED_NAME_U771 = "some_unrelated_feature_u771" @@ -72,7 +73,7 @@ def _universal_matcher_warnings( record for record in caplog.records if record.levelno == logging.WARNING - and record.name == FEATURE_CHAIN_PARSER_LOGGER + and record.name == AUTHOR_GUARDS_LOGGER and "ALLOW_UNIVERSAL_MATCHER" in record.getMessage() ] if class_name is not None: @@ -339,7 +340,7 @@ class _SelfRefConditionalU771l(FeatureChainParserMixin, FeatureGroup): spurious = [ record for record in caplog.records - if record.name == FEATURE_CHAIN_PARSER_LOGGER + if record.name == AUTHOR_GUARDS_LOGGER and "required_when" in record.getMessage() and "raised" in record.getMessage() ] @@ -413,7 +414,7 @@ def test_contained_probe_logs_str_not_exception(self, caplog: pytest.LogCaptureF A record holding the exception pins its traceback, frame and the probed class, defeating the registry-pollution cleanup under DEBUG logging (the utils.safe_field discipline). """ - with caplog.at_level(logging.DEBUG, logger=FEATURE_CHAIN_PARSER_LOGGER): + with caplog.at_level(logging.DEBUG, logger=AUTHOR_GUARDS_LOGGER): class _DebugRaiserU771y(FeatureChainParserMixin, FeatureGroup): PROPERTY_MAPPING = {"opt_u771y": PropertySpec("optional", default=None)} diff --git a/tests/test_core/test_prepare/test_required_when_rejection_recording.py b/tests/test_core/test_prepare/test_required_when_rejection_recording.py index 6841336d..640ed79e 100644 --- a/tests/test_core/test_prepare/test_required_when_rejection_recording.py +++ b/tests/test_core/test_prepare/test_required_when_rejection_recording.py @@ -5,7 +5,7 @@ matches becomes that candidate's ``value_rejection`` ``Elimination`` and reaches the resolution-failure report. Two presence rules already record there, the name-path required-presence rule (``FeatureChainParser._check_name_path_required_presence``) and the strict ``match_guard`` -(``FeatureChainParserMixin``). ``FeatureChainParser.check_required_when`` only logs at debug and records +(``FeatureChainParserMixin``). ``feature_chain_author_guards.check_required_when`` only logs at debug and records nothing, so a feature group that a declared ``required_when`` turned into a non-match is invisible in the failure report. See ``test_first_pass_rejection_recording.py`` for the seam's full contract.