From 2554a8b417958d8baabaa3e98cc9ddcbe6412b88 Mon Sep 17 00:00:00 2001 From: yoannblot Date: Wed, 25 Mar 2026 09:04:24 +0100 Subject: [PATCH] doc: add project documentation & Claude Code configuration --- .claude/agents/rust-reviewer.md | 94 ++++++++++ .claude/rules/architecture.md | 70 +++++++ .claude/rules/php-ast-patterns.md | 172 +++++++++++++++++ .claude/rules/rule-contract.md | 53 ++++++ .claude/rules/testing-patterns.md | 56 ++++++ .claude/settings.json | 13 ++ .claude/skills/add-new-rule/SKILL.md | 196 ++++++++++++++++++++ .claude/skills/code-quality-issues/SKILL.md | 157 ++++++++++++++++ .claude/skills/project-glossary/SKILL.md | 183 ++++++++++++++++++ .gitignore | 3 + CLAUDE.md | 54 ++++++ README.md | 87 ++++++++- 12 files changed, 1137 insertions(+), 1 deletion(-) create mode 100644 .claude/agents/rust-reviewer.md create mode 100644 .claude/rules/architecture.md create mode 100644 .claude/rules/php-ast-patterns.md create mode 100644 .claude/rules/rule-contract.md create mode 100644 .claude/rules/testing-patterns.md create mode 100644 .claude/settings.json create mode 100644 .claude/skills/add-new-rule/SKILL.md create mode 100644 .claude/skills/code-quality-issues/SKILL.md create mode 100644 .claude/skills/project-glossary/SKILL.md create mode 100644 CLAUDE.md diff --git a/.claude/agents/rust-reviewer.md b/.claude/agents/rust-reviewer.md new file mode 100644 index 0000000..1fc69aa --- /dev/null +++ b/.claude/agents/rust-reviewer.md @@ -0,0 +1,94 @@ +--- +name: rust-reviewer +description: Expert Rust code reviewer specializing in ownership, lifetimes, error handling, unsafe usage, and idiomatic patterns. Use for all Rust code changes. MUST BE USED for Rust projects. +tools: ["Read", "Grep", "Glob", "Bash"] +model: sonnet +--- + +You are a senior Rust code reviewer ensuring high standards of safety, idiomatic patterns, and performance. + +When invoked: +1. Run `cargo check`, `cargo clippy -- -D warnings`, `cargo fmt --check`, and `cargo test` — if any fail, stop and report +2. Run `git diff HEAD~1 -- '*.rs'` (or `git diff main...HEAD -- '*.rs'` for PR review) to see recent Rust file changes +3. Focus on modified `.rs` files +4. If the project has CI or merge requirements, note that review assumes a green CI and resolved merge conflicts where applicable; call out if the diff suggests otherwise. +5. Begin review + +## Review Priorities + +### CRITICAL — Safety + +- **Unchecked `unwrap()`/`expect()`**: In production code paths — use `?` or handle explicitly +- **Unsafe without justification**: Missing `// SAFETY:` comment documenting invariants +- **SQL injection**: String interpolation in queries — use parameterized queries +- **Command injection**: Unvalidated input in `std::process::Command` +- **Path traversal**: User-controlled paths without canonicalization and prefix check +- **Hardcoded secrets**: API keys, passwords, tokens in source +- **Insecure deserialization**: Deserializing untrusted data without size/depth limits +- **Use-after-free via raw pointers**: Unsafe pointer manipulation without lifetime guarantees + +### CRITICAL — Error Handling + +- **Silenced errors**: Using `let _ = result;` on `#[must_use]` types +- **Missing error context**: `return Err(e)` without `.context()` or `.map_err()` +- **Panic for recoverable errors**: `panic!()`, `todo!()`, `unreachable!()` in production paths +- **`Box` in libraries**: Use `thiserror` for typed errors instead + +### HIGH — Ownership and Lifetimes + +- **Unnecessary cloning**: `.clone()` to satisfy borrow checker without understanding the root cause +- **String instead of &str**: Taking `String` when `&str` or `impl AsRef` suffices +- **Vec instead of slice**: Taking `Vec` when `&[T]` suffices +- **Missing `Cow`**: Allocating when `Cow<'_, str>` would avoid it +- **Lifetime over-annotation**: Explicit lifetimes where elision rules apply + +### HIGH — Concurrency + +- **Blocking in async**: `std::thread::sleep`, `std::fs` in async context — use tokio equivalents +- **Unbounded channels**: `mpsc::channel()`/`tokio::sync::mpsc::unbounded_channel()` need justification — prefer bounded channels (`tokio::sync::mpsc::channel(n)` in async, `sync_channel(n)` in sync) +- **`Mutex` poisoning ignored**: Not handling `PoisonError` from `.lock()` +- **Missing `Send`/`Sync` bounds**: Types shared across threads without proper bounds +- **Deadlock patterns**: Nested lock acquisition without consistent ordering + +### HIGH — Code Quality + +- **Large functions**: Over 50 lines +- **Deep nesting**: More than 4 levels +- **Wildcard match on business enums**: `_ =>` hiding new variants +- **Non-exhaustive matching**: Catch-all where explicit handling is needed +- **Dead code**: Unused functions, imports, or variables + +### MEDIUM — Performance + +- **Unnecessary allocation**: `to_string()` / `to_owned()` in hot paths +- **Repeated allocation in loops**: String or Vec creation inside loops +- **Missing `with_capacity`**: `Vec::new()` when size is known — use `Vec::with_capacity(n)` +- **Excessive cloning in iterators**: `.cloned()` / `.clone()` when borrowing suffices +- **N+1 queries**: Database queries in loops + +### MEDIUM — Best Practices + +- **Clippy warnings unaddressed**: Suppressed with `#[allow]` without justification +- **Missing `#[must_use]`**: On non-`must_use` return types where ignoring values is likely a bug +- **Derive order**: Should follow `Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize` +- **Public API without docs**: `pub` items missing `///` documentation +- **`format!` for simple concatenation**: Use `push_str`, `concat!`, or `+` for simple cases + +## Diagnostic Commands + +```bash +cargo clippy -- -D warnings +cargo fmt --check +cargo test +if command -v cargo-audit >/dev/null; then cargo audit; else echo "cargo-audit not installed"; fi +if command -v cargo-deny >/dev/null; then cargo deny check; else echo "cargo-deny not installed"; fi +cargo build --release 2>&1 | head -50 +``` + +## Approval Criteria + +- **Approve**: No CRITICAL or HIGH issues +- **Warning**: MEDIUM issues only +- **Block**: CRITICAL or HIGH issues found + +For detailed Rust code examples and anti-patterns, see `skill: rust-patterns`. diff --git a/.claude/rules/architecture.md b/.claude/rules/architecture.md new file mode 100644 index 0000000..7f86925 --- /dev/null +++ b/.claude/rules/architecture.md @@ -0,0 +1,70 @@ +--- +paths: + - "src/**" +--- + +# Architecture + +## Core Components + +**CLI Entry Point** (`src/main.rs`) +- Reads a single PHP file path from `argv[1]` +- Loads file contents into memory +- Iterates over all registered rules from `all_rules()`, chaining each rule's output into the next +- If final content differs from original, writes the file back +- Prints timing report per rule and peak memory usage + +**Library Root** (`src/lib.rs`) +- Trivial re-export of public modules (`reporter`, `rules`) + +**Rule System** (`src/rules/mod.rs`) +- Defines the rule contract: `type RuleFn = fn(&str) -> Option` + - Input: PHP source code as `&str` + - Output: `None` if no changes needed, `Some(modified_source)` if changed + - Rules are pure functions — no side effects, no state +- `pub fn all_rules() -> Vec<(&'static str, RuleFn)>` — the rule registry + - Single place to register new rules + - Integration tests automatically discover and run all registered rules + +**Observability** (`src/reporter.rs`) +- Measures wall-clock time per rule (via `std::time::Instant`) +- Reports peak memory usage via `libc::getrusage()` (macOS/Linux only; returns 0 elsewhere) +- Formats and prints timing line to stdout: `[INFO] Refactoring took X.XXms (peak memory: X.XMB) | rule1: Xms, rule2: Xms, ...` + +**PHP AST Parsing** +- Uses `mago-syntax` (PHP ecosystem) to parse source into an AST +- Allocates AST into `bumpalo` arena for zero-copy traversal +- Rules walk the AST, collect byte offsets (from spans), then apply text edits back-to-front (to preserve offset validity) + +--- + +## Current Rules + +### `src/rules/quality/add_final_keyword.rs` + +**What it does**: Adds `final` keyword to non-abstract, non-final PHP classes (prevents accidental subclassing) + +**Behavior**: +- Parses PHP source into AST +- Walks all class declarations and classes within namespaces +- For each class: if it has no `final` or `abstract` modifiers, inserts `final ` before the `class` keyword +- If class is `readonly`, inserts before `readonly` to produce `final readonly class Foo {}` +- Applies insertions back-to-front to avoid offset shifting +- Returns `None` if no changes; returns `Some(modified_source)` if changed + +**Skips**: +- Abstract classes +- Classes that already have `final` +- Interfaces and traits (no `final` modifier) +- Enums (PHP enums cannot be marked `final`) + +--- + +## Quality Issues (High Priority) + +See `/code-quality-issues` skill for the full H1–H3 blockers, M1–M7 medium-priority, and L1–L6 low-priority list. + +Key high-priority items to address: +- **H1**: `reporter.rs` unsafe block missing `// SAFETY:` comment +- **H2**: `.unwrap()` in test fixtures lose error context +- **H3**: Unnecessary `String` clone in `main.rs` diff --git a/.claude/rules/php-ast-patterns.md b/.claude/rules/php-ast-patterns.md new file mode 100644 index 0000000..f9d75e7 --- /dev/null +++ b/.claude/rules/php-ast-patterns.md @@ -0,0 +1,172 @@ +--- +paths: + - "src/rules/**" +--- + +# PHP AST Implementation Patterns + +## Overview + +Rules transform PHP code by: +1. Parsing source into an AST using `mago-syntax` +2. Allocating the AST into a `bumpalo` arena (zero-copy, single-free) +3. Walking the AST to collect byte offsets (from spans) +4. Applying text edits back-to-front (to preserve offset validity) +5. Returning `Some(modified_source)` or `None` + +--- + +## Parsing & Allocation + +```rust +use bumpalo::Bump; +use mago_syntax::parser::Parser; + +pub fn apply(source: &str) -> Option { + // Create arena for zero-copy AST allocation + let arena = Bump::new(); + + // Parse into AST + let mut parser = Parser::new(&arena, source.as_bytes()); + let result = parser.parse(); + + // Handle parse errors gracefully — return None, don't panic + let ast = result.ok()?; + + // ... rest of rule logic +} +``` + +--- + +## Walking the AST + +`mago-syntax` provides AST node types like: +- `Statement` (enum of statement variants) +- `Sequence` (collection of statements) +- `Class` (class declaration with modifiers, name, members) +- `Namespace` (namespace wrapper around statements) + +Example: find all top-level and namespaced classes: + +```rust +for statement in &ast.statements { + match statement { + Statement::Class(class) => { + // Process top-level class + } + Statement::Namespace(ns) => { + for inner_stmt in &ns.statements { + if let Statement::Class(class) = inner_stmt { + // Process namespaced class + } + } + } + _ => {} + } +} +``` + +--- + +## Collecting Byte Offsets + +AST nodes have `.span` (a byte offset range). Collect all spans that need edits: + +```rust +let mut edits: Vec<(usize, &'static str)> = vec![]; + +// For each class that needs a `final` keyword +for class in classes_needing_final { + let position = class.span.start; // byte offset + edits.push((position, "final ")); +} +``` + +**Important**: Spans are byte offsets, not character offsets. UTF-8 matters. + +--- + +## Applying Text Edits Back-to-Front + +To avoid offset shifting, apply edits in reverse order (from end of file to start): + +```rust +// Sort edits by position (descending) +edits.sort_by(|a, b| b.0.cmp(&a.0)); + +let mut result = source.to_string(); + +for (pos, text) in edits { + result.insert_str(pos, text); +} + +if result == source { + None // No changes +} else { + Some(result) +} +``` + +**Why back-to-front?** Inserting at position 10 shifts all positions after 10 forward. By working backward, earlier positions remain valid. + +--- + +## Common Patterns + +### Checking Modifiers + +```rust +if class.modifiers.contains_final() { + // Already has `final` + return None; +} + +if class.modifiers.contains_abstract() { + // Skip abstract classes + return None; +} +``` + +### Handling `readonly` Classes + +If inserting before the `class` keyword, check for `readonly`: + +```rust +let insert_before = if class.modifiers.contains_readonly() { + class.readonly_position +} else { + class.class_position +}; + +edits.push((insert_before, "final ")); +``` + +### Safe Byte Offset Handling + +Document UTF-8 assumptions. If casting `usize`, use `usize::try_from()`: + +```rust +let pos: usize = usize::try_from(span.start) + .expect("span offset should fit in usize"); +``` + +--- + +## Error Handling + +- **Never panic on malformed input.** The rule runs on user code. +- **Return `None` if AST walk fails or is incomplete.** Better to skip than crash. +- **Log suspicious assumptions with comments.** Example: "assumes mago-syntax always sets span.start < span.end" + +--- + +## Testing Your Rule + +See `/testing-patterns` for fixture format. + +Key test cases: +- One positive fixture (shows transformation) +- One no-op fixture per skip condition (abstract, final, readonly, interface, trait, enum) +- Edge case: mixing `readonly` with other modifiers +- Idempotency: running rule twice should be idempotent diff --git a/.claude/rules/rule-contract.md b/.claude/rules/rule-contract.md new file mode 100644 index 0000000..e4f22e3 --- /dev/null +++ b/.claude/rules/rule-contract.md @@ -0,0 +1,53 @@ +--- +paths: + - "src/rules/**" +--- + +# Rule Contract + +## Function Signature + +Every rule is a pure function: + +```rust +type RuleFn = fn(&str) -> Option +``` + +**Input**: PHP source code as `&str` +**Output**: +- `None` if no changes needed +- `Some(modified_source)` if the rule transformed the code + +## Constraints + +### Pure Function +- No global state +- No side effects +- Deterministic output — same input always produces same output +- No panic on malformed input — either transform or return `None` + +### Return Semantics +- Return `None` = "no changes needed, skip to next rule" +- Return `Some(String)` = "I changed the code, pass this to next rule" +- Never return an unchanged copy of the input; return `None` instead + +### Idempotency (Critical) +**Applying a rule twice to the same file must produce the same output as applying it once.** + +This is essential for rule chaining. If rule A produces output, then rule B runs on it, then rule A runs again, the result must be identical to the first time rule A ran. + +Test this explicitly: if you write a fixture file, verify that running your rule twice produces the same result. + +### Registration + +Rules are registered in `src/rules/mod.rs` via the `all_rules()` function: + +```rust +pub fn all_rules() -> Vec<(&'static str, RuleFn)> { + vec![ + ("category/rule_name", category::rule_name::apply), + ] +} +``` + +The key (first element) is used in timing reports and test discovery. Use forward slashes and snake_case. diff --git a/.claude/rules/testing-patterns.md b/.claude/rules/testing-patterns.md new file mode 100644 index 0000000..bfef56e --- /dev/null +++ b/.claude/rules/testing-patterns.md @@ -0,0 +1,56 @@ +--- +paths: + - "tests/**" +--- + +# Testing Patterns + +## Test Pattern + +Integration tests in `tests/rules_test.rs`: +- `test_all_rules()` iterates `all_rules()` and runs each rule against its fixtures +- For each fixture file in `tests/rules//`: + - If contains `\n-----\n`: split on separator, run rule on input, assert output matches expected + - If no separator: run rule, assert returns `None` (no-op) + +## Fixture Format + +### Transform Fixture (rule applies transformation) + +```php +// Input above separator +class MyClass {} +----- +// Expected output below separator +final class MyClass {} +``` + +### No-op Fixture (rule skips it) + +```php +// Just the content, no separator +abstract class MyClass {} +``` + +## Running Tests + +```bash +just tests # Run all tests in Docker +cargo test # Run locally (requires Rust toolchain) +cargo test -- --nocapture # Show test output +``` + +## Test Coverage Requirements + +All rules registered in `all_rules()` are automatically tested. Each rule should have at least: +- One positive fixture (shows the transformation) +- One negative fixture per skip case (e.g., `skip_abstract_class.php.inc`) + +For example, `add_final_keyword` has: +- `add_final_keyword.php.inc` — transforms regular class +- `add_final_keyword_in_namespace.php.inc` — transforms class in namespace +- `add_final_keyword_on_readonly_class.php.inc` — transforms readonly class +- `skip_abstract_class.php.inc` — no-op for abstract +- `skip_class_with_final_keyword.php.inc` — no-op for already-final +- `skip_interface.php.inc` — no-op for interfaces +- `skip_trait.php.inc` — no-op for traits diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000..c808b94 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,13 @@ +{ + "$schema": "https://json.schemastore.org/claude-code-settings.json", + "plansDirectory": "./.claude/plans", + "permissions": { + "allow": [ + "Bash(just *)" + ], + "deny": [ + "Bash(rm *)", + "Bash(git push *)" + ] + } +} diff --git a/.claude/skills/add-new-rule/SKILL.md b/.claude/skills/add-new-rule/SKILL.md new file mode 100644 index 0000000..a708e98 --- /dev/null +++ b/.claude/skills/add-new-rule/SKILL.md @@ -0,0 +1,196 @@ +--- +name: add-new-rule +description: | + Step-by-step guide to implement a new transformation rule: create module, register in hierarchy, add test fixtures, write implementation. +user-invocable: true +allowed-tools: Read, Write, Glob, Grep +--- + +# Adding a New Rule + +Follow these four steps to implement a new PHP transformation rule. + +--- + +## 1. Create the Rule Module + +Create `src/rules//.rs`: + +```rust +/// Applies transformation to source code. Returns None if no changes needed. +pub fn apply(source: &str) -> Option { + // Parse source, apply transformation, return Some(modified) or None + todo!() +} +``` + +The rule must be a pure function: no global state, no side effects, deterministic output. + +**File naming**: Use snake_case. Example: `add_final_keyword.rs`, `remove_dead_code.rs`. + +--- + +## 2. Register in Module Hierarchy + +### Create or update `src/rules//mod.rs` + +If the category doesn't exist, create it: + +```rust +pub mod rule_name; +``` + +### Update `src/rules/mod.rs` + +Add your rule to the `all_rules()` registry: + +```rust +pub fn all_rules() -> Vec<(&'static str, RuleFn)> { + vec![ + ("quality/add_final_keyword", quality::add_final_keyword::apply), + ("category/rule_name", category::rule_name::apply), // ← new entry + ] +} +``` + +**Key format**: +- Prefix: category (matches directory name) +- Suffix: rule name (matches file name) +- Use forward slashes and snake_case + +--- + +## 3. Add Test Fixtures + +Create `tests/rules///` directory with `.php.inc` fixture files. + +### Transform Fixture (rule applies transformation) + +```php +// Input above separator +class MyClass {} +----- +// Expected output below separator +final class MyClass {} +``` + +Name it: `.php.inc` or something descriptive like `add_final_keyword.php.inc`. + +### No-op Fixture (rule skips it) + +```php +// Just the content, no separator +abstract class MyClass {} +``` + +Name it: `skip_.php.inc` (e.g., `skip_abstract_class.php.inc`). + +### Run Tests + +```bash +just tests # Run all tests in Docker +cargo test # Run locally +cargo test -- --nocapture # Show output +``` + +Fixtures are discovered and tested automatically via `tests/rules_test.rs`. + +--- + +## 4. Implementation Tips + +### Use mago-syntax AST +Walk `Sequence` to find classes, namespaces, functions, etc. + +```rust +use bumpalo::Bump; +use mago_syntax::parser::Parser; + +let arena = Bump::new(); +let mut parser = Parser::new(&arena, source.as_bytes()); +let result = parser.parse().ok()?; +let ast = result; + +for statement in &ast.statements { + // Process each statement +} +``` + +### Byte Offsets & Text Edits +- AST spans are byte offsets (not character offsets) +- Collect all edits, then apply back-to-front to preserve offset validity +- Return `None` if no changes; return `Some(String)` if changed + +```rust +let mut edits = vec![]; +// ... collect (position, "text") tuples ... +edits.sort_by(|a, b| b.0.cmp(&a.0)); // Sort descending + +let mut result = source.to_string(); +for (pos, text) in edits { + result.insert_str(pos, text); +} + +if result == source { None } else { Some(result) } +``` + +### Allocator +Use `bumpalo::Bump` arena for fast, efficient AST allocation (single-free). + +### Return Semantics +- `None` = no change needed +- `Some(String)` = changed +- Never panic on malformed input — either transform or return `None` + +### Idempotency (Critical) +Applying a rule twice to the same file must produce the same output as once. This is essential for rule chaining. + +Test explicitly: write a fixture, run your rule on its output, verify it returns `None` (no further changes). + +--- + +## Example: Full Skeleton + +```rust +use bumpalo::Bump; +use mago_syntax::parser::Parser; + +pub fn apply(source: &str) -> Option { + let arena = Bump::new(); + let mut parser = Parser::new(&arena, source.as_bytes()); + let result = parser.parse().ok()?; + let ast = result; + + let mut edits = vec![]; + + for statement in &ast.statements { + // Process statements, collect edits + } + + if edits.is_empty() { + return None; + } + + // Apply edits back-to-front + edits.sort_by(|a, b| b.0.cmp(&a.0)); + let mut result = source.to_string(); + for (pos, text) in edits { + result.insert_str(pos, text); + } + + Some(result) +} +``` + +--- + +## Checklist + +- [ ] Rule module created in `src/rules//.rs` +- [ ] Module registered in `src/rules//mod.rs` +- [ ] Rule added to `all_rules()` in `src/rules/mod.rs` +- [ ] At least one positive fixture created (`.php.inc`) +- [ ] At least one no-op fixture per skip case created +- [ ] Tests pass: `just tests` or `cargo test` +- [ ] Rule is idempotent (applying twice = applying once) +- [ ] No panics on malformed input diff --git a/.claude/skills/code-quality-issues/SKILL.md b/.claude/skills/code-quality-issues/SKILL.md new file mode 100644 index 0000000..253a899 --- /dev/null +++ b/.claude/skills/code-quality-issues/SKILL.md @@ -0,0 +1,157 @@ +--- +name: code-quality-issues +description: | + Known code quality issues (High, Medium, Low priority) and recommended fixes. Use when triaging quality debt or planning refactoring. +user-invocable: true +allowed-tools: Read, Grep, Glob +--- + +# Known Code Quality Issues + +Identified issues blocking release or requiring refactoring, categorized by priority. + +--- + +## High Priority (Blocking per rust-reviewer) + +### H1 — `reporter.rs` unsafe block missing `// SAFETY:` comment + +- **Location**: `src/reporter.rs:6-12` +- **Issue**: `libc::getrusage()` return value is ignored; if it fails, the function returns 0 KB silently +- **Risk**: Silent failure mode; no visibility into allocation failures +- **Fix**: + - Check return value of `getrusage()` + - Document safety invariant with `// SAFETY:` comment + - Log or handle failure gracefully instead of silently returning 0 + +--- + +### H2 — `.unwrap()` in test fixtures lose error context + +- **Location**: `tests/rules_test.rs:22-24, 35` +- **Issue**: `e.unwrap()`, `path.file_name().unwrap()`, `.to_str().unwrap()` panic without useful diagnostics +- **Risk**: Test failures are cryptic; hard to debug fixture problems +- **Fix**: Replace with `.expect("descriptive message")` for each: + - `e.unwrap()` → `.expect("failed to read fixture file")` + - `path.file_name().unwrap()` → `.expect("fixture path must have filename")` + - `.to_str().unwrap()` → `.expect("fixture path must be valid UTF-8")` + +--- + +### H3 — Unnecessary `String` clone in `main.rs` + +- **Location**: `src/main.rs:31` +- **Issue**: `let mut content = original.clone()` allocates unnecessarily; use `Cow<'_, str>` or just reference `original` +- **Risk**: Wasted allocation; complicates memory profiling (peak_memory_kb includes unnecessary clone) +- **Fix**: + - Avoid clone; compare as `&str` + - Consider using `String` only for mutations, `&str` for reads + - Or use `Cow<'_, str>` to defer allocation until first mutation + +--- + +## Medium Priority + +### M1 — `unreachable!()` encodes mago library invariant + +- **Location**: `src/rules/quality/add_final_keyword.rs:43` +- **Issue**: Assumes `contains_readonly() && get_readonly() == Readonly` are always consistent; panics if not +- **Risk**: Production panic if mago-syntax behavior changes or invariant doesn't hold +- **Fix**: Fall back to safe default instead of `unreachable!()` + ```rust + let insert_before = if class.modifiers.contains_readonly() { + class.readonly_position.unwrap_or(class.class_position) + } else { + class.class_position + }; + ``` + +--- + +### M2-M3 — Unchecked byte-offset assumptions and casts + +- **Location**: `src/rules/quality/add_final_keyword.rs` +- **Issue**: Byte offset casts assume no overflow; UTF-8 boundary assumptions undocumented +- **Risk**: Subtle bugs if casts overflow or UTF-8 assumptions break +- **Mitigation**: + - Add doc comment: "All spans are byte offsets valid within UTF-8 source" + - Use `usize::try_from(span.offset)` instead of unchecked casts + - Add test for multi-byte UTF-8 in class names (if applicable) + +--- + +### M4 — Misleading error messages + +- **Location**: `src/main.rs:14, 24, 50` +- **Issue**: Generic error messages don't distinguish between usage (wrong args) vs runtime (I/O, parse failure) +- **Risk**: Users confused about what went wrong +- **Fix**: Use distinct messages: + - "Usage: php-refactor " for argument errors + - "Failed to read file: {path}" for I/O errors + - "Failed to parse or transform file" for rule errors + +--- + +### M5 — `all_rules()` allocates `Vec` on every call + +- **Location**: `src/rules/mod.rs` +- **Issue**: `Vec::new()` + `vec![]` macro calls heap allocator every time `all_rules()` runs +- **Risk**: Unnecessary allocations on every file processed +- **Fix**: Return `&'static [(&'static str, RuleFn)]` or a static slice instead + ```rust + pub fn all_rules() -> &'static [(&'static str, RuleFn)] { + &[ + ("quality/add_final_keyword", quality::add_final_keyword::apply), + ] + } + ``` + +--- + +### M6 — Inefficient string building in `format_timing_line` + +- **Location**: `src/reporter.rs` (not specified, but likely in timing output) +- **Issue**: Uses `collect().join()` instead of single `fold` +- **Risk**: Extra allocations for intermediate Vec +- **Fix**: Use single `fold` with a `String`: + ```rust + let rule_times = rules.iter().fold(String::new(), |mut acc, (name, ms)| { + acc.push_str(&format!("{}: {}ms, ", name, ms)); + acc + }); + ``` + +--- + +### M7 — `peak_memory_kb` is public but undocumented + +- **Location**: `src/reporter.rs` +- **Issue**: Public field has no doc comment; return type and units unclear +- **Risk**: API misuse; unclear what value represents +- **Fix**: + - Add doc comment: `/// Peak memory usage in kilobytes (KB), as reported by getrusage()` + - Write unit test verifying function returns a valid usize + +--- + +## Low Priority + +### L1-L6 — Minor Inefficiencies + +- **L1**: Test patterns could use more specific assertions +- **L2**: Missing edge-case fixtures (e.g., empty class, class with only comments) +- **L3**: Direct `libc` dependency; could use safer wrapper +- **L4**: No benchmarking harness for rule performance +- **L5**: Reporter output format not configurable (JSON, CSV options) +- **L6**: No progress indicator for batch processing + +--- + +## Resolution Strategy + +1. **Start with H1–H3**: These block release. Estimate 2–4 hours. +2. **Address M1–M3**: Production safety. Estimate 3–5 hours. +3. **M4–M7**: Quality of life. Can batch into one session, ~2 hours. +4. **L1–L6**: Deferred; revisit after core stability achieved. + +Each fix should include a test demonstrating the issue before the fix and success after. diff --git a/.claude/skills/project-glossary/SKILL.md b/.claude/skills/project-glossary/SKILL.md new file mode 100644 index 0000000..adafacb --- /dev/null +++ b/.claude/skills/project-glossary/SKILL.md @@ -0,0 +1,183 @@ +--- +name: project-glossary +description: | + Definitions of key terms used in the php-refactor project (Rule, Fixture, AST, Span, Bump Arena, Idempotent, mago-syntax). +user-invocable: true +allowed-tools: Read, Grep +--- + +# Project Glossary + +Key definitions for understanding the php-refactor codebase. + +--- + +## **Rule** + +A pure function `fn(&str) -> Option` that transforms PHP code or returns `None`. + +- **Input**: PHP source code as `&str` +- **Output**: `None` if no changes, `Some(modified_source)` if changed +- **Properties**: No state, no side effects, deterministic (same input = same output) +- **Example**: `add_final_keyword::apply` adds `final` keyword to classes + +See `.claude/rules/rule-contract.md` for full contract. + +--- + +## **Fixture** + +A `.php.inc` test file that defines the expected behavior of a rule. + +**Transform fixture** (rule applies transformation): +```php +// Input +class MyClass {} +----- +// Expected output +final class MyClass {} +``` + +**No-op fixture** (rule skips it): +```php +// Just content, no separator +abstract class MyClass {} +``` + +Fixtures are discovered and tested automatically by `tests/rules_test.rs`. + +--- + +## **AST** + +Abstract Syntax Tree — a hierarchical representation of source code structure. + +For PHP, `mago-syntax` parses source into an AST with nodes like: +- `Statement` (class, function, namespace, etc.) +- `Class` (class declaration with modifiers, name, members) +- `Namespace` (wrapper for namespaced statements) + +Rules walk the AST to find and transform code elements. + +--- + +## **Span** + +A byte offset range in source code. `span.start` and `span.end` mark the position of an AST node. + +- **Byte offsets**: Positions in UTF-8 bytes, not characters +- **Example**: A class starting at byte 10 might have `span: {start: 10, end: 35}` +- **Why it matters**: Text edits use spans to know where to insert/delete + +--- + +## **Bump Arena** (bumpalo) + +A memory allocator optimized for allocating many short-lived objects that are freed all at once. + +- **Used for**: AST allocation in mago-syntax parsing +- **Benefit**: Zero-copy traversal; single-free (arena destruction frees all nodes at once) +- **Pattern**: Create one arena per rule invocation + +```rust +let arena = Bump::new(); +let mut parser = Parser::new(&arena, source.as_bytes()); +let ast = parser.parse().ok()?; +// arena freed when dropped +``` + +--- + +## **Idempotent** + +A property where applying an operation twice yields the same result as applying it once. + +For rules: **Applying a rule twice to the same file must produce the same output as applying it once.** + +**Why it matters**: Rules are chained. If rule A modifies file → rule B modifies result → rule A runs again, rule A must not re-trigger on its own output. + +**Test it**: +```rust +let output1 = rule::apply(input); +let output2 = rule::apply(&output1.unwrap_or(input)); +assert_eq!(output2, None); // Second run should be no-op +``` + +--- + +## **mago-syntax** + +A PHP parser written in Rust. Provides: +- `Parser` struct for parsing source into AST +- AST node types (`Statement`, `Class`, `Namespace`, etc.) +- Span information for each node + +**Key types**: +- `Statement` — enum of statement variants +- `Sequence` — collection of AST nodes +- `Class` — class declaration with modifiers and members + +See `.claude/rules/php-ast-patterns.md` for usage patterns. + +--- + +## **Rule Registry** + +The `all_rules()` function in `src/rules/mod.rs` returns a list of all registered rules. + +```rust +pub fn all_rules() -> Vec<(&'static str, RuleFn)> { + vec![ + ("quality/add_final_keyword", quality::add_final_keyword::apply), + ] +} +``` + +- **Key**: Rule name (used in timing reports and test discovery) +- **Value**: Function pointer to the rule's `apply` function +- **Single source of truth**: All rules must be registered here +- **Integration tests**: Automatically discover and test all registered rules + +--- + +## **Rule Chaining** + +The process of applying multiple rules in sequence. + +In `src/main.rs`: +1. Load file into `original` +2. For each rule in `all_rules()`: + - Run rule on current content + - If `Some(modified)`, use that as input to next rule + - If `None`, use same content +3. Write result if changed from original + +This is why **idempotency** is critical — each rule sees the output of the previous one. + +--- + +## **Peak Memory Usage** + +The maximum amount of memory consumed during rule execution, reported by `libc::getrusage()`. + +- **Reported in**: Timing output line (e.g., "peak memory: 2.3 MB") +- **What it measures**: Max resident set size during rule chaining +- **Limitation**: Only available on macOS/Linux; returns 0 elsewhere +- **Note**: Includes unnecessary clones and allocations (see H3 code quality issue) + +--- + +## **Timing Report** + +Output line printed after rule execution: + +``` +[INFO] Refactoring took 12.34ms (peak memory: 2.3MB) | quality/add_final_keyword: 5.67ms, another_rule: 1.23ms +``` + +Components: +- **Total time**: Wall-clock time for all rules +- **Peak memory**: Max resident set size +- **Per-rule timing**: Wall-clock time for each rule in sequence + +Reported by `src/reporter.rs`. diff --git a/.gitignore b/.gitignore index f419dc7..5804cc4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,5 @@ /.idea /target + +# Claude Code +.claude/plans diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..5165751 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,54 @@ +# php-refactor — Developer Guide + +A fast PHP refactoring CLI tool written in Rust. Reads a PHP file, applies a series of AST-based transformation rules in sequence, and writes the file back in-place if changed. + +**Purpose**: Automate recurring PHP code quality transformations (formatting, standardization, compliance) at scale without manual review per file. + +## Quick Reference + +### Build & Test + +```bash +just docker-build # Build the Docker image +just build # Build the binary (dev mode) +just build-release # Build optimized release binary +just tests # Run test suite +just quality-check # fmt check, clippy, tests +just quality-tools # Auto-format code with rustfmt +``` + +### Running the Tool + +```bash +cargo run -- path/to/file.php +# or +./target/debug/php-refactor path/to/file.php +``` + +## How to Find What You Need + +- **Writing or modifying a rule?** → See `.claude/rules/rule-contract.md` (function contract, return semantics) +- **Implementing PHP AST transformations?** → See `.claude/rules/php-ast-patterns.md` (mago-syntax, bumpalo, byte-offset editing) +- **Adding a new rule?** → Invoke `/add-new-rule` skill for the full 4-step walkthrough +- **Triaging code quality issues?** → Invoke `/code-quality-issues` skill for the H/M/L backlog +- **Looking at architecture in `src/`?** → `.claude/rules/architecture.md` (loaded automatically when working in src/) +- **Understanding test fixtures in `tests/`?** → `.claude/rules/testing-patterns.md` (loaded automatically when working in tests/) +- **Need a definition?** → Invoke `/glossary` skill (Rule, Fixture, AST, Span, Bump Arena, Idempotent, mago-syntax) + +## Project Structure + +``` +src/ + ├── main.rs # CLI entry, rule chaining, file I/O + ├── lib.rs # Module re-exports + ├── reporter.rs # Timing and memory reporting + └── rules/mod.rs # Rule registry (all_rules) + +tests/ + ├── rules_test.rs # Fixture-based integration test runner + └── rules/ # Test fixtures by rule path + +.github/workflows/quality.yml # CI: check, fmt, clippy, test +Cargo.toml / Cargo.lock # Dependencies +justfile # Task automation +``` diff --git a/README.md b/README.md index c91c961..151e51e 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,88 @@ # PHP Refactor -Fast PHP refactoring tool to help you refactor your code. +A fast, AST-based PHP code refactoring CLI tool written in Rust. Automatically applies transformations like adding `final` keywords, enforcing code standards, and more — with zero manual review needed. + +## Why PHP Refactor? + +- **AST-powered**: Parses PHP into an abstract syntax tree for safe, accurate transformations (not regex-based) +- **Fast**: Single-threaded, compiled Rust binary — processes files in milliseconds +- **Extensible**: Add new rules as pure functions; all rules are automatically tested against fixtures +- **Zero false positives**: Each rule includes fixtures for what it transforms and what it skips +- **Composable**: Rules chain together; you can apply multiple transformations in one pass + +## Installation + +### From Source + +```bash +git clone https://github.com/yoannblot/php-refactor.git +cd php-refactor +cargo build --release +``` + +Binary will be at `target/release/php-refactor`. + +### With Docker + +```bash +docker build -t php-refactor . +docker run --rm -v $(pwd):/workspace php-refactor \ + /app/target/release/php-refactor /workspace/MyClass.php +``` + +## Usage + +```bash +php-refactor path/to/file.php +``` + +**Example:** + +```bash +$ cat MyClass.php +