diff --git a/.flake8 b/.flake8 index a3f342d..920589e 100644 --- a/.flake8 +++ b/.flake8 @@ -10,6 +10,7 @@ extend-exclude = .venv,build,dist [flake8:local-plugins] extension = - RMP = flake8_rampart:RampartChecker + RMP001 = flake8_rampart:AsyncSuffixChecker + RMP002 = flake8_rampart:LazyExportChecker paths = ./tools diff --git a/.github/instructions/coding-standards.instructions.md b/.github/instructions/coding-standards.instructions.md index 989d456..fbbe41b 100644 --- a/.github/instructions/coding-standards.instructions.md +++ b/.github/instructions/coding-standards.instructions.md @@ -609,6 +609,7 @@ Before committing code, ensure: | Convention | Code | Enforced by | |---|---|---| | Async `_async` suffix | `RMP001` | `tools/flake8_rampart.py`, a flake8 local plugin | +| Lazy exports listed in `__all__` | `RMP002` | `tools/flake8_rampart.py`, a flake8 local plugin | ### Async naming (`RMP001`) @@ -625,6 +626,16 @@ that ruff's `RUF102` accepts it instead of rejecting it as an unknown code. `RMP001` applies repo-wide, including to tests: the test standards require the `_async` suffix on async test names too. + +### Lazy public exports (`RMP002`) + +Modules that declare a literal `__lazy_imports__` dictionary MUST include every +literal key in their literal `__all__` collection. Eager public exports may also +appear in `__all__`; the lazy names are a subset, not an exhaustive public API. +Both declarations MUST use literal forms: `__lazy_imports__` requires a dictionary +with string keys, and `__all__` requires a list, tuple, or set containing only +strings. Each name MUST have exactly one direct module-level assignment. Additional +writes and direct method calls are rejected; reads remain allowed. [flake8-local]: https://flake8.pycqa.org/en/latest/user/configuration.html#using-local-plugins --- diff --git a/pyproject.toml b/pyproject.toml index 6e6d58e..50e1110 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -119,7 +119,7 @@ ignore = [ # RMP comes from the flake8 local plugin in tools/. Declaring it here stops # RUF102 (invalid-rule-code) from rejecting `# noqa: RMPXXX` as unknown. -external = ["RMP001"] +external = ["RMP001", "RMP002"] [tool.ruff.lint.per-file-ignores] "scripts/hatch_build.py" = [ diff --git a/tests/unit/tools/test_flake8_rampart.py b/tests/unit/tools/test_flake8_rampart.py index 3ca8cc7..eae4b71 100644 --- a/tests/unit/tools/test_flake8_rampart.py +++ b/tests/unit/tools/test_flake8_rampart.py @@ -1,7 +1,7 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT license. -"""Tests for the flake8-rampart local plugin (RMP001).""" +"""Tests for the flake8-rampart local plugin.""" from __future__ import annotations @@ -11,45 +11,201 @@ from pathlib import Path import pytest -from flake8_rampart import RampartChecker +from flake8_rampart import AsyncSuffixChecker, LazyExportChecker _REPO_ROOT = Path(__file__).resolve().parents[3] -def _messages(source: str) -> list[str]: - """Return the messages the checker reports for a source snippet.""" - return [msg for _, _, msg, _ in RampartChecker(ast.parse(source)).run()] +def _async_suffix_messages(source: str) -> list[str]: + """Return async-suffix messages for a source snippet.""" + return [msg for _, _, msg, _ in AsyncSuffixChecker(ast.parse(source)).run()] + + +def _lazy_export_messages(source: str) -> list[str]: + """Return lazy-export messages for a source snippet.""" + return [msg for _, _, msg, _ in LazyExportChecker(ast.parse(source)).run()] class TestAsyncSuffixRule: def test_flags_async_function_without_suffix(self) -> None: - (message,) = _messages("async def fetch(): ...") + (message,) = _async_suffix_messages("async def fetch(): ...") assert message.startswith("RMP001") assert "`fetch`" in message def test_accepts_async_function_with_suffix(self) -> None: - assert _messages("async def fetch_async(): ...") == [] + assert _async_suffix_messages("async def fetch_async(): ...") == [] def test_ignores_sync_function(self) -> None: - assert _messages("def fetch(): ...") == [] + assert _async_suffix_messages("def fetch(): ...") == [] def test_exempts_dunder(self) -> None: - assert _messages("async def __aenter__(self): ...") == [] + assert _async_suffix_messages("async def __aenter__(self): ...") == [] def test_flags_method_inside_class(self) -> None: source = "class A:\n async def fetch(self): ...\n" - assert len(_messages(source)) == 1 + assert len(_async_suffix_messages(source)) == 1 def test_flags_nested_function(self) -> None: source = "def outer():\n async def inner(): ...\n" - assert len(_messages(source)) == 1 + assert len(_async_suffix_messages(source)) == 1 def test_reports_position_of_definition(self) -> None: - checker = RampartChecker(ast.parse("\n\nasync def fetch(): ...")) + checker = AsyncSuffixChecker(ast.parse("\n\nasync def fetch(): ...")) ((line, col, _, _),) = checker.run() assert (line, col) == (3, 0) +class TestLazyExportRule: + def test_flags_lazy_export_missing_from_all(self) -> None: + source = """ +__lazy_imports__: dict[str, tuple[str, str]] = { + "Heavy": ("package.heavy", "Heavy"), +} +__all__ = [] +""" + + (message,) = _lazy_export_messages(source) + + assert message.startswith("RMP002") + assert "`Heavy`" in message + + def test_accepts_lazy_export_in_all(self) -> None: + source = """ +__lazy_imports__ = {"Heavy": ("package.heavy", "Heavy")} +__all__ = ["Heavy"] +""" + + assert _lazy_export_messages(source) == [] + + def test_allows_eager_exports_in_all(self) -> None: + source = """ +__lazy_imports__ = {"Heavy": ("package.heavy", "Heavy")} +__all__ = ["Eager", "Heavy"] +""" + + assert _lazy_export_messages(source) == [] + + def test_reports_only_lazy_exports_missing_from_all(self) -> None: + source = """ +__lazy_imports__ = { + "Included": ("package.included", "Included"), + "MissingOne": ("package.missing", "MissingOne"), + "MissingTwo": ("package.missing", "MissingTwo"), +} +__all__ = ["Included"] +""" + + messages = _lazy_export_messages(source) + + assert len(messages) == 2 + assert "`MissingOne`" in messages[0] + assert "`MissingTwo`" in messages[1] + + def test_flags_lazy_export_when_all_is_absent(self) -> None: + source = '__lazy_imports__ = {"Heavy": ("package.heavy", "Heavy")}' + + (message,) = _lazy_export_messages(source) + + assert message.startswith("RMP002") + + def test_rejects_dynamic_lazy_registry(self) -> None: + source = """ +__lazy_imports__ = build_lazy_imports() +__all__ = [] +""" + + (message,) = _lazy_export_messages(source) + + assert message.startswith("RMP002") + assert "`__lazy_imports__`" in message + + def test_rejects_dynamic_all(self) -> None: + source = """ +__lazy_imports__ = {"Heavy": ("package.heavy", "Heavy")} +__all__ = build_public_names() +""" + + (message,) = _lazy_export_messages(source) + + assert message.startswith("RMP002") + assert "`__all__`" in message + + @pytest.mark.parametrize( + ("mutation", "expected_name"), + [ + ('__all__ += ["Heavy"]', "__all__"), + ('__all__.extend(["Heavy"])', "__all__"), + ( + '__lazy_imports__["Other"] = ("package.other", "Other")', + "__lazy_imports__", + ), + ], + ) + def test_rejects_incremental_construction( + self, + *, + mutation: str, + expected_name: str, + ) -> None: + source = f""" +__lazy_imports__ = {{"Heavy": ("package.heavy", "Heavy")}} +__all__ = ["Heavy"] +{mutation} +""" + + (message,) = _lazy_export_messages(source) + + assert message.startswith("RMP002") + assert f"`{expected_name}`" in message + + @pytest.mark.parametrize("name", ["__all__", "__lazy_imports__"]) + def test_rejects_multiple_assignments(self, *, name: str) -> None: + replacement = "{}" if name == "__lazy_imports__" else "[]" + source = f""" +__lazy_imports__ = {{"Heavy": ("package.heavy", "Heavy")}} +__all__ = ["Heavy"] +{name} = {replacement} +""" + + (message,) = _lazy_export_messages(source) + + assert message.startswith("RMP002") + assert f"`{name}`" in message + + def test_allows_reads(self) -> None: + source = """ +__lazy_imports__ = {"Heavy": ("package.heavy", "Heavy")} +__all__ = ["Heavy"] + +observed_exports = __all__ +observed_registry = __lazy_imports__ + +def resolve(name): + return __lazy_imports__[name] + +def public_names(): + return __all__ + +class Namespace: + public = __all__ +""" + + assert _lazy_export_messages(source) == [] + + def test_reports_position_of_missing_key(self) -> None: + source = """ +__lazy_imports__ = { + "Heavy": ("package.heavy", "Heavy"), +} +__all__ = [] +""" + checker = LazyExportChecker(ast.parse(source)) + + ((line, col, _, _),) = checker.run() + + assert (line, col) == (3, 4) + + @pytest.mark.slow class TestPluginWiring: """Guard against the plugin silently failing to load. @@ -60,9 +216,18 @@ class TestPluginWiring: otherwise. """ - def _run_flake8(self, target: Path) -> subprocess.CompletedProcess[str]: + def _run_flake8( + self, + *, + target: Path, + select: str | None = None, + ) -> subprocess.CompletedProcess[str]: + args = [sys.executable, "-m", "flake8"] + if select is not None: + args.extend(["--select", select]) + args.append(str(target)) return subprocess.run( # ruff: ignore[subprocess-without-shell-equals-true] - [sys.executable, "-m", "flake8", str(target)], + args, cwd=_REPO_ROOT, capture_output=True, text=True, @@ -73,11 +238,56 @@ def test_reports_violation_through_flake8(self, tmp_path: Path) -> None: target = tmp_path / "sample.py" target.write_text("async def fetch():\n pass\n", encoding="utf-8") - result = self._run_flake8(target) + result = self._run_flake8(target=target) assert result.returncode == 1 assert "RMP001" in result.stdout + def test_reports_lazy_export_violation_through_flake8( + self, + *, + tmp_path: Path, + ) -> None: + target = tmp_path / "sample.py" + target.write_text( + '__lazy_imports__ = {"Heavy": ("package.heavy", "Heavy")}\n__all__ = []\n', + encoding="utf-8", + ) + + result = self._run_flake8(target=target) + + assert result.returncode == 1 + assert "RMP002" in result.stdout + + @pytest.mark.parametrize( + ("select", "expected", "unexpected"), + [ + ("RMP001", "RMP001", "RMP002"), + ("RMP002", "RMP002", "RMP001"), + ], + ) + def test_selects_registered_checker_independently( + self, + *, + tmp_path: Path, + select: str, + expected: str, + unexpected: str, + ) -> None: + target = tmp_path / "sample.py" + target.write_text( + "async def fetch(): ...\n" + '__lazy_imports__ = {"Heavy": ("package.heavy", "Heavy")}\n' + "__all__ = []\n", + encoding="utf-8", + ) + + result = self._run_flake8(target=target, select=select) + + assert result.returncode == 1 + assert expected in result.stdout + assert unexpected not in result.stdout + def test_honors_noqa_through_flake8(self, tmp_path: Path) -> None: target = tmp_path / "sample.py" target.write_text( @@ -85,7 +295,7 @@ def test_honors_noqa_through_flake8(self, tmp_path: Path) -> None: encoding="utf-8", ) - result = self._run_flake8(target) + result = self._run_flake8(target=target) assert result.returncode == 0, result.stdout @@ -94,6 +304,6 @@ def test_runs_no_rules_other_than_rmp(self, tmp_path: Path) -> None: target = tmp_path / "sample.py" target.write_text("import os\nx=1\n", encoding="utf-8") - result = self._run_flake8(target) + result = self._run_flake8(target=target) assert result.returncode == 0, result.stdout diff --git a/tools/flake8_rampart.py b/tools/flake8_rampart.py index 5e2ade4..cf703d5 100644 --- a/tools/flake8_rampart.py +++ b/tools/flake8_rampart.py @@ -9,6 +9,7 @@ Rules: RMP001: Async functions must be named with an ``_async`` suffix. + RMP002: Lazy exports must be included in ``__all__``. """ from __future__ import annotations @@ -20,6 +21,15 @@ from collections.abc import Iterator RMP001 = "RMP001 async function `{name}` must be named with an `_async` suffix" +RMP002 = "RMP002 lazy export `{name}` must be included in `__all__`" +RMP002_DYNAMIC_ALL = ( + "RMP002 `__all__` must be declared as one list, tuple, or set literal " + "containing only strings" +) +RMP002_DYNAMIC_REGISTRY = ( + "RMP002 `__lazy_imports__` must be declared as one dictionary literal " + "with string keys" +) def _is_dunder(name: str) -> bool: @@ -34,11 +44,191 @@ def _is_dunder(name: str) -> bool: return name.startswith("__") and name.endswith("__") -class RampartChecker: +def _direct_assignment( + *, + statement: ast.stmt, + name: str, +) -> tuple[ast.expr, ast.expr] | None: + """Return a direct assignment target and value for a name. + + Args: + statement (ast.stmt): Module-level statement to inspect. + name (str): Assignment target to find. + + Returns: + tuple[ast.expr, ast.expr] | None: Assignment target and value, or + ``None`` when the statement is not a direct assignment. + """ + if isinstance(statement, ast.Assign) and len(statement.targets) == 1: + target = statement.targets[0] + if isinstance(target, ast.Name) and target.id == name: + return target, statement.value + elif ( + isinstance(statement, ast.AnnAssign) + and isinstance(statement.target, ast.Name) + and statement.target.id == name + and statement.value is not None + ): + return statement.target, statement.value + return None + + +def _unsupported_reference(*, statement: ast.stmt, name: str) -> ast.expr | None: + """Return a module-level write or direct call involving a name. + + Args: + statement (ast.stmt): Module-level statement to inspect. + name (str): Governed declaration name. + + Returns: + ast.expr | None: Unsupported expression, or ``None`` when absent. + """ + for node in ast.walk(statement): + if isinstance(node, ast.Call): + root = node.func + while isinstance(root, (ast.Attribute, ast.Subscript)): + root = root.value + if isinstance(root, ast.Name) and root.id == name: + return node + if not isinstance(node, (ast.Attribute, ast.Name, ast.Subscript)): + continue + if not isinstance(node.ctx, (ast.Del, ast.Store)): + continue + if any( + isinstance(child, ast.Name) and child.id == name for child in ast.walk(node) + ): + return node + return None + + +def _module_declaration( + *, + tree: ast.AST, + name: str, +) -> tuple[ast.expr | None, ast.expr | None]: + """Return one direct declaration and any unsupported reference. + + Args: + tree (ast.AST): Parsed module syntax tree. + name (str): Declaration name to inspect. + + Returns: + tuple[ast.expr | None, ast.expr | None]: The sole assigned value and + an unsupported reference. A second assignment is unsupported and + returned as the reference. + """ + if not isinstance(tree, ast.Module): + return None, None + + value: ast.expr | None = None + for statement in tree.body: + assignment = _direct_assignment(statement=statement, name=name) + if assignment is not None: + target, assigned_value = assignment + if value is not None: + return None, target + value = assigned_value + continue + + reference = _unsupported_reference(statement=statement, name=name) + if reference is not None: + return None, reference + + return value, None + + +def _literal_string_dict_keys( + value: ast.expr, +) -> list[tuple[str, ast.Constant]] | None: + """Return string keys and nodes from a literal dictionary. + + Args: + value (ast.expr): Expression expected to contain a dictionary. + + Returns: + list[tuple[str, ast.Constant]] | None: Literal string keys and their + nodes, or ``None`` when the expression is dynamic. + """ + if not isinstance(value, ast.Dict): + return None + + keys: list[tuple[str, ast.Constant]] = [] + for key in value.keys: + if not isinstance(key, ast.Constant) or not isinstance(key.value, str): + return None + keys.append((key.value, key)) + return keys + + +def _literal_string_collection(value: ast.expr) -> set[str] | None: + """Return strings from a literal list, tuple, or set. + + Args: + value (ast.expr): Expression expected to contain string values. + + Returns: + set[str] | None: Literal strings, or ``None`` when the expression is + dynamic. + """ + if not isinstance(value, (ast.List, ast.Set, ast.Tuple)): + return None + + names: set[str] = set() + for element in value.elts: + if not isinstance(element, ast.Constant) or not isinstance( + element.value, + str, + ): + return None + names.add(element.value) + return names + + +def _lazy_export_violations(tree: ast.AST) -> Iterator[tuple[ast.expr, str]]: + """Yield invalid declarations and lazy exports missing from ``__all__``. + + Args: + tree (ast.AST): Parsed module syntax tree. + + Yields: + tuple[ast.expr, str]: Invalid expression and formatted violation. + """ + lazy_value, lazy_reference = _module_declaration( + tree=tree, + name="__lazy_imports__", + ) + if lazy_reference is not None: + yield lazy_reference, RMP002_DYNAMIC_REGISTRY + return + if lazy_value is None: + return + lazy_exports = _literal_string_dict_keys(lazy_value) + if lazy_exports is None: + yield lazy_value, RMP002_DYNAMIC_REGISTRY + return + + all_value, all_reference = _module_declaration(tree=tree, name="__all__") + if all_reference is not None: + yield all_reference, RMP002_DYNAMIC_ALL + return + if all_value is None: + yield lazy_value, RMP002_DYNAMIC_ALL + return + public_names = _literal_string_collection(all_value) + if public_names is None: + yield all_value, RMP002_DYNAMIC_ALL + return + + for name, key in lazy_exports: + if name not in public_names: + yield key, RMP002.format(name=name) + + +class AsyncSuffixChecker: """Flake8 checker enforcing RAMPART's async naming convention.""" - name: ClassVar[str] = "flake8-rampart" - version: ClassVar[str] = "1.0.0" + name: ClassVar[str] = "flake8-rampart-async-suffix" + version: ClassVar[str] = "1.1.0" def __init__(self, tree: ast.AST) -> None: """Store the module AST supplied by flake8. @@ -67,3 +257,33 @@ def run(self) -> Iterator[tuple[int, int, str, type]]: RMP001.format(name=node.name), type(self), ) + + +class LazyExportChecker: + """Flake8 checker ensuring lazy exports remain public.""" + + name: ClassVar[str] = "flake8-rampart-lazy-export" + version: ClassVar[str] = "1.1.0" + + def __init__(self, tree: ast.AST) -> None: + """Store the module AST supplied by flake8. + + Args: + tree (ast.AST): Parsed syntax tree for the file under check. + """ + self._tree = tree + + def run(self) -> Iterator[tuple[int, int, str, type]]: + """Yield a violation for every lazy export missing from ``__all__``. + + Yields: + tuple[int, int, str, type]: Line, column, message, and checker + type, in the 4-tuple shape flake8 expects. + """ + for expression, message in _lazy_export_violations(self._tree): + yield ( + expression.lineno, + expression.col_offset, + message, + type(self), + )