diff --git a/.github/actions/bc-lint/action.yml b/.github/actions/bc-lint/action.yml index 13369d5dda..02d306e90d 100644 --- a/.github/actions/bc-lint/action.yml +++ b/.github/actions/bc-lint/action.yml @@ -61,6 +61,7 @@ runs: shell: bash run: | set -eux + python3 -m pip install -r ../_test-infra/tools/stronghold/requirements.runtime.txt ../_test-infra/tools/stronghold/bin/build-check-api-compatibility ../_test-infra/tools/stronghold/bin/check-api-compatibility \ --base-commit=${{ inputs.base_sha }} \ diff --git a/tools/stronghold/docs/bc_linter_config.md b/tools/stronghold/docs/bc_linter_config.md new file mode 100644 index 0000000000..594cf20cb0 --- /dev/null +++ b/tools/stronghold/docs/bc_linter_config.md @@ -0,0 +1,70 @@ +# BC Linter Configuration (beta) + +This document describes the configuration format for the Stronghold BC linter. +The config enables repo‑specific path selection, rule suppression, and custom +annotations to include/exclude specific APIs. + +### Config file location +- Place a YAML file named `.bc-linter.yml` at the repository root being linted + (the target repo). +- If the file is missing or empty, defaults are applied (see below). + +### Schema (YAML) +```yml +version: 1 + +paths: + include: + - "**/*.py" # globs of files to consider (default) + exclude: + - "**/.*/**" # exclude hidden directories by default + - "**/.*" # exclude hidden files by default + +scan: + functions: true # check free functions and methods + classes: true # check classes/dataclasses + public_only: true # ignore names starting with "_" at any level + +annotations: + include: # decorators that force‑include a symbol + - name: "bc_linter_include" # matched by simple name or dotted suffix + propagate_to_members: false # for classes, include methods/inner classes + exclude: # decorators that force‑exclude a symbol + - name: "bc_linter_skip" # matched by simple name or dotted suffix + propagate_to_members: true # for classes, exclude methods/inner classes + +excluded_violations: [] # e.g. ["ParameterRenamed", "FieldTypeChanged"] +``` + +### Behavior notes +- Paths precedence: `annotations.exclude` > `annotations.include` > `paths`. + Annotations can override file include/exclude rules. +- Name matching for annotations: A decorator matches if either its simple name + equals the configured `name` (e.g., `@bc_linter_skip`) or if its dotted + attribute ends with the configured `name` (e.g., `@proj.bc_linter_skip`). +- `public_only`: When true, any symbol whose qualified name contains a component + that starts with `_` is ignored (e.g., `module._Internal.func`, `Class._m`). +- Rule suppression: `excluded_violations` contains class names from + `api.violations` to omit from output (e.g., `FieldTypeChanged`). +- Invariants not affected by config: + - Deleted methods of a deleted class are not double‑reported (only the class). + - Nested class deletions collapse to the outermost deleted class. + - Dataclass detection and field inference are unchanged. + +### Defaults +If `.bc-linter.yml` is missing or empty, the following defaults apply: + +``` +version: 1 +paths: + include: ["**/*.py"] + exclude: [".*", ".*/**", ".*/**/*", "**/.*/**", "**/.*"] +scan: + functions: true + classes: true + public_only: true +annotations: + include: [] + exclude: [] +excluded_violations: [] +``` diff --git a/tools/stronghold/requirements.runtime.txt b/tools/stronghold/requirements.runtime.txt new file mode 100644 index 0000000000..1f1aea2226 --- /dev/null +++ b/tools/stronghold/requirements.runtime.txt @@ -0,0 +1,2 @@ +PyYAML==6.0.2 +pathspec==0.12.1 diff --git a/tools/stronghold/requirements.txt b/tools/stronghold/requirements.txt index 4916d0e3a5..6eedf8c4c5 100644 --- a/tools/stronghold/requirements.txt +++ b/tools/stronghold/requirements.txt @@ -3,3 +3,5 @@ flake8==5.0.4 mypy==0.990 pip==23.3 pytest==7.2.0 +PyYAML==6.0.2 +pathspec==0.12.1 diff --git a/tools/stronghold/src/api/__init__.py b/tools/stronghold/src/api/__init__.py index a0c039a7f4..65d70e2126 100644 --- a/tools/stronghold/src/api/__init__.py +++ b/tools/stronghold/src/api/__init__.py @@ -22,6 +22,8 @@ class Parameters: variadic_kwargs: bool # The line where the function is defined. line: int + # Decorator names applied to this function/method (simple or dotted form). + decorators: Sequence[str] = () @dataclasses.dataclass @@ -62,6 +64,8 @@ class Class: fields: Sequence[Field] line: int dataclass: bool = False + # Decorator names applied to the class (simple or dotted form). + decorators: Sequence[str] = () @dataclasses.dataclass diff --git a/tools/stronghold/src/api/ast.py b/tools/stronghold/src/api/ast.py index 6b088770b6..551cc7a1f4 100644 --- a/tools/stronghold/src/api/ast.py +++ b/tools/stronghold/src/api/ast.py @@ -80,11 +80,13 @@ def _function_def_to_parameters(node: ast.FunctionDef) -> api.Parameters: ) for i, arg in enumerate(args.kwonlyargs) ] + dec_names = tuple(_decorator_to_name(d) for d in node.decorator_list) return api.Parameters( parameters=params, variadic_args=args.vararg is not None, variadic_kwargs=args.kwarg is not None, line=node.lineno, + decorators=dec_names, ) @@ -106,10 +108,9 @@ def visit_ClassDef(self, node: ast.ClassDef) -> None: # class name pushed onto a new context. if self._classes is not None: name = ".".join(self._context + [node.name]) + dec_names = [_decorator_to_name(d) for d in node.decorator_list] is_dataclass = any( - (isinstance(dec, ast.Name) and dec.id == "dataclass") - or (isinstance(dec, ast.Attribute) and dec.attr == "dataclass") - for dec in node.decorator_list + (n == "dataclass" or n.endswith(".dataclass")) for n in dec_names ) fields: list[api.Field] = [] for stmt in node.body: @@ -180,7 +181,10 @@ def _is_field_func(f: ast.AST) -> bool: ) ) self._classes[name] = api.Class( - fields=fields, line=node.lineno, dataclass=is_dataclass + fields=fields, + line=node.lineno, + dataclass=is_dataclass, + decorators=tuple(dec_names), ) _ContextualNodeVisitor( @@ -191,3 +195,39 @@ def visit_FunctionDef(self, node: ast.FunctionDef) -> None: # Records this function. name = ".".join(self._context + [node.name]) self._functions[name] = node + + def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> None: + # Treat async functions similarly by normalizing to FunctionDef shape. + name = ".".join(self._context + [node.name]) + fnode = ast.FunctionDef( + name=node.name, + args=node.args, + body=node.body, + decorator_list=node.decorator_list, + returns=node.returns, + type_comment=node.type_comment, + ) + self._functions[name] = fnode + + +def _decorator_to_name(expr: ast.AST) -> str: + """Extracts a dotted name for a decorator expression. + For calls like @dec(...), returns the callee name "dec". + For attributes like @pkg.mod.dec, returns "pkg.mod.dec". + For names like @dec, returns "dec". + """ + + def _attr_chain(a: ast.AST) -> str | None: + if isinstance(a, ast.Name): + return a.id + if isinstance(a, ast.Attribute): + left = _attr_chain(a.value) + if left is None: + return a.attr + return f"{left}.{a.attr}" + return None + + if isinstance(expr, ast.Call): + expr = expr.func + name = _attr_chain(expr) + return name or "" diff --git a/tools/stronghold/src/api/checker.py b/tools/stronghold/src/api/checker.py index 97863a4059..d54062a9b6 100644 --- a/tools/stronghold/src/api/checker.py +++ b/tools/stronghold/src/api/checker.py @@ -1,11 +1,14 @@ """Runs the API backward compatibility on the head commit.""" import argparse +import dataclasses +import json import pathlib import subprocess import sys import api.compatibility +import api.config import api.git import api.github @@ -22,6 +25,13 @@ def run() -> None: help="Failures are suppressed" "(alternative to #suppress-api-compatibility-check commit message tag).", ) + parser.add_argument( + "--verbose", + default=False, + required=False, + action="store_true", + help="Enable verbose output", + ) args = parser.parse_args(sys.argv[1:]) repo = api.git.Repository(pathlib.Path(".")) @@ -35,8 +45,21 @@ def run() -> None: repo.run(["fetch", "origin", args.base_commit], check=True) print("::endgroup::") + # Load config and optionally print when detected + cfg, cfg_status = api.config.load_config_with_status(repo.dir) + if cfg_status == "parsed": + # Explicitly log successful config discovery and parsing + print("BC-linter: Using .bc-linter.yml (parsed successfully)") + if args.verbose: + print("BC-linter: Parsed config:") + print(json.dumps(dataclasses.asdict(cfg), indent=2, sort_keys=True)) + elif args.verbose: + # In verbose mode, also indicate fallback to defaults + reason = "missing" if cfg_status == "default_missing" else "invalid/malformed" + print(f"BC-linter: No usable config ({reason}); using defaults") + violations = api.compatibility.check_range( - repo, head=args.head_commit, base=args.base_commit + repo, head=args.head_commit, base=args.base_commit, config=cfg ) if len(violations) == 0: return diff --git a/tools/stronghold/src/api/compatibility.py b/tools/stronghold/src/api/compatibility.py index 4d95c2fe5a..5684645025 100644 --- a/tools/stronghold/src/api/compatibility.py +++ b/tools/stronghold/src/api/compatibility.py @@ -9,38 +9,27 @@ import api import api.ast +import api.config import api.git import api.violations def check_range( - repo: api.git.Repository, *, head: str, base: str + repo: api.git.Repository, + *, + head: str, + base: str, + config: api.config.Config | None = None, ) -> Mapping[pathlib.Path, Sequence[api.violations.Violation]]: + cfg = config or api.config.default_config() result = {} for file in repo.get_files_in_range(f"{base}..{head}"): - # Someday, we'll want to customize the filters we use to - # ignore files. + # Only consider Python files. if file.suffix != ".py": - # Only consider Python files. - continue - if any(dir.name.startswith("_") for dir in file.parents): - # Ignore any internal packages. - continue - if any(dir.name.startswith(".") for dir in file.parents): - # Ignore any internal packages and ci modules - continue - if file.name.startswith("_"): - # Ignore internal modules. - continue - if any(dir.name == "test" for dir in file.parents): - # Ignore tests (not part of PyTorch package). - continue - if any(dir.name == "benchmarks" for dir in file.parents): - # Ignore benchmarks (not part of PyTorch package). - continue - if file.name.startswith("test_") or file.stem.endswith("_test"): - # Ignore test files. continue + # Note: path allow/deny is applied at symbol-level inside `check` so that + # annotation-based overrides can include symbols even from otherwise + # excluded files. # Get the contents before and after the diff. # @@ -57,7 +46,9 @@ def check_range( after_path = pathlib.Path(after_file.name) after_path.write_text(after) - violations = api.compatibility.check(before_path, after_path) + violations = api.compatibility.check( + before_path, after_path, file_path=file, config=cfg + ) if len(violations) > 0: result[file] = violations @@ -65,9 +56,14 @@ def check_range( def check( - before: pathlib.Path, after: pathlib.Path + before: pathlib.Path, + after: pathlib.Path, + *, + file_path: pathlib.Path | None = None, + config: api.config.Config | None = None, ) -> Sequence[api.violations.Violation]: """Identifies API compatibility issues between two files.""" + cfg = config or api.config.default_config() before_api = api.ast.extract(before, include_classes=True) after_api = api.ast.extract(after, include_classes=True) before_funcs = before_api.functions @@ -75,12 +71,78 @@ def check( before_classes = before_api.classes after_classes = after_api.classes + # File path allowance (paths include/exclude) influences default symbol inclusion + file_allowed = True + if file_path is not None: + file_allowed = api.config.match_any( + file_path, cfg.include + ) and not api.config.match_any(file_path, cfg.exclude) + + def _is_public(name: str) -> bool: + """Returns True if no qualname component starts with an underscore.""" + return not any(token.startswith("_") for token in name.split(".")) + + def _matches_ann( + decorators: Sequence[str], specs: Sequence[api.config.AnnotationSpec] + ) -> bool: + if not decorators or not specs: + return False + for d in decorators: + for spec in specs: + if d == spec.name or d.endswith(f".{spec.name}"): + return True + return False + + def _symbol_included(name: str, kind: str) -> bool: + # kind: 'func' or 'class' + if cfg.scan.public_only and not _is_public(name): + return False + + # decorators on the symbol in `after` + decs: Sequence[str] = () + if kind == "func": + decs = after_funcs.get(name).decorators if name in after_funcs else () + else: + decs = after_classes.get(name).decorators if name in after_classes else () + + # decorators on ancestor classes (apply only if propagate_to_members is True) + def _ancestor_has(specs: Sequence[api.config.AnnotationSpec]) -> bool: + tokens = name.split(".") + for i in range(1, len(tokens)): + cls_name = ".".join(tokens[:i]) + cls = after_classes.get(cls_name) + if cls is None: + continue + if not cls.decorators: + continue + for d in cls.decorators: + for s in specs: + if not s.propagate_to_members: + continue + if d == s.name or d.endswith(f".{s.name}"): + return True + return False + + excluded_by_ann = _matches_ann(decs, cfg.annotations_exclude) or _ancestor_has( + cfg.annotations_exclude + ) + if excluded_by_ann: + return False + + included_by_ann = _matches_ann(decs, cfg.annotations_include) or _ancestor_has( + cfg.annotations_include + ) + if included_by_ann: + return True + + # Fallback to file-level allow + return file_allowed + # Identify deleted classes to avoid double-reporting their methods as deleted deleted_classes = { name for name in before_classes - if not any(token.startswith("_") for token in name.split(".")) - and name not in after_classes + if (not cfg.scan.public_only or _is_public(name)) and name not in after_classes } def _under_deleted_class(func_name: str) -> bool: @@ -93,50 +155,64 @@ def _under_deleted_class(func_name: str) -> bool: return False violations: list[api.violations.Violation] = [] - for name, before_def in before_funcs.items(): - if any(token.startswith("_") for token in name.split(".")): - continue - if _under_deleted_class(name): - # Will be reported as a class deletion instead - continue - - after_def = after_funcs.get(name) - if after_def is None: - violations.append(api.violations.FunctionDeleted(func=name, line=1)) - continue - - # Let's refine some terminology. Parameters come in three flavors: - # * positional only - # * keyword only - # * flexible: may be provided positionally or via keyword - # - # Required parameter: a parameter that must be provided as an - # argument by callers. In other words, the function does not - # have a default value. - # - # Variadic parameters: additional arguments that may only be - # provided positionally, traditionally specified as *args in a - # function definition. - # - # Variadic keywords: additional arguments that may only be - # provided by name, traditionally specified as **kwargs in a - # function definition. + if cfg.scan.functions: + for name, before_def in before_funcs.items(): + if not _symbol_included(name, "func"): + continue + if _under_deleted_class(name): + # Will be reported as a class deletion instead + continue + + after_def = after_funcs.get(name) + if after_def is None: + violations.append(api.violations.FunctionDeleted(func=name, line=1)) + continue + + # Let's refine some terminology. Parameters come in three flavors: + # * positional only + # * keyword only + # * flexible: may be provided positionally or via keyword + # + # Required parameter: a parameter that must be provided as an + # argument by callers. In other words, the function does not + # have a default value. + # + # Variadic parameters: additional arguments that may only be + # provided positionally, traditionally specified as *args in a + # function definition. + # + # Variadic keywords: additional arguments that may only be + # provided by name, traditionally specified as **kwargs in a + # function definition. + + violations += _check_by_name(name, before_def, after_def) + violations += _check_by_position(name, before_def, after_def) + violations += _check_by_requiredness(name, before_def, after_def) + violations += _check_variadic_parameters(name, before_def, after_def) + + if cfg.scan.classes: + for name, before_class in before_classes.items(): + if not _symbol_included(name, "class"): + continue + after_class = after_classes.get(name) + if after_class is None: + continue + violations += list(_check_class_fields(name, before_class, after_class)) - violations += _check_by_name(name, before_def, after_def) - violations += _check_by_position(name, before_def, after_def) - violations += _check_by_requiredness(name, before_def, after_def) - violations += _check_variadic_parameters(name, before_def, after_def) + # Classes deleted between before and after + def _class_included(name: str) -> bool: + return _symbol_included(name, "class") - for name, before_class in before_classes.items(): - if any(token.startswith("_") for token in name.split(".")): - continue - after_class = after_classes.get(name) - if after_class is None: - continue - violations += list(_check_class_fields(name, before_class, after_class)) + violations += list( + _check_deleted_classes( + before_classes, after_classes, include_pred=_class_included + ) + ) - # Classes deleted between before and after - violations += list(_check_deleted_classes(before_classes, after_classes)) + # Apply violation suppression based on config + if cfg.excluded_violations: + vset = set(cfg.excluded_violations) + violations = [v for v in violations if v.__class__.__name__ not in vset] return violations @@ -321,15 +397,13 @@ def _check_class_fields( def _check_deleted_classes( - before_classes: Mapping[str, api.Class], after_classes: Mapping[str, api.Class] + before_classes: Mapping[str, api.Class], + after_classes: Mapping[str, api.Class], + *, + include_pred=None, ) -> Iterable[api.violations.Violation]: """Emits violations for classes deleted between before and after.""" - deleted = [ - name - for name in before_classes - if not any(token.startswith("_") for token in name.split(".")) - and name not in after_classes - ] + deleted = [name for name in before_classes if name not in after_classes] deleted_set = set(deleted) @@ -342,6 +416,8 @@ def has_deleted_ancestor(class_name: str) -> bool: for name in deleted: if not has_deleted_ancestor(name): + if include_pred is not None and not include_pred(name): + continue # Align with FunctionDeleted's use of line=1 yield api.violations.ClassDeleted(func=name, line=1) diff --git a/tools/stronghold/src/api/config.py b/tools/stronghold/src/api/config.py new file mode 100644 index 0000000000..a98ccf907c --- /dev/null +++ b/tools/stronghold/src/api/config.py @@ -0,0 +1,169 @@ +"""Configuration loader for the BC linter.""" + +from __future__ import annotations + +import dataclasses +import pathlib +from typing import Any, Iterable, Sequence + + +@dataclasses.dataclass +class AnnotationSpec: + name: str + propagate_to_members: bool = False + + +@dataclasses.dataclass +class ScanSpec: + functions: bool = True + classes: bool = True + public_only: bool = True + + +@dataclasses.dataclass +class Config: + version: int = 1 + include: Sequence[str] = dataclasses.field(default_factory=lambda: ["**/*.py"]) + exclude: Sequence[str] = dataclasses.field( + # Exclude hidden paths both at top-level and nested directories + # Path matching uses pathspec (gitwildmatch), so these patterns will + # catch top-level and nested dot directories/files. + default_factory=lambda: [".*", ".*/**", "**/.*/**", "**/.*"] + ) + scan: ScanSpec = dataclasses.field(default_factory=ScanSpec) + annotations_include: Sequence[AnnotationSpec] = dataclasses.field( + default_factory=list + ) + annotations_exclude: Sequence[AnnotationSpec] = dataclasses.field( + default_factory=list + ) + excluded_violations: Sequence[str] = dataclasses.field(default_factory=list) + + +def _as_list_str(v: Any) -> list[str]: + if v is None: + return [] + if isinstance(v, str): + return [v] + if isinstance(v, Iterable): + return [str(x) for x in v] + return [str(v)] + + +def default_config() -> Config: + return Config() + + +def load_config_with_status(repo_root: pathlib.Path) -> tuple[Config, str]: + """Loads configuration from `.bc-linter.yml` in the given repository root. + + Returns (config, status) where status is one of: + - 'parsed' -> config file existed and parsed successfully + - 'default_missing' -> no config file found + - 'default_error' -> file existed but YAML missing/invalid or parser unavailable + """ + cfg_path = repo_root / ".bc-linter.yml" + if not cfg_path.exists(): + return (default_config(), "default_missing") + + data: dict[str, Any] | None = None + try: + import yaml # type: ignore[import-not-found] + + with cfg_path.open("r", encoding="utf-8") as fh: + loaded = yaml.safe_load(fh) # may be None for empty file + if loaded is None: + return (default_config(), "default_error") + assert isinstance(loaded, dict) + data = loaded # type: ignore[assignment] + except Exception: + # If PyYAML is not available or parsing fails, fall back to defaults. + return (default_config(), "default_error") + + version = int(data.get("version", 1)) + + # Accept both nested `paths: {include, exclude}` and top-level + # `include`/`exclude` keys for convenience. + raw_paths = data.get("paths") + paths: dict[str, Any] = {} + if isinstance(raw_paths, dict): + paths.update(raw_paths) + # Fallback: if top-level include/exclude are present, use them unless + # already provided under `paths`. + if "include" in data and "include" not in paths: + paths["include"] = data["include"] + if "exclude" in data and "exclude" not in paths: + paths["exclude"] = data["exclude"] + include = _as_list_str(paths.get("include", ["**/*.py"])) + exclude = _as_list_str(paths.get("exclude", [".*", ".*/**", "**/.*/**", "**/.*"])) + + scan = data.get("scan", {}) or {} + scan_spec = ScanSpec( + functions=bool(scan.get("functions", True)), + classes=bool(scan.get("classes", True)), + public_only=bool(scan.get("public_only", True)), + ) + + annotations = data.get("annotations", {}) or {} + anns_inc_raw = annotations.get("include", []) or [] + anns_exc_raw = annotations.get("exclude", []) or [] + + def _ann_list(raw: Any) -> list[AnnotationSpec]: + out: list[AnnotationSpec] = [] + if isinstance(raw, dict): + raw = [raw] + if isinstance(raw, list): + for item in raw: + if isinstance(item, str): + out.append(AnnotationSpec(name=item, propagate_to_members=False)) + elif isinstance(item, dict): + out.append( + AnnotationSpec( + name=str(item.get("name", "")), + propagate_to_members=bool( + item.get("propagate_to_members", False) + ), + ) + ) + return out + + anns_inc = _ann_list(anns_inc_raw) + anns_exc = _ann_list(anns_exc_raw) + + excluded_violations = _as_list_str(data.get("excluded_violations", [])) + + cfg = Config( + version=version, + include=include, + exclude=exclude, + scan=scan_spec, + annotations_include=anns_inc, + annotations_exclude=anns_exc, + excluded_violations=excluded_violations, + ) + return (cfg, "parsed") + + +def load_config(repo_root: pathlib.Path) -> Config: + """Loads configuration from `.bc-linter.yml` in the given repository root. + + If the file does not exist or cannot be parsed, returns defaults. + """ + cfg, _ = load_config_with_status(repo_root) + return cfg + + +def match_any(path: pathlib.Path, patterns: Sequence[str]) -> bool: + """Returns True if the path matches any of the glob patterns. + + Patterns are matched against POSIX-style paths. + """ + # An empty pattern list should be a no-op (match everything) + # to allow "include: []" semantics -> do not restrict by include. + if not patterns: + return True + # Require pathspec; no fallback to avoid divergent semantics. + import pathspec + + spec = pathspec.PathSpec.from_lines("gitwildmatch", patterns) + return spec.match_file(path.as_posix()) diff --git a/tools/stronghold/tests/api/test_config.py b/tools/stronghold/tests/api/test_config.py new file mode 100644 index 0000000000..1ad04a4777 --- /dev/null +++ b/tools/stronghold/tests/api/test_config.py @@ -0,0 +1,239 @@ +import pathlib +import textwrap + +import api.compatibility +from api.config import AnnotationSpec, Config +from testing import git, source + + +def test_public_only_false_includes_private(tmp_path: pathlib.Path) -> None: + class _C: + def m(self): + pass + + before = source.make_file(tmp_path, _C) + + after_src = textwrap.dedent( + """ + class _C: + pass + """ + ) + after = tmp_path / "after.py" + after.write_text(after_src) + + # Default public_only=True should ignore private class + assert api.compatibility.check(before, after) == [] + + # Now configure public_only=False to include it + cfg = Config() + cfg.scan.public_only = False + out = api.compatibility.check(before, after, config=cfg) + assert [type(v).__name__ for v in out] == ["FunctionDeleted"] + + +def test_excluded_violations_suppresses(tmp_path: pathlib.Path) -> None: + def f(x: int): + pass + + before = source.make_file(tmp_path, f) + + def f(x: str): # type: ignore[no-redef] + pass + + after = source.make_file(tmp_path, f) + + # Without suppression, we should report ParameterTypeChanged + out = api.compatibility.check(before, after) + assert any(v.__class__.__name__ == "ParameterTypeChanged" for v in out) + + # Suppress via excluded_violations + cfg = Config() + cfg.excluded_violations = ["ParameterTypeChanged"] + out2 = api.compatibility.check(before, after, config=cfg) + assert out2 == [] + + +def test_annotations_exclude_suppresses_class(tmp_path: pathlib.Path) -> None: + before = tmp_path / "before.py" + before.write_text( + textwrap.dedent( + """ + import dataclasses + @dataclasses.dataclass + class C: + a: int + """ + ) + ) + + after = tmp_path / "after.py" + after.write_text( + textwrap.dedent( + """ + import dataclasses + @dataclasses.dataclass + @bc_linter_skip + class C: + a: int + b: int + """ + ) + ) + + # With exclude annotation configured, FieldAdded should be suppressed + cfg = Config() + cfg.annotations_exclude = [ + AnnotationSpec(name="bc_linter_skip", propagate_to_members=True) + ] + out = api.compatibility.check(before, after, file_path=after, config=cfg) + assert out == [] + + +def test_annotations_include_overrides_path(tmp_path: pathlib.Path) -> None: + before = tmp_path / "sub" / "before.py" + before.parent.mkdir(parents=True, exist_ok=True) + before.write_text( + textwrap.dedent( + """ + import dataclasses + @dataclasses.dataclass + class C: + a: int + """ + ) + ) + + after = tmp_path / "sub" / "after.py" + after.write_text( + textwrap.dedent( + """ + import dataclasses + @dataclasses.dataclass + @bc_linter_include + class C: + a: int + b: int + """ + ) + ) + + # Configure includes to a different tree so this file is not allowed by path, + # but the include annotation should still include the symbol. + cfg = Config() + cfg.include = ["elsewhere/**/*.py"] + cfg.exclude = ["**/.*/**", "**/.*"] + cfg.annotations_include = [ + AnnotationSpec(name="bc_linter_include", propagate_to_members=True) + ] + out = api.compatibility.check(before, after, file_path=after, config=cfg) + assert [type(v).__name__ for v in out] == ["FieldAdded"] + + +def test_check_range_respects_path_filters(git_repo) -> None: + # Create two files changed across a commit range: one in hidden dir, one normal. + before_hidden = textwrap.dedent( + """ + def f(x): + pass + """ + ) + after_hidden = textwrap.dedent( + """ + def f(): + pass + """ + ) + before_norm = textwrap.dedent( + """ + def g(): + pass + """ + ) + after_norm = textwrap.dedent( + """ + def g(y): + pass + """ + ) + + # Initial commit + git.commit_file(git_repo, pathlib.Path(".hidden/a.py"), before_hidden) + git.commit_file(git_repo, pathlib.Path("src/b.py"), before_norm) + base = "HEAD" + + # Change both + git.commit_file(git_repo, pathlib.Path(".hidden/a.py"), after_hidden) + git.commit_file(git_repo, pathlib.Path("src/b.py"), after_norm) + head = "HEAD" + + # Default config excludes hidden paths; only src/b.py should be reported + cfg = Config() # defaults include hidden excludes + out = api.compatibility.check_range( + git_repo, head=head, base=f"{base}~", config=cfg + ) + keys = {p.as_posix() for p in out.keys()} + assert keys == {"src/b.py"} + assert [type(v).__name__ for v in out[pathlib.Path("src/b.py")]] == [ + "ParameterNowRequired" + ] + + +def test_disable_class_deleted_reports_method_deletions(tmp_path: pathlib.Path) -> None: + before = tmp_path / "before_class_del.py" + before.write_text( + textwrap.dedent( + """ + class C: + def m(self): + pass + """ + ) + ) + + after = tmp_path / "after_class_del.py" + after.write_text("") + + # With default config, class deletion is reported and method is suppressed + out_default = api.compatibility.check(before, after) + assert [type(v).__name__ for v in out_default] == ["ClassDeleted"] + + # Disable ClassDeleted; nested violations should not be emitted either + cfg = Config() + cfg.excluded_violations = ["ClassDeleted"] + out = api.compatibility.check(before, after, config=cfg) + assert out == [] + + +def test_disable_class_deleted_reports_inner_methods(tmp_path: pathlib.Path) -> None: + before = tmp_path / "before_inner.py" + before.write_text( + textwrap.dedent( + """ + class Outer: + class Inner: + def m(self): + pass + """ + ) + ) + + after = tmp_path / "after_inner.py" + after.write_text( + textwrap.dedent( + """ + class Outer: + pass + """ + ) + ) + + # Default: emits ClassDeleted for Outer.Inner + out_default = api.compatibility.check(before, after) + assert [type(v).__name__ for v in out_default] == ["ClassDeleted"] + + # Disable ClassDeleted; do not emit nested violations + cfg = Config() + cfg.excluded_violations = ["ClassDeleted"] + out = api.compatibility.check(before, after, config=cfg) + assert out == [] diff --git a/tools/stronghold/tests/api/test_config_loader.py b/tools/stronghold/tests/api/test_config_loader.py new file mode 100644 index 0000000000..7a4eb0f42c --- /dev/null +++ b/tools/stronghold/tests/api/test_config_loader.py @@ -0,0 +1,136 @@ +import pathlib +import textwrap + +from api.config import load_config_with_status + + +def test_top_level_include_exclude_are_accepted(tmp_path: pathlib.Path) -> None: + # Write YAML with top-level include/exclude instead of nested paths + yml = textwrap.dedent( + """ + version: 1 + include: ["src/**/*.py"] + exclude: ["**/tests/**", ".*/**"] + """ + ) + (tmp_path / ".bc-linter.yml").write_text(yml) + + cfg, status = load_config_with_status(tmp_path) + assert status == "parsed" + assert cfg.include == ["src/**/*.py"] + assert cfg.exclude == ["**/tests/**", ".*/**"] + + +def test_nested_paths_prefer_paths_over_top_level(tmp_path: pathlib.Path) -> None: + yml = textwrap.dedent( + """ + version: 1 + include: ["top/**/*.py"] + exclude: ["**/ignore-me/**"] + paths: + include: ["src/**/*.py"] + exclude: ["**/tests/**"] + """ + ) + (tmp_path / ".bc-linter.yml").write_text(yml) + + cfg, status = load_config_with_status(tmp_path) + assert status == "parsed" + # Should prefer the nested `paths` values + assert cfg.include == ["src/**/*.py"] + assert cfg.exclude == ["**/tests/**"] + + +def test_missing_config_defaults(tmp_path: pathlib.Path) -> None: + cfg, status = load_config_with_status(tmp_path) + assert status == "default_missing" + # defaults + assert cfg.version == 1 + assert cfg.include == ["**/*.py"] + assert cfg.exclude == [".*", ".*/**", "**/.*/**", "**/.*"] + assert cfg.scan.functions is True + assert cfg.scan.classes is True + assert cfg.scan.public_only is True + assert cfg.annotations_include == [] + assert cfg.annotations_exclude == [] + assert cfg.excluded_violations == [] + + +def test_empty_config_defaults(tmp_path: pathlib.Path) -> None: + (tmp_path / ".bc-linter.yml").write_text("") + cfg, status = load_config_with_status(tmp_path) + assert status == "default_error" + assert cfg.include == ["**/*.py"] + assert cfg.exclude == [".*", ".*/**", "**/.*/**", "**/.*"] + + +def test_invalid_yaml_defaults(tmp_path: pathlib.Path) -> None: + (tmp_path / ".bc-linter.yml").write_text("version: [1, 2\n") # broken yaml + cfg, status = load_config_with_status(tmp_path) + assert status == "default_error" + assert cfg.include == ["**/*.py"] + assert cfg.exclude == [".*", ".*/**", "**/.*/**", "**/.*"] + + +def test_paths_include_exclude_string_and_list(tmp_path: pathlib.Path) -> None: + yml = """ + version: 1 + paths: + include: "src/**/*.py" + exclude: + - "**/gen/**" + - ".*/**" + """ + (tmp_path / ".bc-linter.yml").write_text(yml) + cfg, status = load_config_with_status(tmp_path) + assert status == "parsed" + assert cfg.include == ["src/**/*.py"] + assert cfg.exclude == ["**/gen/**", ".*/**"] + + +def test_scan_flags_parsed(tmp_path: pathlib.Path) -> None: + yml = """ + version: 1 + scan: + functions: false + classes: true + public_only: false + """ + (tmp_path / ".bc-linter.yml").write_text(yml) + cfg, status = load_config_with_status(tmp_path) + assert status == "parsed" + assert cfg.scan.functions is False + assert cfg.scan.classes is True + assert cfg.scan.public_only is False + + +def test_annotations_and_excluded_violations(tmp_path: pathlib.Path) -> None: + yml = """ + version: 1 + annotations: + include: + - "forced" + - name: "force_class" + propagate_to_members: true + exclude: + - name: "skip" + - "skip2" + excluded_violations: ["ClassDeleted", "ParameterRenamed"] + """ + (tmp_path / ".bc-linter.yml").write_text(yml) + cfg, status = load_config_with_status(tmp_path) + assert status == "parsed" + # include + assert len(cfg.annotations_include) == 2 + assert cfg.annotations_include[0].name == "forced" + assert cfg.annotations_include[0].propagate_to_members is False + assert cfg.annotations_include[1].name == "force_class" + assert cfg.annotations_include[1].propagate_to_members is True + # exclude + assert len(cfg.annotations_exclude) == 2 + assert cfg.annotations_exclude[0].name == "skip" + assert cfg.annotations_exclude[0].propagate_to_members is False + assert cfg.annotations_exclude[1].name == "skip2" + assert cfg.annotations_exclude[1].propagate_to_members is False + # excluded violations + assert cfg.excluded_violations == ["ClassDeleted", "ParameterRenamed"] diff --git a/tools/stronghold/tests/api/test_path_globs.py b/tools/stronghold/tests/api/test_path_globs.py new file mode 100644 index 0000000000..dff57640c4 --- /dev/null +++ b/tools/stronghold/tests/api/test_path_globs.py @@ -0,0 +1,77 @@ +import pathlib + +import api.config + + +def test_match_any_empty_patterns_matches_all() -> None: + p = pathlib.Path("src/module/file.py") + assert api.config.match_any(p, []) is True + + +def test_match_any_basic_globs() -> None: + assert api.config.match_any(pathlib.Path("a/b/c.py"), ["**/*.py"]) is True + assert api.config.match_any(pathlib.Path("a/b/c.txt"), ["**/*.py"]) is False + assert api.config.match_any(pathlib.Path("a/b/c.py"), ["**/*"]) is True + assert api.config.match_any(pathlib.Path("a/b/c.py"), ["**"]) is True + assert api.config.match_any(pathlib.Path(".a/b/c.py"), ["**"]) is True + # Hidden top-level dir and nested match with simplified defaults + assert api.config.match_any(pathlib.Path(".a/b/c.py"), [".*/**"]) is True + assert api.config.match_any(pathlib.Path(".a/b/b/c.py"), [".*/**"]) is True + assert api.config.match_any(pathlib.Path(".a/b/b/c.py"), ["**/.*/**"]) is True + assert api.config.match_any(pathlib.Path(".a.py"), [".*"]) is True + assert api.config.match_any(pathlib.Path("b/.a.py"), [".*"]) is True + assert api.config.match_any(pathlib.Path("b/.a.py"), ["**/.*"]) is True + + +def test_hidden_path_exclude_glob() -> None: + # Exclude patterns are intended to filter hidden segments inside the path. + # Note: with fnmatch semantics, '**/.*/**' does not match a leading dot + # segment at the very start of the path (no preceding '/'). + hidden_file = pathlib.Path("pkg/.hidden/dir/file.py") + normal_file = pathlib.Path("pkg/sub/file.py") + exclude = [".*", ".*/**", "**/.*/**", "**/.*"] + # Hidden path excluded + assert api.config.match_any(hidden_file, exclude) is True + # Normal path not excluded by hidden patterns + assert api.config.match_any(normal_file, exclude) is False + + +def test_include_exclude_interaction() -> None: + # Emulate file-level allow logic: include and not exclude + f1 = pathlib.Path("pkg/file.py") + f2 = pathlib.Path(".git/file.py") + include = ["**/*.py"] + exclude = [".*", ".*/**", "**/.*/**", "**/.*"] + assert api.config.match_any(f1, include) and not api.config.match_any(f1, exclude) + # With pathspec (gitwildmatch), '.*/**' excludes files under top-level hidden dirs. + assert not ( + api.config.match_any(f2, include) and not api.config.match_any(f2, exclude) + ) + + +def test_top_level_benchmarks_and_tests_excluded_by_double_star_dir() -> None: + # Verify '**/benchmarks/**' and '**/tests/**' match top-level too under pathspec + f_bench = pathlib.Path("benchmarks/foo.py") + f_tests = pathlib.Path("tests/foo.py") + exclude = ["**/benchmarks/**", "**/tests/**"] + assert api.config.match_any(f_bench, exclude) is True + assert api.config.match_any(f_tests, exclude) is True + + +def test_default_exclude_minimal_sample_set() -> None: + # Verify minimal sample set using default excludes + cfg = api.config.default_config() + ex = cfg.exclude + assert ( + api.config.match_any(pathlib.Path(".a/b/b/c.py"), ex) is True + ) # hidden dir nested + assert ( + api.config.match_any(pathlib.Path(".a.py"), ex) is True + ) # hidden file top-level + assert ( + api.config.match_any(pathlib.Path("b/.a.py"), ex) is True + ) # hidden file nested + assert ( + api.config.match_any(pathlib.Path("b/.a.py"), ex) is True + ) # also matched by **/.* + assert api.config.match_any(pathlib.Path("pkg/sub/file.py"), ex) is False