From ccc2bd74ce5054f7e17bde4e117338e00d8c25c1 Mon Sep 17 00:00:00 2001 From: Frantisek Skala Date: Tue, 21 Oct 2025 07:00:10 +0200 Subject: [PATCH 1/2] feat: Add --fix and --export-fixes support This change introduces an automatic fixing capability to `clangd-tidy`, allowing it to apply clang-tidy's suggested fixes, including complex cross-file refactorings like renames. Cross-file fixes are something not supported by clang-tidy, only by clangd via LSP. This is implemented via two new flags: - `--fix`: Directly applies fixes to the source files. - `--export-fixes `: Exports the fixes to a YAML file in the format used by `clang-apply-replacements`. To support this complex, stateful interaction with `clangd`, the tool's architecture has been significantly refactored. ### Event-Driven Architecture The newly introduced complex LSP interactions are expressed using `Flow`s. Their focus is to make it somewhat easier to reason about the asynchronous operations and state management involved in interacting with `clangd`. To support this, a new event-driven architecture has been introduced using an `EventBus`. This new architecture brings several benefits: - **Crash Recovery:** The tool can now gracefully recover from `clangd` crashes, retrying incomplete operations without losing state. - **Extensibility:** The flow-based design makes it easier to add new features in the future. - **Clarity:** Complex interactions with the LSP server are now encapsulated within their respective flows, making the code easier to understand and maintain. ### Fix Application Logic To support cross-file fixes, which are not available through inline code actions, the tool now performs a more sophisticated dance with the LSP server: 1. It waits for `clangd`'s background indexing to complete to ensure it has a complete view of the project. 2. It explicitly requests code actions for each diagnostic. 3. It executes the returned commands (like `clangd.applyRename`). 4. It handles the resulting `workspace/applyEdit` requests from the server, which may contain edits for multiple files. ### Other Changes - Bump `cattrs` to `>= 25.1.0` to support newly introduced `Union` types. - **Python 3.10+ is now required.** This is necessary for the `cattrs` version. Python 3.9 is already EOL. - A set of end-to-end tests has been added for the new fixing functionality, as well as the existing diagnostic collection functionality. - Use `logging` for warnings instead of printing directly to stderr. ### Performance This has been evaluated on a non-trivial C++ codebase (~50 files). Using just the `--diagnostics` mode, the performance impact is around 1% overhead compared to the previous version. --- .github/workflows/publish.yml | 2 +- README.md | 59 +- clangd_tidy/__main__.py | 3 +- clangd_tidy/args.py | 26 + clangd_tidy/event_bus.py | 101 +++ clangd_tidy/flows/__init__.py | 0 clangd_tidy/flows/base.py | 217 +++++++ clangd_tidy/flows/diagnostics.py | 126 ++++ clangd_tidy/flows/fix.py | 381 +++++++++++ clangd_tidy/flows/formatting.py | 88 +++ clangd_tidy/line_filter.py | 2 +- clangd_tidy/lsp/__init__.py | 3 +- clangd_tidy/lsp/clangd.py | 216 ++++++- clangd_tidy/lsp/client.py | 123 +++- clangd_tidy/lsp/messages.py | 293 ++++++++- clangd_tidy/lsp/rpc.py | 2 +- clangd_tidy/main_cli.py | 604 ++++++++++++++---- clangd_tidy/progress.py | 82 +++ clangd_tidy/replacements.py | 182 ++++++ pyproject.toml | 8 +- test/fixtures/README.md | 28 + test/fixtures/basic/.clang-tidy | 2 + test/fixtures/basic/test_fix.cpp | 9 + test/fixtures/basic/test_fix.cpp.fixed | 9 + .../basic/test_fix.cpp.fixed.formatted | 9 + test/fixtures/clangd_config/.clangd | 3 + .../clangd_config/compile_commands.json | 7 + test/fixtures/clangd_config/test_fix.cpp | 9 + .../fixtures/clangd_config/test_fix.cpp.fixed | 9 + .../test_fix.cpp.fixed.formatted | 9 + test/fixtures/combined_config/.clang-format | 4 + test/fixtures/combined_config/.clang-tidy | 4 + test/fixtures/combined_config/.clangd | 7 + test/fixtures/combined_config/foo.clangd.yaml | 38 ++ test/fixtures/combined_config/foo.yaml | 40 ++ test/fixtures/combined_config/test_fix.cpp | 10 + .../combined_config/test_fix.cpp.fixed | 10 + .../test_fix.cpp.fixed.formatted | 10 + test/fixtures/compilation_errors/.clangd | 4 + .../compilation_errors/test_errors.cpp | 25 + test/fixtures/cross_file/.clang-tidy | 7 + test/fixtures/cross_file/math_utils.cpp | 10 + test/fixtures/cross_file/math_utils.cpp.fixed | 10 + .../cross_file/math_utils.cpp.fixed.formatted | 8 + test/fixtures/cross_file/math_utils.h | 7 + test/fixtures/cross_file/math_utils.h.fixed | 7 + .../cross_file/math_utils.h.fixed.formatted | 7 + test/fixtures/error_recovery/.clangd | 6 + .../error_recovery/compile_commands.json | 7 + test/fixtures/error_recovery/test_crash.cpp | 10 + .../error_recovery/test_crash.cpp.fixed | 10 + test/fixtures/multiple_fixes/.clang-tidy | 8 + .../multiple_fixes/test_multiple_fixes.cpp | 5 + .../test_multiple_fixes.cpp.fixed | 5 + .../tool_specific_config/.clang-format | 5 + .../fixtures/tool_specific_config/.clang-tidy | 2 + .../tool_specific_config/test_fix.cpp | 9 + .../tool_specific_config/test_fix.cpp.fixed | 9 + .../test_fix.cpp.fixed.formatted | 9 + test/fixtures/utf8/.clang-tidy | 2 + test/fixtures/utf8/test_utf8.cpp | 21 + test/fixtures/utf8/test_utf8.cpp.fixed | 21 + .../utf8/test_utf8.cpp.fixed.formatted | 22 + test/test_basic.py | 128 ++++ test/test_diagnostics.py | 64 ++ test/test_fix.py | 178 ++++++ test/test_utils.py | 124 ++++ 67 files changed, 3260 insertions(+), 205 deletions(-) create mode 100644 clangd_tidy/event_bus.py create mode 100644 clangd_tidy/flows/__init__.py create mode 100644 clangd_tidy/flows/base.py create mode 100644 clangd_tidy/flows/diagnostics.py create mode 100644 clangd_tidy/flows/fix.py create mode 100644 clangd_tidy/flows/formatting.py create mode 100644 clangd_tidy/progress.py create mode 100644 clangd_tidy/replacements.py create mode 100644 test/fixtures/README.md create mode 100644 test/fixtures/basic/.clang-tidy create mode 100644 test/fixtures/basic/test_fix.cpp create mode 100644 test/fixtures/basic/test_fix.cpp.fixed create mode 100644 test/fixtures/basic/test_fix.cpp.fixed.formatted create mode 100644 test/fixtures/clangd_config/.clangd create mode 100644 test/fixtures/clangd_config/compile_commands.json create mode 100644 test/fixtures/clangd_config/test_fix.cpp create mode 100644 test/fixtures/clangd_config/test_fix.cpp.fixed create mode 100644 test/fixtures/clangd_config/test_fix.cpp.fixed.formatted create mode 100644 test/fixtures/combined_config/.clang-format create mode 100644 test/fixtures/combined_config/.clang-tidy create mode 100644 test/fixtures/combined_config/.clangd create mode 100644 test/fixtures/combined_config/foo.clangd.yaml create mode 100644 test/fixtures/combined_config/foo.yaml create mode 100644 test/fixtures/combined_config/test_fix.cpp create mode 100644 test/fixtures/combined_config/test_fix.cpp.fixed create mode 100644 test/fixtures/combined_config/test_fix.cpp.fixed.formatted create mode 100644 test/fixtures/compilation_errors/.clangd create mode 100644 test/fixtures/compilation_errors/test_errors.cpp create mode 100644 test/fixtures/cross_file/.clang-tidy create mode 100644 test/fixtures/cross_file/math_utils.cpp create mode 100644 test/fixtures/cross_file/math_utils.cpp.fixed create mode 100644 test/fixtures/cross_file/math_utils.cpp.fixed.formatted create mode 100644 test/fixtures/cross_file/math_utils.h create mode 100644 test/fixtures/cross_file/math_utils.h.fixed create mode 100644 test/fixtures/cross_file/math_utils.h.fixed.formatted create mode 100644 test/fixtures/error_recovery/.clangd create mode 100644 test/fixtures/error_recovery/compile_commands.json create mode 100644 test/fixtures/error_recovery/test_crash.cpp create mode 100644 test/fixtures/error_recovery/test_crash.cpp.fixed create mode 100644 test/fixtures/multiple_fixes/.clang-tidy create mode 100644 test/fixtures/multiple_fixes/test_multiple_fixes.cpp create mode 100644 test/fixtures/multiple_fixes/test_multiple_fixes.cpp.fixed create mode 100644 test/fixtures/tool_specific_config/.clang-format create mode 100644 test/fixtures/tool_specific_config/.clang-tidy create mode 100644 test/fixtures/tool_specific_config/test_fix.cpp create mode 100644 test/fixtures/tool_specific_config/test_fix.cpp.fixed create mode 100644 test/fixtures/tool_specific_config/test_fix.cpp.fixed.formatted create mode 100644 test/fixtures/utf8/.clang-tidy create mode 100644 test/fixtures/utf8/test_utf8.cpp create mode 100644 test/fixtures/utf8/test_utf8.cpp.fixed create mode 100644 test/fixtures/utf8/test_utf8.cpp.fixed.formatted create mode 100644 test/test_basic.py create mode 100644 test/test_diagnostics.py create mode 100644 test/test_fix.py create mode 100644 test/test_utils.py diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 6f1e82b..a51ee6f 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -18,7 +18,7 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 with: - python-version: "3.8" + python-version: "3.10" - run: | python3 -m pip install --upgrade build python3 -m build diff --git a/README.md b/README.md index fad5f59..adba730 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,6 @@ Unfortunately, there seems to be no plan within LLVM to accelerate the standalon **Cons:** -- clangd-tidy lacks support for the `--fix` option. (Consider using code actions provided by your editor if you have clangd properly configured, as clangd-tidy is primarily designed for speeding up CI checks.) - clangd-tidy silently disables [several](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/TidyProvider.cpp#L197) checks not supported by clangd. - Diagnostics generated by clangd-tidy might be marginally less aesthetically pleasing compared to clang-tidy. - Other known discrepancies between clangd and clang-tidy behavior: #7, #15, #16. @@ -43,8 +42,8 @@ Unfortunately, there seems to be no plan within LLVM to accelerate the standalon ## Prerequisites - [clangd](https://clangd.llvm.org/) -- Python 3.8+ (may work on older versions, but not tested) -- [attrs](https://www.attrs.org/) and [cattrs](https://catt.rs/) (automatically installed if clangd-tidy is installed via pip) +- Python 3.10+ +- [attrs](https://www.attrs.org/) and [cattrs >= 25.1.0](https://catt.rs/) (automatically installed if clangd-tidy is installed via pip) - [tqdm](https://github.com/tqdm/tqdm) (optional, required for progress bar support) ## Installation @@ -59,8 +58,10 @@ pip install clangd-tidy ``` usage: clangd-tidy [--allow-extensions ALLOW_EXTENSIONS] - [--fail-on-severity SEVERITY] [-f] [-o OUTPUT] - [--line-filter LINE_FILTER] [--tqdm] [--github] + [--fail-on-severity SEVERITY] [-f] [--fix] + [--format-style ] [--export-fixes EXPORT_FIXES] + [--clang-apply-replacements-executable CLANG_APPLY_REPLACEMENTS_EXECUTABLE] + [-o OUTPUT] [--line-filter LINE_FILTER] [--tqdm] [--github] [--git-root GIT_ROOT] [-c] [--context CONTEXT] [--color {auto,always,never}] [-v] [-p COMPILE_COMMANDS_DIR] [-j JOBS] @@ -86,6 +87,23 @@ check options: -f, --format Also check code formatting with clang-format. Exits with a non-zero status if any file violates formatting rules. + --fix Apply suggested fixes to the files. Supports cross- + file refactorings. Disables --fail-on-severity. + --format-style + Style for formatting code around applied fixes (only + used with --fix). Options: 'none' (default, no + formatting), 'file' (use .clang-format), 'llvm', + 'google', 'webkit', 'mozilla', or '{key: value, ...}' + for inline configuration. See clang-format + documentation for details. Matches clang-tidy's + --format-style behavior. + --export-fixes EXPORT_FIXES + Export fixes to a YAML file. If this is used, --fix is + ignored. Unlike clang-tidy, always uses absolute paths + in the output YAML. Disables --fail-on-severity. + --clang-apply-replacements-executable CLANG_APPLY_REPLACEMENTS_EXECUTABLE + clang-apply-replacements executable. [default: clang- + apply-replacements] output options: -o OUTPUT, --output OUTPUT @@ -159,6 +177,37 @@ Example usage with git: ``` +## Applying Fixes + +`clangd-tidy` can automatically apply the suggested fixes for diagnostics. + +### Direct Fixes + +To apply fixes directly to your files, use the `--fix` flag: + +```bash +clangd-tidy --fix your_source_file.cpp +``` + +You can control the formatting of the code around the applied fixes using the `--format-style` argument. For example, to use the LLVM style: + +```bash +clangd-tidy --fix --format-style=llvm your_source_file.cpp +``` + +See the `clang-format` documentation for more details on the available styles and `clang-apply-replacements` for formatting behavior. + +### Exporting Fixes + +Alternatively, you can export the fixes to a YAML file, which can be applied later using `clang-apply-replacements`: + +```bash +clangd-tidy --export-fixes fixes.yaml your_source_file.cpp +clang-apply-replacements . < fixes.yaml +``` + +**Note:** When using `--fix` or `--export-fixes`, cross-file fixes may be missed for files not in the compilation database. Make sure all source files in your project are listed in the compilation database to avoid this issue. This is a limitation of clangd. + ## Acknowledgement Special thanks to [@yeger00](https://github.com/yeger00) for his [pylspclient](https://github.com/yeger00/pylspclient), which inspired earlier versions of this project. diff --git a/clangd_tidy/__main__.py b/clangd_tidy/__main__.py index eb27aa0..f634227 100644 --- a/clangd_tidy/__main__.py +++ b/clangd_tidy/__main__.py @@ -1,3 +1,4 @@ +import sys from .main_cli import main_cli -main_cli() +sys.exit(main_cli()) diff --git a/clangd_tidy/args.py b/clangd_tidy/args.py index 0753a77..d761275 100644 --- a/clangd_tidy/args.py +++ b/clangd_tidy/args.py @@ -70,6 +70,32 @@ def parse_args() -> argparse.Namespace: action="store_true", help="Also check code formatting with clang-format. Exits with a non-zero status if any file violates formatting rules.", ) + check_group.add_argument( + "--fix", + action="store_true", + help="Apply suggested fixes to the files. Supports cross-file refactorings. Disables --fail-on-severity.", + ) + check_group.add_argument( + "--format-style", + type=str, + default="none", + metavar="", + help="Style for formatting code around applied fixes (only used with --fix). " + "Options: 'none' (default, no formatting), 'file' (use .clang-format), " + "'llvm', 'google', 'webkit', 'mozilla', or '{key: value, ...}' for inline configuration. " + "See clang-format documentation for details. Matches clang-tidy's --format-style behavior.", + ) + check_group.add_argument( + "--export-fixes", + type=pathlib.Path, + default=None, + help="Export fixes to a YAML file. If this is used, --fix is ignored. Unlike clang-tidy, always uses absolute paths in the output YAML. Disables --fail-on-severity.", + ) + check_group.add_argument( + "--clang-apply-replacements-executable", + default="clang-apply-replacements", + help="clang-apply-replacements executable. [default: clang-apply-replacements]", + ) output_group = parser.add_argument_group("output options") output_group.add_argument( diff --git a/clangd_tidy/event_bus.py b/clangd_tidy/event_bus.py new file mode 100644 index 0000000..067e239 --- /dev/null +++ b/clangd_tidy/event_bus.py @@ -0,0 +1,101 @@ +import asyncio +import logging +from typing import Any, Callable, Coroutine, List, Type, Tuple, TypeVar, TypeAlias + + +T = TypeVar("T") +Handler: TypeAlias = Callable[[T], Coroutine[Any, Any, Any]] +Listener: TypeAlias = Tuple[Type[T], Handler[T]] + + +class EventBus: + """ + A simple asynchronous event bus for dispatching events to registered listeners. + Events are expected to be instances of LSP message types or similar data structures. + """ + + def __init__(self): + self._listeners: List[Listener[Any]] = [] + self._queue: asyncio.Queue[Any] = asyncio.Queue() + self._finished = asyncio.Future[None]() + self._handler_tasks: set[asyncio.Task[Any]] = set() + + def subscribe(self, event_type: Type[T], handler: Handler[T]): + """ + Registers a handler coroutine for a specific event type. + + Args: + event_type: The type of event (e.g., an LSP message class) to subscribe to. + handler: An async callable that will be called when an event of the given type is published. + """ + self._listeners.append((event_type, handler)) + + def publish(self, event: Any): + """ + Publishes an event to the bus. The event will be put into a queue + and dispatched asynchronously. + + Args: + event: The event instance (e.g., an LSP message object) to publish. + """ + self._queue.put_nowait(event) + + def _check_handler_task_exception(self, task: asyncio.Task[Any]): + """Check if a handler task raised an exception and log it.""" + if not task.cancelled(): + try: + task.result() + except Exception as exc: + logging.exception( + "Error in event handler task", + exc_info=exc, + ) + self._finished.set_exception(exc) + + async def run(self): + """ + The main dispatch loop that continuously pulls events from the queue + and dispatches them to registered listeners. + """ + while not self._finished.done(): + # Get the next event from the queue (or exit early if finished) + event_task = asyncio.create_task(self._queue.get()) + done, _ = await asyncio.wait( + {event_task, self._finished}, return_when=asyncio.FIRST_COMPLETED + ) + + if self._finished in done: + # If the finished future is done, cancel the event task. We only set + # _finished in case of error, so propagate the exception. + event_task.cancel() + break + + event = event_task.result() + try: + for event_type, handler in self._listeners: + if isinstance(event, event_type): + # Schedule handler as a separate task to avoid blocking the + # dispatch loop. This avoids deadlocks if a handler only + # completes after another event is processed. + task = asyncio.create_task(handler(event)) + task.add_done_callback(self._handler_tasks.discard) + task.add_done_callback(self._check_handler_task_exception) + self._handler_tasks.add(task) + except (Exception, asyncio.CancelledError) as e: + logging.error( + f"Error dispatching event {type(event).__name__}: {e}", + exc_info=e, + ) + finally: + self._queue.task_done() + + # Cancel any remaining handler tasks + for task in self._handler_tasks: + task.cancel() + try: + await task + except asyncio.CancelledError: + pass + + logging.debug("Stopping EventBus dispatch loop...") + return await self._finished # Propagate any exception diff --git a/clangd_tidy/flows/__init__.py b/clangd_tidy/flows/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/clangd_tidy/flows/base.py b/clangd_tidy/flows/base.py new file mode 100644 index 0000000..aaa59e4 --- /dev/null +++ b/clangd_tidy/flows/base.py @@ -0,0 +1,217 @@ +import asyncio +import logging +from typing import Any, Coroutine, TypeVar + +from ..lsp.messages import ClientCapabilities +from ..lsp.clangd import ClangdAsync +from ..event_bus import EventBus + +T = TypeVar("T") + + +class BaseFlow: + """ + A "Flow" represents a distinct, high-level operation or set of operations + that interacts with the clangd LSP server to achieve a specific goal. + + Examples of flows include: + - Analyzing a set of files for diagnostics. + - Requesting and applying code actions. + - Formatting a set of files. + + A single flow can spawn multiple asynchronous tasks to perform its work, + and can track the state of these tasks to determine when the flow is complete. + + A BaseFlow provides convenience methods for managing its lifecycle, including: + - Spawning tasks tied to the flow's lifecycle. + - Resetting the flow state after a clangd crash. + - Adding required client capabilities for the flow. + - Checking clangd capabilities after initialization. + """ + + def __init__( + self, + clangd_client: ClangdAsync, + event_bus: EventBus, + ): + self._clangd = clangd_client + self._event_bus = event_bus + self._futures: dict[Any, asyncio.Future[Any]] = {} + + async def bootstrap(self): + """ + Bootstrap the flow state. Handles clangd start/restart. + + This should set-up any initial state required for the flow to run, clearing any stale state from + previous runs. Any state that needs to be retained after clangd crash should be persisted by subclasses. + + Any state that needs to be retained after clangd crash should be persisted by subclasses. + For example, a list of files to be processed. + """ + await self._check_futures_purge_pending(log_errors=False) + + async def shutdown(self): + """ + Cleans up the flow state. Called when the flow is being shut down. + + This should clean up any state that is no longer needed, and ensure that + any pending tasks are cancelled and exceptions are logged. + """ + await self._check_futures_purge_pending(log_errors=True) # Log remaining errors + + def add_required_capabilities( + self, capabilities: ClientCapabilities + ) -> ClientCapabilities: + """Adds any required client capabilities for this flow. Can be overridden by subclasses.""" + return capabilities + + def check_clangd_capabilities(self, init_result: Any) -> None: + """Checks if clangd supports the capabilities required by this flow. Can be overridden by subclasses. + + The capabilities are passed as-is from the clangd initialize response.""" + pass + + def pending_futures(self) -> frozenset[asyncio.Future[Any]]: + """Returns all pending futures handled by this flow.""" + return frozenset( + awaitable for awaitable in self._futures.values() if not awaitable.done() + ) + + def completed(self) -> bool: + """ + Returns True if the flow has completed all its work. + + This method considers certain exceptions, such as connection errors, as + incomplete work that can be retried. + """ + for future in self._futures.values(): + # Any incomplete future means the flow is not complete + if not future.done(): + return False + + # Also several types of exceptions indicate incomplete work, as they can be + # retried on flow restart. + exception = future.exception() + if exception is not None: + if isinstance(exception, (ConnectionError, EOFError)): + # Communication errors - clangd crashed, let's retry + return False + if isinstance(exception, asyncio.CancelledError): + # Cancelled futures were cancelled as part of flow reset, retry + return False + return True + + async def run(self) -> None: + """ + Runs the flow. This method can be overridden by subclasses to + perform the main logic of the flow. + """ + pass + + def _create_future(self, key: Any) -> asyncio.Future[Any]: + """ + Creates a new future that is managed by the flow. + + Use this method to create futures for work that should be cancelled if + the flow is reset (e.g., due to a clangd crash). + + Args: + - key: A unique identifier for the future. + + Returns: + The created future. + + Raises: + KeyError if a future with the same key already exists. + """ + if key in self._futures: + raise KeyError(f"Future with key {key} already tracked") + future: asyncio.Future[Any] = asyncio.Future() + self._futures[key] = future + return future + + def _resolve_future( + self, key: Any, result: Any, ignore_duplicates: bool = False + ) -> None: + """ + Resolves a future tied to this flow's lifecycle. + + Args: + - key: The unique identifier for the future. + - result: The result to set on the future. + + Raises: + KeyError if a future with the specified key does not exist. + asyncio.InvalidStateError if the future is already done. + """ + if key not in self._futures: + raise KeyError(f"Future with key {key} not found") + + self._futures[key].set_result(result) + + def _spawn_task( + self, key: Any, coro: Coroutine[Any, Any, T], limited: bool = True + ) -> asyncio.Future[T]: + """ + Spawns a new asyncio task tied to this flow's lifecycle. + + It automatically creates a future to track the task's completion and resolves + it when the task completes. + + Args: + - key: A unique identifier for the task. + - coro: The coroutine to run in the new task. + - limited: If True, the task will acquire a semaphore from the clangd client + before running, limiting the number of concurrent tasks. + + Returns: + The new task. + + Raises: + KeyError if a task with the same key already exists. + """ + if key in self._futures: + raise KeyError(f"Task with key {key} already spawned") + + async def run_with_semaphore() -> T: + async with self._clangd.get_semaphore(): + return await coro + + if limited: + task = asyncio.create_task(run_with_semaphore()) + else: + task = asyncio.create_task(coro) + self._futures[key] = task + return task + + async def _check_futures_purge_pending(self, log_errors: bool = True): + """ + Checks all futures spawned by this flow and removes any that are pending. + + Logs an error for any futures that had an exception (other than CancelledError). + """ + for key in list(self._futures.keys()): + future = self._futures[key] + future.cancel() # Does nothing for already completed futures + try: + # Await it to ensure any exception is propagated + await future + except asyncio.CancelledError: + # If the future was cancelled, remove the future from tracking to let it + # be re-created in case of flow restart. We only persist completed + # futures. + del self._futures[key] + except (ConnectionError, EOFError): + # Similarly, futures that failed due to connection issues are likely + # transient errors that will be fixed on flow restart. Clangd closes + # the connection on crash, so we don't want to persist these errors. + del self._futures[key] + except Exception as e: + if log_errors: + logging.exception( + f"Future with key {key} failed in {self.__class__.__name__}: {e}", + exc_info=e, + ) + # Otherwise silently ignore the error. This is done during + # bootstrap to avoid logging errors for tasks that will be + # retried anyway. diff --git a/clangd_tidy/flows/diagnostics.py b/clangd_tidy/flows/diagnostics.py new file mode 100644 index 0000000..473b53b --- /dev/null +++ b/clangd_tidy/flows/diagnostics.py @@ -0,0 +1,126 @@ +import asyncio +import logging +import pathlib +from typing import Collection, List + +from .base import BaseFlow + +from ..event_bus import EventBus +from ..lsp import ClangdAsync +from ..lsp.messages import ( + Diagnostic, + PublishDiagnosticsNotificationMessage, + uri_to_path, +) + + +class DiagnosticsFlow(BaseFlow): + """ + Manages diagnostics collection for multiple files via clangd. + + This flow orchestrates diagnostic gathering by: + - Opening files with textDocument/didOpen notifications + - Collecting textDocument/publishDiagnostics notifications from clangd + - Tracking completion state per file using futures + + Flow diagram: + ┌────────────────────────────────────────┬──────────────────────────────────┐ + │ Main Flow (run()) │ Event Handler (async) │ + ├────────────────────────────────────────┼──────────────────────────────────┤ + │ 1. spawn_task per file │ │ + │ 2. create_future((file, 'diagnostics'))│ │ + │ 3. send didOpen → clangd │ │ + │ 4. await diagnostics future (blocked) │ ← clangd sends publishDiagnostics│ + │ │ → _publish_diagnostics_cb() │ + │ │ → resolve_future() (unblocks) │ + │ 5. return diagnostics │ │ + └────────────────────────────────────────┴──────────────────────────────────┘ + + Note: When clangd restarts after a crash, files are reopened and new + diagnostics are received. The flow handles these duplicates gracefully. + """ + + def __init__( + self, + clangd_client: ClangdAsync, + event_bus: EventBus, + files: Collection[pathlib.Path], + ): + super().__init__(clangd_client, event_bus) + self._files = set(files) + + self._event_bus.subscribe( + PublishDiagnosticsNotificationMessage, self._publish_diagnostics_cb + ) + + async def run(self) -> None: + logging.debug(f"Spawning diagnostics flows for {len(self._files)} files") + for path in self._files: + if path in self._futures and self._futures[path].done(): + # Skip already completed diagnostics flows + continue + self._spawn_task(path, self._open_and_get_diagnostics(path)) + + async def _open_and_get_diagnostics(self, path: pathlib.Path) -> List[Diagnostic]: + """ + Get the diagnostics for a single file. + + 1. --> textDocument/didOpen (Notify `clangd` that the file is open.) + 2. --- Wait for `clangd` to publish diagnostics for the file. + """ + try: + # Create or re-use the diagnostics future for this file. We always want to + # open the file, but we might be able to re-use an existing diagnostics. + if (path, "diagnostics") not in self._futures: + diagnostics_future: asyncio.Future[List[Diagnostic]] = ( + self._create_future((path, "diagnostics")) + ) + else: + diagnostics_future = self._futures[(path, "diagnostics")] + await self._clangd.ensure_file_opened(path) + diagnostics = await diagnostics_future + return diagnostics + except (ConnectionError, EOFError) as e: + logging.warning( + f"Diagnostics failed for '{path}' due to connection error and will be retried: {e}" + ) + raise + except Exception as e: + logging.warning(f"Diagnostics failed for '{path}': {e}", exc_info=True) + raise + + async def _publish_diagnostics_cb( + self, + message: PublishDiagnosticsNotificationMessage, + ): + """ + Processes a PublishDiagnostics notification from `clangd`. + + This simply resolves the corresponding Future with the received diagnostics, + unblocking the `_get_file_diagnostics` method. + """ + path = uri_to_path(message.params.uri) + logging.debug( + f"Received {len(message.params.diagnostics)} diagnostics for {path}" + ) + # Set the internal future to unblock _get_file_diagnostics. + # We also receive diagnostics for files we don't track (such as .clangd), + # so we skip them. + if path in self._files: + self._resolve_future((path, "diagnostics"), message.params.diagnostics) + + def get_diagnostics(self) -> dict[pathlib.Path, List[Diagnostic]]: + """Get all collected diagnostics from completed file flows.""" + results: dict[pathlib.Path, List[Diagnostic]] = {} + for key, future in self._futures.items(): + # From all the futures, extract only the diagnostics futures and return their diagnostics + match key: + case (pathlib.Path() as path, "diagnostics"): + if not future.done(): + raise RuntimeError( + f"Diagnostics for '{path}' are not yet complete" + ) + results[path] = future.result() + case _: + continue + return results diff --git a/clangd_tidy/flows/fix.py b/clangd_tidy/flows/fix.py new file mode 100644 index 0000000..ea4bd4f --- /dev/null +++ b/clangd_tidy/flows/fix.py @@ -0,0 +1,381 @@ +import asyncio +from dataclasses import dataclass +import logging +import pathlib +from typing import Any, Collection, Dict, List, Tuple + +from .diagnostics import DiagnosticsFlow + +from ..event_bus import EventBus +from ..lsp import ClangdAsync +from ..lsp.messages import ( + ApplyWorkspaceEditRequest, + ClientCapabilities, + CodeAction, + CodeActionClientCapabilities, + Diagnostic, + TextDocumentClientCapabilities, + WorkspaceClientCapabilities, + WorkspaceEdit, +) +from ..progress import IndexingProgressTracker + +FixitKey = Tuple[pathlib.Path, str, int, int, int, int] + + +@dataclass +class RejectedEdit: + """Marker class indicating that a workspace edit was rejected and should not be applied.""" + + error: str + + +class DiagnosticsWithFixesFlow(DiagnosticsFlow): + """ + Extends DiagnosticsFlow to automatically apply fixes for all diagnostics. + + This flow orchestrates the complete fix application process by: + - Collecting diagnostics for each file (inherited from DiagnosticsFlow) + - Waiting for clangd background indexing to complete (critical for renames) + - Requesting code actions for each fixable diagnostic + - Executing fixes via direct edits or workspace commands + - Handling workspace/applyEdit server requests from clangd + + Flow diagram (per file): + ┌──────────────────────────────────────────────────────────────────────────┐ + │ File Task (limited concurrency) │ + ├──────────────────────────────────────────────────────────────────────────┤ + │ 1. Get diagnostics (inherited) │ + │ 2. Filter diagnostics with "(fix available)" in message │ + │ 3. Spawn fix task per fixable diagnostic (unlimited concurrency) │ + │ 4. Gather all fix results │ + │ 5. Return WorkspaceEdits │ + └──────────────────────────────────────────────────────────────────────────┘ + + Fix task flow (per diagnostic, runs concurrently): + ┌─────────────────────────────────┬────────────────────────────────────────┐ + │ Fix Task (_run_fix_flow) │ Event Handlers / Responses │ + ├─────────────────────────────────┼────────────────────────────────────────┤ + │ 1. Wait for indexing complete │ │ + │ 2. Request code action │ │ + │ → textDocument/codeAction │ ← Response with CodeAction[] │ + │ 3. Select best fix action │ │ + │ │ │ + │ Case A: Direct edit available │ │ + │ ──────────────────────────────────────────────────────────────────────── │ + │ 4a. Return edit immediately │ │ + │ │ │ + │ Case B: Command (e.g., rename) │ │ + │ ──────────────────────────────────────────────────────────────────────── │ + │ 4b. Execute command │ │ + │ → workspace/executeCommand │ ← Response (success/error) │ + │ 5b. Create future for edit │ │ + │ 6b. Await workspace edit │ ← workspace/applyEdit (server request) │ + │ (blocked) │ → _apply_workspace_edit_cb() │ + │ │ → Match range, resolve future │ + │ │ → Respond {"applied": true} │ + │ 7b. Return edit (unblocked) │ │ + └─────────────────────────────────┴────────────────────────────────────────┘ + + Concurrency notes: + - File tasks are limited by semaphore to avoid overwhelming clangd + - Fix tasks within a file run with unlimited concurrency to prevent deadlock + - Commands may trigger cross-file edits (e.g., rename across headers) + + Future keys: + - (path,): File-level fix processing task + - (path, "diagnostics"): Diagnostics collection (inherited) + - (path, diagnostic): Complete fix flow for this diagnostic + - (path, diagnostic, "command"): Command execution result (workspace edit) + + Known limitations: + - Cross-file fixes may be missed for files not in the compilation database. + Clangd can only provide cross-file fixes correctly for files it has indexed or + opened. Only files in the compilation database are guaranteed to be indexed + automatically. + This sometimes works if the file is opened directly, but we process files in + non-deterministic order, which can lead to inconsistent results. To avoid this, + ensure all source files are listed in the compilation database. + """ + + def __init__( + self, + clangd_client: ClangdAsync, + event_bus: EventBus, + files: Collection[pathlib.Path], + indexing_progress_tracker: IndexingProgressTracker, + ): + super().__init__(clangd_client, event_bus, files) + self._indexing_progress_tracker = indexing_progress_tracker + + # Pending commands mapped by request ID - used to correlate ApplyWorkspaceEdit + # requests to diagnostics + self._commands: Dict[ + int, + Tuple[ + asyncio.Future[WorkspaceEdit | RejectedEdit], Diagnostic, pathlib.Path + ], + ] = {} + + self._event_bus.subscribe( + ApplyWorkspaceEditRequest, self._apply_workspace_edit_cb + ) + + async def bootstrap(self): + # If we have not finished processing fixes for some files, we need to wait for + # their diagnostics again. For that reason we purge any existing diagnostics + # futures for files that have incomplete fix flows. + for key in list(self._futures.keys()): + if self._futures[key].done(): + continue + match key: + case pathlib.Path() as path, Diagnostic(): + self._futures.pop((path, "diagnostics"), None) + case _: + continue + # Also delete any _commands, they will be re-created as needed. They are tied + # to clangd state and must not persist across restarts. + self._commands.clear() + await super().bootstrap() + + async def run(self): + for path in self._files: + if path in self._futures and self._futures[path].done(): + # Skip already completed diagnostics flows + continue + self._spawn_task(path, self._process_file_fixes(path)) + + async def _process_file_fixes(self, path: pathlib.Path) -> List[WorkspaceEdit]: + """ + Orchestrates all fix-it operations for a single file, running them concurrently. + """ + # 1. Get diagnostics for the file. This also ensures the file is opened in + # clangd, which is necessary for workspace + diagnostics = await self._open_and_get_diagnostics(path) + + # Process all diagnostics with available fixes concurrently. + # Note that diagnostics can contain multiple fixes, in which case the suffix + # would be "(fixes available)". Since for these the user must pick one fix among + # several, we currently do not attempt to auto-apply them. + # We might have also received diag.codeActions for some diagnostics. However, + # these inline codeActions do not handle cross-file fixes, so we always request + # fresh code actions from clangd instead to ensure we get complete fixes. + diags_with_fixes = [ + diag for diag in diagnostics if "(fix available)" in diag.message + ] + + logging.debug( + f"Found {len(diags_with_fixes)} diagnostics with fixes for '{path}'." + ) + fix_tasks: List[asyncio.Future[WorkspaceEdit | RejectedEdit]] = [] + for diag in diags_with_fixes: + # Run fix flow for each diagnostic that has a fix + if ((path, diag) in self._futures) and self._futures[(path, diag)].done(): + continue + # Spawn a new fix task. We do not limit concurrency here as we are already + # limiting it at the file level. This avoids a possible deadlock when + # processing many files - if all file-level tasks were waiting for + # concurrency slots, no fix tasks could proceed to complete them. + task: asyncio.Future[WorkspaceEdit | RejectedEdit] = self._spawn_task( + (path, diag), self._run_fix_flow(path, diag), limited=False + ) + fix_tasks.append(task) + + if fix_tasks: + edits = await asyncio.gather(*fix_tasks, return_exceptions=True) + # Filter out exceptions and rejected edits and log them + valid_edits: List[WorkspaceEdit] = [] + for edit in edits: + if isinstance(edit, WorkspaceEdit): + valid_edits.append(edit) + elif isinstance(edit, RejectedEdit): + pass + elif isinstance(edit, Exception): + logging.exception( + f"Unexpected error during fix application: {edit}", + exc_info=edit, + ) + elif isinstance(edit, asyncio.CancelledError): + logging.warning( + f"Fix application was cancelled and will be retried: {edit}", + ) + else: + raise RuntimeError( + f"Unexpected result type from fix task: {type(edit)}" + ) + return valid_edits + return [] + + async def _apply_workspace_edit_cb(self, request: ApplyWorkspaceEditRequest): + """ + Callback for the `workspace/applyEdit` request from the server. + + This method finds the command that triggered this edit request by matching + the file path and diagnostic range, and then resolves the corresponding + future with the edit. + """ + for _, (cmd_future, diag, path) in self._commands.items(): + assert request.params.edit.changes is not None + for edit in request.params.edit.changes.get(path.as_uri(), []): + if edit.range == diag.range: + cmd_future.set_result(request.params.edit) + logging.debug( + f" Added workspace edit for diagnostic: {diag.message}" + ) + await self._clangd.respond_to_server(request.id, {"applied": True}) + # We must respond to the request, otherwise clangd will hang. + break + else: + continue + break + else: + logging.warning( + "No matching execute command found for ApplyWorkspaceEditRequest." + ) + + async def _run_fix_flow( + self, path: pathlib.Path, diag: Diagnostic + ) -> WorkspaceEdit | RejectedEdit: + """Applies a single fix for a given diagnostic.""" + try: + await self._indexing_progress_tracker.wait_for_indexing_to_finish() + + logging.debug(f"Requesting code action for: {diag.message}") + code_action_response = await self._clangd.code_action( + path, + diag.range.start, + diag.range.end, + [diag], + ) + + # Check for error response + if code_action_response.error: + raise RuntimeError( + f"Code action request failed: {code_action_response.error.message}" + ) + + code_actions: List[CodeAction] = code_action_response.result + if not code_actions: + raise RuntimeError(f"No code actions returned for: {diag.message}") + + best_action = self._clangd.select_best_fix_action( + code_actions, diag.message + ) + + if best_action.edit: + logging.debug(f"Applied direct edit for: {diag.message}") + return best_action.edit + elif best_action.command: + if (path, diag, "command") in self._futures: + assert self._futures[(path, diag, "command")].done() + logging.debug( + f"Command {best_action.command.command} already executed for: {diag.message}, re-using result" + ) + return self._futures[(path, diag, "command")].result() + + cmd_future: asyncio.Future[WorkspaceEdit | RejectedEdit] = ( + self._create_future((path, diag, "command")) + ) + + def execute_id_callback(request_id: int): + # Register the command future to be resolved when the + # ApplyWorkspaceEdit request arrives. The response to + # execute_command only arrives after we process + # _apply_workspace_edit_cb and execute_command blocks until then, so + # this ensures the future is resolved correctly. + self._commands[request_id] = (cmd_future, diag, path) + + response = await self._clangd.execute_command( + best_action.command.command, + best_action.command.arguments, + execute_id_callback, + ) + if response.error: + err = ( + f"Command execution failed for '{diag.message}'" + f": {response.error.message}" + ) + logging.error(err) + # We want to log the error but continue processing other fixes. This + # allows us to collect as many fixes as possible in one run, + # recovering from individual fix failures caused by clangd + # limitations. + cmd_future.set_result(RejectedEdit(error=response.error.message)) + + edit = await cmd_future + logging.debug(f"Command executed for: {diag.message}") + return edit + else: + raise RuntimeError( + f"No edit or command in selected action for: {diag.message}" + ) + except Exception as e: + logging.exception( + f"Fix failed for '{diag.message}' at {path}:{diag.range.start.line}: {e}" + ) + raise + + def get_edits(self) -> List[Tuple[Diagnostic, WorkspaceEdit]]: + """Get all collected edits from completed file flows.""" + results: List[Tuple[Diagnostic, WorkspaceEdit]] = [] + for key, future in self._futures.items(): + # From all the futures, extract only the fix command futures and return their diagnostics/edits + match key: + case pathlib.Path() as path, Diagnostic() as diag: + if not future.done(): + raise RuntimeError( + f"Fix for '{diag.message}' in '{path}' is not yet complete" + ) + result = future.result() + if isinstance(result, RejectedEdit): + continue + results.append((diag, result)) + case _: + continue + return results + + def add_required_capabilities( + self, capabilities: ClientCapabilities + ) -> ClientCapabilities: + capabilities.offsetEncoding = ["utf-8"] + capabilities.positionEncodings = ["utf-8"] + capabilities.textDocument = ( + capabilities.textDocument or TextDocumentClientCapabilities() + ) + capabilities.textDocument.codeAction = ( + capabilities.textDocument.codeAction or CodeActionClientCapabilities() + ) + capabilities.workspace = capabilities.workspace or WorkspaceClientCapabilities() + capabilities.workspace.applyEdit = True + return capabilities + + def check_clangd_capabilities(self, init_result: Any) -> None: + # Verify server uses UTF-8 encoding (required for our byte offset calculations) + # Check both LSP 3.17 standard field and deprecated clangd extension field + capabilities = init_result.get("capabilities", {}) + encoding = capabilities.get("positionEncoding") or init_result.get( + "offsetEncoding" + ) + if encoding != "utf-8": + raise RuntimeError( + f"clangd did not accept UTF-8 encoding (got: {encoding!r}). " + f"This tool requires UTF-8 for correct byte offset handling. " + f"Please use clangd version 9+ which supports UTF-8 offsets." + ) + + # Check that server confirmes support for 'clangd.applyRename' command and + # code actions + commands = capabilities.get("executeCommandProvider", {}).get("commands", []) + if "clangd.applyRename" not in commands: + raise RuntimeError( + f"clangd does not support 'clangd.applyRename' command. " + f"This tool requires this command to apply fixes. " + ) + code_action_kinds = capabilities.get("codeActionProvider", {}).get( + "codeActionKinds", [] + ) + if not code_action_kinds: + raise RuntimeError( + f"clangd does not support any code actions. " + f"This tool requires code actions to apply fixes. " + ) diff --git a/clangd_tidy/flows/formatting.py b/clangd_tidy/flows/formatting.py new file mode 100644 index 0000000..012b401 --- /dev/null +++ b/clangd_tidy/flows/formatting.py @@ -0,0 +1,88 @@ +import logging +import pathlib +from typing import Collection, List + +from .base import BaseFlow + +from ..diagnostic_formatter import DiagnosticCollection +from ..event_bus import EventBus +from ..lsp import ClangdAsync +from ..lsp.messages import ( + Diagnostic, + Position, + Range, +) + + +class FormattingFlow(BaseFlow): + """ + Manages the formatting of a collection of files. + + This class orchestrates the entire formatting process, including: + - Spawning formatting flows for each file. + - Running `clang-format` to get formatting diagnostics. + + Simplified flow description: + ┌─────────────────────────────────┐ + │ Main Flow (run()) │ + ├─────────────────────────────────┤ + │ 1. spawn_task per file │ + │ 2. _run_formatting_flow(file) │ + │ 3. send textDocument/formatting │ + │ 4. await response │ + │ 5. convert TextEdits to │ + │ Diagnostics │ + │ 6. return diagnostics │ + └─────────────────────────────────┘ + """ + + def __init__( + self, + clangd_client: ClangdAsync, + event_bus: EventBus, + files: Collection[pathlib.Path], + ): + super().__init__(clangd_client, event_bus) + self._files = set(files) + + async def _run_formatting_flow(self, path: pathlib.Path) -> List[Diagnostic]: + """ + Runs clang-format on a file and returns a list of formatting diagnostics. + + 1. --> textDocument/formatting (Request formatting for the file.) + 2. <-- response (response.result contains a list of `TextEdit` objects.) + 3. Convert `TextEdit` objects to `Diagnostic` objects. + """ + try: + response = await self._clangd.formatting(path) + if response.result: + return [ + Diagnostic( + range=Range(start=Position(0, 0), end=Position(0, 0)), + message="File does not conform to the formatting rules (run `clang-format` to fix)", + source="clang-format", + ) + ] + else: + return [] + except Exception as e: + logging.warning(f"Formatting failed for '{path}': {e}", exc_info=True) + raise + + async def run(self) -> None: + for file_path in self._files: + if file_path in self._futures and self._futures[file_path].done(): + # Skip already completed formatting flows + continue + self._spawn_task(file_path, self._run_formatting_flow(file_path)) + + def get_formatting_diagnostics(self) -> DiagnosticCollection: + """Get all collected formatting diagnostics from completed file flows.""" + diagnostics: DiagnosticCollection = {} + for path, future in self._futures.items(): + if not future.done(): + raise RuntimeError(f"Formatting for '{path}' is not yet complete") + file_diagnostics = future.result() + if file_diagnostics: + diagnostics[path] = file_diagnostics + return diagnostics diff --git a/clangd_tidy/line_filter.py b/clangd_tidy/line_filter.py index 7f1e6e3..321c380 100644 --- a/clangd_tidy/line_filter.py +++ b/clangd_tidy/line_filter.py @@ -42,7 +42,7 @@ class FileLineFilter: File path """ - lines: List[LineRange] = Factory(list) + lines: List[LineRange] = Factory(list[LineRange]) """ List of inclusive line ranges where diagnostics will be emitted diff --git a/clangd_tidy/lsp/__init__.py b/clangd_tidy/lsp/__init__.py index effdea4..2ba02de 100644 --- a/clangd_tidy/lsp/__init__.py +++ b/clangd_tidy/lsp/__init__.py @@ -1,5 +1,4 @@ from .clangd import ClangdAsync -from .client import RequestResponsePair from . import messages -__all__ = ["ClangdAsync", "RequestResponsePair", "messages"] +__all__ = ["ClangdAsync", "messages"] diff --git a/clangd_tidy/lsp/clangd.py b/clangd_tidy/lsp/clangd.py index 347999d..416055d 100644 --- a/clangd_tidy/lsp/clangd.py +++ b/clangd_tidy/lsp/clangd.py @@ -2,17 +2,29 @@ import os import pathlib import sys -from typing import Union +import logging +from typing import Callable, Optional, Set, Any, List -from .client import ClientAsync, RequestResponsePair +import cattrs + +from ..event_bus import EventBus +from .client import ClientAsync from .messages import ( + ClientCapabilities, + CodeAction, + CodeActionContext, + CodeActionParams, + Diagnostic, DidOpenTextDocumentParams, DocumentFormattingParams, + ExecuteCommandParams, InitializeParams, LanguageId, - LspNotificationMessage, NotificationMethod, + Position, + Range, RequestMethod, + ResponseMessage, TextDocumentIdentifier, TextDocumentItem, WorkspaceFolder, @@ -21,16 +33,37 @@ __all__ = ["ClangdAsync"] +converter = cattrs.Converter() + class ClangdAsync: + """ + An LSP client for clangd that manages the complexities of the Language + Server Protocol, including asynchronous message handling, state tracking, + and crash recovery. + + This class is responsible for: + - Starting and stopping the clangd process. + - Initializing the LSP session and advertising client capabilities. + - Sending requests and notifications to the server. + - Receiving and dispatching responses and notifications from the server. + - Tracking the state of in-flight requests (diagnostics, code actions, etc.). + - Handling background indexing progress notifications. + - Orchestrating complex, multi-step interactions like applying a fix. + - Recovering from clangd crashes by restarting the server and retrying + incomplete operations. + """ + def __init__( self, clangd_executable: str, + event_bus: EventBus, *, compile_commands_dir: str, jobs: int, verbose: bool, query_driver: str, + use_background_index: bool, ): self._clangd_cmd = [ clangd_executable, @@ -40,11 +73,30 @@ def __init__( "--pch-storage=memory", "--enable-config", ] + if use_background_index: + self._clangd_cmd.append("--background-index") if query_driver: self._clangd_cmd.append(f"--query-driver={query_driver}") + if verbose: + self._clangd_cmd.append("-log=verbose") self._stderr = sys.stderr if verbose else open(os.devnull, "w") + self._event_bus = event_bus + + max_pending_requests = jobs * 2 + self._semaphore = asyncio.Semaphore(max_pending_requests) + + # --- Runtime State --- + # These attributes are reset every time clangd is (re)started. + + # The clangd subprocess. + self._process: Optional[asyncio.subprocess.Process] = None + # The low-level LSP client for sending/receiving messages. + self._client: ClientAsync + # The set of files that have been opened in the LSP session. + self._opened_files: Set[pathlib.Path] = set() async def start(self) -> None: + self._opened_files.clear() self._process = await asyncio.create_subprocess_exec( *self._clangd_cmd, stdin=asyncio.subprocess.PIPE, @@ -55,28 +107,42 @@ async def start(self) -> None: rpc = RpcEndpointAsync(self._process.stdout, self._process.stdin) self._client = ClientAsync(rpc) - async def recv_response_or_notification( - self, - ) -> Union[RequestResponsePair, LspNotificationMessage]: - return await self._client.recv() + async def recv_and_publish(self) -> None: + """Wait for a message from the server and publish it to the event bus.""" + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") + resp = await self._client.recv() + self._event_bus.publish(resp) - async def initialize(self, root: pathlib.Path) -> None: + async def initialize( + self, root: pathlib.Path, capabilities: ClientCapabilities + ) -> ResponseMessage: assert root.is_dir() - await self._client.request( + + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") + if not self._process: + raise RuntimeError("LSP process is not initialized. Call start() first.") + return await self._client.request_and_await_response( RequestMethod.INITIALIZE, InitializeParams( processId=self._process.pid, workspaceFolders=[ - WorkspaceFolder(name="foo", uri=root.as_uri()), + WorkspaceFolder(name=root.name, uri=root.as_uri()), ], + capabilities=capabilities, ), ) async def initialized(self) -> None: + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") await self._client.notify(NotificationMethod.INITIALIZED) - async def did_open(self, path: pathlib.Path) -> None: + async def _did_open(self, path: pathlib.Path) -> None: assert path.is_file() + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") await self._client.notify( NotificationMethod.DID_OPEN, DidOpenTextDocumentParams( @@ -88,22 +154,132 @@ async def did_open(self, path: pathlib.Path) -> None: ) ), ) + self._opened_files.add(path) - async def formatting(self, path: pathlib.Path) -> None: - assert path.is_file() - await self._client.request( + async def ensure_file_opened(self, file: pathlib.Path) -> None: + """Ensure a file is opened. Opens it if not already opened.""" + if file not in self._opened_files: + await self._did_open(file) + + async def respond_to_server(self, request_id: int, result: Any) -> None: + """Respond to a server request.""" + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") + await self._client.respond(request_id, result) + + def select_best_fix_action( + self, code_actions: list[CodeAction], diag_message: str + ) -> CodeAction: + """Select fix action for a diagnostic, filtering out unrelated tweaks. + + When requesting code actions for a diagnostic, clangd uses two mechanisms: + 1. It provides fix actions related to the diagnostic in the context.diagnostics + field of the CodeActionParams. + 2. It also provides generic tweaks based on the range/cursor position. + We only want the first kind, so we filter out the second. We also ensure exactly + one fix action remains after filtering, as the alternative means we haven't + identified the correct fix for the diagnostic. + + Raises RuntimeError if 0 or >1 fix actions remain after filtering. + """ + logging.debug(f"Got {len(code_actions)} code actions for: {diag_message}") + + # Filter out clangd.applyTweak actions (generic tweaks unrelated to diagnostics) + fix_actions = [ + action + for action in code_actions + if not (action.command and action.command.command == "clangd.applyTweak") + ] + + if len(fix_actions) == 0: + raise RuntimeError( + f"Expected exactly 1 fix action, got 0 for: {diag_message}" + ) + + if len(fix_actions) > 1: + titles = [a.title for a in fix_actions] + raise RuntimeError( + f"Expected exactly 1 fix action, got {len(fix_actions)} for: {diag_message}\n" + f"Actions: {titles}" + ) + + logging.debug(f"Selected: {fix_actions[0].title}") + return fix_actions[0] + + async def formatting(self, file: pathlib.Path) -> ResponseMessage: + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") + return await self._client.request_and_await_response( RequestMethod.FORMATTING, DocumentFormattingParams( - textDocument=TextDocumentIdentifier(uri=path.as_uri()), options={} + textDocument=TextDocumentIdentifier(uri=file.as_uri()) ), ) + async def code_action( + self, + file: pathlib.Path, + range_start: Position, + range_end: Position, + diagnostics: list[Diagnostic], + ) -> ResponseMessage: + """Request code actions for a specific range with diagnostics. + + Awaits the response and returns the response message. + """ + params = CodeActionParams( + textDocument=TextDocumentIdentifier(uri=file.as_uri()), + range=Range(start=range_start, end=range_end), + context=CodeActionContext(diagnostics=diagnostics), + ) + + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") + response = await self._client.request_and_await_response( + RequestMethod.CODE_ACTION, params + ) + if response.result is not None: + response.result = cattrs.structure(response.result, List[CodeAction]) + return response + + async def execute_command( + self, + command: str, + arguments: List[Any], + request_id_cb: Optional[Callable[[int], Any]] = None, + ) -> ResponseMessage: + """ + Execute a workspace command and wait for the response. + + For complex tasks that require the request ID before getting the response, + a callback can be provided to receive the request ID when the request is sent. + + Returns the response message from the server. + """ + params = ExecuteCommandParams( + command=command, + arguments=arguments, + ) + + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") + return await self._client.request_and_await_response( + RequestMethod.EXECUTE_COMMAND, params, request_id_cb=request_id_cb + ) + async def shutdown(self) -> None: + + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") await self._client.request(RequestMethod.SHUTDOWN) async def exit(self) -> None: + if not self._client: + raise RuntimeError("LSP client is not initialized. Call start() first.") + if not self._process: + raise RuntimeError("LSP process is not initialized. Call start() first.") await self._client.notify(NotificationMethod.EXIT) - self._process.kill() # PERF: much faster than waiting for clangd to exit + self._process.kill() # Fastest way to ensure process is terminated await self._process.wait() # HACK: prevent RuntimeError('Event loop is closed') before Python 3.11 @@ -111,4 +287,10 @@ async def exit(self) -> None: if sys.version_info < (3, 11): self._process._transport.close() # type: ignore - self._stderr.close() + await self._client.shutdown() + + if self._stderr is not sys.stderr: + self._stderr.close() + + def get_semaphore(self) -> asyncio.Semaphore: + return self._semaphore diff --git a/clangd_tidy/lsp/client.py b/clangd_tidy/lsp/client.py index 69875c7..993ad18 100644 --- a/clangd_tidy/lsp/client.py +++ b/clangd_tidy/lsp/client.py @@ -1,56 +1,137 @@ -from dataclasses import dataclass +import asyncio import itertools -from typing import Dict, Union +import logging +from typing import Callable, Dict, Optional, Any, cast import cattrs from .messages import ( - LspNotificationMessage, + AbstractNotificationMessage, + AbstractResponseMessage, + LspMessage, NotificationMethod, Params, RequestMessage, - ResponseMessage, RequestMethod, ) from .rpc import RpcEndpointAsync -__all__ = ["ClientAsync", "RequestResponsePair"] +__all__ = ["ClientAsync"] -@dataclass -class RequestResponsePair: - request: RequestMessage - response: ResponseMessage +class ClientAsync: + """A pure LSP protocol client. + Handles low-level protocol concerns: + - Request/response correlation + - Message serialization/deserialization + - Protocol-level send/recv + + Does NOT handle application-level concerns like progress tracking + or workspace edit correlation - those belong in higher layers. + """ -class ClientAsync: def __init__(self, rpc: RpcEndpointAsync): self._rpc = rpc self._id = itertools.count() - self._requests: Dict[int, RequestMessage] = {} + self._logger = logging.getLogger(__name__) + self._pending_requests: Dict[int, asyncio.Future[AbstractResponseMessage]] = {} - async def request(self, method: RequestMethod, params: Params = Params()) -> None: + async def request(self, method: RequestMethod, params: Params = Params()) -> int: + """ + Send a request and return the request ID. + + The caller can use this ID to correlate the request with a response + received later. However, for most cases, it is easier to use + `request_and_await_response`. + """ id = next(self._id) message = RequestMessage( id=id, method=method, params=cattrs.unstructure(params) ) - self._requests[id] = message await self._rpc.send(cattrs.unstructure(message)) + return id + + async def request_and_await_response( + self, + method: RequestMethod, + params: Params = Params(), + request_id_cb: Optional[Callable[[int], Any]] = None, + ) -> AbstractResponseMessage: + """Send a request and wait for the corresponding response. + + Optionally calls request_id_cb with the request ID once the request is sent. + Args: + method: The LSP request method to send. + params: The parameters for the request. + request_id_cb: Optional callback to receive the request ID. + Returns: + The result from the response message. + """ + + request_id = await self.request(method, params) + + request_future: asyncio.Future[AbstractResponseMessage] = ( + asyncio.get_event_loop().create_future() + ) + self._pending_requests[request_id] = request_future + + if request_id_cb is not None: + request_id_cb(request_id) + return await request_future async def notify( self, method: NotificationMethod, params: Params = Params() ) -> None: - message = LspNotificationMessage( + message = AbstractNotificationMessage( method=method, params=cattrs.unstructure(params) ) await self._rpc.send(cattrs.unstructure(message)) - async def recv(self) -> Union[RequestResponsePair, LspNotificationMessage]: + async def respond(self, id: int, result: Any = None) -> None: + """Send a response to a server request.""" + response = AbstractResponseMessage(id=id, result=result) + await self._rpc.send(cattrs.unstructure(response)) + + async def shutdown(self) -> None: + """Clean up the client, cancelling any pending requests.""" + for future in self._pending_requests.values(): + future.cancel() + try: + await future + except asyncio.CancelledError: + pass + self._pending_requests.clear() + + async def recv( + self, + ) -> LspMessage: + """Receive and parse the next message from the server. + + LSP is bidirectional: both client and server can send requests/notifications. + + Returns: + LspMessage: The received message, which can be one of: + - ResponseMessage: Response to our request (we sent request, got response) + - NotificationMessage: Notification from server (has method, no id) + - AbstractServerRequest: Request from server (has method AND id, needs + response) + + Message classification: + - has "id" but no "method": ResponseMessage + - has "method" and "id": AbstractServerRequest (needs response) + - has "method" but no "id": NotificationMessage (no response) + """ content = await self._rpc.recv() - if "method" in content: - return cattrs.structure(content, LspNotificationMessage) - else: - resp = cattrs.structure(content, ResponseMessage) - req = self._requests.pop(resp.id) - return RequestResponsePair(request=req, response=resp) + self._logger.debug(f"Received message: {content}") + + message: LspMessage = cast(LspMessage, cattrs.structure(content, LspMessage)) # type: ignore[arg-type] + + if isinstance(message, AbstractResponseMessage): + # Pair the response with its request if we track it + pending_request = self._pending_requests.pop(message.id, None) + if pending_request is not None: + pending_request.set_result(message) + + return message diff --git a/clangd_tidy/lsp/messages.py b/clangd_tidy/lsp/messages.py index d5ca032..ef3e55a 100644 --- a/clangd_tidy/lsp/messages.py +++ b/clangd_tidy/lsp/messages.py @@ -1,11 +1,32 @@ +""" +This module defines the data structures for the Language Server Protocol (LSP) +messages used in this project. + +The messages are defined using `attrs` classes and are structured as follows: +- Base classes like `Message`, `AbstractResponseMessage`, etc. +- Concrete message types for specific requests, responses, and notifications. +- `Union` types like `LspMessage` to represent any possible message. +- A `cattrs` structure hook to automatically deserialize messages into the + correct type. +""" + +from __future__ import annotations +from abc import ABC from enum import Enum, unique from functools import total_ordering -from typing import Any, Dict, List, Optional +import pathlib +from typing import Any, Dict, List, Optional, Tuple, Union, Literal +from urllib.parse import unquote, urlparse +import cattrs from attrs import Factory, define from typing_extensions import Self +def uri_to_path(uri: str) -> pathlib.Path: + return pathlib.Path(unquote(urlparse(uri).path)) + + @define class Message: jsonrpc: str = "2.0" @@ -16,6 +37,8 @@ class RequestMethod(Enum): INITIALIZE = "initialize" SHUTDOWN = "shutdown" FORMATTING = "textDocument/formatting" + CODE_ACTION = "textDocument/codeAction" + EXECUTE_COMMAND = "workspace/executeCommand" @unique @@ -23,7 +46,6 @@ class NotificationMethod(Enum): INITIALIZED = "initialized" EXIT = "exit" DID_OPEN = "textDocument/didOpen" - PUBLISH_DIAGNOSTICS = "textDocument/publishDiagnostics" @unique @@ -40,7 +62,7 @@ class Params: class RequestMessage(Message): id: int method: RequestMethod - params: Dict[str, Any] = Factory(dict) + params: Dict[str, Any] = Factory(dict[str, Any]) @define @@ -51,16 +73,98 @@ class ResponseError: @define(kw_only=True) -class ResponseMessage(Message): +class AbstractResponseMessage(Message): id: int result: Any = None error: Optional[ResponseError] = None @define(kw_only=True) -class LspNotificationMessage(Message): - method: NotificationMethod - params: Dict[str, Any] = Factory(dict) +class InitializeParams(Params): + processId: Optional[int] = None + rootUri: Optional[str] = None + initializationOptions: Any = None + capabilities: Optional[ClientCapabilities] = None + workspaceFolders: List[WorkspaceFolder] + + +@define(kw_only=True) +class InitializeResponseMessage(AbstractResponseMessage): + params: InitializeParams + + +@define +class AbstractServerRequest(ABC): + id: int + method: Any # Let the concrete subclasses define the method type + params: Any # Let the concrete subclasses define the params type + + +@define(kw_only=True) +class AbstractNotificationMessage(Message, ABC): + method: Any # Let the concrete subclasses define the method type + params: Any # Let the concrete subclasses define the params type + + +@define +class PublishDiagnosticsParams(Params): + uri: str + diagnostics: List[Diagnostic] + version: Optional[int] = None + + +@define(kw_only=True) +class PublishDiagnosticsNotificationMessage(AbstractNotificationMessage): + method: Literal["textDocument/publishDiagnostics"] = ( + "textDocument/publishDiagnostics" + ) + params: PublishDiagnosticsParams + + +@define +class ProgressParams: + token: str + value: WorkDoneProgressValue + + +@define(kw_only=True) +class ProgressNotificationMessage(AbstractNotificationMessage): + method: Literal["$/progress"] = "$/progress" + params: ProgressParams + + +@define +class ApplyWorkspaceEditParams: + edit: WorkspaceEdit + + +@define +class ApplyWorkspaceEditRequest(AbstractServerRequest): + params: ApplyWorkspaceEditParams + method: Literal["workspace/applyEdit"] = "workspace/applyEdit" + + +@define +class WorkDoneProgressCreateParams: + token: str + + +@define +class WorkDoneProgressCreateRequest(AbstractServerRequest): + params: WorkDoneProgressCreateParams + method: Literal["window/workDoneProgress/create"] = "window/workDoneProgress/create" + + +NotificationMessage = Union[ + PublishDiagnosticsNotificationMessage, + ProgressNotificationMessage, +] +ServerRequest = Union[ + WorkDoneProgressCreateRequest, + ApplyWorkspaceEditRequest, +] +ResponseMessage = Union[AbstractResponseMessage,] +LspMessage = Union[ResponseMessage, NotificationMessage, ServerRequest] @define @@ -70,12 +174,54 @@ class WorkspaceFolder: @define -class InitializeParams(Params): - processId: Optional[int] = None - rootUri: Optional[str] = None - initializationOptions: Any = None - capabilities: Any = None - workspaceFolders: List[WorkspaceFolder] = Factory(list) +class CodeActionKindValueSet: + valueSet: List[str] = Factory( + lambda: [ + "quickfix", + "refactor", + ] + ) + + +@define +class CodeActionLiteralSupport: + codeActionKind: CodeActionKindValueSet = Factory(CodeActionKindValueSet) + + +@define +class CodeActionClientCapabilities: + dynamicRegistration: bool = False + codeActionLiteralSupport: Optional[CodeActionLiteralSupport] = Factory( + CodeActionLiteralSupport + ) + isPreferredSupport: bool = True + disabledSupport: bool = True + + +@define +class TextDocumentClientCapabilities: + codeAction: Optional[CodeActionClientCapabilities] = None + + +@define +class WindowClientCapabilities: + workDoneProgress: bool = False + + +@define +class WorkspaceClientCapabilities: + applyEdit: bool = False + + +@define +class ClientCapabilities: + textDocument: Optional[TextDocumentClientCapabilities] = None + window: Optional[WindowClientCapabilities] = None + workspace: Optional[WorkspaceClientCapabilities] = None + # LSP 3.17 standard position encoding (preferred) + positionEncodings: Optional[List[str]] = None + # Deprecated clangd extension (for older clangd versions) + offsetEncoding: Optional[List[str]] = None @define @@ -91,13 +237,13 @@ class DidOpenTextDocumentParams(Params): textDocument: TextDocumentItem -@define +@define(frozen=True) class Position: line: int character: int -@define +@define(frozen=True) class Range: start: Position end: Position @@ -117,12 +263,12 @@ def __lt__(self, other: Self) -> bool: return NotImplemented -@define +@define(frozen=True) class CodeDescription: href: str -@define +@define(frozen=True) class Diagnostic: range: Range message: str @@ -130,17 +276,11 @@ class Diagnostic: code: Any = None codeDescription: Optional[CodeDescription] = None source: Optional[str] = None - tags: Optional[List[Any]] = None - relatedInformation: Optional[List[Any]] = None + tags: Optional[Tuple[Any, ...]] = None + relatedInformation: Optional[Tuple[Any, ...]] = None data: Any = None uri: Optional[str] = None # not in LSP spec, but clangd sends it - - -@define -class PublishDiagnosticsParams(Params): - uri: str - diagnostics: List[Diagnostic] - version: Optional[int] = None + codeActions: Optional[Tuple[CodeAction, ...]] = None @define @@ -156,4 +296,103 @@ class TextDocumentIdentifier: @define(kw_only=True) class DocumentFormattingParams(WorkDoneProgressParams): textDocument: TextDocumentIdentifier - options: Dict[str, Any] = Factory(dict) + options: Dict[str, Any] = Factory(dict[str, Any]) + + +@define +class TextEdit: + range: Range + newText: str + + +@define +class WorkspaceEdit: + changes: Optional[Dict[str, List[TextEdit]]] = None + + +@define +class Command: + title: str + arguments: List[Any] + command: str + + +@define +class CodeAction: + title: str + kind: Optional[str] = None + diagnostics: Optional[List[Diagnostic]] = None + edit: Optional[WorkspaceEdit] = None + command: Optional[Command] = None + isPreferred: Optional[bool] = None + disabled: Optional[Dict[str, str]] = None + + +@define +class CodeActionContext: + diagnostics: List[Diagnostic] + only: Optional[List[str]] = None + + +@define +class CodeActionParams(Params): + textDocument: TextDocumentIdentifier + range: Range + context: CodeActionContext + + +@define +class ExecuteCommandParams(Params): + command: str + arguments: List[Any] + + +# Work Done Progress structures for background indexing +@define +class WorkDoneProgressBegin: + kind: Literal["begin"] = "begin" + title: str = "" + cancellable: bool = False + message: Optional[str] = None + percentage: Optional[int] = None + + +@define +class WorkDoneProgressReport: + kind: Literal["report"] = "report" + cancellable: bool = False + message: Optional[str] = None + percentage: Optional[int] = None + + +@define +class WorkDoneProgressEnd: + kind: Literal["end"] = "end" + message: Optional[str] = None + + +WorkDoneProgressValue = Union[ + WorkDoneProgressBegin, WorkDoneProgressReport, WorkDoneProgressEnd +] + + +@cattrs.register_structure_hook +def disambiguate_lsp_message(obj: Dict[str, Any], type_: Any) -> LspMessage: + """ + A cattrs structure hook to deserialize a dictionary into the correct + LSP message type. + + This is necessary because LSP messages are ambiguous and can only be + differentiated by the presence of "id" and "method" fields, which is not + directly supported by cattrs. + """ + # We're using Unions which are supported by cattrs, but not handled by cattrs type + # annotations, so we disable the corresponding type checks. + if "method" in obj and "id" in obj: + return cattrs.structure(obj, ServerRequest) # type: ignore[arg-type] + elif "method" in obj: + return cattrs.structure(obj, NotificationMessage) # type: ignore[arg-type] + elif "id" in obj: + return cattrs.structure(obj, ResponseMessage) # type: ignore[arg-type] + else: + raise ValueError("Unknown LSP message type") diff --git a/clangd_tidy/lsp/rpc.py b/clangd_tidy/lsp/rpc.py index 24d61ee..534d9b0 100644 --- a/clangd_tidy/lsp/rpc.py +++ b/clangd_tidy/lsp/rpc.py @@ -63,7 +63,7 @@ async def send(self, data: Dict[str, Any]) -> None: async def recv(self) -> Dict[str, Any]: header = ProtocolHeader() while True: - header_line = await self._in_stream.readline() + header_line = await self._in_stream.readuntil() Protocol.parse_header(header_line, header) if header.complete: break diff --git a/clangd_tidy/main_cli.py b/clangd_tidy/main_cli.py index df2f1e5..b7f5538 100644 --- a/clangd_tidy/main_cli.py +++ b/clangd_tidy/main_cli.py @@ -1,39 +1,59 @@ #!/usr/bin/env python3 +import argparse import asyncio +import logging import pathlib import sys -from typing import Collection, List, Optional, TextIO +import tempfile +from typing import ( + Any, + Collection, + Dict, + List, + Optional, + TextIO, + Tuple, + Type, +) +from types import TracebackType from unittest.mock import MagicMock -from urllib.parse import unquote, urlparse + import cattrs +import yaml +from clangd_tidy.flows.base import BaseFlow + +from .flows.diagnostics import DiagnosticsFlow +from .flows.formatting import FormattingFlow from .args import SEVERITY_INT, parse_args +from .replacements import ( + create_replacements, +) +from .flows.fix import DiagnosticsWithFixesFlow from .diagnostic_formatter import ( CompactDiagnosticFormatter, DiagnosticCollection, FancyDiagnosticFormatter, GithubActionWorkflowCommandDiagnosticFormatter, ) -from .line_filter import LineFilter -from .lsp import ClangdAsync, RequestResponsePair +from .lsp import ClangdAsync from .lsp.messages import ( + ClientCapabilities, Diagnostic, - DocumentFormattingParams, - LspNotificationMessage, - NotificationMethod, - Position, - PublishDiagnosticsParams, - Range, - RequestMethod, + uri_to_path, + WindowClientCapabilities, + WorkspaceEdit, ) +from .progress import IndexingProgressTracker +from .event_bus import EventBus -__all__ = ["main_cli"] +__all__ = ["main_cli"] -def _uri_to_path(uri: str) -> pathlib.Path: - return pathlib.Path(unquote(urlparse(uri).path)) +# Cattrs converter +converter = cattrs.Converter() def _is_output_supports_color(output: TextIO) -> bool: @@ -54,131 +74,371 @@ def _try_import_tqdm(enabled: bool): return MagicMock() +class ClangdRecoverableError(Exception): + """Indicates a recoverable error in clangd operation, such as a crash.""" + + pass + + class ClangdRunner: + """ + Orchestrates the overall analysis process, managing the `ClangdAsync` + client and handling the high-level logic of fetching diagnostics, applying + fixes, and reporting results. + + This class is responsible for: + - Managing the lifecycle of the `ClangdAsync` client. + - Iterating over the files to be analyzed. + - Collecting all diagnostics and edits. + - Handling crash recovery at the application level. + - Applying fixes via `clang-apply-replacements` or exporting them. + - Formatting and printing diagnostics to the console. + """ + def __init__( self, clangd: ClangdAsync, files: Collection[pathlib.Path], run_format: bool, + clang_apply_replacements_executable: str, + run_fix: bool, + format_style: str, tqdm: bool, - max_pending_requests: int, + event_bus: EventBus, ): self._clangd = clangd self._files = files self._run_format = run_format + self._clang_apply_replacements_executable = clang_apply_replacements_executable + self._run_fix = run_fix + self._format_style = format_style self._tqdm = tqdm - self._max_pending_requests = max_pending_requests + self._system_tasks: dict[Any, asyncio.Task[None]] = ( + {} + ) # Tasks for internal systems + self._tasks: set[asyncio.Task[Any]] = set() # Tasks spawned by flows etc. + self._event_bus = event_bus - def acquire_diagnostics(self) -> DiagnosticCollection: - return asyncio.run(self._acquire_diagnostics()) + self._pbar = _try_import_tqdm(self._tqdm)( + total=len(self._files) * (2 if self._run_format else 1) + + (1 if self._run_fix else 0), + desc="Analyzing files", + unit="file", + ) - async def _request_diagnostics(self) -> None: - self._sem = asyncio.Semaphore(self._max_pending_requests) - for file in self._files: - await self._sem.acquire() - await self._clangd.did_open(file) - if self._run_format: - await self._sem.acquire() - await self._clangd.formatting(file) + self._flows: List[BaseFlow] = [] - async def _collect_diagnostics(self) -> DiagnosticCollection: - diagnostics: DiagnosticCollection = {} - formatting_diagnostics: DiagnosticCollection = ( - {} if self._run_format else {file: [] for file in self._files} + self._indexing_progress_tracker = IndexingProgressTracker( + self._clangd, self._event_bus + ) + self._flows.append(self._indexing_progress_tracker) + + self._diagnostics_flow = self._create_diagnostics_flow() + self._flows.append(self._diagnostics_flow) + + self._formatting_flow: Optional[FormattingFlow] = None + if self._run_format: + self._formatting_flow = FormattingFlow( + self._clangd, + self._event_bus, + self._files, + ) + self._flows.append(self._formatting_flow) + + def _create_diagnostics_flow(self) -> DiagnosticsFlow: + if self._run_fix: + return DiagnosticsWithFixesFlow( + self._clangd, + self._event_bus, + self._files, + self._indexing_progress_tracker, + ) + return DiagnosticsFlow( + self._clangd, + self._event_bus, + self._files, ) - nfiles = len(self._files) - tqdm = _try_import_tqdm(self._tqdm) - with tqdm( - total=nfiles, - desc="Collecting diagnostics", - ) as pbar: - while len(diagnostics) < nfiles or len(formatting_diagnostics) < nfiles: - resp = await self._clangd.recv_response_or_notification() - if isinstance(resp, LspNotificationMessage): - if resp.method == NotificationMethod.PUBLISH_DIAGNOSTICS: - params = cattrs.structure(resp.params, PublishDiagnosticsParams) - file = _uri_to_path(params.uri) - if file in self._files: - diagnostics[file] = params.diagnostics - tqdm.update(pbar) # type: ignore - self._sem.release() - else: - assert resp.request.method == RequestMethod.FORMATTING - assert resp.response.error is None, "Formatting failed" - params = cattrs.structure( - resp.request.params, DocumentFormattingParams - ) - file = _uri_to_path(params.textDocument.uri) - formatting_diagnostics[file] = ( - [ - Diagnostic( - range=Range(start=Position(0, 0), end=Position(0, 0)), - message="File does not conform to the formatting rules (run `clang-format` to fix)", - source="clang-format", - ) - ] - if resp.response.result - else [] - ) - self._sem.release() - return { - file: formatting_diagnostics[file] + diagnostics[file] - for file in self._files - } - async def _acquire_diagnostics(self) -> DiagnosticCollection: + def _required_capabilities(self) -> ClientCapabilities: + client_capabilities = ClientCapabilities( + window=WindowClientCapabilities(workDoneProgress=True), + offsetEncoding=["utf-8"], + positionEncodings=["utf-8"], + ) + + for flow in self._flows: + client_capabilities = flow.add_required_capabilities(client_capabilities) + return client_capabilities + + async def _bootstrap(self): + """ + Bootstraps the runner after a start/clangd restart. + """ + for flow in self._flows: + await flow.bootstrap() + await self._clangd.start() - await self._clangd.initialize(pathlib.Path.cwd()) - init_resp = await self._clangd.recv_response_or_notification() - assert isinstance(init_resp, RequestResponsePair) - assert init_resp.request.method == RequestMethod.INITIALIZE - assert init_resp.response.error is None, "Initialization failed" + if not "event_bus" in self._system_tasks: + # Event bus persists across restarts, so only start it once + self._system_tasks["event_bus"] = asyncio.create_task(self._event_bus.run()) + + # Start accepting and dispatching messages from clangd + self._system_tasks["message_processor"] = asyncio.create_task( + self._message_processor_task() + ) + + init_resp = await self._clangd.initialize( + pathlib.Path.cwd(), self._required_capabilities() + ) + assert init_resp.error is None, "Initialization failed" + + result: Dict[str, Any] = init_resp.result or {} + for flow in self._flows: + flow.check_clangd_capabilities(result) + await self._clangd.initialized() - _, file_diagnostics = await asyncio.gather( - self._request_diagnostics(), self._collect_diagnostics() + + for flow in self._flows: + await flow.run() + + async def _cleanup(self) -> None: + """Cleans up the runner by cancelling system tasks.""" + for system_task in self._system_tasks.values(): + system_task.cancel() + cleanup_results = await asyncio.gather( + *self._system_tasks.values(), return_exceptions=True ) - await self._clangd.shutdown() - await self._clangd.exit() - return file_diagnostics + for result in cleanup_results: + if isinstance(result, Exception): + logging.exception( + f"System task raised an exception during cleanup: {result}", + exc_info=result, + ) + self._system_tasks.clear() + async def __aenter__(self): + try: + await self._bootstrap() + return self + except Exception: + # Stop everything on exception + await self._cleanup() + raise -def main_cli(): - args = parse_args() + async def __aexit__( + self, + exc_type: Optional[Type[BaseException]], + exc_val: Optional[BaseException], + exc_tb: Optional[TracebackType], + ): + await self._cleanup() + for flow in self._flows: + await flow.shutdown() + await self.shutdown() - files: List[pathlib.Path] = args.filename - files = [ - file.resolve() for file in files if file.suffix[1:] in args.allow_extensions - ] - missing_files = [str(file) for file in files if not file.is_file()] - if missing_files: - print(f"File(s) not found: {', '.join(missing_files)}", file=sys.stderr) - sys.exit(1) - - file_diagnostics = ClangdRunner( - clangd=ClangdAsync( - args.clangd_executable, - compile_commands_dir=args.compile_commands_dir, - jobs=args.jobs, - verbose=args.verbose, - query_driver=args.query_driver, - ), - files=files, - run_format=args.format, - tqdm=args.tqdm, - max_pending_requests=args.jobs * 2, - ).acquire_diagnostics() + async def shutdown(self): + """Shutdown the clangd process.""" + logging.debug("Shutting down clangd...") + try: + await self._clangd.shutdown() + await self._clangd.exit() + logging.debug("Clangd shutdown complete") + except (ConnectionError, EOFError) as e: + # Already dead, that's fine + logging.debug(f"Clangd already terminated during shutdown: {e}") + pass + + async def export_fixes( + self, + edits: List[Tuple[Diagnostic, WorkspaceEdit]], + output_path: pathlib.Path, + ) -> None: + """Export the given edits to a YAML file.""" + all_replacements = create_replacements(edits) + if not all_replacements: + return + + main_source_file = "" + if edits: + # Find the first edit with changes to determine the main source file + for _, edit in edits: + if edit.changes: + first_uri = next(iter(edit.changes.keys()), None) + if first_uri: + main_source_file = str(uri_to_path(first_uri)) + break + + with open(output_path, "w") as f: + logging.debug(f"Exporting fixes to {output_path}") + fixes = yaml.dump( + { + "MainSourceFile": main_source_file, + "Diagnostics": converter.unstructure(all_replacements), + }, + ) + logging.debug(f"Fixes: \n{fixes}") + f.write(fixes) + + async def apply_fixes(self, edits: List[Tuple[Diagnostic, WorkspaceEdit]]) -> None: + """Apply the given edits to the files.""" + all_replacements = create_replacements(edits) + if not all_replacements: + return + + # Create a temporary directory to store the replacements file + with tempfile.TemporaryDirectory() as tmpdir: + await self.export_fixes(edits, pathlib.Path(tmpdir) / "fixes.yaml") + + # Build command arguments + cmd_args = [self._clang_apply_replacements_executable] + + # Add formatting flags if format_style is specified and not 'none' + if self._format_style and self._format_style.lower() != "none": + cmd_args.append("--format") + cmd_args.append(f"--style={self._format_style}") + + cmd_args.append(str(tmpdir)) # Pass the directory containing the YAML file + + logging.debug(f"Running: {' '.join(cmd_args)}") - line_filter: Optional[LineFilter] = args.line_filter - if line_filter is not None: + process = await asyncio.create_subprocess_exec( + *cmd_args, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + _, stderr = await process.communicate() + + if process.returncode != 0: + error_msg = f"clang-apply-replacements failed with return code {process.returncode}" + if stderr: + error_msg += f": {stderr.decode().strip()}" + logging.error(error_msg) + print(f"Error: {error_msg}", file=sys.stderr) + raise RuntimeError(error_msg) + + async def _message_processor_task( + self, + ) -> None: + """Background task that continuously processes messages from clangd.""" + try: + while True: + await self._clangd.recv_and_publish() + except (ConnectionError, EOFError) as e: + logging.error(f"Clangd crashed in message processor: {e}") + raise ClangdRecoverableError from e + except Exception as e: + logging.error(f"Unexpected error in message processor: {e}", exc_info=True) + raise + + async def collect_analysis( + self, + ) -> Tuple[DiagnosticCollection, List[Tuple[Diagnostic, WorkspaceEdit]]]: + + try: + while True: + flow_futures: set[asyncio.Future[Any]] = set() + for flow in self._flows: + flow_futures.update(flow.pending_futures()) + + if not flow_futures: + # If everything is done, exit the loop. + break + + await asyncio.wait( + [*self._system_tasks.values(), *flow_futures], + return_when=asyncio.FIRST_COMPLETED, + ) + + done_system_tasks = [ + system_task + for system_task in self._system_tasks.values() + if system_task.done() + ] + recovering = False + for system_task in done_system_tasks: + try: + system_task.result() + except ClangdRecoverableError: + logging.info("Recovering from clangd crash...") + try: + recovering |= await self._recover_from_clangd_crash() + except Exception as e: + logging.error( + f"Failed to recover from clangd crash: {e}", + exc_info=e, + ) + raise + except Exception as e: + logging.exception( + f"System task {system_task} failed: {e}", exc_info=e + ) + break # If we had any non-recoverable error, stop + else: + # We didn't break, meaning no non-recoverable error occurred + continue + if done_system_tasks and not recovering: + # Any system task finished cleanly - we should exit the loop + break + + except KeyboardInterrupt: + logging.warning("Analysis cancelled by user (Ctrl+C)") + + raise + except Exception as e: + logging.error(f"Unexpected error during analysis: {e}", exc_info=True) + raise + finally: + self._pbar.close() + + edits: List[Tuple[Diagnostic, WorkspaceEdit]] = ( + self._diagnostics_flow.get_edits() + if isinstance(self._diagnostics_flow, DiagnosticsWithFixesFlow) + else [] + ) + diagnostics: DiagnosticCollection = {} + if self._diagnostics_flow: + diagnostics.update(self._diagnostics_flow.get_diagnostics()) + if self._formatting_flow: + diagnostics.update(self._formatting_flow.get_formatting_diagnostics()) + return diagnostics, edits + + async def _recover_from_clangd_crash(self) -> bool: + """Handles recovery from a clangd crash. + + Returns: + True if recovery was attempted, False if not.""" + if any(not flow.completed() for flow in self._flows): + logging.warning(f"Clangd crashed with pending work - attempting recovery") + await self.shutdown() # Ensure we do a proper cleanup + await self._bootstrap() # Restart everything. This doesn't touch state that + # needs to be preserved, such as failed fixes. + return True + else: + logging.info("Clangd crashed but no pending work - exiting") + return False + + +def _print_diagnostics( + file_diagnostics: DiagnosticCollection, args: argparse.Namespace +) -> None: + """Print diagnostics to output. + + Args: + file_diagnostics: Dictionary mapping file paths to lists of diagnostics + args: Parsed command-line arguments + """ + # Apply line filter if specified + if args.line_filter is not None: file_diagnostics = { file: [ diagnostic for diagnostic in diagnostics - if line_filter.passes_line_filter(file, diagnostic) + if args.line_filter.passes_line_filter(file, diagnostic) ] for file, diagnostics in file_diagnostics.items() } + # Format diagnostics formatter = ( FancyDiagnosticFormatter( extra_context=args.context, @@ -191,23 +451,117 @@ def main_cli(): if not args.compact else CompactDiagnosticFormatter() ) - print(formatter.format(file_diagnostics), file=args.output) + + # Print formatted diagnostics + formatted_diagnostics = formatter.format(file_diagnostics) + print(formatted_diagnostics, file=args.output) + + # Print GitHub Actions format if requested if args.github: - print( - GithubActionWorkflowCommandDiagnosticFormatter(args.git_root).format( - file_diagnostics - ), - file=args.output, - ) - if any( - any( - ( - diagnostic.severity - and diagnostic.severity <= SEVERITY_INT[args.fail_on_severity] - ) - or diagnostic.source == "clang-format" - for diagnostic in diagnostics + github_formatted_diagnostics = GithubActionWorkflowCommandDiagnosticFormatter( + args.git_root + ).format(file_diagnostics) + print(github_formatted_diagnostics, file=args.output) + + +def main_cli() -> int: + """The main entry point for the CLI.""" + return asyncio.run(main_cli_async()) + + +async def main_cli_async() -> int: + """The main entry point for the CLI.""" + args = parse_args() + + log_level = logging.DEBUG if args.verbose else logging.INFO + logging.basicConfig( + level=log_level, + format="%(levelname)s:%(name)s:%(message)s", + stream=sys.stderr, + ) + + files: List[pathlib.Path] = args.filename + files = [ + file.resolve() for file in files if file.suffix[1:] in args.allow_extensions + ] + missing_files = [str(file) for file in files if not file.is_file()] + if missing_files: + logging.error(f"File(s) not found: {', '.join(missing_files)}") + print(f"File(s) not found: {', '.join(missing_files)}", file=sys.stderr) + return 1 + + # Create EventBus instance + event_bus = EventBus() + + # Create clangd instance + clangd = ClangdAsync( + clangd_executable=args.clangd_executable, + event_bus=event_bus, + compile_commands_dir=args.compile_commands_dir, + jobs=args.jobs, + verbose=args.verbose, + query_driver=args.query_driver, + use_background_index=args.fix or bool(args.export_fixes), + ) + + runner = ClangdRunner( + clangd=clangd, + files=files, + run_format=args.format, + clang_apply_replacements_executable=args.clang_apply_replacements_executable, + run_fix=args.fix or bool(args.export_fixes), + format_style=args.format_style, + tqdm=args.tqdm, + event_bus=event_bus, + ) + + return_code = 0 + try: + async with runner: + file_diagnostics, edits = await runner.collect_analysis() + # Always print diagnostics (unless in quiet mode or similar) + _print_diagnostics(file_diagnostics, args) + + # Handle export-fixes mode + if args.export_fixes: + if edits: + await runner.export_fixes(edits, args.export_fixes) + print(f"Exported fixes to {args.export_fixes}", file=sys.stderr) + else: + # Create an empty file + with open(args.export_fixes, "w"): + pass + print( + f"No fixes to export, created empty file at {args.export_fixes}", + file=sys.stderr, + ) + return 0 + + # Handle fix mode + if args.fix: + if edits: + await runner.apply_fixes(edits) + else: + logging.debug("No fixes to apply.") + return 0 + + # No action mode - just check severity for exit code + if any( + any( + ( + diagnostic.severity + and diagnostic.severity <= SEVERITY_INT[args.fail_on_severity] + ) + or diagnostic.source == "clang-format" + for diagnostic in diagnostics + ) + for diagnostics in file_diagnostics.values() + ): + return_code = 1 + except Exception as e: + logging.error( + f"An unexpected error occurred during analysis: {e}", exc_info=True ) - for diagnostics in file_diagnostics.values() - ): - exit(1) + return_code = 1 + + return return_code diff --git a/clangd_tidy/progress.py b/clangd_tidy/progress.py new file mode 100644 index 0000000..f8201b7 --- /dev/null +++ b/clangd_tidy/progress.py @@ -0,0 +1,82 @@ +import asyncio +import logging +from typing import Optional + +import cattrs + +from .flows.base import BaseFlow + +from .lsp.clangd import ClangdAsync +from .event_bus import EventBus +from .lsp.messages import ( + ProgressNotificationMessage, + WorkDoneProgressCreateRequest, + WorkDoneProgressEnd, +) + +# Cattrs converter +converter = cattrs.Converter() + + +class IndexingProgressTracker(BaseFlow): + """ + Tracks the state of clangd's background indexing and provides a method to wait for it to complete. + """ + + def __init__( + self, + clangd_client: ClangdAsync, + event_bus: EventBus, + ): + super().__init__(clangd_client, event_bus) + + self._indexing_token: Optional[str] = None + self._is_indexing = False + self._indexing_finished_event = asyncio.Event() + self._event_bus.subscribe( + WorkDoneProgressCreateRequest, self._handle_progress_create + ) + self._event_bus.subscribe( + ProgressNotificationMessage, self._handle_progress_notification + ) + + async def wait_for_indexing_to_finish(self): + """Waits until the current indexing process has finished.""" + if self._is_indexing: + await self._indexing_finished_event.wait() + + async def run(self): + """Runs the tracker (no-op, exists to satisfy BaseFlow).""" + pass + + async def bootstrap(self): + """Resets the state of the tracker after crash.""" + logging.debug("Resetting IndexingProgressTracker state.") + self._indexing_token = None + self._is_indexing = False + # Clear the event so wait_for_indexing_to_finish will wait for the next indexing cycle + self._indexing_finished_event.clear() + + async def _handle_progress_create(self, request: WorkDoneProgressCreateRequest): + """ + Handles the window/workDoneProgress/create request from the server. + """ + if request.params.token == "backgroundIndexProgress": + # Send a null response to acknowledge the request + await self._clangd.respond_to_server(request.id, None) + if not self._is_indexing: + logging.debug("Background indexing started.") + self._indexing_token = request.params.token + self._is_indexing = True + self._indexing_finished_event.clear() + + async def _handle_progress_notification( + self, notification: ProgressNotificationMessage + ): + """ + Handles $/progress notifications from the server. + """ + if notification.params.token == self._indexing_token: + if isinstance(notification.params.value, WorkDoneProgressEnd): + logging.debug("Background indexing finished.") + self._indexing_finished_event.set() diff --git a/clangd_tidy/replacements.py b/clangd_tidy/replacements.py new file mode 100644 index 0000000..afc66a5 --- /dev/null +++ b/clangd_tidy/replacements.py @@ -0,0 +1,182 @@ +from __future__ import annotations +from functools import cache +import logging +import pathlib +from typing import List, Optional, Tuple + +from attrs import define + +from .lsp.messages import Diagnostic, Position, WorkspaceEdit, uri_to_path + + +@define +class Replacement: + FilePath: str + Offset: int + Length: int + ReplacementText: str + + +@define +class DiagnosticMessage: + Message: str + FilePath: str + FileOffset: int + Replacements: List[Replacement] + + +@define +class ClangApplyReplacementsDiagnostic: + DiagnosticName: str + DiagnosticMessage: DiagnosticMessage + Level: str + BuildDirectory: str + + +@cache +def _get_line_offsets( + path: pathlib.Path, +) -> Optional[List[int]]: + """Get or compute line start byte offsets for a file. + + Returns list where index i contains the byte offset of line i's first character. + With UTF-8 encoding negotiated, we can convert LSP Position to absolute byte offset + using: absolute_offset = line_offsets[position.line] + position.character + + Args: + path: Path to the file + + Returns: + List of byte offsets for each line, or None if file cannot be read/decoded + """ + # Read file as bytes (avoid decode/re-encode overhead) + try: + file_bytes = path.read_bytes() + except OSError as e: + logging.warning(f"cannot read file {path}: {e}") + return None + + # Verify it's valid UTF-8 + try: + file_bytes.decode("utf-8") + except UnicodeDecodeError as e: + logging.warning( + f"cannot decode file {path} as utf-8, skipping fixes for it: {e}" + ) + return None + + # Build byte offset table by scanning for newlines + # Use bytes.split() which is C-implemented and fast + line_offsets = [0] + offset = 0 + for line in file_bytes.split(b"\n"): + offset += len(line) + 1 # +1 for the \n separator + line_offsets.append(offset) + line_offsets.pop() # Remove final entry (no line after last \n or EOF) + + return line_offsets + + +def _position_to_offset(line_offsets: List[int], position: Position) -> int: + """Convert LSP Position to absolute byte offset. + + With UTF-8 encoding, Position.character is a byte count from line start. + For positions beyond EOF, clamps to the last valid offset. + + Args: + line_offsets: Byte offsets for each line (from _get_line_offsets) + position: LSP Position to convert + + Returns: + Absolute byte offset in the file + """ + # Clamp line to valid range (handle edits at/beyond EOF) + line = min(position.line, len(line_offsets) - 1) + return line_offsets[line] + position.character + + +def create_replacements( + edits: List[Tuple[Diagnostic, WorkspaceEdit]], +) -> List[ClangApplyReplacementsDiagnostic]: + """Create a list of replacements from the given edits. + + Converts LSP WorkspaceEdit objects into clang-apply-replacements format. + Handles: + - Cross-file refactorings (edits may span multiple files) + - UTF-8 encoding and character offset calculations + - Diagnostic grouping (all edits from one diagnostic share MainSourceFile) + + Args: + edits: List of (Diagnostic, WorkspaceEdit) pairs to convert + + Returns: + List of replacement dictionaries in clang-apply-replacements format + """ + all_replacements: List[ClangApplyReplacementsDiagnostic] = [] + + for diag, edit in edits: + if not edit.changes: + continue + + # Collect replacements across all files for this diagnostic + replacements: List[Replacement] = [] + first_path = None + first_offset = None + + for uri, text_edits in edit.changes.items(): + path = uri_to_path(uri) + + line_offsets = _get_line_offsets(path) + if line_offsets is None: + continue + + for text_edit in text_edits: + # Convert LSP positions to absolute byte offsets + start_offset = _position_to_offset(line_offsets, text_edit.range.start) + end_offset = _position_to_offset(line_offsets, text_edit.range.end) + + replacements.append( + Replacement( + FilePath=str(path), + Offset=start_offset, + Length=end_offset - start_offset, + ReplacementText=text_edit.newText, + ) + ) + + # Track first file and offset for diagnostic header + # clang-apply-replacements format requires each diagnostic to have: + # - MainSourceFile: where the diagnostic was reported + # - FileOffset: byte position in that file + # This is used for grouping/sorting diagnostics in output + if first_path is None: + first_path = path + first_offset = start_offset + + # Transform diagnostic message for clang-apply-replacements format + # Remove clangd's " (fix available)" suffix + message = diag.message.replace(" (fix available)", "") + # Convert first letter to lowercase (clang-tidy convention) + if message and message[0].isupper(): + message = message[0].lower() + message[1:] + + # Map severity levels to match clang-tidy output + severity = diag.severity.name.capitalize() if diag.severity else "Warning" + # Special case: write all Hints as Warnings, as Hints break clang-apply-replacements + if severity == "Hint": + severity = "Warning" + + all_replacements.append( + ClangApplyReplacementsDiagnostic( + DiagnosticName=diag.code, + DiagnosticMessage=DiagnosticMessage( + Message=message, + FilePath=str(first_path) if first_path else "", + FileOffset=first_offset if first_offset is not None else 0, + Replacements=replacements, + ), + Level=severity, + BuildDirectory=str(first_path.parent) if first_path else "", + ) + ) + return all_replacements diff --git a/pyproject.toml b/pyproject.toml index 08fa39e..a101def 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,8 +5,8 @@ build-backend = "setuptools.build_meta" [project] name = "clangd-tidy" dynamic = ["version"] -dependencies = ["attrs", "cattrs", "typing-extensions"] -requires-python = ">=3.8" +dependencies = ["attrs", "cattrs >= 25.1.0", "typing-extensions"] +requires-python = ">=3.10" authors = [{ name = "lljbash", email = "lljbash@gmail.com" }] description = "A faster alternative to clang-tidy" readme = "README.md" @@ -37,8 +37,8 @@ include = '\.pyi?$' required-version = "25" [tool.basedpyright] -include = ["clangd_tidy"] -pythonVersion = "3.8" +include = ["clangd_tidy", "test"] +pythonVersion = "3.10" pythonPlatform = "Linux" typeCheckingMode = "strict" diff --git a/test/fixtures/README.md b/test/fixtures/README.md new file mode 100644 index 0000000..63d8569 --- /dev/null +++ b/test/fixtures/README.md @@ -0,0 +1,28 @@ +# Test Fixtures + +This directory contains pre-created test fixtures for the clangd-tidy test suite. Each fixture directory is self-contained with source files, configuration files, and a compilation database. The source code files can be viewed directly in an IDE to see diagnostics detected by clangd via LSP and also try out the fixits manually. + +## Structure + +Each fixture directory contains: +- Source files (`.cpp`, `.h`) +- Configuration files (a combination of `.clang-tidy`, `.clangd`, `.clang-format`) +- Compilation database (`compile_commands.json`) +- For `--fix` tests: `*.fixed` files for each source file +- For `--fix --format-style` tests: `*.fixed.formatted` + +### Compile commands + +The `compile_commands.json` files use relative paths to make directories easily copyable. For some tests we need absolute paths, which can be automatically converted to by using `make_paths_absolute=True` when calling `copy_fixture_directory`. + +### Code formatting +Notes: Standard use of `--format-style` with `clang-tidy` assumes the input files were already formatted correctly, and only formats parts of code affected by `--fix`. Given that it's not trivial to ensure code formatting would have any effect after fixes on a well-formatted input code, we use a misformatted input code and only have local formatting changes in `*.fixed.formatted` files. + +## Usage + +Tests use the `copy_fixture(fixture_name, tmp_path)` helper function: + +The `copy_fixture()` function: +1. Copies the entire fixture directory to `tmp_path` +2. If `make_paths_absolute` is set to `True`, replaces all relative paths in `compile_commands.json` with absolute paths (using `tmp_path` from step 1.) +3. Returns the path to the copied directory diff --git a/test/fixtures/basic/.clang-tidy b/test/fixtures/basic/.clang-tidy new file mode 100644 index 0000000..46a2781 --- /dev/null +++ b/test/fixtures/basic/.clang-tidy @@ -0,0 +1,2 @@ +--- +Checks: 'modernize-use-nullptr' diff --git a/test/fixtures/basic/test_fix.cpp b/test/fixtures/basic/test_fix.cpp new file mode 100644 index 0000000..12e78b6 --- /dev/null +++ b/test/fixtures/basic/test_fix.cpp @@ -0,0 +1,9 @@ +#include +void foo(int* p){ + if(p==NULL){ + } +} +int main(){ + foo(NULL); + return 0; +} diff --git a/test/fixtures/basic/test_fix.cpp.fixed b/test/fixtures/basic/test_fix.cpp.fixed new file mode 100644 index 0000000..d8f96b4 --- /dev/null +++ b/test/fixtures/basic/test_fix.cpp.fixed @@ -0,0 +1,9 @@ +#include +void foo(int* p){ + if(p==nullptr){ + } +} +int main(){ + foo(nullptr); + return 0; +} diff --git a/test/fixtures/basic/test_fix.cpp.fixed.formatted b/test/fixtures/basic/test_fix.cpp.fixed.formatted new file mode 100644 index 0000000..946847c --- /dev/null +++ b/test/fixtures/basic/test_fix.cpp.fixed.formatted @@ -0,0 +1,9 @@ +#include +void foo(int* p){ + if (p == nullptr) { + } +} +int main(){ + foo(nullptr); + return 0; +} diff --git a/test/fixtures/clangd_config/.clangd b/test/fixtures/clangd_config/.clangd new file mode 100644 index 0000000..f44b140 --- /dev/null +++ b/test/fixtures/clangd_config/.clangd @@ -0,0 +1,3 @@ +Diagnostics: + ClangTidy: + Add: [modernize-use-nullptr] diff --git a/test/fixtures/clangd_config/compile_commands.json b/test/fixtures/clangd_config/compile_commands.json new file mode 100644 index 0000000..8534bac --- /dev/null +++ b/test/fixtures/clangd_config/compile_commands.json @@ -0,0 +1,7 @@ +[ + { + "directory": ".", + "command": "c++ -c test_fix.cpp", + "file": "test_fix.cpp" + } +] diff --git a/test/fixtures/clangd_config/test_fix.cpp b/test/fixtures/clangd_config/test_fix.cpp new file mode 100644 index 0000000..12e78b6 --- /dev/null +++ b/test/fixtures/clangd_config/test_fix.cpp @@ -0,0 +1,9 @@ +#include +void foo(int* p){ + if(p==NULL){ + } +} +int main(){ + foo(NULL); + return 0; +} diff --git a/test/fixtures/clangd_config/test_fix.cpp.fixed b/test/fixtures/clangd_config/test_fix.cpp.fixed new file mode 100644 index 0000000..d8f96b4 --- /dev/null +++ b/test/fixtures/clangd_config/test_fix.cpp.fixed @@ -0,0 +1,9 @@ +#include +void foo(int* p){ + if(p==nullptr){ + } +} +int main(){ + foo(nullptr); + return 0; +} diff --git a/test/fixtures/clangd_config/test_fix.cpp.fixed.formatted b/test/fixtures/clangd_config/test_fix.cpp.fixed.formatted new file mode 100644 index 0000000..946847c --- /dev/null +++ b/test/fixtures/clangd_config/test_fix.cpp.fixed.formatted @@ -0,0 +1,9 @@ +#include +void foo(int* p){ + if (p == nullptr) { + } +} +int main(){ + foo(nullptr); + return 0; +} diff --git a/test/fixtures/combined_config/.clang-format b/test/fixtures/combined_config/.clang-format new file mode 100644 index 0000000..8ae451a --- /dev/null +++ b/test/fixtures/combined_config/.clang-format @@ -0,0 +1,4 @@ +--- +BasedOnStyle: LLVM +IndentWidth: 6 +ColumnLimit: 100 diff --git a/test/fixtures/combined_config/.clang-tidy b/test/fixtures/combined_config/.clang-tidy new file mode 100644 index 0000000..9baba54 --- /dev/null +++ b/test/fixtures/combined_config/.clang-tidy @@ -0,0 +1,4 @@ +--- +Checks: + - modernize-use-nullptr +HeaderFilterRegex: '.*' diff --git a/test/fixtures/combined_config/.clangd b/test/fixtures/combined_config/.clangd new file mode 100644 index 0000000..30d7dac --- /dev/null +++ b/test/fixtures/combined_config/.clangd @@ -0,0 +1,7 @@ +InlayHints: + Enabled: false +CompileFlags: + Add: [-Wextra] +Diagnostics: + ClangTidy: + Add: [modernize-use-nullptr, modernize-use-bool-literals] diff --git a/test/fixtures/combined_config/foo.clangd.yaml b/test/fixtures/combined_config/foo.clangd.yaml new file mode 100644 index 0000000..33d0940 --- /dev/null +++ b/test/fixtures/combined_config/foo.clangd.yaml @@ -0,0 +1,38 @@ +Diagnostics: +- BuildDirectory: /home/vscode/workspace/test/fixtures/combined_config + DiagnosticMessage: + FileOffset: 45 + FilePath: /home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp + Message: use nullptr + Replacements: + - FilePath: /home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp + Length: 4 + Offset: 45 + ReplacementText: nullptr + DiagnosticName: modernize-use-nullptr + Level: Warning +- BuildDirectory: /home/vscode/workspace/test/fixtures/combined_config + DiagnosticMessage: + FileOffset: 65 + FilePath: /home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp + Message: converting integer literal to bool, use bool literal instead + Replacements: + - FilePath: /home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp + Length: 1 + Offset: 65 + ReplacementText: 'true' + DiagnosticName: modernize-use-bool-literals + Level: Hint +- BuildDirectory: /home/vscode/workspace/test/fixtures/combined_config + DiagnosticMessage: + FileOffset: 88 + FilePath: /home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp + Message: use nullptr + Replacements: + - FilePath: /home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp + Length: 4 + Offset: 88 + ReplacementText: nullptr + DiagnosticName: modernize-use-nullptr + Level: Warning +MainSourceFile: /home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp diff --git a/test/fixtures/combined_config/foo.yaml b/test/fixtures/combined_config/foo.yaml new file mode 100644 index 0000000..bb57aa5 --- /dev/null +++ b/test/fixtures/combined_config/foo.yaml @@ -0,0 +1,40 @@ +--- +MainSourceFile: '/home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp' +Diagnostics: + - DiagnosticName: modernize-use-nullptr + DiagnosticMessage: + Message: use nullptr + FilePath: '/home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp' + FileOffset: 45 + Replacements: + - FilePath: '/home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp' + Offset: 45 + Length: 4 + ReplacementText: nullptr + Level: Warning + BuildDirectory: '/home/vscode/workspace/test/fixtures/combined_config' + - DiagnosticName: modernize-use-bool-literals + DiagnosticMessage: + Message: converting integer literal to bool, use bool literal instead + FilePath: '/home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp' + FileOffset: 65 + Replacements: + - FilePath: '/home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp' + Offset: 65 + Length: 1 + ReplacementText: 'true' + Level: Warning + BuildDirectory: '/home/vscode/workspace/test/fixtures/combined_config' + - DiagnosticName: modernize-use-nullptr + DiagnosticMessage: + Message: use nullptr + FilePath: '/home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp' + FileOffset: 88 + Replacements: + - FilePath: '/home/vscode/workspace/test/fixtures/combined_config/test_fix.cpp' + Offset: 88 + Length: 4 + ReplacementText: nullptr + Level: Warning + BuildDirectory: '/home/vscode/workspace/test/fixtures/combined_config' +... diff --git a/test/fixtures/combined_config/test_fix.cpp b/test/fixtures/combined_config/test_fix.cpp new file mode 100644 index 0000000..51a2e21 --- /dev/null +++ b/test/fixtures/combined_config/test_fix.cpp @@ -0,0 +1,10 @@ +#include +bool foo(int* p){ + if(p==NULL){ + } + return 1; +} +int main(){ + foo(NULL); + return 0; +} diff --git a/test/fixtures/combined_config/test_fix.cpp.fixed b/test/fixtures/combined_config/test_fix.cpp.fixed new file mode 100644 index 0000000..aafe6fc --- /dev/null +++ b/test/fixtures/combined_config/test_fix.cpp.fixed @@ -0,0 +1,10 @@ +#include +bool foo(int* p){ + if(p==nullptr){ + } + return true; +} +int main(){ + foo(nullptr); + return 0; +} diff --git a/test/fixtures/combined_config/test_fix.cpp.fixed.formatted b/test/fixtures/combined_config/test_fix.cpp.fixed.formatted new file mode 100644 index 0000000..dc2997e --- /dev/null +++ b/test/fixtures/combined_config/test_fix.cpp.fixed.formatted @@ -0,0 +1,10 @@ +#include +bool foo(int* p){ + if (p == nullptr) { + } + return true; +} +int main(){ + foo(nullptr); + return 0; +} diff --git a/test/fixtures/compilation_errors/.clangd b/test/fixtures/compilation_errors/.clangd new file mode 100644 index 0000000..e8416ed --- /dev/null +++ b/test/fixtures/compilation_errors/.clangd @@ -0,0 +1,4 @@ +Diagnostics: + ClangTidy: + Add: [modernize-use-nullptr] + UnusedIncludes: None diff --git a/test/fixtures/compilation_errors/test_errors.cpp b/test/fixtures/compilation_errors/test_errors.cpp new file mode 100644 index 0000000..2ca1bd5 --- /dev/null +++ b/test/fixtures/compilation_errors/test_errors.cpp @@ -0,0 +1,25 @@ +#include + +// This file contains three types of issues: +// 1. Syntax error (will cause compilation to fail) +// 2. Compiler warning (unused variable) +// 3. clang-tidy diagnostic (modernize-use-nullptr) + +void function_with_issues() { + // Issue #2: Compiler warning - unused variable + int unused_var = 42; + + // Issue #3: clang-tidy diagnostic - use nullptr instead of NULL + int* ptr = NULL; + + if (ptr == NULL) { + // Do something + } +} + +int main() { + // Issue #1: Syntax error - missing semicolon + int x = 5 + + return 0; +} diff --git a/test/fixtures/cross_file/.clang-tidy b/test/fixtures/cross_file/.clang-tidy new file mode 100644 index 0000000..8d163c4 --- /dev/null +++ b/test/fixtures/cross_file/.clang-tidy @@ -0,0 +1,7 @@ +--- +Checks: + - 'readability-identifier-naming' + - 'modernize-use-nullptr' +CheckOptions: + - key: readability-identifier-naming.FunctionCase + value: lower_case diff --git a/test/fixtures/cross_file/math_utils.cpp b/test/fixtures/cross_file/math_utils.cpp new file mode 100644 index 0000000..4686861 --- /dev/null +++ b/test/fixtures/cross_file/math_utils.cpp @@ -0,0 +1,10 @@ +#include "math_utils.h" + +int addNumbers(int a,int b){ + return a+b; +} + +int main(){ + int result=addNumbers(5,3); + return result; +} diff --git a/test/fixtures/cross_file/math_utils.cpp.fixed b/test/fixtures/cross_file/math_utils.cpp.fixed new file mode 100644 index 0000000..24bb324 --- /dev/null +++ b/test/fixtures/cross_file/math_utils.cpp.fixed @@ -0,0 +1,10 @@ +#include "math_utils.h" + +int add_numbers(int a,int b){ + return a+b; +} + +int main(){ + int result=add_numbers(5,3); + return result; +} diff --git a/test/fixtures/cross_file/math_utils.cpp.fixed.formatted b/test/fixtures/cross_file/math_utils.cpp.fixed.formatted new file mode 100644 index 0000000..95025dc --- /dev/null +++ b/test/fixtures/cross_file/math_utils.cpp.fixed.formatted @@ -0,0 +1,8 @@ +#include "math_utils.h" + +int add_numbers(int a, int b) { return a + b; } + +int main(){ + int result = add_numbers(5, 3); + return result; +} diff --git a/test/fixtures/cross_file/math_utils.h b/test/fixtures/cross_file/math_utils.h new file mode 100644 index 0000000..c2a5635 --- /dev/null +++ b/test/fixtures/cross_file/math_utils.h @@ -0,0 +1,7 @@ +#ifndef MATH_UTILS_H +#define MATH_UTILS_H + +// This function violates readability-identifier-naming (should be lower_case) +int addNumbers(int a,int b); + +#endif diff --git a/test/fixtures/cross_file/math_utils.h.fixed b/test/fixtures/cross_file/math_utils.h.fixed new file mode 100644 index 0000000..b49ded7 --- /dev/null +++ b/test/fixtures/cross_file/math_utils.h.fixed @@ -0,0 +1,7 @@ +#ifndef MATH_UTILS_H +#define MATH_UTILS_H + +// This function violates readability-identifier-naming (should be lower_case) +int add_numbers(int a,int b); + +#endif diff --git a/test/fixtures/cross_file/math_utils.h.fixed.formatted b/test/fixtures/cross_file/math_utils.h.fixed.formatted new file mode 100644 index 0000000..f0a8b73 --- /dev/null +++ b/test/fixtures/cross_file/math_utils.h.fixed.formatted @@ -0,0 +1,7 @@ +#ifndef MATH_UTILS_H +#define MATH_UTILS_H + +// This function violates readability-identifier-naming (should be lower_case) +int add_numbers(int a, int b); + +#endif diff --git a/test/fixtures/error_recovery/.clangd b/test/fixtures/error_recovery/.clangd new file mode 100644 index 0000000..ca7c204 --- /dev/null +++ b/test/fixtures/error_recovery/.clangd @@ -0,0 +1,6 @@ +Diagnostics: + ClangTidy: + Add: [readability-identifier-naming] + CheckOptions: + readability-identifier-naming.NamespaceCase: lower_case + readability-identifier-naming.FunctionCase: lower_case diff --git a/test/fixtures/error_recovery/compile_commands.json b/test/fixtures/error_recovery/compile_commands.json new file mode 100644 index 0000000..1db04d7 --- /dev/null +++ b/test/fixtures/error_recovery/compile_commands.json @@ -0,0 +1,7 @@ +[ + { + "directory": ".", + "command": "c++ -c test_crash.cpp", + "file": "test_crash.cpp" + } +] diff --git a/test/fixtures/error_recovery/test_crash.cpp b/test/fixtures/error_recovery/test_crash.cpp new file mode 100644 index 0000000..9fcbeee --- /dev/null +++ b/test/fixtures/error_recovery/test_crash.cpp @@ -0,0 +1,10 @@ +namespace BadNamespace { + void BadFunction() { + // Do something + } +} + +int main() { + BadNamespace::BadFunction(); + return 0; +} diff --git a/test/fixtures/error_recovery/test_crash.cpp.fixed b/test/fixtures/error_recovery/test_crash.cpp.fixed new file mode 100644 index 0000000..81097d7 --- /dev/null +++ b/test/fixtures/error_recovery/test_crash.cpp.fixed @@ -0,0 +1,10 @@ +namespace BadNamespace { + void bad_function() { + // Do something + } +} + +int main() { + BadNamespace::bad_function(); + return 0; +} diff --git a/test/fixtures/multiple_fixes/.clang-tidy b/test/fixtures/multiple_fixes/.clang-tidy new file mode 100644 index 0000000..b5d027c --- /dev/null +++ b/test/fixtures/multiple_fixes/.clang-tidy @@ -0,0 +1,8 @@ +--- +Checks: + - readability-identifier-naming +CheckOptions: + - key: readability-identifier-naming.ParameterCase + value: lower_case + - key: readability-identifier-naming.FunctionCase + value: lower_case diff --git a/test/fixtures/multiple_fixes/test_multiple_fixes.cpp b/test/fixtures/multiple_fixes/test_multiple_fixes.cpp new file mode 100644 index 0000000..c2501d4 --- /dev/null +++ b/test/fixtures/multiple_fixes/test_multiple_fixes.cpp @@ -0,0 +1,5 @@ +class Test { + void setFoo(int newFoo) { foo = newFoo; } + private: + int foo; +}; diff --git a/test/fixtures/multiple_fixes/test_multiple_fixes.cpp.fixed b/test/fixtures/multiple_fixes/test_multiple_fixes.cpp.fixed new file mode 100644 index 0000000..1808fcc --- /dev/null +++ b/test/fixtures/multiple_fixes/test_multiple_fixes.cpp.fixed @@ -0,0 +1,5 @@ +class Test { + void set_foo(int new_foo) { foo = new_foo; } + private: + int foo; +}; diff --git a/test/fixtures/tool_specific_config/.clang-format b/test/fixtures/tool_specific_config/.clang-format new file mode 100644 index 0000000..f69096c --- /dev/null +++ b/test/fixtures/tool_specific_config/.clang-format @@ -0,0 +1,5 @@ +--- +BasedOnStyle: LLVM +IndentWidth: 8 +PointerAlignment: Left +ColumnLimit: 100 diff --git a/test/fixtures/tool_specific_config/.clang-tidy b/test/fixtures/tool_specific_config/.clang-tidy new file mode 100644 index 0000000..46a2781 --- /dev/null +++ b/test/fixtures/tool_specific_config/.clang-tidy @@ -0,0 +1,2 @@ +--- +Checks: 'modernize-use-nullptr' diff --git a/test/fixtures/tool_specific_config/test_fix.cpp b/test/fixtures/tool_specific_config/test_fix.cpp new file mode 100644 index 0000000..12e78b6 --- /dev/null +++ b/test/fixtures/tool_specific_config/test_fix.cpp @@ -0,0 +1,9 @@ +#include +void foo(int* p){ + if(p==NULL){ + } +} +int main(){ + foo(NULL); + return 0; +} diff --git a/test/fixtures/tool_specific_config/test_fix.cpp.fixed b/test/fixtures/tool_specific_config/test_fix.cpp.fixed new file mode 100644 index 0000000..d8f96b4 --- /dev/null +++ b/test/fixtures/tool_specific_config/test_fix.cpp.fixed @@ -0,0 +1,9 @@ +#include +void foo(int* p){ + if(p==nullptr){ + } +} +int main(){ + foo(nullptr); + return 0; +} diff --git a/test/fixtures/tool_specific_config/test_fix.cpp.fixed.formatted b/test/fixtures/tool_specific_config/test_fix.cpp.fixed.formatted new file mode 100644 index 0000000..946847c --- /dev/null +++ b/test/fixtures/tool_specific_config/test_fix.cpp.fixed.formatted @@ -0,0 +1,9 @@ +#include +void foo(int* p){ + if (p == nullptr) { + } +} +int main(){ + foo(nullptr); + return 0; +} diff --git a/test/fixtures/utf8/.clang-tidy b/test/fixtures/utf8/.clang-tidy new file mode 100644 index 0000000..46a2781 --- /dev/null +++ b/test/fixtures/utf8/.clang-tidy @@ -0,0 +1,2 @@ +--- +Checks: 'modernize-use-nullptr' diff --git a/test/fixtures/utf8/test_utf8.cpp b/test/fixtures/utf8/test_utf8.cpp new file mode 100644 index 0000000..5c0ea60 --- /dev/null +++ b/test/fixtures/utf8/test_utf8.cpp @@ -0,0 +1,21 @@ +// Test file with non-ASCII UTF-8 characters: café, naïve, 日本語 +#include + +// Function with UTF-8 comment: Москва, Αθήνα, 北京 +void test_function(int* ptr){ + const char* greeting="Héllo Wörld"; // UTF-8 string: José, François + // Check pointer - should suggest nullptr + if(ptr==NULL){ + return; + } + + // UTF-8 characters BEFORE the fix target on the same line + // This tests that byte offset calculation handles multi-byte UTF-8 correctly + int* x=/* café */ NULL; // café is 5 chars but 6 bytes (é = 2 bytes) + int* y=/* 日本語 */ NULL; // 3 chars but 9 bytes (each Japanese char = 3 bytes) +} + +int main(){ + test_function(NULL); // Should be fixed to nullptr + return 0; +} diff --git a/test/fixtures/utf8/test_utf8.cpp.fixed b/test/fixtures/utf8/test_utf8.cpp.fixed new file mode 100644 index 0000000..7bfb079 --- /dev/null +++ b/test/fixtures/utf8/test_utf8.cpp.fixed @@ -0,0 +1,21 @@ +// Test file with non-ASCII UTF-8 characters: café, naïve, 日本語 +#include + +// Function with UTF-8 comment: Москва, Αθήνα, 北京 +void test_function(int* ptr){ + const char* greeting="Héllo Wörld"; // UTF-8 string: José, François + // Check pointer - should suggest nullptr + if(ptr==nullptr){ + return; + } + + // UTF-8 characters BEFORE the fix target on the same line + // This tests that byte offset calculation handles multi-byte UTF-8 correctly + int* x=/* café */ nullptr; // café is 5 chars but 6 bytes (é = 2 bytes) + int* y=/* 日本語 */ nullptr; // 3 chars but 9 bytes (each Japanese char = 3 bytes) +} + +int main(){ + test_function(nullptr); // Should be fixed to nullptr + return 0; +} diff --git a/test/fixtures/utf8/test_utf8.cpp.fixed.formatted b/test/fixtures/utf8/test_utf8.cpp.fixed.formatted new file mode 100644 index 0000000..b722fa0 --- /dev/null +++ b/test/fixtures/utf8/test_utf8.cpp.fixed.formatted @@ -0,0 +1,22 @@ +// Test file with non-ASCII UTF-8 characters: café, naïve, 日本語 +#include + +// Function with UTF-8 comment: Москва, Αθήνα, 北京 +void test_function(int* ptr){ + const char* greeting="Héllo Wörld"; // UTF-8 string: José, François + // Check pointer - should suggest nullptr + if (ptr == nullptr) { + return; + } + + // UTF-8 characters BEFORE the fix target on the same line + // This tests that byte offset calculation handles multi-byte UTF-8 correctly + int *x = /* café */ nullptr; // café is 5 chars but 6 bytes (é = 2 bytes) + int *y = /* 日本語 */ nullptr; // 3 chars but 9 bytes (each Japanese char = 3 + // bytes) +} + +int main(){ + test_function(nullptr); // Should be fixed to nullptr + return 0; +} diff --git a/test/test_basic.py b/test/test_basic.py new file mode 100644 index 0000000..cad4d2b --- /dev/null +++ b/test/test_basic.py @@ -0,0 +1,128 @@ +""" +Test suite for basic clangd-tidy operations. + +This file contains parametrized tests that cover the core functionality +across multiple fixtures with different configurations. +""" + +import pathlib +import pytest + +from test_utils import ( + run_clangd_tidy, + copy_fixture_directory, + assert_file_matches_expected, +) + + +@pytest.mark.parametrize( + "fixture_name", + [ + "basic", + "utf8", + "cross_file", + "clangd_config", + "tool_specific_config", + "combined_config", + "multiple_fixes", + ], +) +def test_format_checking(tmp_path: pathlib.Path, fixture_name: str): + """Test that --format detects formatting violations.""" + test_dir = copy_fixture_directory(fixture_name, tmp_path) + + source_files = sorted(test_dir.glob("*.cpp")) + sorted(test_dir.glob("*.h")) + formatted_source_files = sorted(test_dir.glob("*.formatted")) + + for source_file in source_files + formatted_source_files: + args = ["--format"] + args.append(str(source_file)) + + # Run with --format to check formatting (should detect violations in unformatted files) + process = run_clangd_tidy(args, check=False) + if source_file in formatted_source_files: + # The file is already formatted, so --format should pass + assert ( + process.returncode == 0 + ), f"Expected no formatting violations in {source_file}, but some were reported." + else: + # The file is unformatted, so --format should report violations + assert ( + process.returncode != 0 + ), f"Expected formatting violations in {source_file}, but none were reported." + + +@pytest.mark.parametrize( + "fixture_name", + [ + "basic", + "utf8", + "cross_file", + "clangd_config", + "tool_specific_config", + "combined_config", + "multiple_fixes", + ], +) +def test_fixes(tmp_path: pathlib.Path, fixture_name: str): + """Test that --fix applies fixes correctly across different fixtures.""" + # cross_file needs absolute paths + make_absolute = fixture_name == "cross_file" + test_dir = copy_fixture_directory( + fixture_name, tmp_path, make_paths_absolute=make_absolute + ) + + # Find all source files in fixture + source_files = sorted(test_dir.glob("*.cpp")) + sorted(test_dir.glob("*.h")) + + run_clangd_tidy( + [ + "--fix", + "-p", + str(test_dir), + *[str(source_file) for source_file in source_files], + ] + ) + + for source_file in source_files: + assert_file_matches_expected( + source_file, source_file.with_name(f"{source_file.name}.fixed") + ) + + +@pytest.mark.parametrize( + "fixture_name", + [ + "basic", + "utf8", + "cross_file", + "clangd_config", + "tool_specific_config", + "combined_config", + ], +) +def test_fix_with_format_style(tmp_path: pathlib.Path, fixture_name: str): + """Test that --fix --format-style applies both fixes and formatting.""" + # cross_file needs absolute paths + make_absolute = fixture_name == "cross_file" + test_dir = copy_fixture_directory( + fixture_name, tmp_path, make_paths_absolute=make_absolute + ) + + # Find all source files in fixture + source_files = sorted(test_dir.glob("*.cpp")) + sorted(test_dir.glob("*.h")) + + run_clangd_tidy( + [ + "--fix", + "--format-style=file", + "-p", + str(test_dir), + *[str(source_file) for source_file in source_files], + ] + ) + + for source_file in source_files: + assert_file_matches_expected( + source_file, source_file.with_name(f"{source_file.name}.fixed.formatted") + ) diff --git a/test/test_diagnostics.py b/test/test_diagnostics.py new file mode 100644 index 0000000..0fcb797 --- /dev/null +++ b/test/test_diagnostics.py @@ -0,0 +1,64 @@ +""" +Test suite for clangd-tidy diagnostics reporting (without --fix). + +These tests verify that clangd-tidy can report diagnostics without applying fixes. +""" + +import pathlib + +from test_utils import ( + run_clangd_tidy, + copy_fixture_directory, +) + + +def test_report_nullptr_diagnostics(tmp_path: pathlib.Path): + """Test that diagnostics are reported.""" + test_dir = copy_fixture_directory("basic", tmp_path) + test_file = test_dir / "test_fix.cpp" # Contains NULL + + # Run without --fix, should only report diagnostics + process = run_clangd_tidy([str(test_file)], check=False) + + # Verify diagnostics appear in output + assert ( + "modernize-use-nullptr" in process.stdout + ), "Expected modernize-use-nullptr diagnostic in output" + + # Verify file was NOT modified (no fixes applied) + original_content = test_file.read_text() + assert "NULL" in original_content, "File should still contain NULL (not fixed)" + assert ( + "nullptr" not in original_content + ), "File should not contain nullptr without --fix" + + +def test_diagnostics_on_compilation_errors(tmp_path: pathlib.Path): + """Test that diagnostics are reported even when code has compilation errors. + + Verifies that clangd-tidy reports three types of issues: + 1. Syntax errors (actual compilation failures) + 2. Compiler warnings (e.g., unused variables) + 3. clang-tidy diagnostics (style/modernization issues) + """ + test_dir = copy_fixture_directory("compilation_errors", tmp_path) + test_file = test_dir / "test_errors.cpp" + + # Run clangd-tidy on file with various errors + process = run_clangd_tidy([str(test_file)], check=False) + + output = process.stdout + + assert ( + "[expected_semi_declaration]" in output.lower() + ), "Should mention missing semicolon error" + assert ( + "[modernize-use-nullptr]" in output + ), "Should report clang-tidy diagnostic (nullptr)" + + # Verify we got diagnostics of different severity levels + # We should see Error (compiler) and Warning or Hint (tidy) + assert "Error:" in output, "Should include Error level diagnostics" + assert ( + "Warning:" in output or "Hint:" in output + ), "Should include Warning or Hint level diagnostics" diff --git a/test/test_fix.py b/test/test_fix.py new file mode 100644 index 0000000..fe64a1d --- /dev/null +++ b/test/test_fix.py @@ -0,0 +1,178 @@ +""" +Test suite for clangd-tidy --fix and --export-fixes features. +""" + +import pathlib +import shutil +import subprocess +import yaml + +from test_utils import ( + assert_file_matches_expected, + copy_fixture_directory, + run_clangd_tidy, +) + + +def test_fix_it(tmp_path: pathlib.Path): + """Test that --fix applies changes directly to the file.""" + test_dir = copy_fixture_directory("basic", tmp_path) + test_file = test_dir / "test_fix.cpp" + expected_file = test_dir / "test_fix.cpp.fixed" + + # Run clangd-tidy to apply fixes directly + run_clangd_tidy(["--verbose", "--fix", str(test_file)]) + + assert_file_matches_expected(test_file, expected_file) + + +def test_export_fixes_parity(tmp_path: pathlib.Path): + """Test that --export-fixes produces output compatible with clang-tidy.""" + # Copy the fixture to a temporary directory. Make paths absolute as clang-tidy itself doesn't handle relative paths well. + test_dir = copy_fixture_directory("basic", tmp_path, make_paths_absolute=True) + test_file = test_dir / "test_fix.cpp" + + # 1. Run clang-tidy to generate a baseline fixes file + clang_tidy_executable = "clang-tidy" + assert shutil.which( + clang_tidy_executable + ), f"{clang_tidy_executable} not found in PATH" + + clang_tidy_fixes_path = tmp_path / "clang_tidy_fixes.yaml" + clang_tidy_command = [ + clang_tidy_executable, + f"-p={test_dir}", + f"--export-fixes={clang_tidy_fixes_path}", + str(test_file), + ] + subprocess.run(clang_tidy_command, check=True, capture_output=True, text=True) + + # 2. Run clangd-tidy to generate its fixes file + clangd_tidy_fixes_path = tmp_path / "clangd_tidy_fixes_2.yaml" + run_clangd_tidy(["--export-fixes", str(clangd_tidy_fixes_path), str(test_file)]) + + # 3. Compare the two YAML files + with open(clang_tidy_fixes_path, "r") as f: + clang_tidy_fixes = yaml.safe_load(f) + + with open(clangd_tidy_fixes_path, "r") as f: + clangd_tidy_fixes_generated = yaml.safe_load(f) + + # We only care about the diagnostics, not the main source file + # Sort the diagnostics by file offset and filepath to ensure consistent order + def get_sort_key( + diagnostic_entry: dict[str, dict[str, int | str]], + ) -> tuple[int, str]: + """Return (FileOffset, FilePath) tuple for stable sorting.""" + msg = diagnostic_entry["DiagnosticMessage"] + assert "FileOffset" in msg and "FilePath" in msg + assert isinstance(msg["FileOffset"], int) + assert isinstance(msg["FilePath"], str) + return (msg["FileOffset"], msg["FilePath"]) + + clang_tidy_fixes["Diagnostics"].sort(key=get_sort_key) + clangd_tidy_fixes_generated["Diagnostics"].sort(key=get_sort_key) + assert clang_tidy_fixes["Diagnostics"] == clangd_tidy_fixes_generated["Diagnostics"] + + +def test_fix_it_from_different_directory(tmp_path: pathlib.Path): + """Test that --fix works when running from a different directory.""" + test_dir = copy_fixture_directory("basic", tmp_path) + test_file = test_dir / "test_fix.cpp" + expected_file = test_dir / "test_fix.cpp.fixed" + + # Create a subdirectory to run the command from + run_dir = tmp_path / "run_dir" + run_dir.mkdir() + + # Run clangd-tidy from a different directory using absolute path + run_clangd_tidy(["--fix", str(test_file.absolute())], cwd=run_dir) + + assert_file_matches_expected(test_file, expected_file) + + +def test_fix_across_files(tmp_path: pathlib.Path): + """Test that --fix works across multiple files (header + source). + + When analyzing a header file, clangd should report naming convention violations + and provide fixes not just for the header, but also for all source files that use it + (as long as they're in the clangd index). + """ + # Copy the fixture to a temporary directory. We need absolute paths in compile_commands for clangd to resolve cross-file references. + test_dir = copy_fixture_directory("cross_file", tmp_path, make_paths_absolute=True) + header_file = test_dir / "math_utils.h" + source_file = test_dir / "math_utils.cpp" + header_expected = test_dir / "math_utils.h.fixed" + source_expected = test_dir / "math_utils.cpp.fixed" + + # Run --fix on the HEADER file only + # Clangd should detect the violation in the header and provide fixes for both + # the header declaration and the source file usage + run_clangd_tidy(["--fix", "-p", str(test_dir), str(header_file)]) + + # Verify fixes were applied to both files + assert_file_matches_expected(header_file, header_expected) + assert_file_matches_expected(source_file, source_expected) + + +def test_fix_it_with_clangd_config(tmp_path: pathlib.Path): + """Test that clangd-tidy respects .clangd configuration files.""" + test_dir = copy_fixture_directory("clangd_config", tmp_path) + test_file = test_dir / "test_fix.cpp" + expected_file = test_dir / "test_fix.cpp.fixed" + + # Run clangd-tidy to apply fixes + run_clangd_tidy(["--fix", str(test_file)]) + + assert_file_matches_expected(test_file, expected_file) + + +def test_error_recovery_continue_after_failed_command(tmp_path: pathlib.Path): + """Test that clangd-tidy continues processing after a command error. + + This test uses a file with TWO naming violations that will result in + two separate fix commands: + 1. A namespace naming violation (unsupported - executes first, fails with error) + 2. A function naming violation (supported - executes second, succeeds) + + The test verifies our intelligent error handling: + - Both commands are sent to clangd (queued and executed) + - The namespace rename fails with an error response from clangd + - The error is logged and reported to the user + - We recover, restart clangd and continue processing the second command + - The function fix IS applied despite the namespace error and a clangd crash + + This tests that we don't skip valid commands just because another command fails. + """ + test_dir = copy_fixture_directory("error_recovery", tmp_path) + test_file = test_dir / "test_crash.cpp" + expected_file = test_dir / "test_crash.cpp.fixed" + + process = run_clangd_tidy(["--fix", str(test_file)]) + + # Verify that the error was logged + assert ( + "Cannot rename symbol" in process.stderr or "Cannot apply fix" in process.stderr + ), "Should mention the error for namespace rename in stderr" + + assert_file_matches_expected(test_file, expected_file) + + +def test_utf8_encoding(tmp_path: pathlib.Path): + """Test that fixes work correctly with UTF-8 encoded files containing non-ASCII characters. + + This verifies that character offset calculations handle multi-byte UTF-8 sequences correctly. + LSP spec uses UTF-16 code units, but clangd can use UTF-8 in practice. + + The critical test case: UTF-8 multi-byte characters appearing BEFORE the replacement + position on the same line. This ensures byte offset calculations account for the + difference between character count and byte count (café: 5 chars/6 bytes, 日本語: 3 chars/9 bytes). + """ + test_dir = copy_fixture_directory("utf8", tmp_path) + test_file = test_dir / "test_utf8.cpp" + expected_file = test_dir / "test_utf8.cpp.fixed" + + # Run clangd-tidy to apply fixes, specifying the test directory for compile_commands.json + run_clangd_tidy(["--verbose", "--fix", "-p", str(test_dir), str(test_file)]) + + assert_file_matches_expected(test_file, expected_file) diff --git a/test/test_utils.py b/test/test_utils.py new file mode 100644 index 0000000..318de73 --- /dev/null +++ b/test/test_utils.py @@ -0,0 +1,124 @@ +""" +Shared test utilities for clangd-tidy test suite. + +This module provides common functionality for testing clangd-tidy features: +- Running clangd-tidy with proper environment setup +- Managing test fixtures +- Assertion helpers +""" + +import json +import os +import pathlib +import shutil +import subprocess +import sys +from typing import List, Optional + + +def run_clangd_tidy( + args: List[str], + cwd: Optional[pathlib.Path] = None, + capture_output: bool = True, + check: bool = True, +) -> subprocess.CompletedProcess[str]: + """Run clangd-tidy with proper environment setup. + + Args: + args: Command-line arguments (excluding 'python3 -m clangd_tidy') + cwd: Working directory (default: None, uses current directory) + capture_output: Whether to capture stdout/stderr + + Returns: + CompletedProcess object with stdout, stderr, and returncode + + Raises: + AssertionError: If clangd_tidy exits with non-zero status + """ + command = [sys.executable, "-m", "clangd_tidy"] + args + env = os.environ.copy() + project_root = pathlib.Path(__file__).parent.parent + # Ensure we use the current project root for imports to avoid running tests against another installed version + env["PYTHONPATH"] = str(project_root) + os.pathsep + env.get("PYTHONPATH", "") + + process = subprocess.run( + command, capture_output=capture_output, text=True, env=env, cwd=cwd, check=check + ) + print(process.stdout) + print(process.stderr, file=sys.stderr) + return process + + +def copy_fixture_directory( + fixture_name: str, tmp_path: pathlib.Path, make_paths_absolute: bool = False +) -> pathlib.Path: + """Copy a test fixture directory to a temporary directory and optionally update paths. + + Args: + fixture_name: Name of the fixture directory in test/fixtures/ + tmp_path: Temporary directory provided by pytest + make_paths_absolute: If True, convert relative paths in compile_commands.json to absolute paths + + Returns: + Path to the copied fixture directory + """ + fixtures_dir = pathlib.Path(__file__).parent / "fixtures" + source_dir = fixtures_dir / fixture_name + dest_dir = tmp_path / fixture_name + + # Copy the entire fixture directory + shutil.copytree(source_dir, dest_dir) + + # Update compile_commands.json to use absolute paths if requested + compile_commands_file = dest_dir / "compile_commands.json" + + if not compile_commands_file.exists(): + return dest_dir + + with compile_commands_file.open("r") as f: + compile_commands = json.load(f) + if make_paths_absolute: + for entry in compile_commands: + directory_abs = ( + pathlib.Path(dest_dir, entry["directory"]).resolve().as_posix() + if not os.path.isabs(entry["directory"]) + else entry["directory"] + ) + file_abs = ( + pathlib.Path(directory_abs, entry["file"]).resolve().as_posix() + if not os.path.isabs(entry["file"]) + else entry["file"] + ) + # Ensure the command uses absolute paths if it contains the file path + # This is necessary because we always generate absolute paths in replacement files, + # while clang-tidy uses the paths from the compile command + if entry["file"] in entry["command"]: + entry["command"] = entry["command"].replace(entry["file"], file_abs) + entry["directory"] = directory_abs + entry["file"] = file_abs + + with compile_commands_file.open("w") as f: + json.dump(compile_commands, f, indent=4) + + return dest_dir + + +def assert_file_matches_expected( + actual_file: pathlib.Path, expected_file: pathlib.Path +): + """Assert that the actual file content matches the expected file content. + + Args: + actual_file: Path to the file with actual content + expected_file: Path to the file with expected content + + Raises: + AssertionError: If file contents don't match + """ + actual_content = actual_file.read_text(encoding="utf-8") + expected_content = expected_file.read_text(encoding="utf-8") + + assert actual_content == expected_content, ( + f"File {actual_file} does not match expected content.\n" + f"Expected file: {expected_file}" + ) From 37101f626b11e2b8fb15a62bf49603a15865c3fc Mon Sep 17 00:00:00 2001 From: Frantisek Skala Date: Sun, 16 Nov 2025 21:33:15 +0100 Subject: [PATCH 2/2] Fix missing dependency specifications --- pyproject.toml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index a101def..164902b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,7 @@ build-backend = "setuptools.build_meta" [project] name = "clangd-tidy" dynamic = ["version"] -dependencies = ["attrs", "cattrs >= 25.1.0", "typing-extensions"] +dependencies = ["attrs", "cattrs >= 25.1.0", "typing-extensions", "pyyaml"] requires-python = ">=3.10" authors = [{ name = "lljbash", email = "lljbash@gmail.com" }] description = "A faster alternative to clang-tidy" @@ -44,5 +44,6 @@ typeCheckingMode = "strict" [dependency-groups] dev = [ - "basedpyright" + "basedpyright", + "pytest", ]