diff --git a/c2rust-postprocess/README.md b/c2rust-postprocess/README.md index 3a0de3726f..1b90e42e08 100644 --- a/c2rust-postprocess/README.md +++ b/c2rust-postprocess/README.md @@ -26,8 +26,8 @@ attempts to produce fully safe and idiomatic Rust; not a replacement for these e `c2rust-postprocess` has a few ways to filter/exclude the function identifiers that are processed. -`--ident-filter` simply takes a regex. -Anything matching the regex is filtered out of being processed. +`--ident-filter` takes a regex. +Only identifiers matching the regex are processed. This is very useful for on-the-fly filtering that's easy to change quickly. `--exclude-file` is for more granular, more permanent filtering/exclusion. @@ -70,4 +70,3 @@ uv run pytest -v tests/test_utils.py # filter tests to run - `uv run ruff format` to format - `uv run ruff check --fix .` to lint - diff --git a/c2rust-postprocess/postprocess/__init__.py b/c2rust-postprocess/postprocess/__init__.py index 6cd6a24222..1f6753af5b 100644 --- a/c2rust-postprocess/postprocess/__init__.py +++ b/c2rust-postprocess/postprocess/__init__.py @@ -11,12 +11,15 @@ from postprocess.cache import DirectoryCache, FrozenCache from postprocess.exclude_list import IdentifierExcludeList, format_exclude_entries -from postprocess.models import api_key_from_env, get_model_by_id +from postprocess.models import ( + AbstractGenerativeModel, + api_key_from_env, + get_model_by_id, +) from postprocess.models.gpt import GPTModel from postprocess.models.mock import MockGenerativeModel from postprocess.transforms import get_transform_by_id from postprocess.transforms.base import TransformError, TransformResult -from postprocess.transforms.comments import AbstractGenerativeModel from postprocess.utils import existing_file from postprocess.validate import BaselineError, make_validator @@ -132,9 +135,6 @@ def build_arg_parser() -> argparse.ArgumentParser: ), ) - # TODO: add option to select model - # TODO: add option to configure cache - return parser diff --git a/c2rust-postprocess/postprocess/exclude_list.py b/c2rust-postprocess/postprocess/exclude_list.py index eec947892c..9ae829e849 100644 --- a/c2rust-postprocess/postprocess/exclude_list.py +++ b/c2rust-postprocess/postprocess/exclude_list.py @@ -40,18 +40,12 @@ def __init__(self, src_path: Path | None) -> None: path = check_isinstance(path, str) identifiers = check_isinstance(identifiers, list) identifiers = [check_isinstance(ident, str) for ident in identifiers] - path = Path(path) - existing_identifiers = self.paths.get(path) - if existing_identifiers is None: - self.paths[path] = set(identifiers) - else: - existing_identifiers.update(*identifiers) + path = (src_path.parent / path).resolve() + self.paths.setdefault(path, set()).update(identifiers) def contains(self, path: Path, identifier: str) -> bool: # No `src_path` means an empty exclude list. if self.src_path is None: return False - # Consider paths relative to `src_path`, the location of the exclude file. - rel_path: Path = path.relative_to(self.src_path.parent) - identifiers = self.paths.get(rel_path, set()) + identifiers = self.paths.get(path.resolve(), set()) return identifier in identifiers diff --git a/c2rust-postprocess/postprocess/models/base.py b/c2rust-postprocess/postprocess/models/base.py index 3739b876be..4473476b24 100644 --- a/c2rust-postprocess/postprocess/models/base.py +++ b/c2rust-postprocess/postprocess/models/base.py @@ -15,36 +15,6 @@ def __init__(self, id: str): def id(self) -> str: return self._id - # @abstractmethod - # async def agenerate_with_tools( - # self, - # messages: list[dict[str, Any]], - # tools: list[Callable] | None = None, - # max_tool_loops: int = 5 - # ) -> Any: - # """ - # Generate a response using native automatic function calling. - - # Args: - # messages: Chat history. - # tools: List of Python functions. - # max_tool_loops: Maximum number of times the model can call tools - # consecutively. - - # Returns: - # The final natural language response from the model. - # """ - # pass - - # def generate_with_tools( - # self, - # messages: list[dict[str, Any]], - # tools: Iterable[Callable[..., Any]] = (), - # max_tool_loops: int = 5 - # ) -> str | None: - # """Synchronous wrapper for agenerate_response.""" - # return asyncio.run(self.agenerate_with_tools(messages, tools, max_tool_loops)) - @abstractmethod def generate_with_tools( self, diff --git a/c2rust-postprocess/postprocess/transforms/base.py b/c2rust-postprocess/postprocess/transforms/base.py index f446d2efeb..b6e21ecd8a 100644 --- a/c2rust-postprocess/postprocess/transforms/base.py +++ b/c2rust-postprocess/postprocess/transforms/base.py @@ -14,7 +14,8 @@ update_rust_definition, ) from postprocess.exclude_list import IdentifierExcludeList -from postprocess.models import AbstractGenerativeModel, api_key_from_env +from postprocess.models import AbstractGenerativeModel +from postprocess.models.mock import MockGenerativeModel from postprocess.utils import get_highlighted_c from postprocess.validate import BatchValidator, Candidate @@ -143,7 +144,7 @@ def generate( f"failed validation: {stale}" ) - if api_key_from_env(self.model.id) is None: + if isinstance(self.model, MockGenerativeModel): if error is not None: # Can't regenerate the invalid cached response without a key. raise error @@ -336,13 +337,3 @@ def apply_file( ) return result - - -# TODO: We probably want a an interface that generates validators specialized to -# each individual prompt so maybe this should take in some transform- -# specific parameters and return a callable that only takes the LLM -# response as input. -# class AbstractValidator(ABC): -# @abstractmethod -# def validate_response(self, response: str) -> str: -# pass diff --git a/c2rust-postprocess/postprocess/utils.py b/c2rust-postprocess/postprocess/utils.py index 2627b5c69a..b577fb187d 100644 --- a/c2rust-postprocess/postprocess/utils.py +++ b/c2rust-postprocess/postprocess/utils.py @@ -11,7 +11,6 @@ from pygments.lexer import RegexLexer from pygments.lexers.c_cpp import CLexer from pygments.lexers.rust import RustLexer -from requests.exceptions import JSONDecodeError def check_isinstance[T](value: object, typ: type[T]) -> T: @@ -53,7 +52,7 @@ def existing_file(value: str) -> Path: def get_compile_commands(compile_commands_path: Path) -> list[dict[str, Any]]: try: compile_commands = json.loads(compile_commands_path.read_text()) - except JSONDecodeError as exc: + except json.JSONDecodeError as exc: raise RuntimeError( f"Failed to parse JSON from {compile_commands_path}: {exc}" ) from exc diff --git a/c2rust-postprocess/tests/test_comments_transform.py b/c2rust-postprocess/tests/test_comments_transform.py index 0cf697ccab..ee594a0e71 100644 --- a/c2rust-postprocess/tests/test_comments_transform.py +++ b/c2rust-postprocess/tests/test_comments_transform.py @@ -5,6 +5,7 @@ from postprocess.cache import AbstractCache from postprocess.definitions import CDefinition +from postprocess.models import AbstractGenerativeModel from postprocess.models.mock import MockGenerativeModel from postprocess.transforms import base from postprocess.transforms.base import TransformError @@ -237,11 +238,11 @@ def invalidate(self, *, transform: str, identifier: str) -> None: raise AssertionError("no invalidation expected") -class QueuedModel(MockGenerativeModel): +class QueuedModel(AbstractGenerativeModel): """Mock model that returns queued responses.""" def __init__(self, responses: list[str]): - super().__init__() + super().__init__("test-model") self.responses = responses self.calls = 0 @@ -269,7 +270,7 @@ def generate_with_tools(self, messages, tools=(), max_tool_loops=5): def apply_to_body_comment_fn( - cache: AbstractCache, model: MockGenerativeModel + cache: AbstractCache, model: AbstractGenerativeModel ) -> str | None: transform = CommentsTransform(cache=cache, model=model) return transform.apply_ident( @@ -281,8 +282,7 @@ def apply_to_body_comment_fn( ) -def test_rejected_response_is_regenerated(monkeypatch) -> None: - monkeypatch.setattr(base, "api_key_from_env", lambda model_id: "test-key") +def test_rejected_response_is_regenerated() -> None: cache = RecordingCache(None) model = QueuedModel([BAD_RESPONSE, GOOD_RESPONSE]) @@ -297,8 +297,7 @@ def test_rejected_response_is_regenerated(monkeypatch) -> None: assert response == GOOD_RESPONSE -def test_invalid_cached_response_is_regenerated(monkeypatch) -> None: - monkeypatch.setattr(base, "api_key_from_env", lambda model_id: "test-key") +def test_invalid_cached_response_is_regenerated() -> None: cache = RecordingCache(BAD_RESPONSE) model = QueuedModel([GOOD_RESPONSE]) @@ -312,8 +311,7 @@ def test_invalid_cached_response_is_regenerated(monkeypatch) -> None: assert response == GOOD_RESPONSE -def test_code_changing_cached_response_is_regenerated(monkeypatch) -> None: - monkeypatch.setattr(base, "api_key_from_env", lambda model_id: "test-key") +def test_code_changing_cached_response_is_regenerated() -> None: cache = RecordingCache(BAD_CODE_RESPONSE) model = QueuedModel([GOOD_RESPONSE]) diff --git a/c2rust-postprocess/tests/test_exclude_list.py b/c2rust-postprocess/tests/test_exclude_list.py new file mode 100644 index 0000000000..380b84da08 --- /dev/null +++ b/c2rust-postprocess/tests/test_exclude_list.py @@ -0,0 +1,48 @@ +from pathlib import Path + +import pytest + +from postprocess.exclude_list import IdentifierExcludeList + + +@pytest.mark.parametrize("absolute_exclude", [False, True]) +@pytest.mark.parametrize("absolute_source", [False, True]) +def test_matches_relative_and_absolute_paths( + tmp_path: Path, monkeypatch, absolute_exclude: bool, absolute_source: bool +) -> None: + monkeypatch.chdir(tmp_path) + Path("project").mkdir() + exclude_path = Path("project/exclude.yml") + exclude_path.write_text("src/lib.rs:\n - excluded\n") + source_path = Path("project/src/lib.rs") + if absolute_exclude: + exclude_path = exclude_path.resolve() + if absolute_source: + source_path = source_path.resolve() + + exclude_list = IdentifierExcludeList(exclude_path) + + assert exclude_list.contains(source_path, "excluded") + assert not exclude_list.contains(source_path, "included") + + +def test_paths_resolve_relative_to_exclude_file(tmp_path: Path) -> None: + config_dir = tmp_path / "config" + config_dir.mkdir() + exclude_path = config_dir / "exclude.yml" + exclude_path.write_text("../src/lib.rs:\n - excluded\n") + exclude_list = IdentifierExcludeList(exclude_path) + + assert exclude_list.contains(tmp_path / "src/lib.rs", "excluded") + assert not exclude_list.contains(tmp_path / "other/lib.rs", "excluded") + + +def test_combines_identifiers_for_equivalent_paths(tmp_path: Path) -> None: + exclude_path = tmp_path / "exclude.yml" + exclude_path.write_text("src/lib.rs:\n - first\n./src/lib.rs:\n - second\n") + exclude_list = IdentifierExcludeList(exclude_path) + source_path = tmp_path / "src/lib.rs" + + assert exclude_list.contains(source_path, "first") + assert exclude_list.contains(source_path, "second") + assert not exclude_list.contains(source_path, "s")