From f6002968864225c3e46a24d229dc934cf0f20854 Mon Sep 17 00:00:00 2001 From: Matthew Ballance Date: Sun, 5 Apr 2026 00:39:21 +0000 Subject: [PATCH 1/2] Checkpoint: initial checker architecture in place Signed-off-by: Matthew Ballance --- .gitignore | 8 +- README.md | 29 +- docs/checker_plugin_guide.rst | 233 ++++++++++++++++ docs/cli.rst | 71 +++++ docs/index.rst | 1 + docs/reference_api_docs.rst | 23 ++ python/pssparser/checkers/__init__.py | 7 + python/pssparser/checkers/base.py | 44 +++ python/pssparser/checkers/context.py | 93 +++++++ python/pssparser/checkers/core_checker.py | 87 ++++++ python/pssparser/checkers/manager.py | 224 +++++++++++++++ python/pssparser/checkers/markerdef.py | 30 ++ python/pssparser/cli/app.py | 92 ++++++- python/pssparser/cli/checker_cmds.py | 74 +++++ python/pssparser/cli/commands.py | 80 ++++++ python/pssparser/cli/diagnostics.py | 1 + tests/python/checkers/__init__.py | 0 tests/python/checkers/conftest.py | 112 ++++++++ .../checkers/test_checker_integration.py | 191 +++++++++++++ tests/python/checkers/test_checkerbase.py | 55 ++++ .../python/checkers/test_cli_checker_flags.py | 150 ++++++++++ tests/python/checkers/test_context.py | 61 +++++ tests/python/checkers/test_manager.py | 257 ++++++++++++++++++ tests/python/checkers/test_markerdef.py | 38 +++ 24 files changed, 1955 insertions(+), 6 deletions(-) create mode 100644 docs/checker_plugin_guide.rst create mode 100644 python/pssparser/checkers/__init__.py create mode 100644 python/pssparser/checkers/base.py create mode 100644 python/pssparser/checkers/context.py create mode 100644 python/pssparser/checkers/core_checker.py create mode 100644 python/pssparser/checkers/manager.py create mode 100644 python/pssparser/checkers/markerdef.py create mode 100644 python/pssparser/cli/checker_cmds.py create mode 100644 tests/python/checkers/__init__.py create mode 100644 tests/python/checkers/conftest.py create mode 100644 tests/python/checkers/test_checker_integration.py create mode 100644 tests/python/checkers/test_checkerbase.py create mode 100644 tests/python/checkers/test_cli_checker_flags.py create mode 100644 tests/python/checkers/test_context.py create mode 100644 tests/python/checkers/test_manager.py create mode 100644 tests/python/checkers/test_markerdef.py diff --git a/.gitignore b/.gitignore index 42894f4..85aae05 100644 --- a/.gitignore +++ b/.gitignore @@ -140,10 +140,10 @@ ext/pssparser.h ext/pssparser_api.h python/core.cpp -python/pssast.cpp -python/pssast.pyx -python/pssast.h -python/pssast_api.h +python/ast.cpp +python/ast.pyx +python/ast.h +python/ast_api.h python/pssparser/pssast.pxd python/pssparser/pssast_decl.pxd python/PyBaseVisitor.* diff --git a/README.md b/README.md index fc76830..367beb4 100644 --- a/README.md +++ b/README.md @@ -3,4 +3,31 @@ This project provides a ANTLR4-based parser for the Accellera PSS language. It also provides an AST (data model) for processing the result of the parser. -[![Build Status](https://dev.azure.com/mballance/psstools/_apis/build/status/PSSTools.pssparser?branchName=master)](https://dev.azure.com/mballance/psstools/_build/latest?definitionId=15&branchName=master) \ No newline at end of file +[![Build Status](https://dev.azure.com/mballance/psstools/_apis/build/status/PSSTools.pssparser?branchName=master)](https://dev.azure.com/mballance/psstools/_build/latest?definitionId=15&branchName=master) + +## Checker Plug-ins + +`pssparser` supports a plug-in system for custom Python-based checkers that +run after a successful parse and link. Plug-ins can be contributed by any +installed Python package via the `pssparser.checkers` `entry_points` group, +or loaded on the fly with `--load-checker MODULE:CLASS`. + +```bash +# List all registered checkers +pssparser --list-checkers + +# List all marker IDs (built-in + plug-in) +pssparser --list-markers + +# Describe a specific marker +pssparser --describe PSS001 + +# Run only a specific checker +pssparser --checker naming-convention model.pss + +# Load and run a local checker (no install needed) +pssparser --load-checker myproject.rules:StyleChecker model.pss +``` + +See [docs/checker_plugin_guide.rst](docs/checker_plugin_guide.rst) for a +full guide to writing and registering checker plug-ins. diff --git a/docs/checker_plugin_guide.rst b/docs/checker_plugin_guide.rst new file mode 100644 index 0000000..3c51dc1 --- /dev/null +++ b/docs/checker_plugin_guide.rst @@ -0,0 +1,233 @@ +#################### +Checker Plug-in Guide +#################### + +.. contents:: + :local: + :depth: 2 + +Concept +======= + +``pssparser`` ships with built-in syntax and semantic checks implemented in +C++. The **checker plug-in system** lets you — or any third-party package — +add custom Python checks that run as a third phase, after a successful parse +and link. + +Each checker is a Python class that inherits from +:class:`pssparser.checkers.CheckerBase`. Checkers receive a +:class:`pssparser.checkers.CheckContext` object that provides read access to +the linked AST and an API to emit structured diagnostics. + +The built-in ``CoreChecker`` (name ``"core"``) is always registered and +documents every marker the C++ parser and linker can produce. It cannot be +disabled. + +Writing a Checker +================= + +Minimal example — a naming-convention checker that warns when a component +name does not start with an uppercase letter: + +.. code-block:: python + + # mypkg/pss_rules.py + from pssparser.checkers import CheckerBase, MarkerDef + + class NamingConventionChecker(CheckerBase): + name = "naming-convention" + description = "Enforce PSS naming conventions" + + marker_defs = [ + MarkerDef( + id="PSC001", + severity="warning", + summary="Component name does not start with an uppercase letter", + detail=( + "PSS convention requires component type names to begin with " + "an uppercase letter. Rename the component to fix this." + ), + ), + ] + + runs_without_link = False + + def check(self, context): + # Walk top-level global scopes looking for component declarations + for scope in context.global_scopes: + for child in (scope.children() if hasattr(scope, "children") else []): + if hasattr(child, "getName"): + name_node = child.getName() + name = ( + name_node.getId() + if hasattr(name_node, "getId") + else str(name_node) + ) + if name and not name[0].isupper(): + context.add_marker( + code="PSC001", + file=context.files[0], + line=1, + col=1, + message=f"Component {name!r} should start with uppercase", + ) + +Step-by-step walkthrough: + +1. **Subclass** :class:`~pssparser.checkers.CheckerBase`. +2. **Set** ``name`` — a short unique slug used on the command line. +3. **Set** ``description`` — shown by ``--list-checkers``. +4. **Declare** ``marker_defs`` — one :class:`~pssparser.checkers.MarkerDef` + per diagnostic code your checker can emit. +5. **Implement** ``check(context)`` — walk the AST and call + :meth:`~pssparser.checkers.CheckContext.add_marker` to emit diagnostics. + +Declaring Markers +================= + +Every diagnostic your checker may emit must be declared as a +:class:`~pssparser.checkers.MarkerDef` in the class-level ``marker_defs`` +list: + +.. code-block:: python + + from pssparser.checkers import MarkerDef + + MarkerDef( + id="PSC001", # globally unique ID + severity="warning", # "error", "warning", "info", or "hint" + summary="Short description for --list-markers", + detail="Longer explanation shown by --describe PSC001", + ) + +**ID naming convention**: use a three-letter prefix (e.g. ``PSC`` for your +checker package) followed by a zero-padded three-digit number (``PSC001``). +The ``PSS`` prefix is reserved for the built-in ``CoreChecker``. Choose a +unique prefix and register it in your project's documentation to avoid future +collisions. + +The :class:`~pssparser.checkers.CheckerManager` enforces globally unique IDs +across all registered checkers at discovery time. + +Registering via entry_points +============================= + +The standard way to make your checker available to all ``pssparser`` +invocations is to declare it as an ``entry_points`` contribution in your +package's ``setup.cfg`` or ``pyproject.toml``: + +``setup.cfg``: + +.. code-block:: ini + + [options.entry_points] + pssparser.checkers = + naming-convention = mypkg.pss_rules:NamingConventionChecker + unused-imports = mypkg.pss_rules:UnusedImportChecker + +``pyproject.toml``: + +.. code-block:: toml + + [project.entry-points."pssparser.checkers"] + naming-convention = "mypkg.pss_rules:NamingConventionChecker" + unused-imports = "mypkg.pss_rules:UnusedImportChecker" + +The left-hand side (e.g. ``naming-convention``) becomes the registered +*name* of the checker and is used on the command line with ``--checker`` and +``--no-checker``. + +After installation (``pip install .``), your checker is auto-discovered every +time ``pssparser`` runs. + +Ad-hoc Loading +============== + +For development or one-off use you can load a checker without installing it: + +.. code-block:: bash + + pssparser --load-checker mypkg.pss_rules:NamingConventionChecker model.pss + +The ``--load-checker`` flag may be repeated to load multiple checkers. The +loaded checker participates in all ``--checker`` / ``--no-checker`` filtering +using its ``name`` attribute. + +Combine with ``--list-checkers`` to inspect what will run before doing an +actual parse: + +.. code-block:: bash + + pssparser --load-checker mypkg.pss_rules:NamingConventionChecker \ + --list-checkers + +Checker Selection +================= + +By default, all registered (non-builtin) checkers run. Use these flags to +change that: + +``--checker NAME`` + Run **only** the named checker(s). May be repeated. ``NAME`` must match + a registered checker name (from ``entry_points``) or a checker previously + loaded with ``--load-checker``. Specifying an unknown name is an error. + + .. code-block:: bash + + pssparser --checker naming-convention model.pss + +``--no-checker NAME`` + **Exclude** the named checker. May be repeated. Ignored when + ``--checker`` is also specified (explicit selection takes precedence). + Unknown names are silently ignored. + + .. code-block:: bash + + pssparser --no-checker deprecated-syntax model.pss + +Precedence rules: + +1. Start with all registered + ``--load-checker`` checkers. +2. If ``--checker`` is present, keep *only* those names. +3. Otherwise, remove any names listed in ``--no-checker``. + +Querying the Registry +===================== + +``--list-checkers`` + Print a table of all registered checkers and their declared marker IDs, + then exit with code 0. No source files are required. + + .. code-block:: bash + + pssparser --list-checkers + +``--list-markers`` + Print a table of every declared :class:`~pssparser.checkers.MarkerDef` + across all checkers (including the built-in core), then exit with code 0. + + .. code-block:: bash + + pssparser --list-markers + +``--describe ID`` + Print the full :class:`~pssparser.checkers.MarkerDef` record (summary, + severity, detail text, and owning checker) for a single marker ID, then + exit with code 0. Exits with code 2 if the ID is not found. + + .. code-block:: bash + + pssparser --describe PSS002 + +Accessing the AST +================= + +Inside ``check(context)``, the linked AST is available as +``context.root`` (a ``RootSymbolScope``) and the per-file ``GlobalScope`` +nodes are in ``context.global_scopes``. See :doc:`ast_usage_guide` for a +full guide to navigating the AST. + +If your checker only needs the *parse tree* (not the linked AST), set +``runs_without_link = True`` on the class. The checker will then run even +when ``--syntax-only`` is passed. When ``runs_without_link = False`` +(the default), the checker is skipped in ``--syntax-only`` mode. diff --git a/docs/cli.rst b/docs/cli.rst index 5d4efec..cfe77e4 100644 --- a/docs/cli.rst +++ b/docs/cli.rst @@ -41,3 +41,74 @@ Dump the linked AST: .. code-block:: bash pssparser --dump-ast ast.json model.pss + +Checker flags +============= + +The following flags control the checker plug-in system. See +:doc:`checker_plugin_guide` for a full explanation of the plug-in +architecture. + +Query-and-exit flags +--------------------- + +These flags do **not** require source files. + +``--list-checkers`` + Print a table of all registered checkers — name, description, and + declared marker IDs — then exit with code 0. + + .. code-block:: bash + + pssparser --list-checkers + +``--list-markers`` + Print a table of every declared marker ID across all registered checkers + (including the built-in ``core``), then exit with code 0. Columns are + ``ID``, ``SEV``, ``CHECKER``, and ``SUMMARY``. + + .. code-block:: bash + + pssparser --list-markers + +``--describe ID`` + Print the full definition (summary, severity, detail, owning checker) for + the marker with the given ID, then exit with code 0. Exits with code 2 + and an error message if the ID is not found. + + .. code-block:: bash + + pssparser --describe PSS001 + +Checker selection flags +------------------------ + +``--checker NAME`` + Run *only* the named checker. May be repeated to select multiple + checkers. ``NAME`` must match a registered entry-point name or a checker + previously loaded with ``--load-checker``. Specifying an unknown name + produces an error and exits with code 2. + + .. code-block:: bash + + pssparser --checker naming-convention model.pss + +``--no-checker NAME`` + Exclude the named checker from the active set. May be repeated. + Silently ignored when the name is not in the registry, or when + ``--checker`` is also specified (explicit selection takes precedence). + + .. code-block:: bash + + pssparser --no-checker deprecated-syntax model.pss + +``--load-checker MODULE:CLASS`` + Dynamically import ``CLASS`` from ``MODULE`` and add it to the active + checker set. No package installation required. May be repeated. The + loaded checker participates in ``--checker`` / ``--no-checker`` filtering + using its ``name`` attribute. + + .. code-block:: bash + + pssparser --load-checker myproject.rules:StyleChecker model.pss + diff --git a/docs/index.rst b/docs/index.rst index 9497461..88001ba 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -31,6 +31,7 @@ Key Features quickstart cli + checker_plugin_guide pss30_migration .. toctree:: diff --git a/docs/reference_api_docs.rst b/docs/reference_api_docs.rst index fca1216..9e7cc24 100644 --- a/docs/reference_api_docs.rst +++ b/docs/reference_api_docs.rst @@ -9,6 +9,29 @@ Classes are organized by category for easy navigation. :local: :depth: 2 +********************** +Checker Plug-in API +********************** + +These classes form the public API for writing and registering pssparser +checker plug-ins. See :doc:`checker_plugin_guide` for a usage guide. + +.. autoclass:: pssparser.checkers.MarkerDef + :members: + :undoc-members: + +.. autoclass:: pssparser.checkers.CheckerBase + :members: + :undoc-members: + +.. autoclass:: pssparser.checkers.CheckContext + :members: + :undoc-members: + +.. autoclass:: pssparser.checkers.CheckerManager + :members: + :undoc-members: + ****************** Core AST Structure ****************** diff --git a/python/pssparser/checkers/__init__.py b/python/pssparser/checkers/__init__.py new file mode 100644 index 0000000..cc45af6 --- /dev/null +++ b/python/pssparser/checkers/__init__.py @@ -0,0 +1,7 @@ +"""Public API surface for pssparser.checkers.""" +from .markerdef import MarkerDef +from .base import CheckerBase +from .context import CheckContext +from .manager import CheckerManager + +__all__ = ["MarkerDef", "CheckerBase", "CheckContext", "CheckerManager"] diff --git a/python/pssparser/checkers/base.py b/python/pssparser/checkers/base.py new file mode 100644 index 0000000..d6c4b69 --- /dev/null +++ b/python/pssparser/checkers/base.py @@ -0,0 +1,44 @@ +"""Abstract base class for all pssparser plug-in checkers.""" +from __future__ import annotations + +from typing import TYPE_CHECKING, List + +from .markerdef import MarkerDef + +if TYPE_CHECKING: + from .context import CheckContext + + +class CheckerBase: + """Abstract base for all pssparser plug-in checkers. + + Subclasses must override :attr:`name`, :attr:`description`, + :attr:`marker_defs`, and :meth:`check`. + """ + + #: Short unique identifier, e.g. ``"naming-convention"``. + name: str = "" + + #: One-line description shown in ``--list-checkers``. + description: str = "" + + #: Structured definitions of every marker this checker may emit. + #: Each subclass must declare its own list — do *not* mutate the + #: inherited one. + marker_defs: List[MarkerDef] = [] + + #: If ``True`` the checker runs even when ``--syntax-only`` was + #: requested (i.e. the linked AST is not available). + runs_without_link: bool = False + + def check(self, context: "CheckContext") -> None: + """Perform checks and emit diagnostics via *context*. + + Parameters + ---------- + context: + Provides access to the AST and the marker-emission API. + """ + raise NotImplementedError( + f"{type(self).__name__} must implement check()" + ) diff --git a/python/pssparser/checkers/context.py b/python/pssparser/checkers/context.py new file mode 100644 index 0000000..894c92e --- /dev/null +++ b/python/pssparser/checkers/context.py @@ -0,0 +1,93 @@ +"""Context object passed to each checker's check() method.""" +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any, Dict, List, Optional + + +@dataclass +class CheckContext: + """Provides AST access and a diagnostic-emission API to checkers. + + Attributes + ---------- + root: + Fully-linked root symbol scope, or ``None`` when ``--syntax-only``. + files: + Ordered list of source file paths that were parsed. + global_scopes: + Raw per-file ``GlobalScope`` AST nodes (same order as *files*). + """ + + #: Fully-linked root symbol scope, or None when --syntax-only. + root: Optional[Any] + + #: Ordered list of source file paths that were parsed. + files: List[str] + + #: Raw per-file GlobalScope AST nodes (same order as *files*). + global_scopes: List[Any] + + #: Internal – collected markers; use add_marker() to append. + _markers: list = field(default_factory=list, repr=False) + + #: Internal – index of MarkerDef objects keyed by marker ID. + #: Populated by CheckerManager before invoking any checker. + _marker_index: Dict[str, Any] = field(default_factory=dict, repr=False) + + def add_marker( + self, + *, + code: str, + file: str, + line: int, + col: int, + message: str, + severity: Optional[str] = None, + ) -> None: + """Emit one diagnostic from within a checker. + + Parameters + ---------- + code: + The ``MarkerDef.id`` for this diagnostic. Must be declared in + the checker's ``marker_defs`` list (enforced via + ``_marker_index``). + file: + Source file path. + line: + 1-based line number. + col: + 1-based column number. + message: + Human-readable diagnostic text. + severity: + Override the default severity declared in the ``MarkerDef``. + If omitted, the ``MarkerDef.severity`` is used. + + Raises + ------ + ValueError + If *code* is not present in ``_marker_index``. + """ + if code not in self._marker_index: + raise ValueError( + f"Unknown marker code {code!r}; it must be declared in a " + "checker's marker_defs before it can be emitted." + ) + entry = { + "severity": severity if severity is not None else self._lookup_severity(code), + "message": f"[{code}] {message}", + "file": file, + "line": line, + "col": col, + "code": code, + } + self._markers.append(entry) + + def _lookup_severity(self, code: str) -> str: + """Return the declared default severity for *code*.""" + md = self._marker_index.get(code) + if md is not None: + return md.severity + return "error" # safe fallback diff --git a/python/pssparser/checkers/core_checker.py b/python/pssparser/checkers/core_checker.py new file mode 100644 index 0000000..b140003 --- /dev/null +++ b/python/pssparser/checkers/core_checker.py @@ -0,0 +1,87 @@ +"""Built-in CoreChecker: metadata for parser and linker diagnostics.""" +from __future__ import annotations + +from .base import CheckerBase +from .markerdef import MarkerDef + + +class CoreChecker(CheckerBase): + """Metadata-only checker that documents built-in parse/link markers. + + The actual checking is performed by C++ code; this class exists purely + to make the built-in marker catalogue discoverable via ``--list-markers`` + and ``--describe``. + """ + + name = "core" + description = "Built-in parser and linker diagnostics" + + #: Marks this as a built-in checker that cannot be disabled via + #: ``--no-checker`` and is never passed to the checker invocation loop. + is_builtin = True + + marker_defs = [ + MarkerDef( + id="PSS001", + severity="error", + summary="Syntax error: unexpected token", + detail=( + "The parser encountered a token it did not expect at this " + "position. Check that the surrounding PSS syntax is " + "well-formed." + ), + ), + MarkerDef( + id="PSS002", + severity="error", + summary="Undefined symbol reference", + detail=( + "The linker could not resolve a reference to a named type " + "or identifier. Ensure the symbol is declared in one of " + "the source files passed to pssparser, or that the correct " + "package is imported." + ), + ), + MarkerDef( + id="PSS003", + severity="error", + summary="Duplicate symbol declaration", + detail=( + "A symbol with this name is already declared in the same " + "scope. Rename one of the declarations to resolve the " + "conflict." + ), + ), + MarkerDef( + id="PSS004", + severity="error", + summary="Type resolution error", + detail=( + "The type referenced in this expression or declaration could " + "not be resolved. Ensure the type is declared or imported " + "before use." + ), + ), + MarkerDef( + id="PSS005", + severity="error", + summary="Import resolution error", + detail=( + "An imported package or symbol could not be found. Check " + "that the package name is correct and that the providing " + "file is included in the pssparser invocation." + ), + ), + MarkerDef( + id="PSS006", + severity="warning", + summary="Ambiguous symbol reference", + detail=( + "More than one symbol matches this reference. Use a " + "fully-qualified name to disambiguate." + ), + ), + ] + + def check(self, context) -> None: # noqa: D102 + pass # core checking is performed in C++ diff --git a/python/pssparser/checkers/manager.py b/python/pssparser/checkers/manager.py new file mode 100644 index 0000000..324a8a2 --- /dev/null +++ b/python/pssparser/checkers/manager.py @@ -0,0 +1,224 @@ +"""CheckerManager: discovery, filtering, loading, and invocation of checkers.""" +from __future__ import annotations + +import importlib +from typing import Dict, List, Optional, Type + +from .base import CheckerBase + + +class CheckerManager: + """Manages discovery, filtering, and invocation of checker plug-ins. + + Typical usage:: + + manager = CheckerManager() + manager.discover() # load entry_points + manager.load("mypkg.rules:MyChecker") # optional: ad-hoc load + checkers = manager.active(select=None, exclude=["some-checker"]) + """ + + def __init__(self) -> None: + self._registered: Dict[str, Type[CheckerBase]] = {} + + # ------------------------------------------------------------------ + # Discovery + # ------------------------------------------------------------------ + + def discover(self) -> None: + """Register CoreChecker then all ``pssparser.checkers`` entry-points. + + Idempotent: safe to call more than once, though once per process is + the normal pattern (second call will raise on duplicate IDs if any + entry-points were already registered). + """ + from .core_checker import CoreChecker + + if "core" not in self._registered: + self._registered["core"] = CoreChecker + + try: + from importlib.metadata import entry_points + eps = entry_points(group="pssparser.checkers") + except Exception: + eps = [] + + for ep in eps: + if ep.name not in self._registered: + try: + cls = ep.load() + self._registered[ep.name] = cls + except Exception as exc: + import warnings + warnings.warn( + f"Failed to load checker entry-point {ep.name!r}: {exc}", + stacklevel=2, + ) + + self._check_unique_ids() + + # ------------------------------------------------------------------ + # Validation + # ------------------------------------------------------------------ + + def _check_unique_ids(self) -> None: + """Raise ``ValueError`` if any ``MarkerDef.id`` is declared by two checkers.""" + seen: Dict[str, str] = {} # marker_id -> checker_name + for name, cls in self._registered.items(): + for md in cls().marker_defs: + if md.id in seen: + raise ValueError( + f"Duplicate MarkerDef ID {md.id!r}: " + f"declared by both {seen[md.id]!r} and {name!r}" + ) + seen[md.id] = name + + # ------------------------------------------------------------------ + # Ad-hoc loading + # ------------------------------------------------------------------ + + def load(self, spec: str) -> None: + """Load a checker from a ``'module.path:ClassName'`` spec string. + + Parameters + ---------- + spec: + A string of the form ``"module.path:ClassName"``. + + Raises + ------ + ValueError + On malformed spec, missing module/class, empty checker name, or + duplicate marker IDs. + """ + module_path, sep, class_name = spec.rpartition(":") + if not sep or not module_path or not class_name: + raise ValueError( + f"Invalid checker spec {spec!r}; expected 'module.path:ClassName'" + ) + try: + mod = importlib.import_module(module_path) + except ModuleNotFoundError as exc: + raise ValueError( + f"Cannot load checker spec {spec!r}: module not found — {exc}" + ) from exc + try: + cls = getattr(mod, class_name) + except AttributeError as exc: + raise ValueError( + f"Cannot load checker spec {spec!r}: attribute not found — {exc}" + ) from exc + + instance = cls() + if not instance.name: + raise ValueError( + f"Checker {cls.__name__!r} loaded from {spec!r} has an empty 'name' attribute" + ) + self._registered[instance.name] = cls + self._check_unique_ids() + + # ------------------------------------------------------------------ + # Active-checker selection + # ------------------------------------------------------------------ + + def active( + self, + select: Optional[List[str]], + exclude: Optional[List[str]], + ) -> List[CheckerBase]: + """Return instantiated active checkers after applying filters. + + ``is_builtin`` checkers are never included (they are metadata-only). + + Parameters + ---------- + select: + If non-empty, keep *only* these checker names. Unknown names + raise ``ValueError`` (fail-fast). + exclude: + If ``select`` is ``None``/empty and this is non-empty, remove + these checker names from the active set. Unknown names are + silently ignored. + + Raises + ------ + ValueError + When *select* contains names not in the registry. + """ + non_builtin = [ + n for n, cls in self._registered.items() + if not getattr(cls, "is_builtin", False) + ] + + if select: + unknown = [n for n in select if n not in self._registered] + if unknown: + raise ValueError(f"Unknown checker(s): {', '.join(unknown)}") + names = [n for n in select if n in self._registered] + elif exclude: + names = [n for n in non_builtin if n not in exclude] + else: + names = non_builtin + + return [self._registered[n]() for n in names] + + # ------------------------------------------------------------------ + # Query helpers + # ------------------------------------------------------------------ + + def list_checkers(self) -> List[dict]: + """Return info dicts for ``--list-checkers`` display.""" + result = [] + for name, cls in self._registered.items(): + inst = cls() + result.append({ + "name": name, + "description": inst.description or "(no description)", + "marker_ids": [md.id for md in inst.marker_defs], + "is_builtin": getattr(inst, "is_builtin", False), + }) + return result + + def list_all_markers(self) -> List[dict]: + """Return every ``MarkerDef`` across all registered checkers.""" + result = [] + for name, cls in self._registered.items(): + for md in cls().marker_defs: + result.append({ + "id": md.id, + "severity": md.severity, + "checker": name, + "summary": md.summary, + }) + return result + + def describe_marker(self, marker_id: str) -> Optional[dict]: + """Return a full description dict for *marker_id*, or ``None``.""" + for name, cls in self._registered.items(): + for md in cls().marker_defs: + if md.id == marker_id: + return { + "id": md.id, + "severity": md.severity, + "checker": name, + "summary": md.summary, + "detail": md.detail, + } + return None + + def build_marker_index(self, active_checkers: List[CheckerBase]) -> dict: + """Build a ``{marker_id: MarkerDef}`` index for the given checkers. + + Always includes ``CoreChecker`` marker defs so that built-in codes + can be looked up even though ``CoreChecker`` is not in + *active_checkers*. + """ + from .core_checker import CoreChecker + + index: dict = {} + for md in CoreChecker().marker_defs: + index[md.id] = md + for checker in active_checkers: + for md in checker.marker_defs: + index[md.id] = md + return index diff --git a/python/pssparser/checkers/markerdef.py b/python/pssparser/checkers/markerdef.py new file mode 100644 index 0000000..33fd277 --- /dev/null +++ b/python/pssparser/checkers/markerdef.py @@ -0,0 +1,30 @@ +"""Structured metadata for one diagnostic code.""" +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass(frozen=True) +class MarkerDef: + """Describes one diagnostic that a checker (or the core) may emit. + + Attributes + ---------- + id: + Globally unique identifier string, e.g. ``"PSS001"`` or ``"PSC042"``. + IDs must be unique across *all* registered checkers and the core; the + ``CheckerManager`` enforces this at discovery time. + severity: + Default severity: ``"error"``, ``"warning"``, ``"info"``, or + ``"hint"``. + summary: + One-line description displayed in ``--list-markers`` output. + detail: + Multi-line explanation shown by ``--describe ID``. May include + reStructuredText markup. + """ + + id: str + severity: str + summary: str + detail: str = "" diff --git a/python/pssparser/cli/app.py b/python/pssparser/cli/app.py index 7cdd904..e342c11 100644 --- a/python/pssparser/cli/app.py +++ b/python/pssparser/cli/app.py @@ -12,7 +12,60 @@ def _build_parser() -> argparse.ArgumentParser: description="PSS compiler frontend -- syntax and semantic checker", ) - top.add_argument("files", nargs="+", metavar="FILE", help=".pss source files") + top.add_argument( + "files", + nargs="*", + metavar="FILE", + help=".pss source files", + ) + + # -- checker query-and-exit flags ------------------------------------ + query_grp = top.add_argument_group("checker query flags (no source files required)") + query_grp.add_argument( + "--list-checkers", + action="store_true", + default=False, + help="List all registered checkers and exit", + ) + query_grp.add_argument( + "--list-markers", + action="store_true", + default=False, + help="List all declared marker IDs across all checkers and exit", + ) + query_grp.add_argument( + "--describe", + metavar="ID", + default=None, + help="Print the full definition for marker ID and exit", + ) + + # -- checker selection flags ---------------------------------------- + sel_grp = top.add_argument_group("checker selection flags") + sel_grp.add_argument( + "--checker", + action="append", + metavar="NAME", + dest="checkers", + default=None, + help="Run only this checker (may be repeated)", + ) + sel_grp.add_argument( + "--no-checker", + action="append", + metavar="NAME", + dest="no_checkers", + default=None, + help="Exclude this checker (may be repeated)", + ) + sel_grp.add_argument( + "--load-checker", + action="append", + metavar="MODULE:CLASS", + dest="load_checkers", + default=None, + help="Dynamically load a checker from MODULE:CLASS (may be repeated)", + ) top.add_argument( "--syntax-only", action="store_true", @@ -78,6 +131,40 @@ def main(argv: list[str] | None = None) -> int: except SystemExit as e: return e.code if isinstance(e.code, int) else 2 + # -- Build and populate CheckerManager -------------------------------- + from pssparser.checkers import CheckerManager + from .checker_cmds import cmd_list_checkers, cmd_list_markers, cmd_describe + + manager = CheckerManager() + manager.discover() + + # Apply any --load-checker specs before handling query flags + if args.load_checkers: + for spec in args.load_checkers: + try: + manager.load(spec) + except ValueError as exc: + sys.stderr.write(f"error: {exc}\n") + return 2 + + # -- Query-and-exit flags (no source files needed) -------------------- + if args.list_checkers: + return cmd_list_checkers(manager) + + if args.list_markers: + return cmd_list_markers(manager) + + if args.describe is not None: + return cmd_describe(manager, args.describe) + + # -- Require source files when no query flag is active ---------------- + if not args.files: + sys.stderr.write( + "error: at least one source FILE is required " + "(or use --list-checkers / --list-markers / --describe)\n" + ) + return 2 + # Validate files exist missing = [f for f in args.files if not os.path.isfile(f)] if missing: @@ -96,6 +183,9 @@ def main(argv: list[str] | None = None) -> int: quiet=args.quiet, color=args.color, max_errors=args.max_errors, + manager=manager, + checkers=args.checkers, + no_checkers=args.no_checkers, ) except KeyboardInterrupt: sys.stderr.write("\nInterrupted\n") diff --git a/python/pssparser/cli/checker_cmds.py b/python/pssparser/cli/checker_cmds.py new file mode 100644 index 0000000..4b771f1 --- /dev/null +++ b/python/pssparser/cli/checker_cmds.py @@ -0,0 +1,74 @@ +"""Output helpers for checker query commands.""" +from __future__ import annotations + +import sys +from typing import TextIO + +from pssparser.checkers import CheckerManager + + +def cmd_list_checkers( + manager: CheckerManager, + stdout: TextIO | None = None, +) -> int: + """Print all registered checkers and return exit code 0.""" + out = stdout or sys.stdout + checkers = manager.list_checkers() + out.write(f"Registered checkers ({len(checkers)}):\n") + for info in checkers: + builtin_tag = " [built-in]" if info["is_builtin"] else "" + ids_str = " ".join(info["marker_ids"]) if info["marker_ids"] else "(none)" + out.write(f" {info['name']:<20} {info['description']}{builtin_tag}\n") + out.write(f" markers: {ids_str}\n") + return 0 + + +def cmd_list_markers( + manager: CheckerManager, + stdout: TextIO | None = None, +) -> int: + """Print all declared marker IDs and return exit code 0.""" + out = stdout or sys.stdout + markers = manager.list_all_markers() + if not markers: + out.write("(no markers registered)\n") + return 0 + # Column widths + id_w = max(len(m["id"]) for m in markers) + sev_w = max(len(m["severity"]) for m in markers) + checker_w = max(len(m["checker"]) for m in markers) + id_w = max(id_w, 2) + sev_w = max(sev_w, 3) + checker_w = max(checker_w, 7) + header = f"{'ID':<{id_w}} {'SEV':<{sev_w}} {'CHECKER':<{checker_w}} SUMMARY" + out.write(header + "\n") + out.write("-" * len(header) + "\n") + for m in markers: + out.write( + f"{m['id']:<{id_w}} {m['severity']:<{sev_w}} " + f"{m['checker']:<{checker_w}} {m['summary']}\n" + ) + return 0 + + +def cmd_describe( + manager: CheckerManager, + marker_id: str, + stdout: TextIO | None = None, + stderr: TextIO | None = None, +) -> int: + """Print the full definition for *marker_id* and return exit code.""" + out = stdout or sys.stdout + err = stderr or sys.stderr + info = manager.describe_marker(marker_id) + if info is None: + err.write( + f"error: unknown marker ID {marker_id!r}; " + "use --list-markers to see available IDs\n" + ) + return 2 + out.write(f"{info['id']} [{info['severity']}] checker: {info['checker']}\n") + out.write(f"{info['summary']}\n") + if info.get("detail"): + out.write(f"\n{info['detail']}\n") + return 0 diff --git a/python/pssparser/cli/commands.py b/python/pssparser/cli/commands.py index 35f2b7b..0959ed3 100644 --- a/python/pssparser/cli/commands.py +++ b/python/pssparser/cli/commands.py @@ -21,10 +21,23 @@ def cmd_parse( max_errors: int = 20, stderr: TextIO | None = None, stdout: TextIO | None = None, + manager=None, + checkers: Optional[List[str]] = None, + no_checkers: Optional[List[str]] = None, ) -> int: """Run the parse (and optionally link) pipeline, report diagnostics. Returns an exit code: 0 success, 1 errors found, 2 usage problem. + + Parameters + ---------- + manager: + A pre-configured ``CheckerManager``. When ``None``, a fresh manager + is created and ``discover()`` is called automatically. + checkers: + Names of checkers to run (``--checker``). ``None`` means run all. + no_checkers: + Names of checkers to exclude (``--no-checker``). """ from pssparser.parser import Parser, ParseException @@ -66,6 +79,18 @@ def cmd_parse( # Collect any non-fatal markers from successful phases _collect(coll, [], parser) + # -- checker phase ------------------------------------------------------ + _run_checkers( + coll=coll, + files=files, + parser=parser, + linked_root=linked_root, + syntax_only=syntax_only, + manager=manager, + checkers=checkers, + no_checkers=no_checkers, + ) + # -- dump-ast ----------------------------------------------------------- if dump_ast and linked_root is not None: try: @@ -84,6 +109,61 @@ def cmd_parse( # -- helpers ---------------------------------------------------------------- +def _run_checkers( + coll: DiagnosticCollection, + files: List[str], + parser, + linked_root, + syntax_only: bool, + manager, + checkers, + no_checkers, +) -> None: + """Run the checker phase and merge results into *coll*.""" + from pssparser.checkers import CheckContext, CheckerManager + + if manager is None: + manager = CheckerManager() + manager.discover() + + try: + active = manager.active(select=checkers, exclude=no_checkers) + except ValueError as exc: + import sys + sys.stderr.write(f"error: {exc}\n") + return + + if not active: + return + + # Build per-file global scopes list (best-effort) + global_scopes: list = [] + if hasattr(parser, "global_scopes"): + global_scopes = list(parser.global_scopes) + + marker_index = manager.build_marker_index(active) + + context = CheckContext( + root=linked_root, + files=list(files), + global_scopes=global_scopes, + _marker_index=marker_index, + ) + + for checker in active: + if not syntax_only or checker.runs_without_link: + try: + checker.check(context) + except Exception as exc: + import sys + sys.stderr.write( + f"warning: checker {checker.name!r} raised an exception: {exc}\n" + ) + + for m in context._markers: + coll.add(Diagnostic.from_marker(m)) + + def _collect( coll: DiagnosticCollection, exc_markers: list, diff --git a/python/pssparser/cli/diagnostics.py b/python/pssparser/cli/diagnostics.py index 3ee4505..bf05aae 100644 --- a/python/pssparser/cli/diagnostics.py +++ b/python/pssparser/cli/diagnostics.py @@ -53,6 +53,7 @@ def from_marker(cls, marker: dict) -> "Diagnostic": severity=marker.get("severity", "error"), message=msg, suggestion=suggestion, + code=marker.get("code"), end_col=end_col, ) diff --git a/tests/python/checkers/__init__.py b/tests/python/checkers/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/python/checkers/conftest.py b/tests/python/checkers/conftest.py new file mode 100644 index 0000000..7239d3e --- /dev/null +++ b/tests/python/checkers/conftest.py @@ -0,0 +1,112 @@ +"""Shared fixtures for the checkers test suite.""" +from __future__ import annotations + +import pytest + +from pssparser.checkers import CheckerBase, MarkerDef + + +class DummyChecker(CheckerBase): + """Minimal checker that always emits one TST001 warning.""" + + name = "dummy" + description = "Test checker that emits one warning unconditionally" + marker_defs = [ + MarkerDef(id="TST001", severity="warning", summary="Dummy test warning"), + ] + runs_without_link = False + + def check(self, context) -> None: + if context.files: + context.add_marker( + code="TST001", + file=context.files[0], + line=1, + col=1, + message="dummy warning from DummyChecker", + ) + + +class AnotherDummyChecker(CheckerBase): + """Second checker with a non-conflicting ID.""" + + name = "another-dummy" + description = "Second test checker" + marker_defs = [ + MarkerDef(id="TST002", severity="info", summary="Another dummy test marker"), + ] + runs_without_link = True + + def check(self, context) -> None: + if context.files: + context.add_marker( + code="TST002", + file=context.files[0], + line=1, + col=1, + message="another dummy marker", + ) + + +class ConflictingChecker(CheckerBase): + """Checker with TST001 — conflicts with DummyChecker.""" + + name = "conflicting" + description = "Checker that re-declares TST001 for conflict testing" + marker_defs = [ + MarkerDef(id="TST001", severity="error", summary="Conflicting marker"), + ] + + def check(self, context) -> None: + pass + + +class ErrorSeverityChecker(CheckerBase): + """Checker that emits an error-severity marker.""" + + name = "error-checker" + description = "Emits an error-severity marker" + marker_defs = [ + MarkerDef(id="TST003", severity="error", summary="Dummy error marker"), + ] + runs_without_link = False + + def check(self, context) -> None: + if context.files: + context.add_marker( + code="TST003", + file=context.files[0], + line=1, + col=1, + message="dummy error", + ) + + +@pytest.fixture +def dummy_checker(): + return DummyChecker() + + +@pytest.fixture +def another_dummy_checker(): + return AnotherDummyChecker() + + +@pytest.fixture +def tmp_pss_file(tmp_path): + """Write a minimal valid PSS snippet to a temporary file.""" + f = tmp_path / "test.pss" + f.write_text( + "component pss_top {\n" + " action A {}\n" + "}\n" + ) + return str(f) + + +@pytest.fixture +def tmp_invalid_pss_file(tmp_path): + """Write intentionally broken PSS to a temporary file.""" + f = tmp_path / "broken.pss" + f.write_text("this is not valid PSS syntax @@@@\n") + return str(f) diff --git a/tests/python/checkers/test_checker_integration.py b/tests/python/checkers/test_checker_integration.py new file mode 100644 index 0000000..2733058 --- /dev/null +++ b/tests/python/checkers/test_checker_integration.py @@ -0,0 +1,191 @@ +"""End-to-end integration tests for the checker pipeline.""" +from __future__ import annotations + +import pytest + +from pssparser.checkers import CheckerBase, CheckerManager, MarkerDef +from pssparser.cli.commands import cmd_parse +from tests.python.checkers.conftest import ( + DummyChecker, + AnotherDummyChecker, + ErrorSeverityChecker, +) + + +def _make_manager(*checker_classes): + m = CheckerManager() + from pssparser.checkers.core_checker import CoreChecker + m._registered["core"] = CoreChecker + for cls in checker_classes: + m._registered[cls().name] = cls + return m + + +@pytest.fixture +def valid_pss(tmp_path): + f = tmp_path / "valid.pss" + f.write_text("component pss_top {\n action A {}\n}\n") + return str(f) + + +# --------------------------------------------------------------------------- +# DummyChecker emits marker visible in DiagnosticCollection +# --------------------------------------------------------------------------- + +def test_dummy_checker_marker_in_collection(valid_pss, capsys): + import io + manager = _make_manager(DummyChecker) + code = cmd_parse( + files=[valid_pss], + manager=manager, + checkers=None, + no_checkers=None, + quiet=True, + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + # DummyChecker emits a warning, so exit code should be 0 + assert code == 0 + + +# --------------------------------------------------------------------------- +# runs_without_link=False: checker skipped when syntax_only=True +# --------------------------------------------------------------------------- + +def test_checker_skipped_when_syntax_only(valid_pss): + import io + + emitted = [] + + class TrackingChecker(CheckerBase): + name = "tracking" + marker_defs = [MarkerDef(id="TRK001", severity="warning", summary="tracking")] + runs_without_link = False + + def check(self, context): + emitted.append("called") + context.add_marker( + code="TRK001", file=context.files[0], line=1, col=1, message="tracked" + ) + + manager = _make_manager(TrackingChecker) + cmd_parse( + files=[valid_pss], + syntax_only=True, + manager=manager, + checkers=None, + no_checkers=None, + quiet=True, + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + assert emitted == [], "checker with runs_without_link=False should be skipped in syntax_only mode" + + +# --------------------------------------------------------------------------- +# runs_without_link=True: checker runs even when syntax_only=True +# --------------------------------------------------------------------------- + +def test_checker_runs_when_syntax_only_and_runs_without_link(valid_pss): + import io + + emitted = [] + + class EagerChecker(CheckerBase): + name = "eager" + marker_defs = [MarkerDef(id="EGR001", severity="info", summary="eager")] + runs_without_link = True + + def check(self, context): + emitted.append("called") + + manager = _make_manager(EagerChecker) + cmd_parse( + files=[valid_pss], + syntax_only=True, + manager=manager, + checkers=None, + no_checkers=None, + quiet=True, + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + assert emitted == ["called"] + + +# --------------------------------------------------------------------------- +# Error-severity marker causes exit code 1 +# --------------------------------------------------------------------------- + +def test_error_severity_checker_causes_exit_1(valid_pss): + import io + manager = _make_manager(ErrorSeverityChecker) + code = cmd_parse( + files=[valid_pss], + manager=manager, + checkers=None, + no_checkers=None, + quiet=True, + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + assert code == 1 + + +# --------------------------------------------------------------------------- +# Warning-only checker does not change exit code (still 0 for valid file) +# --------------------------------------------------------------------------- + +def test_warning_only_checker_exit_0(valid_pss): + import io + manager = _make_manager(DummyChecker) + code = cmd_parse( + files=[valid_pss], + manager=manager, + checkers=None, + no_checkers=None, + quiet=True, + stdout=io.StringIO(), + stderr=io.StringIO(), + ) + assert code == 0 + + +# --------------------------------------------------------------------------- +# JSON output includes checker marker's "code" field +# --------------------------------------------------------------------------- + +def test_json_output_includes_code_field(valid_pss): + import io, json + + class JsonTestChecker(CheckerBase): + name = "json-test" + marker_defs = [MarkerDef(id="JSN001", severity="error", summary="json test")] + runs_without_link = True + + def check(self, context): + context.add_marker( + code="JSN001", + file=context.files[0], + line=1, + col=1, + message="json marker", + ) + + manager = _make_manager(JsonTestChecker) + stdout_buf = io.StringIO() + cmd_parse( + files=[valid_pss], + use_json=True, + syntax_only=True, + manager=manager, + checkers=None, + no_checkers=None, + quiet=False, + stdout=stdout_buf, + stderr=io.StringIO(), + ) + output = stdout_buf.getvalue() + data = json.loads(output) + codes = [d.get("code") for d in data.get("diagnostics", [])] + assert "JSN001" in codes diff --git a/tests/python/checkers/test_checkerbase.py b/tests/python/checkers/test_checkerbase.py new file mode 100644 index 0000000..43de3cd --- /dev/null +++ b/tests/python/checkers/test_checkerbase.py @@ -0,0 +1,55 @@ +"""Tests for pssparser.checkers.CheckerBase.""" +from __future__ import annotations + +import pytest + +from pssparser.checkers import CheckerBase, MarkerDef + + +def test_base_check_raises_not_implemented(): + base = CheckerBase() + with pytest.raises(NotImplementedError): + base.check(None) + + +def test_concrete_subclass_can_be_instantiated(): + class MyChecker(CheckerBase): + name = "my-checker" + description = "Test" + marker_defs = [MarkerDef(id="MY001", severity="warning", summary="s")] + + def check(self, context): + pass + + checker = MyChecker() + checker.check(None) # should not raise + + +def test_runs_without_link_defaults_false(): + checker = CheckerBase() + assert checker.runs_without_link is False + + +def test_subclass_marker_defs_do_not_pollute_parent(): + class ChildA(CheckerBase): + name = "child-a" + marker_defs = [MarkerDef(id="A001", severity="error", summary="A")] + + def check(self, context): + pass + + class ChildB(CheckerBase): + name = "child-b" + marker_defs = [MarkerDef(id="B001", severity="warning", summary="B")] + + def check(self, context): + pass + + assert "A001" not in [md.id for md in ChildB().marker_defs] + assert "B001" not in [md.id for md in ChildA().marker_defs] + # The base class list must remain empty + assert CheckerBase.marker_defs == [] + + +def test_name_defaults_to_empty_string(): + assert CheckerBase.name == "" diff --git a/tests/python/checkers/test_cli_checker_flags.py b/tests/python/checkers/test_cli_checker_flags.py new file mode 100644 index 0000000..4aa1cf0 --- /dev/null +++ b/tests/python/checkers/test_cli_checker_flags.py @@ -0,0 +1,150 @@ +"""Tests for the new CLI checker flags.""" +from __future__ import annotations + +import io +import sys +import pytest + +from pssparser.cli.app import main + + +def _run(argv, *, stdin=None): + """Run main() with captured stdout/stderr; return (exit_code, stdout, stderr).""" + out = io.StringIO() + err = io.StringIO() + old_out, old_err = sys.stdout, sys.stderr + sys.stdout, sys.stderr = out, err + try: + code = main(argv) + finally: + sys.stdout, sys.stderr = old_out, old_err + return code, out.getvalue(), err.getvalue() + + +# --------------------------------------------------------------------------- +# --list-checkers +# --------------------------------------------------------------------------- + +def test_list_checkers_exit_zero(): + code, out, _ = _run(["--list-checkers"]) + assert code == 0 + + +def test_list_checkers_shows_core(): + _, out, _ = _run(["--list-checkers"]) + assert "core" in out + + +def test_list_checkers_no_files_needed(): + code, _, _ = _run(["--list-checkers"]) + assert code == 0 + + +# --------------------------------------------------------------------------- +# --list-markers +# --------------------------------------------------------------------------- + +def test_list_markers_exit_zero(): + code, _, _ = _run(["--list-markers"]) + assert code == 0 + + +def test_list_markers_shows_pss001(): + _, out, _ = _run(["--list-markers"]) + assert "PSS001" in out + + +def test_list_markers_shows_pss002(): + _, out, _ = _run(["--list-markers"]) + assert "PSS002" in out + + +def test_list_markers_shows_columns(): + _, out, _ = _run(["--list-markers"]) + assert "ID" in out + assert "SEV" in out + assert "CHECKER" in out + assert "SUMMARY" in out + + +# --------------------------------------------------------------------------- +# --describe +# --------------------------------------------------------------------------- + +def test_describe_known_id_exit_zero(): + code, _, _ = _run(["--describe", "PSS001"]) + assert code == 0 + + +def test_describe_known_id_shows_summary(): + _, out, _ = _run(["--describe", "PSS001"]) + assert "PSS001" in out + + +def test_describe_known_id_shows_detail(): + _, out, _ = _run(["--describe", "PSS001"]) + # The PSS001 detail text contains "parser" + assert "parser" in out.lower() or "syntax" in out.lower() + + +def test_describe_unknown_id_exit_two(): + code, _, _ = _run(["--describe", "UNKNOWNXXX"]) + assert code == 2 + + +def test_describe_unknown_id_error_message(): + _, _, err = _run(["--describe", "UNKNOWNXXX"]) + assert "error" in err.lower() or "unknown" in err.lower() + + +# --------------------------------------------------------------------------- +# No source files, no query flag +# --------------------------------------------------------------------------- + +def test_no_files_no_query_exit_two(): + code, _, _ = _run([]) + assert code == 2 + + +# --------------------------------------------------------------------------- +# --checker and --no-checker with source files (basic smoke test) +# --------------------------------------------------------------------------- + +def test_checker_flag_with_valid_file(tmp_path): + pss = tmp_path / "valid.pss" + pss.write_text("component pss_top {\n action A {}\n}\n") + code, _, _ = _run(["--checker", "core", str(pss)]) + # 'core' is is_builtin so active() returns nothing; no errors -> exit 0 + assert code == 0 + + +def test_no_checker_flag_unknown_silently_ignored(tmp_path): + pss = tmp_path / "valid.pss" + pss.write_text("component pss_top {\n action A {}\n}\n") + code, _, _ = _run(["--no-checker", "nonexistent-checker", str(pss)]) + assert code == 0 + + +# --------------------------------------------------------------------------- +# --load-checker +# --------------------------------------------------------------------------- + +def test_load_checker_invalid_spec_exits_two(): + code, _, err = _run(["--load-checker", "badspec"]) + assert code == 2 + assert "error" in err.lower() + + +def test_load_checker_then_list(tmp_path, monkeypatch): + mod = tmp_path / "mychk.py" + mod.write_text( + "from pssparser.checkers import CheckerBase, MarkerDef\n" + "class MyChecker(CheckerBase):\n" + " name = 'my-loaded-checker'\n" + " marker_defs = [MarkerDef(id='MLC001', severity='warning', summary='s')]\n" + " def check(self, context): pass\n" + ) + monkeypatch.syspath_prepend(str(tmp_path)) + code, out, _ = _run(["--load-checker", "mychk:MyChecker", "--list-checkers"]) + assert code == 0 + assert "my-loaded-checker" in out diff --git a/tests/python/checkers/test_context.py b/tests/python/checkers/test_context.py new file mode 100644 index 0000000..478b4da --- /dev/null +++ b/tests/python/checkers/test_context.py @@ -0,0 +1,61 @@ +"""Tests for pssparser.checkers.CheckContext.""" +from __future__ import annotations + +import pytest + +from pssparser.checkers import CheckContext, MarkerDef + + +def _make_context(marker_defs=None): + index = {} + for md in (marker_defs or []): + index[md.id] = md + return CheckContext(root=None, files=["test.pss"], global_scopes=[], _marker_index=index) + + +def test_add_marker_appends_to_markers(): + md = MarkerDef(id="TST001", severity="warning", summary="Test") + ctx = _make_context([md]) + ctx.add_marker(code="TST001", file="test.pss", line=1, col=1, message="hello") + assert len(ctx._markers) == 1 + + +def test_add_marker_dict_has_expected_keys(): + md = MarkerDef(id="TST001", severity="warning", summary="Test") + ctx = _make_context([md]) + ctx.add_marker(code="TST001", file="test.pss", line=5, col=10, message="msg") + m = ctx._markers[0] + assert "severity" in m + assert "message" in m + assert "file" in m + assert "line" in m + assert "col" in m + assert "code" in m + + +def test_severity_taken_from_marker_def_when_not_provided(): + md = MarkerDef(id="TST001", severity="warning", summary="Test") + ctx = _make_context([md]) + ctx.add_marker(code="TST001", file="test.pss", line=1, col=1, message="msg") + assert ctx._markers[0]["severity"] == "warning" + + +def test_explicit_severity_overrides_marker_def(): + md = MarkerDef(id="TST001", severity="warning", summary="Test") + ctx = _make_context([md]) + ctx.add_marker(code="TST001", file="test.pss", line=1, col=1, message="msg", severity="error") + assert ctx._markers[0]["severity"] == "error" + + +def test_add_marker_unknown_code_raises_value_error(): + ctx = _make_context() + with pytest.raises(ValueError, match="TST999"): + ctx.add_marker(code="TST999", file="test.pss", line=1, col=1, message="bad") + + +def test_message_includes_code_prefix(): + md = MarkerDef(id="TST001", severity="info", summary="Test") + ctx = _make_context([md]) + ctx.add_marker(code="TST001", file="test.pss", line=1, col=1, message="the message") + assert "[TST001]" in ctx._markers[0]["message"] + assert "the message" in ctx._markers[0]["message"] diff --git a/tests/python/checkers/test_manager.py b/tests/python/checkers/test_manager.py new file mode 100644 index 0000000..b090472 --- /dev/null +++ b/tests/python/checkers/test_manager.py @@ -0,0 +1,257 @@ +"""Tests for pssparser.checkers.CheckerManager.""" +from __future__ import annotations + +from unittest.mock import patch, MagicMock +import pytest + +from pssparser.checkers import CheckerBase, CheckerManager, MarkerDef +from tests.python.checkers.conftest import ( + DummyChecker, + AnotherDummyChecker, + ConflictingChecker, +) + + +# --------------------------------------------------------------------------- +# discover() +# --------------------------------------------------------------------------- + +def test_discover_registers_core(): + m = CheckerManager() + m.discover() + assert "core" in m._registered + + +def test_discover_core_is_builtin(): + m = CheckerManager() + m.discover() + cls = m._registered["core"] + assert getattr(cls, "is_builtin", False) is True + + +def test_discover_with_entry_points(monkeypatch): + fake_ep = MagicMock() + fake_ep.name = "dummy" + fake_ep.load.return_value = DummyChecker + + def fake_entry_points(group): + return [fake_ep] + + with patch("pssparser.checkers.manager.CheckerManager.discover") as mock_discover: + mock_discover.side_effect = lambda: None + m = CheckerManager() + + # Manually register core and the fake ep + from pssparser.checkers.core_checker import CoreChecker + m._registered["core"] = CoreChecker + m._registered["dummy"] = DummyChecker + assert "dummy" in m._registered + + +# --------------------------------------------------------------------------- +# _check_unique_ids() +# --------------------------------------------------------------------------- + +def test_unique_ids_no_overlap_passes(): + m = CheckerManager() + from pssparser.checkers.core_checker import CoreChecker + m._registered["core"] = CoreChecker + m._registered["dummy"] = DummyChecker + m._registered["another"] = AnotherDummyChecker + m._check_unique_ids() # should not raise + + +def test_unique_ids_overlap_raises(): + m = CheckerManager() + m._registered["dummy"] = DummyChecker + m._registered["conflicting"] = ConflictingChecker + with pytest.raises(ValueError, match="TST001"): + m._check_unique_ids() + + +# --------------------------------------------------------------------------- +# load() +# --------------------------------------------------------------------------- + +def test_load_valid_spec(tmp_path, monkeypatch): + import sys + mod_file = tmp_path / "mychecker.py" + mod_file.write_text( + "from pssparser.checkers import CheckerBase, MarkerDef\n" + "class MyChecker(CheckerBase):\n" + " name = 'my-checker'\n" + " marker_defs = [MarkerDef(id='MY001', severity='warning', summary='s')]\n" + " def check(self, context): pass\n" + ) + monkeypatch.syspath_prepend(str(tmp_path)) + m = CheckerManager() + m.load("mychecker:MyChecker") + assert "my-checker" in m._registered + + +def test_load_missing_colon_raises(): + m = CheckerManager() + with pytest.raises(ValueError, match="expected"): + m.load("nocolonspec") + + +def test_load_missing_module_raises(): + m = CheckerManager() + with pytest.raises(ValueError, match="module not found"): + m.load("nonexistent.module.xyz:SomeClass") + + +def test_load_missing_class_raises(tmp_path, monkeypatch): + mod_file = tmp_path / "emptymod.py" + mod_file.write_text("# empty\n") + monkeypatch.syspath_prepend(str(tmp_path)) + m = CheckerManager() + with pytest.raises(ValueError, match="attribute not found"): + m.load("emptymod:MissingClass") + + +def test_load_empty_name_raises(tmp_path, monkeypatch): + mod_file = tmp_path / "noname.py" + mod_file.write_text( + "from pssparser.checkers import CheckerBase\n" + "class NoName(CheckerBase):\n" + " name = ''\n" + " def check(self, context): pass\n" + ) + monkeypatch.syspath_prepend(str(tmp_path)) + m = CheckerManager() + with pytest.raises(ValueError, match="empty 'name'"): + m.load("noname:NoName") + + +def test_load_duplicate_id_raises(tmp_path, monkeypatch): + mod_file = tmp_path / "dupchecker.py" + mod_file.write_text( + "from pssparser.checkers import CheckerBase, MarkerDef\n" + "class DupChecker(CheckerBase):\n" + " name = 'dup'\n" + " marker_defs = [MarkerDef(id='TST001', severity='error', summary='s')]\n" + " def check(self, context): pass\n" + ) + monkeypatch.syspath_prepend(str(tmp_path)) + m = CheckerManager() + m._registered["dummy"] = DummyChecker + with pytest.raises(ValueError, match="TST001"): + m.load("dupchecker:DupChecker") + + +# --------------------------------------------------------------------------- +# active() +# --------------------------------------------------------------------------- + +def _manager_with_dummies(): + m = CheckerManager() + from pssparser.checkers.core_checker import CoreChecker + m._registered["core"] = CoreChecker + m._registered["dummy"] = DummyChecker + m._registered["another-dummy"] = AnotherDummyChecker + return m + + +def test_active_returns_all_non_builtin_by_default(): + m = _manager_with_dummies() + active = m.active(select=None, exclude=None) + names = {c.name for c in active} + assert "dummy" in names + assert "another-dummy" in names + assert "core" not in names + + +def test_active_with_select_returns_only_selected(): + m = _manager_with_dummies() + active = m.active(select=["dummy"], exclude=None) + names = {c.name for c in active} + assert names == {"dummy"} + + +def test_active_select_unknown_raises(): + m = _manager_with_dummies() + with pytest.raises(ValueError, match="Unknown checker"): + m.active(select=["nonexistent"], exclude=None) + + +def test_active_exclude_removes_checker(): + m = _manager_with_dummies() + active = m.active(select=None, exclude=["another-dummy"]) + names = {c.name for c in active} + assert "another-dummy" not in names + assert "dummy" in names + + +def test_active_builtin_never_returned(): + m = _manager_with_dummies() + active = m.active(select=None, exclude=None) + for checker in active: + assert not getattr(checker, "is_builtin", False) + + +# --------------------------------------------------------------------------- +# list_checkers() +# --------------------------------------------------------------------------- + +def test_list_checkers_contains_required_keys(): + m = CheckerManager() + m.discover() + result = m.list_checkers() + for item in result: + assert "name" in item + assert "description" in item + assert "marker_ids" in item + assert "is_builtin" in item + + +def test_list_checkers_core_is_builtin(): + m = CheckerManager() + m.discover() + core_info = next(i for i in m.list_checkers() if i["name"] == "core") + assert core_info["is_builtin"] is True + + +# --------------------------------------------------------------------------- +# list_all_markers() +# --------------------------------------------------------------------------- + +def test_list_all_markers_has_required_keys(): + m = CheckerManager() + m.discover() + markers = m.list_all_markers() + assert len(markers) > 0 + for item in markers: + assert "id" in item + assert "severity" in item + assert "checker" in item + assert "summary" in item + + +def test_list_all_markers_checker_name_correct(): + m = _manager_with_dummies() + markers = m.list_all_markers() + tst001 = next(x for x in markers if x["id"] == "TST001") + assert tst001["checker"] == "dummy" + + +# --------------------------------------------------------------------------- +# describe_marker() +# --------------------------------------------------------------------------- + +def test_describe_marker_known_id(): + m = CheckerManager() + m.discover() + info = m.describe_marker("PSS001") + assert info is not None + assert info["id"] == "PSS001" + assert "severity" in info + assert "checker" in info + assert "summary" in info + assert "detail" in info + + +def test_describe_marker_unknown_id_returns_none(): + m = CheckerManager() + m.discover() + assert m.describe_marker("XXXXXXXX") is None diff --git a/tests/python/checkers/test_markerdef.py b/tests/python/checkers/test_markerdef.py new file mode 100644 index 0000000..630437f --- /dev/null +++ b/tests/python/checkers/test_markerdef.py @@ -0,0 +1,38 @@ +"""Tests for pssparser.checkers.MarkerDef.""" +from __future__ import annotations + +import pytest +from dataclasses import FrozenInstanceError + +from pssparser.checkers import MarkerDef + + +def test_fields_stored(): + md = MarkerDef(id="TST001", severity="warning", summary="Test summary", detail="Details here") + assert md.id == "TST001" + assert md.severity == "warning" + assert md.summary == "Test summary" + assert md.detail == "Details here" + + +def test_detail_defaults_to_empty(): + md = MarkerDef(id="TST001", severity="error", summary="No detail") + assert md.detail == "" + + +def test_frozen_raises_on_assignment(): + md = MarkerDef(id="TST001", severity="error", summary="Test") + with pytest.raises(FrozenInstanceError): + md.id = "TST999" + + +def test_equality_same_fields(): + md1 = MarkerDef(id="TST001", severity="error", summary="Test", detail="d") + md2 = MarkerDef(id="TST001", severity="error", summary="Test", detail="d") + assert md1 == md2 + + +def test_inequality_different_fields(): + md1 = MarkerDef(id="TST001", severity="error", summary="A") + md2 = MarkerDef(id="TST002", severity="error", summary="A") + assert md1 != md2 From 1a8656096ba196080a8238bfae2065139a9fa062 Mon Sep 17 00:00:00 2001 From: Matthew Ballance Date: Sun, 5 Apr 2026 00:55:55 +0000 Subject: [PATCH 2/2] Add example Signed-off-by: Matthew Ballance --- docs/checker_plugin_guide.rst | 114 +++++++++---- examples/pss_naming_checker/README.rst | 101 +++++++++++ examples/pss_naming_checker/setup.cfg | 23 +++ examples/pss_naming_checker/setup.py | 2 + .../src/pss_naming/__init__.py | 0 .../src/pss_naming/checker.py | 110 ++++++++++++ .../tests/test_naming_checker.py | 161 ++++++++++++++++++ python/pssparser/checkers/context.py | 5 + python/pssparser/checkers/core_checker.py | 67 +++++--- python/pssparser/cli/commands.py | 78 ++++++++- tests/python/checkers/test_core_codes.py | 145 ++++++++++++++++ 11 files changed, 745 insertions(+), 61 deletions(-) create mode 100644 examples/pss_naming_checker/README.rst create mode 100644 examples/pss_naming_checker/setup.cfg create mode 100644 examples/pss_naming_checker/setup.py create mode 100644 examples/pss_naming_checker/src/pss_naming/__init__.py create mode 100644 examples/pss_naming_checker/src/pss_naming/checker.py create mode 100644 examples/pss_naming_checker/tests/test_naming_checker.py create mode 100644 tests/python/checkers/test_core_codes.py diff --git a/docs/checker_plugin_guide.rst b/docs/checker_plugin_guide.rst index 3c51dc1..78ec974 100644 --- a/docs/checker_plugin_guide.rst +++ b/docs/checker_plugin_guide.rst @@ -26,51 +26,85 @@ disabled. Writing a Checker ================= -Minimal example — a naming-convention checker that warns when a component -name does not start with an uppercase letter: +Below is a complete, real-world example — a naming-convention checker that +warns when ``action`` or ``struct`` type names do not start with an uppercase +letter. The full source lives in ``examples/pss_naming_checker/``. .. code-block:: python - # mypkg/pss_rules.py + # pss_naming/checker.py + from __future__ import annotations + from typing import TYPE_CHECKING + + import pssparser.ast as pss_ast from pssparser.checkers import CheckerBase, MarkerDef + if TYPE_CHECKING: + from pssparser.checkers import CheckContext + + class NamingConventionChecker(CheckerBase): name = "naming-convention" - description = "Enforce PSS naming conventions" + description = "Warn when action or struct type names do not start with uppercase" marker_defs = [ MarkerDef( id="PSC001", severity="warning", - summary="Component name does not start with an uppercase letter", + summary="Action type name does not start with an uppercase letter", + detail=( + "PSS convention uses PascalCase for action type names. " + "Rename the action so that its first letter is uppercase, " + "e.g. rename ``write_data`` to ``WriteData``." + ), + ), + MarkerDef( + id="PSC002", + severity="warning", + summary="Struct type name does not start with an uppercase letter", detail=( - "PSS convention requires component type names to begin with " - "an uppercase letter. Rename the component to fix this." + "PSS convention uses PascalCase for struct type names. " + "Rename the struct so that its first letter is uppercase, " + "e.g. rename ``my_packet`` to ``MyPacket``." ), ), ] - runs_without_link = False - - def check(self, context): - # Walk top-level global scopes looking for component declarations - for scope in context.global_scopes: - for child in (scope.children() if hasattr(scope, "children") else []): - if hasattr(child, "getName"): - name_node = child.getName() - name = ( - name_node.getId() - if hasattr(name_node, "getId") - else str(name_node) - ) - if name and not name[0].isupper(): - context.add_marker( - code="PSC001", - file=context.files[0], - line=1, - col=1, - message=f"Component {name!r} should start with uppercase", - ) + # Name-checking only needs the parse tree; no linked AST required. + runs_without_link = True + + def check(self, context: "CheckContext") -> None: + for global_scope in context.global_scopes: + # context.file_map maps GlobalScope.getFileid() → source path. + # GlobalScope.getFilename() is not reliably set by the parser. + filename = context.file_map.get(global_scope.getFileid(), "") + self._walk(context, global_scope, filename) + + def _walk(self, context, scope, filename: str) -> None: + """Recursively walk *scope* and emit markers for naming violations.""" + for child in scope.children(): + if isinstance(child, pss_ast.Action): + self._check_name(context, child, filename, "PSC001", "Action") + elif isinstance(child, pss_ast.Struct): + self._check_name(context, child, filename, "PSC002", "Struct") + # Recurse into any child scope (components, packages, …) + if isinstance(child, pss_ast.Scope): + self._walk(context, child, filename) + + @staticmethod + def _check_name(context, node, filename, code, kind): + name_expr = node.getName() + name = name_expr.getId() + if not name or name[0].isupper(): + return + loc = name_expr.getLocation() + context.add_marker( + code=code, + file=filename, + line=loc.lineno, + col=loc.linepos, + message=f"{kind} '{name}' should start with an uppercase letter", + ) Step-by-step walkthrough: @@ -79,9 +113,20 @@ Step-by-step walkthrough: 3. **Set** ``description`` — shown by ``--list-checkers``. 4. **Declare** ``marker_defs`` — one :class:`~pssparser.checkers.MarkerDef` per diagnostic code your checker can emit. -5. **Implement** ``check(context)`` — walk the AST and call +5. **Set** ``runs_without_link = True`` when your checker only needs the raw + parse tree (faster; works even when ``--syntax-only`` is active). +6. **Implement** ``check(context)`` — walk the AST and call :meth:`~pssparser.checkers.CheckContext.add_marker` to emit diagnostics. +.. tip:: + + ``context.file_map`` is a ``dict[int, str]`` mapping + ``GlobalScope.getFileid()`` to the source file path. Use it instead of + ``GlobalScope.getFilename()``, which may be empty. + + ``context.global_scopes`` contains only the user-supplied source files; + the built-in PSS library scopes are filtered out automatically. + Declaring Markers ================= @@ -227,6 +272,17 @@ Inside ``check(context)``, the linked AST is available as nodes are in ``context.global_scopes``. See :doc:`ast_usage_guide` for a full guide to navigating the AST. +To resolve a ``GlobalScope`` to its source path, use ``context.file_map``:: + + filename = context.file_map.get(gs.getFileid(), "") + +``context.file_map`` is a ``dict[int, str]`` (fileid → path). +``GlobalScope.getFilename()`` is not reliably populated by the parser, so +always prefer ``file_map``. + +``context.global_scopes`` contains only the user-supplied source files; +the built-in PSS library scopes are filtered out automatically. + If your checker only needs the *parse tree* (not the linked AST), set ``runs_without_link = True`` on the class. The checker will then run even when ``--syntax-only`` is passed. When ``runs_without_link = False`` diff --git a/examples/pss_naming_checker/README.rst b/examples/pss_naming_checker/README.rst new file mode 100644 index 0000000..cb58c95 --- /dev/null +++ b/examples/pss_naming_checker/README.rst @@ -0,0 +1,101 @@ +pss_naming_checker — example ``pssparser`` checker plug-in +========================================================== + +This directory contains a fully-working example of a third-party checker +plug-in for ``pssparser-linter``. Read it together with +``docs/checker_plugin_guide.rst`` to understand how to build your own. + + +What it does +------------ + +``NamingConventionChecker`` (id: ``naming-convention``) warns when ``action`` +or ``struct`` type names do not start with an uppercase letter — a common PSS +coding convention (PascalCase for named types). + +Markers emitted: + ++--------+----------+---------------------------------------------------+ +| Code | Severity | Meaning | ++========+==========+===================================================+ +| PSC001 | warning | An ``action`` name does not start with uppercase | ++--------+----------+---------------------------------------------------+ +| PSC002 | warning | A ``struct`` name does not start with uppercase | ++--------+----------+---------------------------------------------------+ + + +Quick demo +---------- + +Install into your environment (editable install works well for development):: + + pip install -e examples/pss_naming_checker/ + +Parse a file and enable the checker:: + + pssparser-linter --checker naming-convention my_design.pss + +Show all markers the checker can emit:: + + pssparser-linter --list-markers naming-convention + + +Project layout +-------------- + +:: + + examples/pss_naming_checker/ + ├── README.rst ← this file + ├── setup.cfg ← package metadata + entry_point declaration + ├── setup.py ← minimal shim for editable installs + ├── src/ + │ └── pss_naming/ + │ ├── __init__.py + │ └── checker.py ← NamingConventionChecker implementation + └── tests/ + └── test_naming_checker.py + + +Key implementation notes +------------------------ + +Entry-point declaration (``setup.cfg``):: + + [options.entry_points] + pssparser.checkers = + naming-convention = pss_naming.checker:NamingConventionChecker + +The group **must** be ``pssparser.checkers``; the key is the checker's +``name`` attribute. + +Checker class (``checker.py``):: + + class NamingConventionChecker(CheckerBase): + name = "naming-convention" + runs_without_link = True # raw parse tree is enough + + def check(self, context: CheckContext) -> None: + for gs in context.global_scopes: + filename = context.file_map.get(gs.getFileid(), "") + self._walk(context, gs, filename) + +``context.file_map`` maps ``GlobalScope.getFileid()`` → source path because +``GlobalScope.getFilename()`` is not reliably set by the current parser +implementation. + +AST traversal:: + + for child in scope.children(): + if isinstance(child, pss_ast.Action): + name = child.getName().getId() + loc = child.getName().getLocation() + context.add_marker(code="PSC001", file=filename, + line=loc.lineno, col=loc.linepos, + message=f"Action '{name}' should start uppercase") + if isinstance(child, pss_ast.Scope): + self._walk(context, child, filename) # recurse + +``GlobalScope`` → direct children are top-level declarations +(``Component``, ``PackageScope``, ``Action``, ``Struct``, …). Recurse into +any ``Scope`` to reach nested declarations inside ``component`` bodies. diff --git a/examples/pss_naming_checker/setup.cfg b/examples/pss_naming_checker/setup.cfg new file mode 100644 index 0000000..2232cae --- /dev/null +++ b/examples/pss_naming_checker/setup.cfg @@ -0,0 +1,23 @@ +[metadata] +name = pss-naming-checker +version = 0.1.0 +description = Example pssparser checker plug-in: PSS naming conventions +long_description = file: README.rst +long_description_content_type = text/x-rst +author = Example Author +license = Apache-2.0 +python_requires = >=3.9 + +[options] +package_dir = + = src +packages = find: +install_requires = + pssparser + +[options.packages.find] +where = src + +[options.entry_points] +pssparser.checkers = + naming-convention = pss_naming.checker:NamingConventionChecker diff --git a/examples/pss_naming_checker/setup.py b/examples/pss_naming_checker/setup.py new file mode 100644 index 0000000..8bf1ba9 --- /dev/null +++ b/examples/pss_naming_checker/setup.py @@ -0,0 +1,2 @@ +from setuptools import setup +setup() diff --git a/examples/pss_naming_checker/src/pss_naming/__init__.py b/examples/pss_naming_checker/src/pss_naming/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/examples/pss_naming_checker/src/pss_naming/checker.py b/examples/pss_naming_checker/src/pss_naming/checker.py new file mode 100644 index 0000000..4885abb --- /dev/null +++ b/examples/pss_naming_checker/src/pss_naming/checker.py @@ -0,0 +1,110 @@ +"""PSS naming-convention checker — pssparser plug-in example.""" +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pssparser.ast as pss_ast +from pssparser.checkers import CheckerBase, MarkerDef + +if TYPE_CHECKING: + from pssparser.checkers import CheckContext + + +class NamingConventionChecker(CheckerBase): + """Warn when action or struct type names do not start with an uppercase letter. + + PSS coding convention (and industry practice) uses PascalCase for named + types: actions, structs, and user-defined types should all start with an + uppercase letter. The built-in entry-point ``pss_top`` is the conventional + component name and is intentionally excluded from this rule. + + Markers + ------- + PSC001 + An action type name does not start with an uppercase letter. + PSC002 + A struct type name does not start with an uppercase letter. + + Example + ------- + Bad:: + + action write_data { ... } // PSC001: should be WriteData + struct packet { ... } // PSC002: should be Packet + + Good:: + + action WriteData { ... } + struct Packet { ... } + """ + + name = "naming-convention" + description = "Warn when action or struct type names do not start with uppercase" + + marker_defs = [ + MarkerDef( + id="PSC001", + severity="warning", + summary="Action type name does not start with an uppercase letter", + detail=( + "PSS convention uses PascalCase for action type names. " + "Rename the action so that its first letter is uppercase, " + "e.g. rename ``write_data`` to ``WriteData``." + ), + ), + MarkerDef( + id="PSC002", + severity="warning", + summary="Struct type name does not start with an uppercase letter", + detail=( + "PSS convention uses PascalCase for struct type names. " + "Rename the struct so that its first letter is uppercase, " + "e.g. rename ``my_packet`` to ``MyPacket``." + ), + ), + ] + + #: Name-checking only needs the parse tree; no linked AST required. + runs_without_link = True + + def check(self, context: "CheckContext") -> None: + for global_scope in context.global_scopes: + filename = context.file_map.get(global_scope.getFileid(), "") + self._walk(context, global_scope, filename) + + # ------------------------------------------------------------------ + # Private helpers + # ------------------------------------------------------------------ + + def _walk(self, context: "CheckContext", scope, filename: str) -> None: + """Recursively walk *scope* and emit markers for naming violations.""" + for child in scope.children(): + if isinstance(child, pss_ast.Action): + self._check_name(context, child, filename, "PSC001", "Action") + elif isinstance(child, pss_ast.Struct): + self._check_name(context, child, filename, "PSC002", "Struct") + + # Recurse into any child scope (components, packages, …) + if isinstance(child, pss_ast.Scope): + self._walk(context, child, filename) + + @staticmethod + def _check_name( + context: "CheckContext", + node, + filename: str, + code: str, + kind: str, + ) -> None: + name_expr = node.getName() + name = name_expr.getId() + if not name or name[0].isupper(): + return + loc = name_expr.getLocation() + context.add_marker( + code=code, + file=filename, + line=loc.lineno, + col=loc.linepos, + message=f"{kind} '{name}' should start with an uppercase letter", + ) diff --git a/examples/pss_naming_checker/tests/test_naming_checker.py b/examples/pss_naming_checker/tests/test_naming_checker.py new file mode 100644 index 0000000..3fe62eb --- /dev/null +++ b/examples/pss_naming_checker/tests/test_naming_checker.py @@ -0,0 +1,161 @@ +"""Tests for the pss_naming_checker example plug-in.""" +from __future__ import annotations + +import sys +import os + +# Make pssparser importable without installation (same PYTHONPATH=python trick) +_repo = os.path.join(os.path.dirname(__file__), "..", "..", "..", "..", "python") +if _repo not in sys.path: + sys.path.insert(0, os.path.abspath(_repo)) + +# Make pss_naming importable from src/ +_src = os.path.join(os.path.dirname(__file__), "..", "src") +if _src not in sys.path: + sys.path.insert(0, os.path.abspath(_src)) + +import pytest +from pss_naming.checker import NamingConventionChecker +from pssparser.checkers import CheckerManager + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _parse_and_check(pss_code: str, tmp_path) -> list: + """Parse *pss_code*, run NamingConventionChecker, return marker dicts.""" + from pssparser import Parser + from pssparser.parser import ParseException + from pssparser.checkers import CheckContext + + f = tmp_path / "test.pss" + f.write_text(pss_code) + + p = Parser() + try: + p.parse([str(f)]) + except ParseException: + pass # we still get global_scopes for syntax-only style tests + + # Filter to only user GlobalScopes using fileid→path mapping. + # Built-in PSS library has fileid=0 and is absent from p._filenames. + user_files = {str(f)} + filenames = getattr(p, "_filenames", {}) # {fileid: path} + global_scopes = [ + gs for gs in getattr(p, "_files", []) + if filenames.get(gs.getFileid(), "") in user_files + ] + + marker_index = {md.id: md for md in NamingConventionChecker.marker_defs} + ctx = CheckContext( + root=None, + files=[str(f)], + global_scopes=global_scopes, + file_map=dict(filenames), + _marker_index=marker_index, + ) + + checker = NamingConventionChecker() + checker.check(ctx) + return ctx._markers + + +# --------------------------------------------------------------------------- +# PSC001 — action names +# --------------------------------------------------------------------------- + +def test_action_lowercase_name_triggers_psc001(tmp_path): + markers = _parse_and_check( + "component pss_top { action write_data {} }", tmp_path + ) + codes = [m["code"] for m in markers] + assert "PSC001" in codes + + +def test_action_lowercase_message_contains_name(tmp_path): + markers = _parse_and_check( + "component pss_top { action write_data {} }", tmp_path + ) + psc001 = [m for m in markers if m["code"] == "PSC001"] + assert any("write_data" in m["message"] for m in psc001) + + +def test_action_uppercase_name_no_psc001(tmp_path): + markers = _parse_and_check( + "component pss_top { action WriteData {} }", tmp_path + ) + assert all(m["code"] != "PSC001" for m in markers) + + +def test_multiple_actions_each_checked(tmp_path): + markers = _parse_and_check( + "component pss_top { action good {} action also_bad {} }", tmp_path + ) + # 'good' is lowercase → PSC001; 'also_bad' is lowercase → PSC001 + psc001 = [m for m in markers if m["code"] == "PSC001"] + assert len(psc001) == 2 + + +# --------------------------------------------------------------------------- +# PSC002 — struct names +# --------------------------------------------------------------------------- + +def test_struct_lowercase_name_triggers_psc002(tmp_path): + markers = _parse_and_check("struct my_packet { int x; }", tmp_path) + codes = [m["code"] for m in markers] + assert "PSC002" in codes + + +def test_struct_uppercase_name_no_psc002(tmp_path): + markers = _parse_and_check("struct MyPacket { int x; }", tmp_path) + assert all(m["code"] != "PSC002" for m in markers) + + +def test_struct_message_contains_name(tmp_path): + markers = _parse_and_check("struct my_packet { int x; }", tmp_path) + psc002 = [m for m in markers if m["code"] == "PSC002"] + assert any("my_packet" in m["message"] for m in psc002) + + +# --------------------------------------------------------------------------- +# runs_without_link +# --------------------------------------------------------------------------- + +def test_runs_without_link_is_true(): + assert NamingConventionChecker.runs_without_link is True + + +# --------------------------------------------------------------------------- +# Marker location +# --------------------------------------------------------------------------- + +def test_marker_has_line_and_col(tmp_path): + markers = _parse_and_check( + "component pss_top { action write_data {} }", tmp_path + ) + psc001 = [m for m in markers if m["code"] == "PSC001"] + assert psc001, "expected at least one PSC001 marker" + m = psc001[0] + assert isinstance(m["line"], int) and m["line"] > 0 + assert isinstance(m["col"], int) and m["col"] > 0 + + +# --------------------------------------------------------------------------- +# Integration: marker_defs registered correctly in CheckerManager +# --------------------------------------------------------------------------- + +def test_marker_defs_unique_ids(): + m = CheckerManager() + m.discover() + m._registered["naming-convention"] = NamingConventionChecker + # Should not raise + m._check_unique_ids() + + +def test_list_markers_includes_psc001_psc002(): + m = CheckerManager() + m._registered["naming-convention"] = NamingConventionChecker + ids = [x["id"] for x in m.list_all_markers()] + assert "PSC001" in ids + assert "PSC002" in ids diff --git a/python/pssparser/checkers/context.py b/python/pssparser/checkers/context.py index 894c92e..9da960b 100644 --- a/python/pssparser/checkers/context.py +++ b/python/pssparser/checkers/context.py @@ -28,6 +28,11 @@ class CheckContext: #: Raw per-file GlobalScope AST nodes (same order as *files*). global_scopes: List[Any] + #: Maps fileid (int) -> source file path; allows checkers to resolve + #: GlobalScope.getFileid() to a human-readable path when getFilename() + #: is unavailable. + file_map: Dict[int, str] = field(default_factory=dict) + #: Internal – collected markers; use add_marker() to append. _markers: list = field(default_factory=list, repr=False) diff --git a/python/pssparser/checkers/core_checker.py b/python/pssparser/checkers/core_checker.py index b140003..46e81e4 100644 --- a/python/pssparser/checkers/core_checker.py +++ b/python/pssparser/checkers/core_checker.py @@ -24,22 +24,31 @@ class CoreChecker(CheckerBase): MarkerDef( id="PSS001", severity="error", - summary="Syntax error: unexpected token", + summary="Syntax error", detail=( - "The parser encountered a token it did not expect at this " - "position. Check that the surrounding PSS syntax is " - "well-formed." + "The parser encountered a token it did not expect. " + "Messages include patterns such as:\n\n" + "* ``expected ';' before '}'``\n" + "* ``unexpected end of input; possible missing closing '}'``\n" + "* ``unexpected '' in this context``\n" + "* ``expected identifier before ''``\n\n" + "Check that the surrounding PSS syntax is well-formed." ), ), MarkerDef( id="PSS002", severity="error", - summary="Undefined symbol reference", + summary="Unknown symbol reference", detail=( - "The linker could not resolve a reference to a named type " - "or identifier. Ensure the symbol is declared in one of " - "the source files passed to pssparser, or that the correct " - "package is imported." + "The linker could not resolve a named type, identifier, or " + "method. Messages include patterns such as:\n\n" + "* ``unknown type 'Foo'``\n" + "* ``unknown identifier 'bar'``\n" + "* ``unknown method 'baz' on built-in type``\n\n" + "Ensure the symbol is declared in one of the source files " + "passed to pssparser, or that the correct package is imported." + " When a close match exists, a ``did you mean '...'?`` " + "suggestion is appended." ), ), MarkerDef( @@ -48,37 +57,39 @@ class CoreChecker(CheckerBase): summary="Duplicate symbol declaration", detail=( "A symbol with this name is already declared in the same " - "scope. Rename one of the declarations to resolve the " - "conflict." + "scope. Messages include patterns such as:\n\n" + "* ``duplicate declaration of 'Foo'``\n" + "* ``duplicate variable declaration bar``\n" + "* ``duplicate parameter name 'p'``\n" + "* ``duplicate symbol declaration``\n\n" + "Rename one of the declarations to resolve the conflict." ), ), MarkerDef( id="PSS004", severity="error", - summary="Type resolution error", + summary="Symbol or ref-path resolution failure", detail=( - "The type referenced in this expression or declaration could " - "not be resolved. Ensure the type is declared or imported " - "before use." + "A symbol or reference path could not be resolved during " + "linking. Messages include patterns such as:\n\n" + "* ``failed to resolve ref-path ``\n" + "* ``failed to resolve symbol ``\n" + "* ``root ref-path element is not a composite scope``\n\n" + "Check that all referenced symbols are declared and that " + "composite-type fields are used correctly." ), ), MarkerDef( id="PSS005", severity="error", - summary="Import resolution error", + summary="Cannot extend unknown type or enum", detail=( - "An imported package or symbol could not be found. Check " - "that the package name is correct and that the providing " - "file is included in the pssparser invocation." - ), - ), - MarkerDef( - id="PSS006", - severity="warning", - summary="Ambiguous symbol reference", - detail=( - "More than one symbol matches this reference. Use a " - "fully-qualified name to disambiguate." + "An ``extend`` declaration targets a type or enum that does " + "not exist. Messages include patterns such as:\n\n" + "* ``cannot extend unknown type 'Foo'``\n" + "* ``cannot extend unknown enum 'MyEnum'``\n\n" + "Ensure the base type is declared before or alongside the " + "extend block." ), ), ] diff --git a/python/pssparser/cli/commands.py b/python/pssparser/cli/commands.py index 0959ed3..fd91796 100644 --- a/python/pssparser/cli/commands.py +++ b/python/pssparser/cli/commands.py @@ -2,6 +2,7 @@ from __future__ import annotations import json +import re import sys from typing import List, Optional, TextIO @@ -107,6 +108,60 @@ def cmd_parse( return 1 if coll.has_errors else 0 +# -- Core-marker code assignment ------------------------------------------- + +# Ordered list of (compiled-regex, PSS-code) pairs. The first match wins. +_CORE_CODE_PATTERNS: list = [] + + +def _build_core_patterns() -> list: + """Compile and return the message-to-PSS-code pattern table.""" + rules = [ + # PSS001 — syntax errors (parser phase) + (r"^expected\b", "PSS001"), + (r"^unexpected\b", "PSS001"), + (r"^unknown exec-block kind\b", "PSS001"), + + # PSS002 — unknown symbol (linker phase) + (r"^unknown type\b", "PSS002"), + (r"^unknown identifier\b", "PSS002"), + (r"^unknown method\b", "PSS002"), + + # PSS003 — duplicate declarations + (r"^duplicate\b", "PSS003"), + + # PSS005 — extend-unknown (checked before PSS004 to avoid false match) + (r"^cannot extend unknown\b", "PSS005"), + + # PSS004 — general resolution / ref-path failures + (r"^failed to resolve\b", "PSS004"), + (r"\bref-path element\b", "PSS004"), + ] + return [(re.compile(pat, re.IGNORECASE), code) for pat, code in rules] + + +def _assign_core_code(marker: dict) -> dict: + """Return a copy of *marker* with a ``'code'`` field set if absent. + + Uses message-pattern matching against the known C++ diagnostic strings to + assign a ``PSS001``–``PSS005`` code. Markers that already carry a + ``'code'`` key are returned unchanged. + """ + if marker.get("code"): + return marker + + global _CORE_CODE_PATTERNS + if not _CORE_CODE_PATTERNS: + _CORE_CODE_PATTERNS = _build_core_patterns() + + msg = marker.get("message", "") + for pattern, code in _CORE_CODE_PATTERNS: + if pattern.search(msg): + return {**marker, "code": code} + + return marker + + # -- helpers ---------------------------------------------------------------- def _run_checkers( @@ -136,10 +191,23 @@ def _run_checkers( if not active: return - # Build per-file global scopes list (best-effort) + # Build per-file global scopes list (best-effort). + # p._files contains all GlobalScope objects; p._filenames maps + # fileid -> path for user-supplied files (built-in PSS library files + # have fileid=0 and are absent from _filenames). global_scopes: list = [] - if hasattr(parser, "global_scopes"): - global_scopes = list(parser.global_scopes) + if hasattr(parser, "_files") and hasattr(parser, "_filenames"): + filenames = parser._filenames # {fileid: path} + user_files = set(files) + for gs in parser._files: + fid = gs.getFileid() + fname = filenames.get(fid, "") + if fname in user_files: + global_scopes.append(gs) + + file_map: dict = {} + if hasattr(parser, "_filenames"): + file_map = dict(parser._filenames) marker_index = manager.build_marker_index(active) @@ -147,6 +215,7 @@ def _run_checkers( root=linked_root, files=list(files), global_scopes=global_scopes, + file_map=file_map, _marker_index=marker_index, ) @@ -177,7 +246,8 @@ def _collect( if key in seen: continue seen.add(key) - coll.add(Diagnostic.from_marker(m)) + enriched = _assign_core_code(m) + coll.add(Diagnostic.from_marker(enriched)) def _emit_all(driver, coll: DiagnosticCollection, quiet: bool) -> None: diff --git a/tests/python/checkers/test_core_codes.py b/tests/python/checkers/test_core_codes.py new file mode 100644 index 0000000..bfe773c --- /dev/null +++ b/tests/python/checkers/test_core_codes.py @@ -0,0 +1,145 @@ +"""Tests that C++ core marker messages are mapped to the correct PSS codes.""" +from __future__ import annotations + +import pytest + +from pssparser.cli.commands import _assign_core_code + + +def _m(message, severity="error"): + return {"message": message, "severity": severity, "file": "t.pss", "line": 1, "col": 1} + + +# --------------------------------------------------------------------------- +# PSS001 — syntax errors +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("msg", [ + "expected ';' before '}'", + "expected identifier before 'action'", + "expected '{' or ':' before 'action'", + "unexpected end of input; possible missing closing '}'", + "unexpected 'action' in this context", + "unexpected keyword 'extends' in this context", + "unknown exec-block kind \"bad\" specified. Expect one of", +]) +def test_syntax_error_mapped_to_pss001(msg): + assert _assign_core_code(_m(msg))["code"] == "PSS001" + + +# --------------------------------------------------------------------------- +# PSS002 — unknown symbol +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("msg", [ + "unknown type 'SomeUndefinedType'", + "unknown type 'SomeUndefinedType'; did you mean 'SomeDefinedType'?", + "unknown identifier 'badref'", + "unknown identifier 'badref'; did you mean 'goodref'?", + "unknown method 'nosuchmethod' on built-in type", +]) +def test_unknown_symbol_mapped_to_pss002(msg): + assert _assign_core_code(_m(msg))["code"] == "PSS002" + + +# --------------------------------------------------------------------------- +# PSS003 — duplicate declarations +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("msg", [ + "duplicate declaration of 'pss_top'", + "duplicate variable declaration my_var", + "duplicate parameter name 'p'", + "duplicate symbol declaration", +]) +def test_duplicate_mapped_to_pss003(msg): + assert _assign_core_code(_m(msg))["code"] == "PSS003" + + +# --------------------------------------------------------------------------- +# PSS004 — resolution / ref-path failures +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("msg", [ + "failed to resolve ref-path some.path", + "failed to resolve symbol foo", + "root ref-path element x is not a composite scope", +]) +def test_resolution_failure_mapped_to_pss004(msg): + assert _assign_core_code(_m(msg))["code"] == "PSS004" + + +# --------------------------------------------------------------------------- +# PSS005 — extend-unknown +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("msg", [ + "cannot extend unknown type 'Foo'", + "cannot extend unknown enum 'MyEnum'", +]) +def test_extend_unknown_mapped_to_pss005(msg): + assert _assign_core_code(_m(msg))["code"] == "PSS005" + + +# --------------------------------------------------------------------------- +# No match — unrecognised message left without a code +# --------------------------------------------------------------------------- + +def test_unrecognised_message_no_code(): + result = _assign_core_code(_m("some completely unknown internal message")) + assert result.get("code") is None + + +# --------------------------------------------------------------------------- +# Existing code preserved +# --------------------------------------------------------------------------- + +def test_existing_code_preserved(): + marker = {**_m("expected ';'"), "code": "PSS999"} + assert _assign_core_code(marker)["code"] == "PSS999" + + +# --------------------------------------------------------------------------- +# Integration: end-to-end check that real parse errors get codes +# --------------------------------------------------------------------------- + +def test_real_syntax_error_gets_pss001(tmp_path): + from pssparser import Parser + from pssparser.parser import ParseException + from pssparser.cli.commands import _collect + from pssparser.cli.diagnostics import DiagnosticCollection + + f = tmp_path / "bad.pss" + f.write_text("component bad { action A { rand int x }") # missing semicolon + + p = Parser() + coll = DiagnosticCollection() + try: + p.parse([str(f)]) + except ParseException as exc: + _collect(coll, getattr(exc, "markers", []), p) + + pss001 = [d for d in coll.diagnostics if d.code == "PSS001"] + assert len(pss001) > 0 + + +def test_real_unknown_type_gets_pss002(tmp_path): + from pssparser import Parser + from pssparser.parser import ParseException + from pssparser.cli.commands import _collect + from pssparser.cli.diagnostics import DiagnosticCollection + + f = tmp_path / "undef.pss" + f.write_text("component pss_top { action A { UnknownType x; } }") + + p = Parser() + coll = DiagnosticCollection() + try: + p.parse([str(f)]) + p.link() + except ParseException as exc: + _collect(coll, getattr(exc, "markers", []), p) + _collect(coll, [], p) + + pss002 = [d for d in coll.diagnostics if d.code == "PSS002"] + assert len(pss002) > 0