feat: add a rule to remove redundant readonly keyword on properties

.claude/rules/architecture.md

@@ -77,6 +77,26 @@ paths:
 
 ---
 
+### `src/rules/quality/remove_redundant_readonly_keyword.rs`
+
+**What it does**: Removes redundant `readonly` keywords from property declarations and constructor-promoted parameters inside classes already declared `readonly` (PHP 8.2+).
+
+**Behavior**:
+- Uses a line-anchored regex (`HEADER_RE`) to find class declarations whose modifier list contains `readonly` in any order (`readonly`, `final readonly`, `readonly final`, `abstract readonly`, etc.). Capture group 1 is the full modifier string; the rule rejects matches whose modifier list does not contain the literal word `readonly`.
+- For each matching header, scans the source bytes to find the matching `}` via `find_matching_brace` (naive depth counter).
+- Within each `(body_start, body_end)` range, applies `STRIP_RE` to remove `readonly` from lines that start with a visibility modifier (`public` / `protected` / `private`), optionally followed by `static`. The same regex handles both property declarations and constructor-promoted parameters (identical syntactic shape).
+- Applies replacements back-to-front (ranges reverse-sorted by `start`) so byte offsets of earlier ranges stay valid.
+- Returns `None` if no ranges produced changes (idempotent on re-run).
+
+**Skips**:
+- Bare `class Foo` declarations — `readonly` on their properties is load-bearing and must not be stripped
+- Classes already free of redundant `readonly`
+- Interfaces, traits, enums (cannot be `readonly` in PHP)
+
+**Known limitations**: The brace counter is string/heredoc/comment-unaware. A `{` or `}` byte inside a string literal inside the class body can skew the scan. Documented as a syntactic limitation in the module header.
+
+---
+
 ## 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.

.claude/skills/add-new-rule/SKILL.md

@@ -1,9 +1,9 @@
 ---
 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.
+  Step-by-step guide to implement a new PHP transformation rule in this project: scaffold the rule module, register in the module hierarchy, add test fixtures, update documentation. Use when the user asks to "add a rule", "add a new rule", "create a rule", "implement a transformation", or invokes /add-new-rule.
 user-invocable: true
-allowed-tools: Read, Write, Glob, Grep
+allowed-tools: Read, Write, Edit, Glob, Grep
 ---
 
 # Adding a New Rule
@@ -149,6 +149,18 @@ Test explicitly: write a fixture, run your rule on its output, verify it returns
 
 ---
 
+## 5. Update Documentation
+
+After the rule passes tests, update these three documentation surfaces so the new rule is discoverable:
+
+- **`docs/rules.md`** — user-facing reference. Add a new `## quality/<rule_name>` section with Summary, When to use, What it does, What it skips, Example (before/after PHP), and Known limitations.
+- **`README.md`** — one-line row in the "Available Rules" table.
+- **`.claude/rules/architecture.md`** — agent-facing implementation notes under "Current Rules". Describe the regex/AST approach, skip conditions, and known limitations.
+
+Each doc has a distinct audience — do not collapse them into one.
+
+---
+
 ## Example: Full Skeleton
 
 ```rust
@@ -194,3 +206,6 @@ pub fn apply(source: &str) -> Option<String> {
 - [ ] Tests pass: `just tests` or `cargo test`
 - [ ] Rule is idempotent (applying twice = applying once)
 - [ ] No panics on malformed input
+- [ ] `docs/rules.md` updated with a user-facing section
+- [ ] `README.md` rules table updated
+- [ ] `.claude/rules/architecture.md` updated with implementation notes

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "php-refactor"
-version = "0.2.0"
+version = "0.3.0"
 edition = "2024"
 
 [[bin]]

README.md

@@ -112,6 +112,7 @@ The tool prints a summary:
 |------|-------------|
 | `quality/add_final_keyword` | Adds `final` keyword to concrete classes |
 | `quality/add_readonly_keyword` | Adds `readonly` keyword to bare concrete classes |
+| `quality/remove_redundant_readonly_keyword` | Removes redundant `readonly` from properties and promoted params inside `readonly` classes |
 
 See [docs/rules.md](docs/rules.md) for detailed rule documentation.
 

docs/rules.md

@@ -87,3 +87,50 @@ Scope the rule via `config.toml` path globs to classes you know are readonly-com
 add_readonly_keyword.paths = ["src/Dto/**/*.php", "src/ValueObject/**/*.php"]
 ```
 
