From 5666c26eaaa55dfa6ddf15cd79ba310e15166da1 Mon Sep 17 00:00:00 2001 From: yoannblot Date: Sun, 19 Apr 2026 09:10:23 +0200 Subject: [PATCH] feat: add a rule to remove redundant readonly keyword on properties --- .claude/rules/architecture.md | 20 ++++ .claude/skills/add-new-rule/SKILL.md | 19 ++- Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 1 + docs/rules.md | 47 ++++++++ src/rules/quality/mod.rs | 1 + .../remove_redundant_readonly_keyword.rs | 112 ++++++++++++++++++ .../skip_already_stripped.php.inc | 10 ++ .../skip_enum.php.inc | 7 ++ .../skip_interface.php.inc | 6 + .../skip_non_readonly_class.php.inc | 6 + .../strip_abstract_readonly_class.php.inc | 14 +++ ...trip_mixed_properties_and_promoted.php.inc | 24 ++++ .../strip_only_in_readonly_class.php.inc | 24 ++++ .../strip_promoted_params.php.inc | 20 ++++ .../strip_property_declaration.php.inc | 14 +++ .../strip_readonly_final_order.php.inc | 14 +++ .../strip_readonly_first_modifier.php.inc | 14 +++ 19 files changed, 353 insertions(+), 4 deletions(-) create mode 100644 src/rules/quality/remove_redundant_readonly_keyword.rs create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/skip_already_stripped.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/skip_enum.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/skip_interface.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/skip_non_readonly_class.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/strip_abstract_readonly_class.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/strip_mixed_properties_and_promoted.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/strip_only_in_readonly_class.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/strip_promoted_params.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/strip_property_declaration.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/strip_readonly_final_order.php.inc create mode 100644 tests/rules/quality/remove_redundant_readonly_keyword/strip_readonly_first_modifier.php.inc diff --git a/.claude/rules/architecture.md b/.claude/rules/architecture.md index 83f7a8e..03e6fe8 100644 --- a/.claude/rules/architecture.md +++ b/.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. diff --git a/.claude/skills/add-new-rule/SKILL.md b/.claude/skills/add-new-rule/SKILL.md index a708e98..93ccaea 100644 --- a/.claude/skills/add-new-rule/SKILL.md +++ b/.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/` 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 { - [ ] 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 diff --git a/Cargo.lock b/Cargo.lock index a59722c..ea7be7d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -129,7 +129,7 @@ checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" [[package]] name = "php-refactor" -version = "0.2.0" +version = "0.3.0" dependencies = [ "glob", "globset", diff --git a/Cargo.toml b/Cargo.toml index 7e6aa8e..c916e23 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "php-refactor" -version = "0.2.0" +version = "0.3.0" edition = "2024" [[bin]] diff --git a/README.md b/README.md index 37a4624..86629d5 100644 --- a/README.md +++ b/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. diff --git a/docs/rules.md b/docs/rules.md index 4b99f44..1a1ee65 100644 --- a/docs/rules.md +++ b/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. + diff --git a/src/rules/quality/mod.rs b/src/rules/quality/mod.rs index 64dbe1b..61eadad 100644 --- a/src/rules/quality/mod.rs +++ b/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; diff --git a/src/rules/quality/remove_redundant_readonly_keyword.rs b/src/rules/quality/remove_redundant_readonly_keyword.rs new file mode 100644 index 0000000..b7965cf --- /dev/null +++ b/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 = 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 = 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 { + 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 { + 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 +} diff --git a/tests/rules/quality/remove_redundant_readonly_keyword/skip_already_stripped.php.inc b/tests/rules/quality/remove_redundant_readonly_keyword/skip_already_stripped.php.inc new file mode 100644 index 0000000..ae22161 --- /dev/null +++ b/tests/rules/quality/remove_redundant_readonly_keyword/skip_already_stripped.php.inc @@ -0,0 +1,10 @@ +