+---
+
+## `quality/remove_redundant_readonly_keyword`
+
+**Summary**: Removes redundant `readonly` from properties and constructor-promoted parameters inside classes already declared `readonly` (PHP 8.2+).
+
+**When to use**: Clean up noise after class-level `readonly` is adopted — the class modifier already makes every instance property readonly, so per-property `readonly` is redundant.
+
+**What it does:**
+- Detects classes whose modifier list contains `readonly` in any order (`readonly class`, `final readonly class`, `readonly final class`, `abstract readonly class`)
+- Inside each matching class body, strips the `readonly` keyword from lines that start with a visibility modifier (`public`, `protected`, `private`), optionally followed by `static`
+- Applies to both regular property declarations and constructor-promoted parameters (they share the same syntactic shape)
+- Uses brace counting to scope edits strictly to the class body
+
+**What it skips:**
+- Bare classes (`class Foo { private readonly Bar $x; }`) — here `readonly` is load-bearing and must NOT be stripped
+- Classes already free of redundant `readonly` (idempotent)
+- Interfaces, traits, enums (cannot be `readonly` in PHP)
+
+**Example:**
+
+```php
+// Before
+final readonly class ActivityDashboardFactory
+{
+    private readonly TeamRepository $teamRepository;
+
+    public function __construct(
+        private readonly PeriodFactory $periodFactory,
+    ) {}
+}
+
+// After
+final readonly class ActivityDashboardFactory
+{
+    private TeamRepository $teamRepository;
+
+    public function __construct(
+        private PeriodFactory $periodFactory,
+    ) {}
+}
+```
+
+**Known limitations (syntactic rule — no string/comment awareness):**
+
+The brace counter used to scope the class body is naive: raw `{` or `}` bytes inside string literals, heredocs, or comments can skew the scan. Scope via `config.toml` path globs to code you trust.
+

src/rules/quality/mod.rs

@@ -1,2 +1,3 @@
 pub mod add_final_keyword;
 pub mod add_readonly_keyword;
+pub mod remove_redundant_readonly_keyword;

src/rules/quality/remove_redundant_readonly_keyword.rs

@@ -0,0 +1,112 @@
+use rayon::prelude::*;
+use regex::Regex;
+use std::fs;
+use std::path::PathBuf;
+use std::sync::LazyLock;
+use std::sync::atomic::{AtomicUsize, Ordering};
+
+// NOTE: This rule is purely syntactic. It uses naive brace counting that is not
+// aware of PHP strings, heredocs, or comments. A `{` or `}` inside a string
+// literal inside a readonly class body can skew the scan. Scope application
+// via `config.toml` path globs to code you trust.
+
+static HEADER_RE: LazyLock<Regex> = LazyLock::new(|| {
+    // Matches a class declaration whose modifier list contains `readonly`
+    // in any order (readonly class, final readonly class, readonly final class,
+    // abstract readonly class, etc.). Capture 1 is the full modifier string;
+    // we post-check that it contains `readonly` to reject `final class` etc.
+    Regex::new(r"(?m)^[ \t]*((?:(?:final|abstract|readonly)[ \t]+)+)class[ \t]+\w+[^{]*\{").unwrap()
+});
+
+static STRIP_RE: LazyLock<Regex> = LazyLock::new(|| {
+    // Matches visibility-prefixed declarations (property or promoted param)
+    // followed by `readonly`. Capture 1 is the prefix we preserve.
+    Regex::new(r"(?m)^(\s*(?:public|protected|private)(?:[ \t]+static)?)[ \t]+readonly\b").unwrap()
+});
+
+/// File-aware entry point: applies the rule to the given set of files in parallel.
+pub fn apply(files: &[PathBuf]) -> crate::rules::RuleResult {
+    let files_matched = AtomicUsize::new(0);
+    let files_changed = AtomicUsize::new(0);
+
+    files.par_iter().for_each(|file_path| {
+        let Ok(original) = fs::read_to_string(file_path) else {
+            return;
+        };
+
+        if let Some(modified) = apply_to_source(&original) {
+            files_matched.fetch_add(1, Ordering::Relaxed);
+            if fs::write(file_path, &modified).is_ok() {
+                files_changed.fetch_add(1, Ordering::Relaxed);
+            }
+        }
+    });
+
+    crate::rules::RuleResult {
+        files_changed: files_changed.load(Ordering::Relaxed),
+        files_matched: files_matched.load(Ordering::Relaxed),
+        files_analyzed: files.len(),
+    }
+}
+
+/// Pure source transformation: used by tests.
+pub fn apply_to_source(source: &str) -> Option<String> {
+    if !source.contains("readonly") || !source.contains("class ") {
+        return None;
+    }
+
+    let bytes = source.as_bytes();
+
+    let mut ranges: Vec<(usize, usize)> = Vec::new();
+    for caps in HEADER_RE.captures_iter(source) {
+        let modifiers = caps.get(1).expect("group 1 is non-optional").as_str();
+        if !modifiers.split_whitespace().any(|w| w == "readonly") {
+            continue;
+        }
+        let header_match = caps.get(0).expect("group 0 is always present on a match");
+        let open_brace_pos = header_match.end() - 1;
+        let Some(close_brace_pos) = find_matching_brace(bytes, open_brace_pos) else {
+            continue;
+        };
+        let body_start = open_brace_pos + 1;
+        let body_end = close_brace_pos;
+        if body_start < body_end {
+            ranges.push((body_start, body_end));
+        }
+    }
+
+    if ranges.is_empty() {
+        return None;
+    }
+
+    ranges.sort_by_key(|r| std::cmp::Reverse(r.0));
+
+    let mut result = source.to_string();
+    for (start, end) in ranges {
+        let replaced = STRIP_RE.replace_all(&result[start..end], "$1").into_owned();
+        result.replace_range(start..end, &replaced);
+    }
+
+    if result == source { None } else { Some(result) }
+}
+
+/// Finds the byte offset of the `}` that closes the `{` at `open_pos`.
+/// Returns `None` if the braces are unbalanced (malformed input).
+fn find_matching_brace(bytes: &[u8], open_pos: usize) -> Option<usize> {
+    let mut depth: usize = 1;
+    let mut i = open_pos + 1;
+    while i < bytes.len() {
+        match bytes[i] {
+            b'{' => depth += 1,
+            b'}' => {
+                depth -= 1;
+                if depth == 0 {
+                    return Some(i);
+                }
+            }
+            _ => {}
+        }
+        i += 1;
+    }
+    None
+}

tests/rules/quality/remove_redundant_readonly_keyword/skip_already_stripped.php.inc

@@ -0,0 +1,10 @@
+<?php
+
+final readonly class AlreadyDone
+{
+    private string $name;
+
+    public function __construct(
+        private int $id,
+    ) {}
+}

tests/rules/quality/remove_redundant_readonly_keyword/skip_enum.php.inc

@@ -0,0 +1,7 @@
+<?php
+
+enum Status: string
+{
+    case Active = 'active';
+    case Inactive = 'inactive';
+}

tests/rules/quality/remove_redundant_readonly_keyword/skip_interface.php.inc

@@ -0,0 +1,6 @@
+<?php
+
+interface ReadonlyLike
+{
+    public function readonly(): void;
+}

tests/rules/quality/remove_redundant_readonly_keyword/skip_non_readonly_class.php.inc

@@ -0,0 +1,6 @@
+<?php
+
+final class NotReadonly
+{
+    private readonly string $loadBearing;
+}

tests/rules/quality/remove_redundant_readonly_keyword/strip_abstract_readonly_class.php.inc

@@ -0,0 +1,14 @@
+<?php
+
+abstract readonly class Base
+{
+    protected readonly string $name;
+}
+
+-----
+<?php
+
+abstract readonly class Base
+{
+    protected string $name;
+}

tests/rules/quality/remove_redundant_readonly_keyword/strip_mixed_properties_and_promoted.php.inc

@@ -0,0 +1,24 @@
+<?php
+
+final readonly class Mixed
+{
+    private readonly string $label;
+
+    public function __construct(
+        private readonly int $id,
+        protected readonly string $name,
+    ) {}
+}
+
+-----
+<?php
+
+final readonly class Mixed
+{
+    private string $label;
+
+    public function __construct(
+        private int $id,
+        protected string $name,
+    ) {}
+}

tests/rules/quality/remove_redundant_readonly_keyword/strip_only_in_readonly_class.php.inc

@@ -0,0 +1,24 @@
+<?php
+
+final readonly class ReadonlyOne
+{
+    private readonly string $a;
+}
+
+class BareTwo
+{
+    private readonly string $b;
+}
+
+-----
+<?php
+
+final readonly class ReadonlyOne
+{
+    private string $a;
+}
+
+class BareTwo
+{
+    private readonly string $b;
+}

tests/rules/quality/remove_redundant_readonly_keyword/strip_promoted_params.php.inc

@@ -0,0 +1,20 @@
+<?php
+
+final readonly class ActivityDashboardFactory
+{
+    public function __construct(
+        private readonly TeamRepository $teamRepository,
+        private readonly PeriodFactory $periodFactory,
+    ) {}
+}
+
+-----
+<?php
+
+final readonly class ActivityDashboardFactory
+{
+    public function __construct(
+        private TeamRepository $teamRepository,
+        private PeriodFactory $periodFactory,
+    ) {}
+}

tests/rules/quality/remove_redundant_readonly_keyword/strip_property_declaration.php.inc

@@ -0,0 +1,14 @@
+<?php
+
+final readonly class ActivityDashboardFactory
+{
+    private readonly TeamRepository $teamRepository;
+}
+
+-----
+<?php
+
+final readonly class ActivityDashboardFactory
+{
+    private TeamRepository $teamRepository;
+}